Re: [PATCH v5 3/4] net: dropreason: use new __print_sym() in tracing

[Paolo Abeni <pabeni@redhat.com>]

On Fri, 2024-06-14 at 10:19 +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> The __print_symbolic() could only ever print the core
> drop reasons, since that's the way the infrastructure
> works. Now that we have __print_sym() with all the
> advantages mentioned in that commit, convert to that
> and get all the drop reasons from all subsystems. As
> we already have a list of them, that's really easy.
> 
> This is a little bit of .text (~100 bytes in my build)
> and saves a lot of .data (~17k).
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  include/net/dropreason.h   |  5 +++++
>  include/trace/events/skb.h | 16 +++-----------
>  net/core/skbuff.c          | 43 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 51 insertions(+), 13 deletions(-)
> 
> diff --git a/include/net/dropreason.h b/include/net/dropreason.h
> index 56cb7be92244..c157070b5303 100644
> --- a/include/net/dropreason.h
> +++ b/include/net/dropreason.h
> @@ -42,6 +42,11 @@ struct drop_reason_list {
>  extern const struct drop_reason_list __rcu *
>  drop_reasons_by_subsys[SKB_DROP_REASON_SUBSYS_NUM];
>  
> +#ifdef CONFIG_TRACEPOINTS
> +const char *drop_reason_lookup(unsigned long long value);
> +void drop_reason_show(struct seq_file *m);
> +#endif
> +
>  void drop_reasons_register_subsys(enum skb_drop_reason_subsys subsys,
>  				  const struct drop_reason_list *list);
>  void drop_reasons_unregister_subsys(enum skb_drop_reason_subsys subsys);
> diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
> index 07e0715628ec..8a1a63f9e796 100644
> --- a/include/trace/events/skb.h
> +++ b/include/trace/events/skb.h
> @@ -8,15 +8,9 @@
>  #include <linux/skbuff.h>
>  #include <linux/netdevice.h>
>  #include <linux/tracepoint.h>
> +#include <net/dropreason.h>
>  
> -#undef FN
> -#define FN(reason)	TRACE_DEFINE_ENUM(SKB_DROP_REASON_##reason);
> -DEFINE_DROP_REASON(FN, FN)
> -
> -#undef FN
> -#undef FNe
> -#define FN(reason)	{ SKB_DROP_REASON_##reason, #reason },
> -#define FNe(reason)	{ SKB_DROP_REASON_##reason, #reason }
> +TRACE_DEFINE_SYM_FNS(drop_reason, drop_reason_lookup, drop_reason_show);
>  
>  /*
>   * Tracepoint for free an sk_buff:
> @@ -44,13 +38,9 @@ TRACE_EVENT(kfree_skb,
>  
>  	TP_printk("skbaddr=%p protocol=%u location=%pS reason: %s",
>  		  __entry->skbaddr, __entry->protocol, __entry->location,
> -		  __print_symbolic(__entry->reason,
> -				   DEFINE_DROP_REASON(FN, FNe)))
> +		  __print_sym(__entry->reason, drop_reason ))

Minor nit: if you have to repost for other reasons,       ^^ here
checkpatch complains for the extra space.

Otherwise LGTM,

Thanks!

Paolo