[PATCH net-next,v2,0/2] net: update netdev_rx_csum_fault() print dump only once

[PATCH net-next,v2,0/2] net: update netdev_rx_csum_fault() print dump only once

[Tanner Love]

From: Tanner Love <tannerlove@google.com>

First patch implements DO_ONCE_LITE to abstract uses of the ".data.once"
trick. It is defined in its own, new header file  -- rather than
alongside the existing DO_ONCE in include/linux/once.h -- because
include/linux/once.h includes include/linux/jump_label.h, and this
causes the build to break for some architectures if
include/linux/once.h is included in include/linux/printk.h or
include/asm-generic/bug.h.

Second patch uses DO_ONCE_LITE in netdev_rx_csum_fault to print dump
only once.

This is a v2 of https://patchwork.kernel.org/project/netdevbpf/patch/20210422194738.2175542-2-tannerlove.kernel@gmail.com/

Tanner Love (2):
  once: implement DO_ONCE_LITE for non-fast-path "do once" functionality
  net: update netdev_rx_csum_fault() print dump only once

 fs/xfs/xfs_message.h      | 13 +++----------
 include/asm-generic/bug.h | 37 +++++++------------------------------
 include/linux/once_lite.h | 24 ++++++++++++++++++++++++
 include/linux/printk.h    | 23 +++--------------------
 kernel/trace/trace.h      | 13 +++----------
 net/core/dev.c            | 14 +++++++++-----
 6 files changed, 49 insertions(+), 75 deletions(-)
 create mode 100644 include/linux/once_lite.h

-- 
2.32.0.rc0.204.g9fa02ecfa5-goog

[PATCH net-next,v2,1/2] once: implement DO_ONCE_LITE for non-fast-path "do once" functionality

[Tanner Love]

From: Tanner Love <tannerlove@google.com>

Certain uses of "do once" functionality reside outside of fast path,
and so do not require jump label patching via static keys, making
existing DO_ONCE undesirable in such cases.

Replace uses of __section(".data.once") with DO_ONCE_LITE(_IF)?

[ i386 build warnings ]
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Tanner Love <tannerlove@google.com>
Acked-by: Eric Dumazet <edumazet@google.com>
Acked-by: Mahesh Bandewar <maheshb@google.com>
---
 fs/xfs/xfs_message.h      | 13 +++----------
 include/asm-generic/bug.h | 37 +++++++------------------------------
 include/linux/once_lite.h | 24 ++++++++++++++++++++++++
 include/linux/printk.h    | 23 +++--------------------
 kernel/trace/trace.h      | 13 +++----------
 5 files changed, 40 insertions(+), 70 deletions(-)
 create mode 100644 include/linux/once_lite.h

diff --git a/fs/xfs/xfs_message.h b/fs/xfs/xfs_message.h
index 3c392b1512ac..5daf73f21509 100644
--- a/fs/xfs/xfs_message.h
+++ b/fs/xfs/xfs_message.h
@@ -2,6 +2,8 @@
 #ifndef __XFS_MESSAGE_H
 #define __XFS_MESSAGE_H 1
 
+#include <linux/once_lite.h>
+
 struct xfs_mount;
 
 extern __printf(2, 3)
@@ -41,16 +43,7 @@ do {									\
 } while (0)
 
 #define xfs_printk_once(func, dev, fmt, ...)			\
-({								\
-	static bool __section(".data.once") __print_once;	\
-	bool __ret_print_once = !__print_once; 			\
-								\
-	if (!__print_once) {					\
-		__print_once = true;				\
-		func(dev, fmt, ##__VA_ARGS__);			\
-	}							\
-	unlikely(__ret_print_once);				\
-})
+	DO_ONCE_LITE(func, dev, fmt, ##__VA_ARGS__)
 
 #define xfs_emerg_ratelimited(dev, fmt, ...)				\
 	xfs_printk_ratelimited(xfs_emerg, dev, fmt, ##__VA_ARGS__)
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index b402494883b6..bafc51f483c4 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -4,6 +4,7 @@
 
 #include <linux/compiler.h>
 #include <linux/instrumentation.h>
+#include <linux/once_lite.h>
 
 #define CUT_HERE		"------------[ cut here ]------------\n"
 
@@ -140,39 +141,15 @@ void __warn(const char *file, int line, void *caller, unsigned taint,
 })
 
 #ifndef WARN_ON_ONCE
-#define WARN_ON_ONCE(condition)	({				\
-	static bool __section(".data.once") __warned;		\
-	int __ret_warn_once = !!(condition);			\
-								\
-	if (unlikely(__ret_warn_once && !__warned)) {		\
-		__warned = true;				\
-		WARN_ON(1);					\
-	}							\
-	unlikely(__ret_warn_once);				\
-})
+#define WARN_ON_ONCE(condition)					\
+	DO_ONCE_LITE_IF(condition, WARN_ON, 1)
 #endif
 
-#define WARN_ONCE(condition, format...)	({			\
-	static bool __section(".data.once") __warned;		\
-	int __ret_warn_once = !!(condition);			\
-								\
-	if (unlikely(__ret_warn_once && !__warned)) {		\
-		__warned = true;				\
-		WARN(1, format);				\
-	}							\
-	unlikely(__ret_warn_once);				\
-})
+#define WARN_ONCE(condition, format...)				\
+	DO_ONCE_LITE_IF(condition, WARN, 1, format)
 
-#define WARN_TAINT_ONCE(condition, taint, format...)	({	\
-	static bool __section(".data.once") __warned;		\
-	int __ret_warn_once = !!(condition);			\
-								\
-	if (unlikely(__ret_warn_once && !__warned)) {		\
-		__warned = true;				\
-		WARN_TAINT(1, taint, format);			\
-	}							\
-	unlikely(__ret_warn_once);				\
-})
+#define WARN_TAINT_ONCE(condition, taint, format...)		\
+	DO_ONCE_LITE_IF(condition, WARN_TAINT, 1, taint, format)
 
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
diff --git a/include/linux/once_lite.h b/include/linux/once_lite.h
new file mode 100644
index 000000000000..861e606b820f
--- /dev/null
+++ b/include/linux/once_lite.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_ONCE_LITE_H
+#define _LINUX_ONCE_LITE_H
+
+#include <linux/types.h>
+
+/* Call a function once. Similar to DO_ONCE(), but does not use jump label
+ * patching via static keys.
+ */
+#define DO_ONCE_LITE(func, ...)						\
+	DO_ONCE_LITE_IF(true, func, ##__VA_ARGS__)
+#define DO_ONCE_LITE_IF(condition, func, ...)				\
+	({								\
+		static bool __section(".data.once") __already_done;	\
+		bool __ret_do_once = !!(condition);			\
+									\
+		if (unlikely(__ret_do_once && !__already_done)) {	\
+			__already_done = true;				\
+			func(__VA_ARGS__);				\
+		}							\
+		unlikely(__ret_do_once);				\
+	})
+
+#endif /* _LINUX_ONCE_LITE_H */
diff --git a/include/linux/printk.h b/include/linux/printk.h
index fe7eb2351610..885379a1c9a1 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -8,6 +8,7 @@
 #include <linux/linkage.h>
 #include <linux/cache.h>
 #include <linux/ratelimit_types.h>
+#include <linux/once_lite.h>
 
 extern const char linux_banner[];
 extern const char linux_proc_banner[];
@@ -436,27 +437,9 @@ extern int kptr_restrict;
 
 #ifdef CONFIG_PRINTK
 #define printk_once(fmt, ...)					\
-({								\
-	static bool __section(".data.once") __print_once;	\
-	bool __ret_print_once = !__print_once;			\
-								\
-	if (!__print_once) {					\
-		__print_once = true;				\
-		printk(fmt, ##__VA_ARGS__);			\
-	}							\
-	unlikely(__ret_print_once);				\
-})
+	DO_ONCE_LITE(printk, fmt, ##__VA_ARGS__)
 #define printk_deferred_once(fmt, ...)				\
-({								\
-	static bool __section(".data.once") __print_once;	\
-	bool __ret_print_once = !__print_once;			\
-								\
-	if (!__print_once) {					\
-		__print_once = true;				\
-		printk_deferred(fmt, ##__VA_ARGS__);		\
-	}							\
-	unlikely(__ret_print_once);				\
-})
+	DO_ONCE_LITE(printk_deferred, fmt, ##__VA_ARGS__)
 #else
 #define printk_once(fmt, ...)					\
 	no_printk(fmt, ##__VA_ARGS__)
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index cd80d046c7a5..d5d8c088a55d 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -20,6 +20,7 @@
 #include <linux/irq_work.h>
 #include <linux/workqueue.h>
 #include <linux/ctype.h>
+#include <linux/once_lite.h>
 
 #ifdef CONFIG_FTRACE_SYSCALLS
 #include <asm/unistd.h>		/* For NR_SYSCALLS	     */
@@ -99,16 +100,8 @@ enum trace_type {
 #include "trace_entries.h"
 
 /* Use this for memory failure errors */
-#define MEM_FAIL(condition, fmt, ...) ({			\
-	static bool __section(".data.once") __warned;		\
-	int __ret_warn_once = !!(condition);			\
-								\
-	if (unlikely(__ret_warn_once && !__warned)) {		\
-		__warned = true;				\
-		pr_err("ERROR: " fmt, ##__VA_ARGS__);		\
-	}							\
-	unlikely(__ret_warn_once);				\
-})
+#define MEM_FAIL(condition, fmt, ...)					\
+	DO_ONCE_LITE_IF(condition, pr_err, "ERROR: " fmt, ##__VA_ARGS__)
 
 /*
  * syscalls are special, and need special handling, this is why
-- 
2.32.0.rc0.204.g9fa02ecfa5-goog

[PATCH net-next,v2,2/2] net: update netdev_rx_csum_fault() print dump only once

[Tanner Love]

From: Tanner Love <tannerlove@google.com>

Printing this stack dump multiple times does not provide additional
useful information, and consumes time in the data path. Printing once
is sufficient.

Signed-off-by: Tanner Love <tannerlove@google.com>
Acked-by: Eric Dumazet <edumazet@google.com>
Acked-by: Mahesh Bandewar <maheshb@google.com>
---
 net/core/dev.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index febb23708184..af2d109b7266 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -148,6 +148,7 @@
 #include <net/devlink.h>
 #include <linux/pm_runtime.h>
 #include <linux/prandom.h>
+#include <linux/once_lite.h>
 
 #include "net-sysfs.h"
 
@@ -3487,13 +3488,16 @@ EXPORT_SYMBOL(__skb_gso_segment);
 
 /* Take action when hardware reception checksum errors are detected. */
 #ifdef CONFIG_BUG
+static void do_netdev_rx_csum_fault(struct net_device *dev, struct sk_buff *skb)
+{
+	pr_err("%s: hw csum failure\n", dev ? dev->name : "<unknown>");
+	skb_dump(KERN_ERR, skb, true);
+	dump_stack();
+}
+
 void netdev_rx_csum_fault(struct net_device *dev, struct sk_buff *skb)
 {
-	if (net_ratelimit()) {
-		pr_err("%s: hw csum failure\n", dev ? dev->name : "<unknown>");
-		skb_dump(KERN_ERR, skb, true);
-		dump_stack();
-	}
+	DO_ONCE_LITE(do_netdev_rx_csum_fault, dev, skb);
 }
 EXPORT_SYMBOL(netdev_rx_csum_fault);
 #endif
-- 
2.32.0.rc0.204.g9fa02ecfa5-goog

Re: [PATCH net-next,v2,1/2] once: implement DO_ONCE_LITE for non-fast-path "do once" functionality

[Jakub Kicinski]

On Wed, 26 May 2021 19:13:35 -0400 Tanner Love wrote:
> From: Tanner Love <tannerlove@google.com>
> 
> Certain uses of "do once" functionality reside outside of fast path,
> and so do not require jump label patching via static keys, making
> existing DO_ONCE undesirable in such cases.
> 
> Replace uses of __section(".data.once") with DO_ONCE_LITE(_IF)?
> 
> [ i386 build warnings ]
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Tanner Love <tannerlove@google.com>
> Acked-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Mahesh Bandewar <maheshb@google.com>
> ---
>  fs/xfs/xfs_message.h      | 13 +++----------
>  include/asm-generic/bug.h | 37 +++++++------------------------------
>  include/linux/once_lite.h | 24 ++++++++++++++++++++++++
>  include/linux/printk.h    | 23 +++--------------------
>  kernel/trace/trace.h      | 13 +++----------

You need to cast a wider net for the CC list here. This is not all
networking code. You should also probably CC LKML on general changes
like this.

>  #define xfs_printk_once(func, dev, fmt, ...)			\
> -({								\
> -	static bool __section(".data.once") __print_once;	\
> -	bool __ret_print_once = !__print_once; 			\
> -								\
> -	if (!__print_once) {					\
> -		__print_once = true;				\
> -		func(dev, fmt, ##__VA_ARGS__);			\
> -	}							\
> -	unlikely(__ret_print_once);				\
> -})
> +	DO_ONCE_LITE(func, dev, fmt, ##__VA_ARGS__)

>  #ifdef CONFIG_PRINTK
>  #define printk_once(fmt, ...)					\
> -({								\
> -	static bool __section(".data.once") __print_once;	\
> -	bool __ret_print_once = !__print_once;			\
> -								\
> -	if (!__print_once) {					\
> -		__print_once = true;				\
> -		printk(fmt, ##__VA_ARGS__);			\
> -	}							\
> -	unlikely(__ret_print_once);				\
> -})
> +	DO_ONCE_LITE(printk, fmt, ##__VA_ARGS__)
>  #define printk_deferred_once(fmt, ...)				\
> -({								\
> -	static bool __section(".data.once") __print_once;	\
> -	bool __ret_print_once = !__print_once;			\
> -								\
> -	if (!__print_once) {					\
> -		__print_once = true;				\
> -		printk_deferred(fmt, ##__VA_ARGS__);		\
> -	}							\
> -	unlikely(__ret_print_once);				\
> -})
> +	DO_ONCE_LITE(printk_deferred, fmt, ##__VA_ARGS__)

These are not equivalent to your new macro below. They used to return
if the print was done or not, now they'll always return true. You need
to double check this doesn't break anything and add a note about it in
the commit message.

> +#define DO_ONCE_LITE_IF(condition, func, ...)				\
> +	({								\
> +		static bool __section(".data.once") __already_done;	\
> +		bool __ret_do_once = !!(condition);			\
> +									\
> +		if (unlikely(__ret_do_once && !__already_done)) {	\
> +			__already_done = true;				\
> +			func(__VA_ARGS__);				\
> +		}							\
> +		unlikely(__ret_do_once);				\
> +	})
> +