From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [patch net-next 1/2] devlink: add hardware messages tracing facility Date: Tue, 12 Jul 2016 10:38:37 +0200 Message-ID: <20160712083837.GE1884@nanopsycho.orion> References: <1468243128-6669-1-git-send-email-jiri@resnulli.us> <20160711120814.504c20cb@gandalf.local.home> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, davem@davemloft.net, idosch@mellanox.com, yotamg@mellanox.com, eladr@mellanox.com, nogahf@mellanox.com, ogerlitz@mellanox.com, ivecera@redhat.com, mingo@redhat.com, jolsa@kernel.org To: Steven Rostedt Return-path: Received: from mail-wm0-f66.google.com ([74.125.82.66]:34625 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751121AbcGLIik (ORCPT ); Tue, 12 Jul 2016 04:38:40 -0400 Received: by mail-wm0-f66.google.com with SMTP id w75so1369812wmd.1 for ; Tue, 12 Jul 2016 01:38:40 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20160711120814.504c20cb@gandalf.local.home> Sender: netdev-owner@vger.kernel.org List-ID: Mon, Jul 11, 2016 at 06:08:14PM CEST, rostedt@goodmis.org wrote: >On Mon, 11 Jul 2016 15:18:47 +0200 >Jiri Pirko wrote: > >> From: Jiri Pirko >> >> Define a tracepoint and allow user to trace messages going to and from >> hardware associated with devlink instance. >> >> Signed-off-by: Jiri Pirko >> --- >> include/net/devlink.h | 8 +++++++ >> include/trace/events/devlink.h | 49 ++++++++++++++++++++++++++++++++++++++++++ >> net/core/devlink.c | 9 ++++++++ >> 3 files changed, 66 insertions(+) >> create mode 100644 include/trace/events/devlink.h >> >> diff --git a/include/net/devlink.h b/include/net/devlink.h >> index c99ffe8..865ade6 100644 >> --- a/include/net/devlink.h >> +++ b/include/net/devlink.h >> @@ -115,6 +115,8 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size); >> int devlink_register(struct devlink *devlink, struct device *dev); >> void devlink_unregister(struct devlink *devlink); >> void devlink_free(struct devlink *devlink); >> +void devlink_trace_hwmsg(const struct devlink *devlink, bool incoming, >> + unsigned long type, const u8 *buf, size_t len); >> int devlink_port_register(struct devlink *devlink, >> struct devlink_port *devlink_port, >> unsigned int port_index); >> @@ -154,6 +156,12 @@ static inline void devlink_free(struct devlink *devlink) >> kfree(devlink); >> } >> >> +static inline void devlink_trace_hwmsg(const struct devlink *devlink, >> + bool incoming, unsigned long type, >> + const u8 *buf, size_t len); >> +{ >> +} >> + >> static inline int devlink_port_register(struct devlink *devlink, >> struct devlink_port *devlink_port, >> unsigned int port_index) >> diff --git a/include/trace/events/devlink.h b/include/trace/events/devlink.h >> new file mode 100644 >> index 0000000..7918c57 >> --- /dev/null >> +++ b/include/trace/events/devlink.h >> @@ -0,0 +1,49 @@ >> +#undef TRACE_SYSTEM >> +#define TRACE_SYSTEM devlink >> + >> +#if !defined(_TRACE_DEVLINK_H) || defined(TRACE_HEADER_MULTI_READ) >> +#define _TRACE_DEVLINK_H >> + >> +#include >> +#include >> +#include >> + >> +/* >> + * Tracepoint for devlink hardware message: >> + */ >> +TRACE_EVENT(devlink_hwmsg, >> + TP_PROTO(const struct devlink *devlink, bool incoming, >> + unsigned long type, const u8 *buf, size_t len), >> + >> + TP_ARGS(devlink, incoming, type, buf, len), >> + >> + TP_STRUCT__entry( >> + __string(bus_name, devlink->dev->bus->name) >> + __string(dev_name, dev_name(devlink->dev)) >> + __string(owner_name, devlink->dev->driver->owner->name) >> + __field(bool, incoming) >> + __field(unsigned long, type) >> + __dynamic_array(u8, buf, len) >> + __field(size_t, len) >> + ), >> + >> + TP_fast_assign( >> + __assign_str(bus_name, devlink->dev->bus->name); >> + __assign_str(dev_name, dev_name(devlink->dev)); >> + __assign_str(owner_name, devlink->dev->driver->owner->name); >> + __entry->incoming = incoming; >> + __entry->type = type; >> + memcpy(__get_dynamic_array(buf), buf, len); >> + __entry->len = len; >> + ), >> + >> + TP_printk("bus_name=%s dev_name=%s owner_name=%s incoming=%d type=%lu buf=0x[%*phD] len=%lu", >> + __get_str(bus_name), __get_str(dev_name), >> + __get_str(owner_name), __entry->incoming, __entry->type, >> + (int) __entry->len, __get_dynamic_array(buf), __entry->len) >> +); >> + >> +#endif /* _TRACE_DEVLINK_H */ >> + >> +/* This part must be outside protection */ >> +#include >> diff --git a/net/core/devlink.c b/net/core/devlink.c >> index b2e592a..8cfa3b0 100644 >> --- a/net/core/devlink.c >> +++ b/net/core/devlink.c >> @@ -26,6 +26,8 @@ >> #include >> #include >> #include >> +#define CREATE_TRACE_POINTS >> +#include >> >> static LIST_HEAD(devlink_list); >> >> @@ -1679,6 +1681,13 @@ void devlink_free(struct devlink *devlink) >> } >> EXPORT_SYMBOL_GPL(devlink_free); >> >> +void devlink_trace_hwmsg(const struct devlink *devlink, bool incoming, >> + unsigned long type, const u8 *buf, size_t len) >> +{ >> + trace_devlink_hwmsg(devlink, incoming, type, buf, len); >> +} >> +EXPORT_SYMBOL_GPL(devlink_trace_hwmsg); >> + > >Instead of having a function that always gets called even when tracing >isn't enabled, why not have the caller call the trace_devlink_hwmsg() >directly? > >In the trace/devlink.h file you could encapsulate it with: > >#if IS_ENABLED(CONFIG_NET_DEVLINK) > >[...] > >#else >static inline void trace_devlink_hwmsg(const struct devlink *devlink, > bool incoming, unsigned long type, > const u8 *buf, size_t len); >{ >} >#endif Hmm, I'm trying to make it work this was but it seems to be a bit more complicated. I did not find any code doing this (having the tracepoint compiled in or not) so I cannot copy&paste the solution. The problem is that trace_devlink_hwmsg is included from include/trace/events/devlink.h when devlink is on and from include/net/devlink.h. Odd but ok. I cannot include include/trace/events/devlink.h in include/net/devlink.h because include/net/devlink.h is included in include/trace/events/devlink.h So I have to directly include include/trace/events/devlink.h from mlxsw driver (trace caller). But when devlink is not compiled in, trecepoint is not created and I'm getting errors. I see no easy way our of this other than the wrapper function I was originally using. Seem simple and elegant. Thanks.