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

[Steven Rostedt]

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?

-- Steve


> 
> I'm not opposed to tracepoints in drivers as it's up to the maintainer
> to have to manage them over the long-term and the issues that surround
> them...
> 
>