Re: [PATCH 10/17] hyper-v: trace vmbus_open()

[Stephen Hemminger]

On Tue, 31 Oct 2017 13:48:00 +0100
Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:

> On Mon, Oct 30, 2017 at 10:31:34AM -0400, Steven Rostedt wrote:
> > On Mon, 30 Oct 2017 11:32:20 +0100
> > Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >   
> > > On Mon, Oct 30, 2017 at 11:07:01AM +0100, Vitaly Kuznetsov wrote:  
> > > > Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
> > > >     
> > > > > On Mon, Oct 30, 2017 at 09:16:19AM +0100, Vitaly Kuznetsov wrote:    
> > > > >> Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
> > > > >>     
> > > > >> > On Sun, Oct 29, 2017 at 12:21:09PM -0700, kys@xxxxxxxxxxxxxxxxxxxxxx wrote:    
> > > > >> >> From: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> > > > >> >> 
> > > > >> >> Add tracepoint to CHANNELMSG_OPENCHANNEL sender.
> > > > >> >> 
> > > > >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> > > > >> >> Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > > > >> >> ---
> > > > >> >>  drivers/hv/channel.c  |  2 ++
> > > > >> >>  drivers/hv/hv_trace.h | 27 +++++++++++++++++++++++++++
> > > > >> >>  2 files changed, 29 insertions(+)
> > > > >> >> 
> > > > >> >> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> > > > >> >> index a406beb10dd0..739b3fe1e0fb 100644
> > > > >> >> --- a/drivers/hv/channel.c
> > > > >> >> +++ b/drivers/hv/channel.c
> > > > >> >> @@ -185,6 +185,8 @@ int vmbus_open(struct vmbus_channel *newchannel, u32 send_ringbuffer_size,
> > > > >> >>  	ret = vmbus_post_msg(open_msg,
> > > > >> >>  			     sizeof(struct vmbus_channel_open_channel), true);
> > > > >> >>  
> > > > >> >> +	trace_vmbus_open(open_msg, ret);    
> > > > >> >
> > > > >> > Why add tracepoints for things that ftrace can handle automatically?    
> > > > >> 
> > > > >> This series adds pretty prints for structures printing what is needed
> > > > >> and in the right format significantly simplifying debugging. And it
> > > > >> wouldn't make sense to add tracepoints to *some* messages-related
> > > > >> functions and skip others where parsing is more trivial.    
> > > > >
> > > > > Tracepoints add memory usage and take up real space.  If you don't need
> > > > > them for something, as there are other ways to already get the
> > > > > information needed, why add new ones that you now need to drag around
> > > > > for all time?
> > > > >    
> > > > 
> > > > Are you opposed to the series as a whole (AKA 'no tracepoints in
> > > > drivers') or only to some tracepoints we add here?    
> > > 
> > > I'm opposed to adding tracepoints for things that are not needed as you
> > > can get the same info already today without the tracepoint.  
> > 
> > I looked at this specific tracepoint, and I don't see how to get the
> > information from the current tracing infrastructure. Maybe an eBPF
> > program attached to a kprobe here might work. But the tracepoint data
> > looks like this:
> > 
> > +           TP_STRUCT__entry(
> > +                   __field(u32, child_relid)
> > +                   __field(u32, openid)
> > +                   __field(u32, gpadlhandle)
> > +                   __field(u32, target_vp)
> > +                   __field(u32, offset)
> > +                   __field(int, ret)
> > +                   ),
> > +           TP_fast_assign(
> > +                   __entry->child_relid = msg->child_relid;
> > +                   __entry->openid = msg->openid;
> > +                   __entry->gpadlhandle = msg->ringbuffer_gpadlhandle;
> > +                   __entry->target_vp = msg->target_vp;
> > +                   __entry->offset = msg->downstream_ringbuffer_pageoffset;
> > +                   __entry->ret = ret;
> > +                   ),
> > 
> > I don't see how that information can be extracted easily without a
> > tracepoint here. Am I missing something?  
> 
> Wasn't one of the outcomes of the conference last week the fact that for
> ftrace + ebpf we could get access to the structures of the function
> parameters?  Or that work would soon be showing up?
> 
> It just feels "wrong" to add a tracepoint for a function call, like it
> is a duplication of work/functionality we already have.
> 
> thanks,
> 
> greg k-h

Just to add some context. VMBus open/close etc are not in critical path.
The reason that tracing of these makes sense is that there have been bugs
in the past with teardown and restart of channels, and having some information
would help. Not sure if the detailed internals of the parameters matter that
much since it has been clear what the parameters were by context.