diff mbox series

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

Message ID 20210526231336.2772011-2-tannerlove.kernel@gmail.com
State Superseded
Headers show
Series [net-next,v2,1/2] once: implement DO_ONCE_LITE for non-fast-path "do once" functionality | expand

Commit Message

Tanner Love May 26, 2021, 11:13 p.m. UTC 
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

Comments

Jakub Kicinski May 27, 2021, 11:27 p.m. UTC | #1 
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);				\

> +	})

> +
diff mbox series

Patch

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