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... > > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
