lkml.org 
[lkml]   [2017]   [Oct]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 10/17] hyper-v: trace vmbus_open()
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@linuxfoundation.org> wrote:
>
> > On Mon, Oct 30, 2017 at 11:07:01AM +0100, Vitaly Kuznetsov wrote:
> > > Greg KH <gregkh@linuxfoundation.org> writes:
> > >
> > > > On Mon, Oct 30, 2017 at 09:16:19AM +0100, Vitaly Kuznetsov wrote:
> > > >> Greg KH <gregkh@linuxfoundation.org> writes:
> > > >>
> > > >> > On Sun, Oct 29, 2017 at 12:21:09PM -0700, kys@exchange.microsoft.com wrote:
> > > >> >> From: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > >> >>
> > > >> >> Add tracepoint to CHANNELMSG_OPENCHANNEL sender.
> > > >> >>
> > > >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > >> >> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > >> >> ---
> > > >> >> 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

\
 
 \ /
  Last update: 2017-10-31 13:48    [W:0.138 / U:0.372 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site