diff mbox series

[net-next,2/3] net: skb: use auto-generation to convert skb drop reason to string

Message ID 20220527071522.116422-3-imagedong@tencent.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series reorganize the code of the enum skb_drop_reason | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 5746 this patch: 5549
netdev/cc_maintainers warning 2 maintainers not CCed: cong.wang@bytedance.com flyingpeng@tencent.com
netdev/build_clang fail Errors and warnings before: 1224 this patch: 1202
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 5894 this patch: 5693
netdev/checkpatch fail ERROR: space prohibited before open square bracket '[' WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Menglong Dong May 27, 2022, 7:15 a.m. UTC 
From: Menglong Dong <imagedong@tencent.com>

It is annoying to add new skb drop reasons to 'enum skb_drop_reason'
and TRACE_SKB_DROP_REASON in trace/event/skb.h, and it's easy to forget
to add the new reasons we added to TRACE_SKB_DROP_REASON.

TRACE_SKB_DROP_REASON is used to convert drop reason of type number
to string. For now, the string we passed to user space is exactly the
same as the name in 'enum skb_drop_reason' with a 'SKB_DROP_REASON_'
prefix. Therefore, we can use 'auto-generation' to generate these
drop reasons to string at build time.

The new header 'dropreason_str.h' will be generated, and the
__DEFINE_SKB_DROP_REASON() in it can do the converting job. Meanwhile,
we use a global array to store these string, which can be used both
in drop_monitor and 'kfree_skb' tracepoint.

Signed-off-by: Menglong Dong <imagedong@tencent.com>
---
 include/linux/dropreason.h |  2 +
 include/trace/events/skb.h | 89 +-------------------------------------
 net/core/.gitignore        |  1 +
 net/core/Makefile          | 14 ++++++
 net/core/drop_monitor.c    | 13 ------
 net/core/skbuff.c          | 12 +++++
 6 files changed, 30 insertions(+), 101 deletions(-)
 create mode 100644 net/core/.gitignore

Comments

kernel test robot May 27, 2022, 2:03 p.m. UTC | #1 
Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/menglong8-dong-gmail-com/reorganize-the-code-of-the-enum-skb_drop_reason/20220527-152050
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 7e062cda7d90543ac8c7700fc7c5527d0c0f22ad
config: x86_64-randconfig-a013 (https://download.01.org/0day-ci/archive/20220527/202205272154.7zdeh6A3-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-1) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/0a1ac892edba0134b4891c9e61e06d462f8262a9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review menglong8-dong-gmail-com/reorganize-the-code-of-the-enum-skb_drop_reason/20220527-152050
        git checkout 0a1ac892edba0134b4891c9e61e06d462f8262a9
        # save the config file
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from net/core/skbuff.c:85:
   ./net/core/dropreason_str.h:1:1: error: stray '\' in program
       1 | \n#define __DEFINE_SKB_DROP_REASON(FN) \
         | ^
   ./net/core/dropreason_str.h:1:3: error: stray '#' in program
       1 | \n#define __DEFINE_SKB_DROP_REASON(FN) \
         |   ^
>> ./net/core/dropreason_str.h:1:2: error: unknown type name 'n'
       1 | \n#define __DEFINE_SKB_DROP_REASON(FN) \
         |  ^
>> ./net/core/dropreason_str.h:1:11: error: expected '=', ',', ';', 'asm' or '__attribute__' before '__DEFINE_SKB_DROP_REASON'
       1 | \n#define __DEFINE_SKB_DROP_REASON(FN) \
         |           ^~~~~~~~~~~~~~~~~~~~~~~~
>> net/core/skbuff.c:101:9: error: implicit declaration of function '__DEFINE_SKB_DROP_REASON' [-Werror=implicit-function-declaration]
     101 |         __DEFINE_SKB_DROP_REASON(FN)
         |         ^~~~~~~~~~~~~~~~~~~~~~~~
>> net/core/skbuff.c:101:34: error: 'FN' undeclared here (not in a function)
     101 |         __DEFINE_SKB_DROP_REASON(FN)
         |                                  ^~
   cc1: some warnings being treated as errors
Jakub Kicinski May 28, 2022, 1:14 a.m. UTC | #2 
On Fri, 27 May 2022 15:15:21 +0800 menglong8.dong@gmail.com wrote:
> +clean-files := dropreason_str.h
> +
> +quiet_cmd_dropreason_str = GEN     $@
> +cmd_dropreason_str = echo '\n\#define __DEFINE_SKB_DROP_REASON(FN) \' > $@;\

echo -n

> +	sed -e '/enum skb_drop_reason {/,/}/!d' $< | \
> +	awk -F ',' '/SKB_DROP_REASON_/{printf "	FN(%s) \\\n", substr($$1, 18)}' >> $@;\
> +	echo '' >> $@

Trying to figure out when we're in the enum could be more robust
in case more stuff gets added to the header:

 | awk -F ',' '/^enum skb_drop/ { dr=1; } 
               /\}\;/           { dr=0; } 
               /^\tSKB_DROP/    { if (dr) {print $1;}}'

> +$(obj)/dropreason_str.h: $(srctree)/include/linux/dropreason.h
> +	$(call cmd,dropreason_str)
> +
> +$(obj)/skbuff.o: $(obj)/dropreason_str.h

Since we just generate the array directly now should we generate
a source file with it directly instead of generating a header with 
the huge define?
Menglong Dong May 28, 2022, 4:26 a.m. UTC | #3 
On Sat, May 28, 2022 at 9:14 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 27 May 2022 15:15:21 +0800 menglong8.dong@gmail.com wrote:
> > +clean-files := dropreason_str.h
> > +
> > +quiet_cmd_dropreason_str = GEN     $@
> > +cmd_dropreason_str = echo '\n\#define __DEFINE_SKB_DROP_REASON(FN) \' > $@;\
>
> echo -n
>
> > +     sed -e '/enum skb_drop_reason {/,/}/!d' $< | \
> > +     awk -F ',' '/SKB_DROP_REASON_/{printf " FN(%s) \\\n", substr($$1, 18)}' >> $@;\
> > +     echo '' >> $@
>
> Trying to figure out when we're in the enum could be more robust
> in case more stuff gets added to the header:
>
>  | awk -F ',' '/^enum skb_drop/ { dr=1; }
>                /\}\;/           { dr=0; }
>                /^\tSKB_DROP/    { if (dr) {print $1;}}'
>
> > +$(obj)/dropreason_str.h: $(srctree)/include/linux/dropreason.h
> > +     $(call cmd,dropreason_str)
> > +
> > +$(obj)/skbuff.o: $(obj)/dropreason_str.h
>
> Since we just generate the array directly now should we generate
> a source file with it directly instead of generating a header with
> the huge define?

This seems to be a good idea, which is able to decouple the
definition of the array with skbuff.c. I'll try this.

Thanks!
Menglong Dong
diff mbox series

Patch

diff --git a/include/linux/dropreason.h b/include/linux/dropreason.h
index ecd18b7b1364..013ff0f2543e 100644
--- a/include/linux/dropreason.h
+++ b/include/linux/dropreason.h
@@ -181,4 +181,6 @@  enum skb_drop_reason {
 			SKB_DR_SET(name, reason);		\
 	} while (0)
 
+extern const char * const drop_reasons[];
+
 #endif
diff --git a/include/trace/events/skb.h b/include/trace/events/skb.h
index a477bf907498..45264e4bb254 100644
--- a/include/trace/events/skb.h
+++ b/include/trace/events/skb.h
@@ -9,92 +9,6 @@ 
 #include <linux/netdevice.h>
 #include <linux/tracepoint.h>
 
-#define TRACE_SKB_DROP_REASON					\
-	EM(SKB_DROP_REASON_NOT_SPECIFIED, NOT_SPECIFIED)	\
-	EM(SKB_DROP_REASON_NO_SOCKET, NO_SOCKET)		\
-	EM(SKB_DROP_REASON_PKT_TOO_SMALL, PKT_TOO_SMALL)	\
-	EM(SKB_DROP_REASON_TCP_CSUM, TCP_CSUM)			\
-	EM(SKB_DROP_REASON_SOCKET_FILTER, SOCKET_FILTER)	\
-	EM(SKB_DROP_REASON_UDP_CSUM, UDP_CSUM)			\
-	EM(SKB_DROP_REASON_NETFILTER_DROP, NETFILTER_DROP)	\
-	EM(SKB_DROP_REASON_OTHERHOST, OTHERHOST)		\
-	EM(SKB_DROP_REASON_IP_CSUM, IP_CSUM)			\
-	EM(SKB_DROP_REASON_IP_INHDR, IP_INHDR)			\
-	EM(SKB_DROP_REASON_IP_RPFILTER, IP_RPFILTER)		\
-	EM(SKB_DROP_REASON_UNICAST_IN_L2_MULTICAST,		\
-	   UNICAST_IN_L2_MULTICAST)				\
-	EM(SKB_DROP_REASON_XFRM_POLICY, XFRM_POLICY)		\
-	EM(SKB_DROP_REASON_IP_NOPROTO, IP_NOPROTO)		\
-	EM(SKB_DROP_REASON_SOCKET_RCVBUFF, SOCKET_RCVBUFF)	\
-	EM(SKB_DROP_REASON_PROTO_MEM, PROTO_MEM)		\
-	EM(SKB_DROP_REASON_TCP_MD5NOTFOUND, TCP_MD5NOTFOUND)	\
-	EM(SKB_DROP_REASON_TCP_MD5UNEXPECTED,			\
-	   TCP_MD5UNEXPECTED)					\
-	EM(SKB_DROP_REASON_TCP_MD5FAILURE, TCP_MD5FAILURE)	\
-	EM(SKB_DROP_REASON_SOCKET_BACKLOG, SOCKET_BACKLOG)	\
-	EM(SKB_DROP_REASON_TCP_FLAGS, TCP_FLAGS)		\
-	EM(SKB_DROP_REASON_TCP_ZEROWINDOW, TCP_ZEROWINDOW)	\
-	EM(SKB_DROP_REASON_TCP_OLD_DATA, TCP_OLD_DATA)		\
-	EM(SKB_DROP_REASON_TCP_OVERWINDOW, TCP_OVERWINDOW)	\
-	EM(SKB_DROP_REASON_TCP_OFOMERGE, TCP_OFOMERGE)		\
-	EM(SKB_DROP_REASON_TCP_OFO_DROP, TCP_OFO_DROP)		\
-	EM(SKB_DROP_REASON_TCP_RFC7323_PAWS, TCP_RFC7323_PAWS)	\
-	EM(SKB_DROP_REASON_TCP_INVALID_SEQUENCE,		\
-	   TCP_INVALID_SEQUENCE)				\
-	EM(SKB_DROP_REASON_TCP_RESET, TCP_RESET)		\
-	EM(SKB_DROP_REASON_TCP_INVALID_SYN, TCP_INVALID_SYN)	\
-	EM(SKB_DROP_REASON_TCP_CLOSE, TCP_CLOSE)		\
-	EM(SKB_DROP_REASON_TCP_FASTOPEN, TCP_FASTOPEN)		\
-	EM(SKB_DROP_REASON_TCP_OLD_ACK, TCP_OLD_ACK)		\
-	EM(SKB_DROP_REASON_TCP_TOO_OLD_ACK, TCP_TOO_OLD_ACK)	\
-	EM(SKB_DROP_REASON_TCP_ACK_UNSENT_DATA,			\
-	   TCP_ACK_UNSENT_DATA)					\
-	EM(SKB_DROP_REASON_TCP_OFO_QUEUE_PRUNE,			\
-	  TCP_OFO_QUEUE_PRUNE)					\
-	EM(SKB_DROP_REASON_IP_OUTNOROUTES, IP_OUTNOROUTES)	\
-	EM(SKB_DROP_REASON_BPF_CGROUP_EGRESS,			\
-	   BPF_CGROUP_EGRESS)					\
-	EM(SKB_DROP_REASON_IPV6DISABLED, IPV6DISABLED)		\
-	EM(SKB_DROP_REASON_NEIGH_CREATEFAIL, NEIGH_CREATEFAIL)	\
-	EM(SKB_DROP_REASON_NEIGH_FAILED, NEIGH_FAILED)		\
-	EM(SKB_DROP_REASON_NEIGH_QUEUEFULL, NEIGH_QUEUEFULL)	\
-	EM(SKB_DROP_REASON_NEIGH_DEAD, NEIGH_DEAD)		\
-	EM(SKB_DROP_REASON_TC_EGRESS, TC_EGRESS)		\
-	EM(SKB_DROP_REASON_QDISC_DROP, QDISC_DROP)		\
-	EM(SKB_DROP_REASON_CPU_BACKLOG, CPU_BACKLOG)		\
-	EM(SKB_DROP_REASON_XDP, XDP)				\
-	EM(SKB_DROP_REASON_TC_INGRESS, TC_INGRESS)		\
-	EM(SKB_DROP_REASON_UNHANDLED_PROTO, UNHANDLED_PROTO)	\
-	EM(SKB_DROP_REASON_SKB_CSUM, SKB_CSUM)			\
-	EM(SKB_DROP_REASON_SKB_GSO_SEG, SKB_GSO_SEG)		\
-	EM(SKB_DROP_REASON_SKB_UCOPY_FAULT, SKB_UCOPY_FAULT)	\
-	EM(SKB_DROP_REASON_DEV_HDR, DEV_HDR)			\
-	EM(SKB_DROP_REASON_DEV_READY, DEV_READY)		\
-	EM(SKB_DROP_REASON_FULL_RING, FULL_RING)		\
-	EM(SKB_DROP_REASON_NOMEM, NOMEM)			\
-	EM(SKB_DROP_REASON_HDR_TRUNC, HDR_TRUNC)		\
-	EM(SKB_DROP_REASON_TAP_FILTER, TAP_FILTER)		\
-	EM(SKB_DROP_REASON_TAP_TXFILTER, TAP_TXFILTER)		\
-	EM(SKB_DROP_REASON_ICMP_CSUM, ICMP_CSUM)		\
-	EM(SKB_DROP_REASON_INVALID_PROTO, INVALID_PROTO)	\
-	EM(SKB_DROP_REASON_IP_INADDRERRORS, IP_INADDRERRORS)	\
-	EM(SKB_DROP_REASON_IP_INNOROUTES, IP_INNOROUTES)	\
-	EM(SKB_DROP_REASON_PKT_TOO_BIG, PKT_TOO_BIG)		\
-	EMe(SKB_DROP_REASON_MAX, MAX)
-
-#undef EM
-#undef EMe
-
-#define EM(a, b)	TRACE_DEFINE_ENUM(a);
-#define EMe(a, b)	TRACE_DEFINE_ENUM(a);
-
-TRACE_SKB_DROP_REASON
-
-#undef EM
-#undef EMe
-#define EM(a, b)	{ a, #b },
-#define EMe(a, b)	{ a, #b }
-
 /*
  * Tracepoint for free an sk_buff:
  */
@@ -121,8 +35,7 @@  TRACE_EVENT(kfree_skb,
 
 	TP_printk("skbaddr=%p protocol=%u location=%p reason: %s",
 		  __entry->skbaddr, __entry->protocol, __entry->location,
-		  __print_symbolic(__entry->reason,
-				   TRACE_SKB_DROP_REASON))
+		  drop_reasons[__entry->reason])
 );
 
 TRACE_EVENT(consume_skb,
diff --git a/net/core/.gitignore b/net/core/.gitignore
new file mode 100644
index 000000000000..2148db0574dc
--- /dev/null
+++ b/net/core/.gitignore
@@ -0,0 +1 @@ 
+dropreason_str.h
\ No newline at end of file
diff --git a/net/core/Makefile b/net/core/Makefile
index a8e4f737692b..73b8d5f4fa09 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -39,3 +39,17 @@  obj-$(CONFIG_NET_SOCK_MSG) += skmsg.o
 obj-$(CONFIG_BPF_SYSCALL) += sock_map.o
 obj-$(CONFIG_BPF_SYSCALL) += bpf_sk_storage.o
 obj-$(CONFIG_OF)	+= of_net.o
+
+clean-files := dropreason_str.h
+
+quiet_cmd_dropreason_str = GEN     $@
+cmd_dropreason_str = echo '\n\#define __DEFINE_SKB_DROP_REASON(FN) \' > $@;\
+	sed -e '/enum skb_drop_reason {/,/}/!d' $< | \
+	awk -F ',' '/SKB_DROP_REASON_/{printf "	FN(%s) \\\n", substr($$1, 18)}' >> $@;\
+	echo '' >> $@
+
+$(obj)/dropreason_str.h: $(srctree)/include/linux/dropreason.h
+	$(call cmd,dropreason_str)
+
+$(obj)/skbuff.o: $(obj)/dropreason_str.h
+
diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 41cac0e4834e..4ad1decce724 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c
@@ -48,19 +48,6 @@ 
 static int trace_state = TRACE_OFF;
 static bool monitor_hw;
 
-#undef EM
-#undef EMe
-
-#define EM(a, b)	[a] = #b,
-#define EMe(a, b)	[a] = #b
-
-/* drop_reasons is used to translate 'enum skb_drop_reason' to string,
- * which is reported to user space.
- */
-static const char * const drop_reasons[] = {
-	TRACE_SKB_DROP_REASON
-};
-
 /* net_dm_mutex
  *
  * An overall lock guarding every operation coming from userspace.
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 1d10bb4adec1..a4f4d3316a03 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -82,6 +82,7 @@ 
 
 #include "dev.h"
 #include "sock_destructor.h"
+#include "dropreason_str.h"
 
 struct kmem_cache *skbuff_head_cache __ro_after_init;
 static struct kmem_cache *skbuff_fclone_cache __ro_after_init;
@@ -91,6 +92,17 @@  static struct kmem_cache *skbuff_ext_cache __ro_after_init;
 int sysctl_max_skb_frags __read_mostly = MAX_SKB_FRAGS;
 EXPORT_SYMBOL(sysctl_max_skb_frags);
 
+/* drop_reasons is used to translate 'enum skb_drop_reason' to string,
+ * which is reported to user space.
+ */
+const char * const drop_reasons[] = {
+#undef FN
+#define FN(name) [SKB_DROP_REASON_##name] = #name,
+	__DEFINE_SKB_DROP_REASON(FN)
+#undef FN
+};
+EXPORT_SYMBOL(drop_reasons);
+
 /**
  *	skb_panic - private function for out-of-line support
  *	@skb:	buffer