From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932151AbdJaMrt (ORCPT ); Tue, 31 Oct 2017 08:47:49 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:55160 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752908AbdJaMrr (ORCPT ); Tue, 31 Oct 2017 08:47:47 -0400 Date: Tue, 31 Oct 2017 13:48:00 +0100 From: Greg KH To: Steven Rostedt Cc: olaf@aepfle.de, sthemmin@microsoft.com, jasowang@redhat.com, linux-kernel@vger.kernel.org, marcelo.cerri@canonical.com, apw@canonical.com, devel@linuxdriverproject.org, Vitaly Kuznetsov , leann.ogasawara@canonical.com Subject: Re: [PATCH 10/17] hyper-v: trace vmbus_open() Message-ID: <20171031124800.GA27947@kroah.com> References: <20171029192030.12356-1-kys@exchange.microsoft.com> <20171029192116.12466-1-kys@exchange.microsoft.com> <20171029192116.12466-10-kys@exchange.microsoft.com> <20171029205958.GA30187@kroah.com> <877evdyrsc.fsf@vitty.brq.redhat.com> <20171030084537.GA14865@kroah.com> <873761ymnu.fsf@vitty.brq.redhat.com> <20171030103220.GA30988@kroah.com> <20171030103134.22086c4e@gandalf.local.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171030103134.22086c4e@gandalf.local.home> User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 30, 2017 at 10:31:34AM -0400, Steven Rostedt wrote: > On Mon, 30 Oct 2017 11:32:20 +0100 > Greg KH wrote: > > > On Mon, Oct 30, 2017 at 11:07:01AM +0100, Vitaly Kuznetsov wrote: > > > Greg KH writes: > > > > > > > On Mon, Oct 30, 2017 at 09:16:19AM +0100, Vitaly Kuznetsov wrote: > > > >> Greg KH writes: > > > >> > > > >> > On Sun, Oct 29, 2017 at 12:21:09PM -0700, kys@exchange.microsoft.com wrote: > > > >> >> From: Vitaly Kuznetsov > > > >> >> > > > >> >> Add tracepoint to CHANNELMSG_OPENCHANNEL sender. > > > >> >> > > > >> >> Signed-off-by: Vitaly Kuznetsov > > > >> >> Signed-off-by: K. Y. Srinivasan > > > >> >> --- > > > >> >> 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