[PATCH] jump_label: jump_label for boot options.

[PATCH] jump_label: jump_label for boot options.

[KAMEZAWA Hiroyuki]

I tried to use jump_label for handling memcg's boot options which sets
global variable true/false and never changes after boot. And found jump_table
is larger than expected. This patch is a trial to allow to place jump_table
in .init section. How do you think ?

This patch is based on linux-next.

==
>From ed8996892c21d4fec642e4fc80bd3ebdd8a48836 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Date: Thu, 1 Dec 2011 11:08:23 +0900
Subject: [PATCH] jump_label for boot options.

Some boot options exists for enable/disable a functionality which cannot be modified
after boot. Using jump_label for such behavior seems atractive but if caller
of statch_branch() is too much, jump_table can be unexpectedly big.

This patch adds static_branch_init_once(), which places its jump_table
in init section and never be updated after boot.

For MODULES, using usual static_branch().

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 arch/mips/include/asm/jump_label.h    |   14 ++++++++++
 arch/powerpc/include/asm/jump_label.h |   15 +++++++++++
 arch/x86/include/asm/jump_label.h     |   15 +++++++++++
 include/asm-generic/vmlinux.lds.h     |    6 ++++
 include/linux/jump_label.h            |   39 +++++++++++++++++++++++++++++
 kernel/jump_label.c                   |   43 ++++++++++++++++++++++++++------
 6 files changed, 124 insertions(+), 8 deletions(-)

diff --git a/arch/mips/include/asm/jump_label.h b/arch/mips/include/asm/jump_label.h
index 1881b31..dd0b8f0 100644
--- a/arch/mips/include/asm/jump_label.h
+++ b/arch/mips/include/asm/jump_label.h
@@ -33,6 +33,20 @@ l_yes:
 	return true;
 }
 
+static __always_inline bool
+arch_static_branch_init_once(struct jump_lable_key *key)
+{
+	asm goto("1:\tnop\n\t"
+		"nop\n\t"
+		".pushsection __jump_table.init, \"aw\"\n\t"
+		WORD_INSN " 1b, %l[l_yes], %0\n\t"
+		".popsection\n\t"
+		: : "i" (key) : : l_yes);
+	return false;
+l_yes:
+	return true;
+}
+
 #endif /* __KERNEL__ */
 
 #ifdef CONFIG_64BIT
diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h
index 938986e..8557f8a 100644
--- a/arch/powerpc/include/asm/jump_label.h
+++ b/arch/powerpc/include/asm/jump_label.h
@@ -30,6 +30,21 @@ l_yes:
 	return true;
 }
 
+static __always_inline bool
+arch_static_branch_init_once(struct jump_label_key *key)
+{
+	asm goto("1:\n\t"
+		 "nop\n\t"
+		 ".pushsection __jump_table.init,  \"aw\"\n\t"
+		 JUMP_ENTRY_TYPE "1b, %l[l_yes], %c0\n\t"
+		 ".popsection \n\t"
+		 : :  "i" (key) : : l_yes);
+	return false;
+l_yes:
+	return true;
+}
+
+
 #ifdef CONFIG_PPC64
 typedef u64 jump_label_t;
 #else
diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index a32b18c..a85ad63 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -25,6 +25,21 @@ l_yes:
 	return true;
 }
 
+static __always_inline
+bool arch_static_branch_init_once(struct jump_label_key *key)
+{
+	asm goto("1:"
+		JUMP_LABEL_INITIAL_NOP
+		".pushsection __jump_table.init, \"aw\" \n\t"
+		_ASM_ALIGN "\n\t"
+		_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
+		".popsection \n\t"
+		: : "i" (key) : : l_yes);
+	return false;
+l_yes:
+	return true;
+}
+
 #endif /* __KERNEL__ */
 
 #ifdef CONFIG_X86_64
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index b5e2e4c..1635f38 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -651,6 +651,11 @@
 		*(.security_initcall.init)				\
 		VMLINUX_SYMBOL(__security_initcall_end) = .;
 
+#define JUMP_LABEL_FOR_INIT						\
+		VMLINUX_SYMBOL(__start___jump_table_at_init) = .;	\
+		*(__jump_table.init)					\
+		VMLINUX_SYMBOL(__stop___jump_table_at_init) = .;
+
 #ifdef CONFIG_BLK_DEV_INITRD
 #define INIT_RAM_FS							\
 	. = ALIGN(4);							\
@@ -799,6 +804,7 @@
 		INIT_CALLS						\
 		CON_INITCALL						\
 		SECURITY_INITCALL					\
+		JUMP_LABEL_FOR_INIT					\
 		INIT_RAM_FS						\
 	}
 
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 388b0d4..b0f37f3 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -38,8 +38,31 @@ static __always_inline bool static_branch(struct jump_label_key *key)
 	return arch_static_branch(key);
 }
 
+/*
+ * Use this when you call static_branch() in __init function or
+ * jump_label is only modified by initcalls. jump_label information
+ * will be discarded after boot. But please be careful. jump_label_key
+ * definition itself should *not* be in __init section because a MODULE
+ * may call static_branch_init_once().
+ *
+ * Useful for changing behavior by boot option.
+ */
+#ifndef MODULE
+static __always_inline bool static_branch_init_once(struct jump_label_key *key)
+{
+	return arch_static_branch_init_once(key);
+}
+#else
+static __always_inline bool static_branch_init_once(struct jump_label_key *key)
+{
+	return static_branch(key);
+}
+#endif
+
 extern struct jump_entry __start___jump_table[];
 extern struct jump_entry __stop___jump_table[];
+extern struct jump_entry __start___jump_table_at_init[];
+extern struct jump_entry __stop___jump_table_at_init[];
 
 extern void jump_label_init(void);
 extern void jump_label_lock(void);
@@ -54,6 +77,12 @@ extern void jump_label_dec(struct jump_label_key *key);
 extern bool jump_label_enabled(struct jump_label_key *key);
 extern void jump_label_apply_nops(struct module *mod);
 
+/*
+ * For jump_label in init section.
+ * This will call jump_label_inc() for usual section, too.
+ */
+extern void jump_label_inc_once(struct jump_label_key *key);
+
 #else  /* !HAVE_JUMP_LABEL */
 
 #include <linux/atomic.h>
@@ -75,6 +104,11 @@ static __always_inline bool static_branch(struct jump_label_key *key)
 	return false;
 }
 
+static __always_inline bool static_branch_init_once(struct jump_label_key *key)
+{
+	return static_branch(key);
+}
+
 static inline void jump_label_inc(struct jump_label_key *key)
 {
 	atomic_inc(&key->enabled);
@@ -102,6 +136,11 @@ static inline int jump_label_apply_nops(struct module *mod)
 {
 	return 0;
 }
+
+static inline void jump_label_inc_once(struct jump_label_key *key)
+{
+	jump_label_inc(key);
+}
 #endif	/* HAVE_JUMP_LABEL */
 
 #endif	/* _LINUX_JUMP_LABEL_H */
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 66ff710..f0e8231 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -134,15 +134,11 @@ static void __jump_label_update(struct jump_label_key *key,
 	}
 }
 
-void __init jump_label_init(void)
+void __init jump_label_transform_all(struct jump_entry *iter_start,
+				     struct jump_entry *iter_stop)
 {
-	struct jump_entry *iter_start = __start___jump_table;
-	struct jump_entry *iter_stop = __stop___jump_table;
-	struct jump_label_key *key = NULL;
 	struct jump_entry *iter;
-
-	jump_label_lock();
-	jump_label_sort_entries(iter_start, iter_stop);
+	struct jump_label_key *key = NULL;
 
 	for (iter = iter_start; iter < iter_stop; iter++) {
 		struct jump_label_key *iterk;
@@ -159,6 +155,24 @@ void __init jump_label_init(void)
 		key->next = NULL;
 #endif
 	}
+
+}
+
+
+void __init jump_label_init(void)
+{
+	struct jump_entry *iter_start = __start___jump_table;
+	struct jump_entry *iter_stop = __stop___jump_table;
+
+
+	jump_label_lock();
+	jump_label_sort_entries(iter_start, iter_stop);
+	jump_label_transform_all(iter_start, iter_stop);
+	/* update jump_table in .init section */
+	iter_start = __start___jump_table_at_init;
+	iter_stop = __stop___jump_table_at_init;
+	jump_label_sort_entries(iter_start, iter_stop);
+	jump_label_transform_all(iter_start, iter_stop);
 	jump_label_unlock();
 }
 
@@ -381,7 +395,8 @@ int jump_label_text_reserved(void *start, void *end)
 
 static void jump_label_update(struct jump_label_key *key, int enable)
 {
-	struct jump_entry *entry = key->entries, *stop = __stop___jump_table;
+	struct jump_entry *entry = key->entries;
+	struct jump_entry *stop = __stop___jump_table;
 
 #ifdef CONFIG_MODULES
 	struct module *mod = __module_address((jump_label_t)key);
@@ -396,4 +411,16 @@ static void jump_label_update(struct jump_label_key *key, int enable)
 		__jump_label_update(key, entry, stop, enable);
 }
 
+/* for boot options */
+void __init jump_label_init_once(struct jump_label_key *key)
+{
+	struct jump_entry *entry = key->entries;
+	struct jump_entry *stop = __stop___jump_table_at_init;
+
+	if (entry)
+		__jump_label_update(key, entry, stop, JUMP_LABEL_ENABLE);
+
+	jump_label_inc(key);
+}
+
 #endif
-- 
1.7.4.1

Re: [PATCH] jump_label: jump_label for boot options.

[Steven Rostedt]

On Thu, 2011-12-01 at 11:53 +0900, KAMEZAWA Hiroyuki wrote:
> I tried to use jump_label for handling memcg's boot options which sets
> global variable true/false and never changes after boot. And found jump_table
> is larger than expected. This patch is a trial to allow to place jump_table
> in .init section. How do you think ?
> 
> This patch is based on linux-next.
> 
> ==
> >From ed8996892c21d4fec642e4fc80bd3ebdd8a48836 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Thu, 1 Dec 2011 11:08:23 +0900
> Subject: [PATCH] jump_label for boot options.
> 
> Some boot options exists for enable/disable a functionality which cannot be modified
> after boot. Using jump_label for such behavior seems atractive but if caller
> of statch_branch() is too much, jump_table can be unexpectedly big.

s/statch/static/

> 
> This patch adds static_branch_init_once(), which places its jump_table
> in init section and never be updated after boot.

I don't like the name static_branch_init_once(). Although I suck at
picking names myself :-p  Maybe just remove the 'init'.
static_branch_once(), or remove the 'once'. static_branch_init()

Your name may be the best, but I'm hoping another name will come up.
static_branch_init_once() just doesn't seem right.


> For MODULES, using usual static_branch().

Why not for modules? It can be forced at module load time, and then
removed.


I also wonder if this could be used for other things not just init.
Maybe at boot up you can determine what hardware is being used, and then
decided which path to take. Things that depend on cpuid and such might
be able to do such a thing.


> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  arch/mips/include/asm/jump_label.h    |   14 ++++++++++
>  arch/powerpc/include/asm/jump_label.h |   15 +++++++++++
>  arch/x86/include/asm/jump_label.h     |   15 +++++++++++
>  include/asm-generic/vmlinux.lds.h     |    6 ++++
>  include/linux/jump_label.h            |   39 +++++++++++++++++++++++++++++
>  kernel/jump_label.c                   |   43 ++++++++++++++++++++++++++------
>  6 files changed, 124 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/mips/include/asm/jump_label.h b/arch/mips/include/asm/jump_label.h
> index 1881b31..dd0b8f0 100644
> --- a/arch/mips/include/asm/jump_label.h
> +++ b/arch/mips/include/asm/jump_label.h
> @@ -33,6 +33,20 @@ l_yes:
>  	return true;
>  }
>  
> +static __always_inline bool
> +arch_static_branch_init_once(struct jump_lable_key *key)
> +{
> +	asm goto("1:\tnop\n\t"
> +		"nop\n\t"
> +		".pushsection __jump_table.init, \"aw\"\n\t"
> +		WORD_INSN " 1b, %l[l_yes], %0\n\t"
> +		".popsection\n\t"
> +		: : "i" (key) : : l_yes);
> +	return false;
> +l_yes:
> +	return true;
> +}
> +
>  #endif /* __KERNEL__ */
>  
>  #ifdef CONFIG_64BIT
> diff --git a/arch/powerpc/include/asm/jump_label.h b/arch/powerpc/include/asm/jump_label.h
> index 938986e..8557f8a 100644
> --- a/arch/powerpc/include/asm/jump_label.h
> +++ b/arch/powerpc/include/asm/jump_label.h
> @@ -30,6 +30,21 @@ l_yes:
>  	return true;
>  }
>  
> +static __always_inline bool
> +arch_static_branch_init_once(struct jump_label_key *key)
> +{
> +	asm goto("1:\n\t"
> +		 "nop\n\t"
> +		 ".pushsection __jump_table.init,  \"aw\"\n\t"
> +		 JUMP_ENTRY_TYPE "1b, %l[l_yes], %c0\n\t"
> +		 ".popsection \n\t"
> +		 : :  "i" (key) : : l_yes);
> +	return false;
> +l_yes:
> +	return true;
> +}
> +
> +
>  #ifdef CONFIG_PPC64
>  typedef u64 jump_label_t;
>  #else
> diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
> index a32b18c..a85ad63 100644
> --- a/arch/x86/include/asm/jump_label.h
> +++ b/arch/x86/include/asm/jump_label.h
> @@ -25,6 +25,21 @@ l_yes:
>  	return true;
>  }
>  
> +static __always_inline
> +bool arch_static_branch_init_once(struct jump_label_key *key)
> +{
> +	asm goto("1:"
> +		JUMP_LABEL_INITIAL_NOP
> +		".pushsection __jump_table.init, \"aw\" \n\t"
> +		_ASM_ALIGN "\n\t"
> +		_ASM_PTR "1b, %l[l_yes], %c0 \n\t"
> +		".popsection \n\t"
> +		: : "i" (key) : : l_yes);
> +	return false;
> +l_yes:
> +	return true;
> +}
> +
>  #endif /* __KERNEL__ */
>  
>  #ifdef CONFIG_X86_64
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index b5e2e4c..1635f38 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -651,6 +651,11 @@
>  		*(.security_initcall.init)				\
>  		VMLINUX_SYMBOL(__security_initcall_end) = .;
>  
> +#define JUMP_LABEL_FOR_INIT						\
> +		VMLINUX_SYMBOL(__start___jump_table_at_init) = .;	\
> +		*(__jump_table.init)					\
> +		VMLINUX_SYMBOL(__stop___jump_table_at_init) = .;
> +
>  #ifdef CONFIG_BLK_DEV_INITRD
>  #define INIT_RAM_FS							\
>  	. = ALIGN(4);							\
> @@ -799,6 +804,7 @@
>  		INIT_CALLS						\
>  		CON_INITCALL						\
>  		SECURITY_INITCALL					\
> +		JUMP_LABEL_FOR_INIT					\
>  		INIT_RAM_FS						\
>  	}
>  
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index 388b0d4..b0f37f3 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -38,8 +38,31 @@ static __always_inline bool static_branch(struct jump_label_key *key)
>  	return arch_static_branch(key);
>  }
>  
> +/*
> + * Use this when you call static_branch() in __init function or
> + * jump_label is only modified by initcalls. jump_label information
> + * will be discarded after boot. But please be careful. jump_label_key
> + * definition itself should *not* be in __init section because a MODULE
> + * may call static_branch_init_once().
> + *
> + * Useful for changing behavior by boot option.
> + */
> +#ifndef MODULE
> +static __always_inline bool static_branch_init_once(struct jump_label_key *key)
> +{
> +	return arch_static_branch_init_once(key);
> +}
> +#else
> +static __always_inline bool static_branch_init_once(struct jump_label_key *key)
> +{
> +	return static_branch(key);
> +}
> +#endif

I still think modules should be able to do this, and then just remove it
later.

> +
>  extern struct jump_entry __start___jump_table[];
>  extern struct jump_entry __stop___jump_table[];
> +extern struct jump_entry __start___jump_table_at_init[];
> +extern struct jump_entry __stop___jump_table_at_init[];
>  
>  extern void jump_label_init(void);
>  extern void jump_label_lock(void);
> @@ -54,6 +77,12 @@ extern void jump_label_dec(struct jump_label_key *key);
>  extern bool jump_label_enabled(struct jump_label_key *key);
>  extern void jump_label_apply_nops(struct module *mod);
>  
> +/*
> + * For jump_label in init section.
> + * This will call jump_label_inc() for usual section, too.
> + */
> +extern void jump_label_inc_once(struct jump_label_key *key);
> +
>  #else  /* !HAVE_JUMP_LABEL */
>  
>  #include <linux/atomic.h>
> @@ -75,6 +104,11 @@ static __always_inline bool static_branch(struct jump_label_key *key)
>  	return false;
>  }
>  
> +static __always_inline bool static_branch_init_once(struct jump_label_key *key)
> +{
> +	return static_branch(key);
> +}
> +
>  static inline void jump_label_inc(struct jump_label_key *key)
>  {
>  	atomic_inc(&key->enabled);
> @@ -102,6 +136,11 @@ static inline int jump_label_apply_nops(struct module *mod)
>  {
>  	return 0;
>  }
> +
> +static inline void jump_label_inc_once(struct jump_label_key *key)
> +{
> +	jump_label_inc(key);
> +}
>  #endif	/* HAVE_JUMP_LABEL */
>  
>  #endif	/* _LINUX_JUMP_LABEL_H */
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index 66ff710..f0e8231 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -134,15 +134,11 @@ static void __jump_label_update(struct jump_label_key *key,
>  	}
>  }
>  
> -void __init jump_label_init(void)
> +void __init jump_label_transform_all(struct jump_entry *iter_start,
> +				     struct jump_entry *iter_stop)
>  {
> -	struct jump_entry *iter_start = __start___jump_table;
> -	struct jump_entry *iter_stop = __stop___jump_table;
> -	struct jump_label_key *key = NULL;
>  	struct jump_entry *iter;
> -
> -	jump_label_lock();
> -	jump_label_sort_entries(iter_start, iter_stop);
> +	struct jump_label_key *key = NULL;
>  
>  	for (iter = iter_start; iter < iter_stop; iter++) {
>  		struct jump_label_key *iterk;
> @@ -159,6 +155,24 @@ void __init jump_label_init(void)
>  		key->next = NULL;
>  #endif
>  	}
> +
> +}
> +
> +
> +void __init jump_label_init(void)
> +{
> +	struct jump_entry *iter_start = __start___jump_table;
> +	struct jump_entry *iter_stop = __stop___jump_table;
> +
> +
> +	jump_label_lock();
> +	jump_label_sort_entries(iter_start, iter_stop);
> +	jump_label_transform_all(iter_start, iter_stop);

Nit, I'd add a space here.

-- Steve

> +	/* update jump_table in .init section */
> +	iter_start = __start___jump_table_at_init;
> +	iter_stop = __stop___jump_table_at_init;
> +	jump_label_sort_entries(iter_start, iter_stop);
> +	jump_label_transform_all(iter_start, iter_stop);
>  	jump_label_unlock();
>  }
>  
> @@ -381,7 +395,8 @@ int jump_label_text_reserved(void *start, void *end)
>  
>  static void jump_label_update(struct jump_label_key *key, int enable)
>  {
> -	struct jump_entry *entry = key->entries, *stop = __stop___jump_table;
> +	struct jump_entry *entry = key->entries;
> +	struct jump_entry *stop = __stop___jump_table;
>  
>  #ifdef CONFIG_MODULES
>  	struct module *mod = __module_address((jump_label_t)key);
> @@ -396,4 +411,16 @@ static void jump_label_update(struct jump_label_key *key, int enable)
>  		__jump_label_update(key, entry, stop, enable);
>  }
>  
> +/* for boot options */
> +void __init jump_label_init_once(struct jump_label_key *key)
> +{
> +	struct jump_entry *entry = key->entries;
> +	struct jump_entry *stop = __stop___jump_table_at_init;
> +
> +	if (entry)
> +		__jump_label_update(key, entry, stop, JUMP_LABEL_ENABLE);
> +
> +	jump_label_inc(key);
> +}
> +
>  #endif

Re: [PATCH] jump_label: jump_label for boot options.

[Peter Zijlstra]

On Thu, 2011-12-01 at 11:53 +0900, KAMEZAWA Hiroyuki wrote:
> I tried to use jump_label for handling memcg's boot options which sets
> global variable true/false and never changes after boot. And found jump_table
> is larger than expected. This patch is a trial to allow to place jump_table
> in .init section. How do you think ? 

I think its much saner to go help hnaz remove this shadow page table and
thus the need for this stupid boot time param.

Re: [PATCH] jump_label: jump_label for boot options.

[Peter Zijlstra]

On Thu, 2011-12-01 at 09:48 -0500, Steven Rostedt wrote:
> 
> I also wonder if this could be used for other things not just init.
> Maybe at boot up you can determine what hardware is being used, and then
> decided which path to take. Things that depend on cpuid and such might
> be able to do such a thing. 

That sounds suspiciously like alternatives..

Re: [PATCH] jump_label: jump_label for boot options.

[Peter Zijlstra]

On Thu, 2011-12-01 at 09:48 -0500, Steven Rostedt wrote:
> > For MODULES, using usual static_branch().
> 
> Why not for modules? It can be forced at module load time, and then
> removed. 

Because modules suck, its official now: http://lwn.net/Articles/469801/

Re: [PATCH] jump_label: jump_label for boot options.

[Jason Baron]

On Thu, Dec 01, 2011 at 11:53:53AM +0900, KAMEZAWA Hiroyuki wrote:
> I tried to use jump_label for handling memcg's boot options which sets
> global variable true/false and never changes after boot. And found jump_table
> is larger than expected. This patch is a trial to allow to place jump_table
> in .init section. How do you think ?
> 

Remeber too, that 'static_branch()' is inherently biased. That is, the
'false' path is assumed to be the the most likely path. Thus, the 'true'
path is move out-of-line. Thus, if the 'true' branch is potentially
used all the time, we would want to make sure that the savings of not
having to check a variable is still worth it. I should probably rename
static_branch() -> 'static_branch_default_false()' to make that clear.

Maybe we need an unbiased static_branch() too, but I'm not sure excatly
how to implement it...

Thanks,

-Jason

Re: [PATCH] jump_label: jump_label for boot options.

[Steven Rostedt]

On Thu, 2011-12-01 at 16:06 +0100, Peter Zijlstra wrote:
> On Thu, 2011-12-01 at 09:48 -0500, Steven Rostedt wrote:
> > 
> > I also wonder if this could be used for other things not just init.
> > Maybe at boot up you can determine what hardware is being used, and then
> > decided which path to take. Things that depend on cpuid and such might
> > be able to do such a thing. 
> 
> That sounds suspiciously like alternatives..

Hehe, you caught me ;)

-- Steve

Re: [PATCH] jump_label: jump_label for boot options.

[Peter Zijlstra]

On Thu, 2011-12-01 at 10:40 -0500, Jason Baron wrote:
> I should probably rename
> static_branch() -> 'static_branch_default_false()' to make that clear.

The rename doesn't really make sense until you can also provide
static_branch_true() also:

> Maybe we need an unbiased static_branch() too, but I'm not sure excatly
> how to implement it... 

I'd think the same way we got in this situation in the first place, go
kick some GCC people ;-)

Re: [PATCH] jump_label: jump_label for boot options.

[Jason Baron]

On Thu, Dec 01, 2011 at 05:28:18PM +0100, Peter Zijlstra wrote:
> On Thu, 2011-12-01 at 10:40 -0500, Jason Baron wrote:
> > I should probably rename
> > static_branch() -> 'static_branch_default_false()' to make that clear.
> 
> The rename doesn't really make sense until you can also provide
> static_branch_true() also:

I think its just a matter of reversing the true and false returns.
That is, instead of:

static __always_inline bool arch_static_branch_default_false(struct jump_label_key
*key)
{
        asm goto("1:"
                JUMP_LABEL_INITIAL_NOP
                ".pushsection __jump_table,  \"aw\" \n\t"
                _ASM_ALIGN "\n\t"
                _ASM_PTR "1b, %l[l_yes], %c0 \n\t"
                ".popsection \n\t"
                : :  "i" (key) : : l_yes);
        return false;
l_yes:
        return true;
}

We just have (untested):

static __always_inline bool arch_static_branch_default_true(struct jump_label_key
*key)
{
        asm goto("1:"
                JUMP_LABEL_INITIAL_NOP
                ".pushsection __jump_table,  \"aw\" \n\t"
                _ASM_ALIGN "\n\t"
                _ASM_PTR "1b, %l[l_no], %c0 \n\t"
                ".popsection \n\t"
                : :  "i" (key) : : l_no);
        return true;
l_no:
        return false;
}

jump_label_inc/dec(), don't need to be changed, they just mean reverse
the branch on 0, 1 transitions. Although using the same key in both
static_branch_true, and static_branch_false, might be confusing. Maybe
we rename jump_label_inc/dec to static_branch_reverse_inc()/dec()?

> 
> > Maybe we need an unbiased static_branch() too, but I'm not sure excatly
> > how to implement it... 
> 
> I'd think the same way we got in this situation in the first place, go
> kick some GCC people ;-)

If you think this is a useful case, I'll try and see how to implement it.
Then we can add static_branch_default_true/false_unbiased() :)

-Jason

Re: [PATCH] jump_label: jump_label for boot options.

[Jason Baron]

On Thu, Dec 01, 2011 at 11:50:09AM -0500, Jason Baron wrote:
> On Thu, Dec 01, 2011 at 05:28:18PM +0100, Peter Zijlstra wrote:
> > On Thu, 2011-12-01 at 10:40 -0500, Jason Baron wrote:
> > > I should probably rename
> > > static_branch() -> 'static_branch_default_false()' to make that clear.
> > 
> > The rename doesn't really make sense until you can also provide
> > static_branch_true() also:
> 
> I think its just a matter of reversing the true and false returns.
> That is, instead of:
> 
> static __always_inline bool arch_static_branch_default_false(struct jump_label_key
> *key)
> {
>         asm goto("1:"
>                 JUMP_LABEL_INITIAL_NOP
>                 ".pushsection __jump_table,  \"aw\" \n\t"
>                 _ASM_ALIGN "\n\t"
>                 _ASM_PTR "1b, %l[l_yes], %c0 \n\t"
>                 ".popsection \n\t"
>                 : :  "i" (key) : : l_yes);
>         return false;
> l_yes:
>         return true;
> }
> 
> We just have (untested):
> 
> static __always_inline bool arch_static_branch_default_true(struct jump_label_key
> *key)
> {
>         asm goto("1:"
>                 JUMP_LABEL_INITIAL_NOP
>                 ".pushsection __jump_table,  \"aw\" \n\t"
>                 _ASM_ALIGN "\n\t"
>                 _ASM_PTR "1b, %l[l_no], %c0 \n\t"
>                 ".popsection \n\t"
>                 : :  "i" (key) : : l_no);
>         return true;
> l_no:
>         return false;
> }
> 
> jump_label_inc/dec(), don't need to be changed, they just mean reverse
> the branch on 0, 1 transitions. Although using the same key in both
> static_branch_true, and static_branch_false, might be confusing. Maybe
> we rename jump_label_inc/dec to static_branch_reverse_inc()/dec()?
> 
> > 
> > > Maybe we need an unbiased static_branch() too, but I'm not sure excatly
> > > how to implement it... 
> > 
> > I'd think the same way we got in this situation in the first place, go
> > kick some GCC people ;-)
> 
> If you think this is a useful case, I'll try and see how to implement it.
> Then we can add static_branch_default_true/false_unbiased() :)
> 
> -Jason

Actually, here's a version to imlement static_branch_true/false, that doesn't
touch any arch code. 


diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 388b0d4..411cd54 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -33,11 +33,16 @@ struct module;
 #define JUMP_LABEL_INIT {ATOMIC_INIT(0), NULL}
 #endif
 
-static __always_inline bool static_branch(struct jump_label_key *key)
+static __always_inline bool static_branch_default_false(struct jump_label_key *key)
 {
 	return arch_static_branch(key);
 }
 
+static __always_inline bool static_branch_default_true(struct jump_label_key *key)
+{
+	return !arch_static_branch(key);
+}
+
 extern struct jump_entry __start___jump_table[];
 extern struct jump_entry __stop___jump_table[];
 
@@ -68,13 +73,20 @@ static __always_inline void jump_label_init(void)
 {
 }
 
-static __always_inline bool static_branch(struct jump_label_key *key)
+static __always_inline bool static_branch_default_false(struct jump_label_key *key)
 {
 	if (unlikely(atomic_read(&key->enabled)))
 		return true;
 	return false;
 }
 
+static __always_inline bool static_branch_default_true(struct jump_label_key *key)
+{
+	if (likely(atomic_read(&key->enabled)))
+		return true;
+	return false;
+}
+
 static inline void jump_label_inc(struct jump_label_key *key)
 {
 	atomic_inc(&key->enabled);
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 1e9ebe5..05a4f92 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -1053,7 +1053,7 @@ perf_sw_event(u32 event_id, u64 nr, struct pt_regs *regs, u64 addr)
 {
 	struct pt_regs hot_regs;
 
-	if (static_branch(&perf_swevent_enabled[event_id])) {
+	if (static_branch_default_false(&perf_swevent_enabled[event_id])) {
 		if (!regs) {
 			perf_fetch_caller_regs(&hot_regs);
 			regs = &hot_regs;
@@ -1067,7 +1067,7 @@ extern struct jump_label_key perf_sched_events;
 static inline void perf_event_task_sched_in(struct task_struct *prev,
 					    struct task_struct *task)
 {
-	if (static_branch(&perf_sched_events))
+	if (static_branch_default_false(&perf_sched_events))
 		__perf_event_task_sched_in(prev, task);
 }
 
@@ -1076,7 +1076,7 @@ static inline void perf_event_task_sched_out(struct task_struct *prev,
 {
 	perf_sw_event(PERF_COUNT_SW_CONTEXT_SWITCHES, 1, NULL, 0);
 
-	if (static_branch(&perf_sched_events))
+	if (static_branch_default_false(&perf_sched_events))
 		__perf_event_task_sched_out(prev, next);
 }
 
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index df0a779..31c5d64 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -143,7 +143,7 @@ static inline void tracepoint_synchronize_unregister(void)
 	extern struct tracepoint __tracepoint_##name;			\
 	static inline void trace_##name(proto)				\
 	{								\
-		if (static_branch(&__tracepoint_##name.key))		\
+		if (static_branch_default_false(&__tracepoint_##name.key))\
 			__DO_TRACE(&__tracepoint_##name,		\
 				TP_PROTO(data_proto),			\
 				TP_ARGS(data_args),			\
diff --git a/kernel/sched.c b/kernel/sched.c
index 0e9344a..c818b01 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -2129,7 +2129,7 @@ static void update_rq_clock_task(struct rq *rq, s64 delta)
 	delta -= irq_delta;
 #endif
 #ifdef CONFIG_PARAVIRT_TIME_ACCOUNTING
-	if (static_branch((&paravirt_steal_rq_enabled))) {
+	if (static_branch_default_false((&paravirt_steal_rq_enabled))) {
 		u64 st;
 
 		steal = paravirt_steal_clock(cpu_of(rq));
@@ -4001,7 +4001,7 @@ void account_idle_time(cputime_t cputime)
 static __always_inline bool steal_account_process_tick(void)
 {
 #ifdef CONFIG_PARAVIRT
-	if (static_branch(&paravirt_steal_enabled)) {
+	if (static_branch_default_false(&paravirt_steal_enabled)) {
 		u64 steal, st = 0;
 
 		steal = paravirt_steal_clock(smp_processor_id());

Re: [PATCH] jump_label: jump_label for boot options.

[Peter Zijlstra]

On Thu, 2011-12-01 at 11:50 -0500, Jason Baron wrote:

> I think its just a matter of reversing the true and false returns.
> That is, instead of:

that's the same as !static_branch()

> jump_label_inc/dec(), don't need to be changed, they just mean reverse
> the branch on 0, 1 transitions. Although using the same key in both
> static_branch_true, and static_branch_false, might be confusing. Maybe
> we rename jump_label_inc/dec to static_branch_reverse_inc()/dec()?

Right, that's the problem really. That makes it impossible to make the
control code generic.

What I'd want is something that doesn't out-of-line the branch, is
possibly enabled by default, but has the same inc/dec behaviour, not the
reversed.

---
Subject: sched: Use jump_labels for sched_feat
From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Wed Jul 06 14:20:14 CEST 2011


Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 kernel/sched/core.c     |   34 +++++++++++++++++++++++++++++-----
 kernel/sched/features.h |   30 +++++++++++++++---------------
 kernel/sched/sched.h    |   27 +++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 20 deletions(-)

Index: linux-2.6/kernel/sched/core.c
===================================================================
--- linux-2.6.orig/kernel/sched/core.c
+++ linux-2.6/kernel/sched/core.c
@@ -149,7 +149,7 @@ static int sched_feat_show(struct seq_fi
 {
 	int i;
 
-	for (i = 0; sched_feat_names[i]; i++) {
+	for (i = 0; i < __SCHED_FEAT_NR; i++) {
 		if (!(sysctl_sched_features & (1UL << i)))
 			seq_puts(m, "NO_");
 		seq_printf(m, "%s ", sched_feat_names[i]);
@@ -182,17 +182,26 @@ sched_feat_write(struct file *filp, cons
 		cmp += 3;
 	}
 
-	for (i = 0; sched_feat_names[i]; i++) {
+	for (i = 0; i < __SCHED_FEAT_NR; i++) {
 		if (strcmp(cmp, sched_feat_names[i]) == 0) {
-			if (neg)
+			if (neg) {
 				sysctl_sched_features &= ~(1UL << i);
-			else
+#ifdef HAVE_JUMP_LABEL
+				if (!jump_label_enabled(&sched_feat_keys[i]))
+					jump_label_inc(&sched_feat_keys[i]);
+#endif
+			} else {
 				sysctl_sched_features |= (1UL << i);
+#ifdef HAVE_JUMP_LABEL
+				if (jump_label_enabled(&sched_feat_keys[i]))
+					jump_label_dec(&sched_feat_keys[i]);
+#endif
+			}
 			break;
 		}
 	}
 
-	if (!sched_feat_names[i])
+	if (i == __SCHED_FEAT_NR)
 		return -EINVAL;
 
 	*ppos += cnt;
@@ -222,7 +231,20 @@ static __init int sched_init_debug(void)
 }
 late_initcall(sched_init_debug);
 
+static __init void sched_init_jump_label(void)
+{
+#ifdef HAVE_JUMP_LABEL
+	int i;
+
+	for (i = 0; i < __SCHED_FEAT_NR; i++) {
+		if (sysctl_sched_features & (1UL << i))
+			jump_label_inc(&sched_feat_keys[i]);
+	}
 #endif
+}
+#else /* CONFIG_SCHED_DEBUG */
+static __init void sched_init_jump_label(void) { }
+#endif /* CONFIG_SCHED_DEBUG */
 
 /*
  * Number of tasks to iterate in a single balance run.
@@ -6748,6 +6770,8 @@ void __init sched_init(void)
 	int i, j;
 	unsigned long alloc_size = 0, ptr;
 
+	sched_init_jump_label();
+
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	alloc_size += 2 * nr_cpu_ids * sizeof(void **);
 #endif
Index: linux-2.6/kernel/sched/features.h
===================================================================
--- linux-2.6.orig/kernel/sched/features.h
+++ linux-2.6/kernel/sched/features.h
@@ -3,13 +3,13 @@
  * them to run sooner, but does not allow tons of sleepers to
  * rip the spread apart.
  */
-SCHED_FEAT(GENTLE_FAIR_SLEEPERS, 1)
+SCHED_FEAT(GENTLE_FAIR_SLEEPERS, true)
 
 /*
  * Place new tasks ahead so that they do not starve already running
  * tasks
  */
-SCHED_FEAT(START_DEBIT, 1)
+SCHED_FEAT(START_DEBIT, true)
 
 /*
  * Based on load and program behaviour, see if it makes sense to place
@@ -17,54 +17,54 @@ SCHED_FEAT(START_DEBIT, 1)
  * improve cache locality. Typically used with SYNC wakeups as
  * generated by pipes and the like, see also SYNC_WAKEUPS.
  */
-SCHED_FEAT(AFFINE_WAKEUPS, 1)
+SCHED_FEAT(AFFINE_WAKEUPS, true)
 
 /*
  * Prefer to schedule the task we woke last (assuming it failed
  * wakeup-preemption), since its likely going to consume data we
  * touched, increases cache locality.
  */
-SCHED_FEAT(NEXT_BUDDY, 0)
+SCHED_FEAT(NEXT_BUDDY, false)
 
 /*
  * Prefer to schedule the task that ran last (when we did
  * wake-preempt) as that likely will touch the same data, increases
  * cache locality.
  */
-SCHED_FEAT(LAST_BUDDY, 1)
+SCHED_FEAT(LAST_BUDDY, true)
 
 /*
  * Consider buddies to be cache hot, decreases the likelyness of a
  * cache buddy being migrated away, increases cache locality.
  */
-SCHED_FEAT(CACHE_HOT_BUDDY, 1)
+SCHED_FEAT(CACHE_HOT_BUDDY, true)
 
 /*
  * Use arch dependent cpu power functions
  */
-SCHED_FEAT(ARCH_POWER, 0)
+SCHED_FEAT(ARCH_POWER, false)
 
-SCHED_FEAT(HRTICK, 0)
-SCHED_FEAT(DOUBLE_TICK, 0)
-SCHED_FEAT(LB_BIAS, 1)
+SCHED_FEAT(HRTICK, false)
+SCHED_FEAT(DOUBLE_TICK, false)
+SCHED_FEAT(LB_BIAS, true)
 
 /*
  * Spin-wait on mutex acquisition when the mutex owner is running on
  * another cpu -- assumes that when the owner is running, it will soon
  * release the lock. Decreases scheduling overhead.
  */
-SCHED_FEAT(OWNER_SPIN, 1)
+SCHED_FEAT(OWNER_SPIN, true)
 
 /*
  * Decrement CPU power based on time not spent running tasks
  */
-SCHED_FEAT(NONTASK_POWER, 1)
+SCHED_FEAT(NONTASK_POWER, true)
 
 /*
  * Queue remote wakeups on the target CPU and process them
  * using the scheduler IPI. Reduces rq->lock contention/bounces.
  */
-SCHED_FEAT(TTWU_QUEUE, 1)
+SCHED_FEAT(TTWU_QUEUE, true)
 
-SCHED_FEAT(FORCE_SD_OVERLAP, 0)
-SCHED_FEAT(RT_RUNTIME_SHARE, 1)
+SCHED_FEAT(FORCE_SD_OVERLAP, false)
+SCHED_FEAT(RT_RUNTIME_SHARE, true)
Index: linux-2.6/kernel/sched/sched.h
===================================================================
--- linux-2.6.orig/kernel/sched/sched.h
+++ linux-2.6/kernel/sched/sched.h
@@ -581,6 +581,7 @@ static inline void __set_task_cpu(struct
  * Tunables that become constants when CONFIG_SCHED_DEBUG is off:
  */
 #ifdef CONFIG_SCHED_DEBUG
+# include <linux/jump_label.h>
 # define const_debug __read_mostly
 #else
 # define const_debug const
@@ -593,11 +594,37 @@ extern const_debug unsigned int sysctl_s
 
 enum {
 #include "features.h"
+	__SCHED_FEAT_NR,
 };
 
 #undef SCHED_FEAT
 
+static __always_inline bool static_branch_true(struct jump_label_key *key)
+{
+	return static_branch(key); /* Not out of line branch. */
+}
+
+static __always_inline bool static_branch_false(struct jump_label_key *key)
+{
+	return static_branch(key); /* Out of line branch. */
+}
+
+#define SCHED_FEAT(name, enabled)					\
+static __always_inline bool static_branch_##name(struct jump_label_key *key) \
+{									\
+	return static_branch_##enabled(key);				\
+}
+
+#include "features.h"
+
+#undef SCHED_FEAT
+
+#if defined(CONFIG_SCHED_DEBUG) && defined(HAVE_JUMP_LABEL)
+static struct jump_label_key sched_feat_keys[__SCHED_FEAT_NR];
+#define sched_feat(x) (static_branch(&sched_feat_keys[__SCHED_FEAT_##x]))
+#else
 #define sched_feat(x) (sysctl_sched_features & (1UL << __SCHED_FEAT_##x))
+#endif
 
 static inline u64 global_rt_period(void)
 {

Re: [PATCH] jump_label: jump_label for boot options.

[Peter Zijlstra]

On Thu, 2011-12-01 at 18:39 +0100, Peter Zijlstra wrote:
> +#define SCHED_FEAT(name, enabled)                                      \
> +static __always_inline bool static_branch_##name(struct jump_label_key *key) \
> +{                                                                      \
> +       return static_branch_##enabled(key);                            \
> +}
> +
> +#include "features.h"
> +
> +#undef SCHED_FEAT
> +
> +#if defined(CONFIG_SCHED_DEBUG) && defined(HAVE_JUMP_LABEL)
> +static struct jump_label_key sched_feat_keys[__SCHED_FEAT_NR];
> +#define sched_feat(x) (static_branch(&sched_feat_keys[__SCHED_FEAT_##x])) 

#define sched_feat(x) static_branch_##x(&sched_feat_keys[__SCHED_FEAT_##x])

of course ;-)

Re: [PATCH] jump_label: jump_label for boot options.

[Peter Zijlstra]

On Thu, 2011-12-01 at 12:16 -0500, Jason Baron wrote:
> +static __always_inline bool static_branch_default_true(struct jump_label_key *key)
> +{
> +       return !arch_static_branch(key);
> +} 

Right, but like said earlier, its useless :/

Re: [PATCH] jump_label: jump_label for boot options.

[Jason Baron]

On Thu, Dec 01, 2011 at 06:39:45PM +0100, Peter Zijlstra wrote:
> On Thu, 2011-12-01 at 11:50 -0500, Jason Baron wrote:
> 
> > I think its just a matter of reversing the true and false returns.
> > That is, instead of:
> 
> that's the same as !static_branch()
> 
> > jump_label_inc/dec(), don't need to be changed, they just mean reverse
> > the branch on 0, 1 transitions. Although using the same key in both
> > static_branch_true, and static_branch_false, might be confusing. Maybe
> > we rename jump_label_inc/dec to static_branch_reverse_inc()/dec()?
> 
> Right, that's the problem really. That makes it impossible to make the
> control code generic.
> 
> What I'd want is something that doesn't out-of-line the branch, is
> possibly enabled by default, but has the same inc/dec behaviour, not the
> reversed.
> 

I think what you have below should work modulo the no out-of-line
branches and the following change:

> -			if (neg)
> +			if (neg) {
>  				sysctl_sched_features &= ~(1UL << i);
> -			else
> +#ifdef HAVE_JUMP_LABEL
> +				if (!jump_label_enabled(&sched_feat_keys[i]))
> +					jump_label_inc(&sched_feat_keys[i]);
> +#endif

I think here its:
			if (jump_label_enabled())
				jump_label_dec();


> +			} else {
>  				sysctl_sched_features |= (1UL << i);
> +#ifdef HAVE_JUMP_LABEL
> +				if (jump_label_enabled(&sched_feat_keys[i]))
> +					jump_label_dec(&sched_feat_keys[i]);
> +#endif
> +			}

Same here:
			if (!jump_label_enabled())
				jump_label_inc()




The inc/dec behavior we have now, in fact will only mess up in the case
where we define 'static_branch_true()'. Because then, in that case the
jump_label_inc() will cause a jump to the false branch. So as long as we
don't introduce 'static_branch_true()' and do an early setting of those
branches which are true via __init code as you have here, I think things are
correct.

Thanks,

-Jason

Re: [PATCH] jump_label: jump_label for boot options.

[Peter Zijlstra]

On Thu, 2011-12-01 at 16:13 -0500, Jason Baron wrote:

> I think what you have below should work modulo the no out-of-line
> branches and the following change:
> 
> > -			if (neg)
> > +			if (neg) {
> >  				sysctl_sched_features &= ~(1UL << i);
> > -			else
> > +#ifdef HAVE_JUMP_LABEL
> > +				if (!jump_label_enabled(&sched_feat_keys[i]))
> > +					jump_label_inc(&sched_feat_keys[i]);
> > +#endif
> 
> I think here its:
> 			if (jump_label_enabled())
> 				jump_label_dec();
> 
> 
> > +			} else {
> >  				sysctl_sched_features |= (1UL << i);
> > +#ifdef HAVE_JUMP_LABEL
> > +				if (jump_label_enabled(&sched_feat_keys[i]))
> > +					jump_label_dec(&sched_feat_keys[i]);
> > +#endif
> > +			}
> 
> Same here:
> 			if (!jump_label_enabled())
> 				jump_label_inc()

Gah, right. An earlier version used !static_branch().

> 
> The inc/dec behavior we have now, in fact will only mess up in the case
> where we define 'static_branch_true()'. Because then, in that case the
> jump_label_inc() will cause a jump to the false branch. So as long as we
> don't introduce 'static_branch_true()' and do an early setting of those
> branches which are true via __init code as you have here, I think things are
> correct.

That's with your definition of static_branch_true(). Not mine :-)

Mine simply doesn't out-of-line the branch, and ideally has a
jump_label_key initializer that initializes to true (1) and has gcc
generate right code.

I can do the initializer, something like the below. What I cannot do is
make gcc generate the enabled case so we don't have the small nop window
(although with the below I'm not sure it really matters).

But the more important point is not getting the branch out-of-line.

---
Index: linux-2.6/include/linux/jump_label.h
===================================================================
--- linux-2.6.orig/include/linux/jump_label.h
+++ linux-2.6/include/linux/jump_label.h
@@ -128,4 +128,6 @@ static inline void jump_label_rate_limit
 }
 #endif	/* HAVE_JUMP_LABEL */
 
+#define jump_label_key_true	((struct jump_label_key){ .enabled = ATOMIC_INIT(1), })
+
 #endif	/* _LINUX_JUMP_LABEL_H */
Index: linux-2.6/kernel/jump_label.c
===================================================================
--- linux-2.6.orig/kernel/jump_label.c
+++ linux-2.6/kernel/jump_label.c
@@ -248,8 +248,13 @@ void jump_label_apply_nops(struct module
 	if (iter_start == iter_stop)
 		return;
 
-	for (iter = iter_start; iter < iter_stop; iter++)
-		arch_jump_label_transform_static(iter, JUMP_LABEL_DISABLE);
+	for (iter = iter_start; iter < iter_stop; iter++) {
+		struct jump_label_key *iterk;
+
+		iterk = (struct jump_label_key *)(unsigned long)iter->key;
+		arch_jump_label_transform_static(iter, jump_label_enabled(iterk) ?
+				JUMP_LABEL_ENABLE : JUMP_LABEL_DISABLE);
+	}
 }
 
 static int jump_label_add_module(struct module *mod)
@@ -289,8 +294,7 @@ static int jump_label_add_module(struct
 		key->next = jlm;
 
 		if (jump_label_enabled(key))
-			__jump_label_update(key, iter, iter_stop,
-					    JUMP_LABEL_ENABLE);
+			__jump_label_update(key, iter, iter_stop, JUMP_LABEL_ENABLE);
 	}
 
 	return 0;

Re: [PATCH] jump_label: jump_label for boot options.

[KAMEZAWA Hiroyuki]

On Thu, 01 Dec 2011 09:48:47 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 2011-12-01 at 11:53 +0900, KAMEZAWA Hiroyuki wrote:
> > I tried to use jump_label for handling memcg's boot options which sets
> > global variable true/false and never changes after boot. And found jump_table
> > is larger than expected. This patch is a trial to allow to place jump_table
> > in .init section. How do you think ?
> > 
> > This patch is based on linux-next.
> > 
> > ==
> > >From ed8996892c21d4fec642e4fc80bd3ebdd8a48836 Mon Sep 17 00:00:00 2001
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Date: Thu, 1 Dec 2011 11:08:23 +0900
> > Subject: [PATCH] jump_label for boot options.
> > 
> > Some boot options exists for enable/disable a functionality which cannot be modified
> > after boot. Using jump_label for such behavior seems atractive but if caller
> > of statch_branch() is too much, jump_table can be unexpectedly big.
> 
> s/statch/static/
> 
will fix.

> > 
> > This patch adds static_branch_init_once(), which places its jump_table
> > in init section and never be updated after boot.
> 
> I don't like the name static_branch_init_once(). Although I suck at
> picking names myself :-p  Maybe just remove the 'init'.
> static_branch_once(), or remove the 'once'. static_branch_init()
> 

ok.

> Your name may be the best, but I'm hoping another name will come up.
> static_branch_init_once() just doesn't seem right.
> 
I don't think my sense of naming is good.

> 
> > For MODULES, using usual static_branch().
> 
> Why not for modules? It can be forced at module load time, and then
> removed.
> 

It can be. I just don't investigate how module's init section is handled.

<snip>


> > +/*
> > + * Use this when you call static_branch() in __init function or
> > + * jump_label is only modified by initcalls. jump_label information
> > + * will be discarded after boot. But please be careful. jump_label_key
> > + * definition itself should *not* be in __init section because a MODULE
> > + * may call static_branch_init_once().
> > + *
> > + * Useful for changing behavior by boot option.
> > + */
> > +#ifndef MODULE
> > +static __always_inline bool static_branch_init_once(struct jump_label_key *key)
> > +{
> > +	return arch_static_branch_init_once(key);
> > +}
> > +#else
> > +static __always_inline bool static_branch_init_once(struct jump_label_key *key)
> > +{
> > +	return static_branch(key);
> > +}
> > +#endif
> 
> I still think modules should be able to do this, and then just remove it
> later.
> 

Hm, ok, I'll look into and prepare an another patch.


> > +
> >  extern struct jump_entry __start___jump_table[];
> >  extern struct jump_entry __stop___jump_table[];
> > +extern struct jump_entry __start___jump_table_at_init[];
> > +extern struct jump_entry __stop___jump_table_at_init[];
> >  
> >  extern void jump_label_init(void);
> >  extern void jump_label_lock(void);
> > @@ -54,6 +77,12 @@ extern void jump_label_dec(struct jump_label_key *key);
> >  extern bool jump_label_enabled(struct jump_label_key *key);
> >  extern void jump_label_apply_nops(struct module *mod);
> >  
> > +/*
> > + * For jump_label in init section.
> > + * This will call jump_label_inc() for usual section, too.
> > + */
> > +extern void jump_label_inc_once(struct jump_label_key *key);
> > +
> >  #else  /* !HAVE_JUMP_LABEL */
> >  
> >  #include <linux/atomic.h>
> > @@ -75,6 +104,11 @@ static __always_inline bool static_branch(struct jump_label_key *key)
> >  	return false;
> >  }
> >  
> > +static __always_inline bool static_branch_init_once(struct jump_label_key *key)
> > +{
> > +	return static_branch(key);
> > +}
> > +
> >  static inline void jump_label_inc(struct jump_label_key *key)
> >  {
> >  	atomic_inc(&key->enabled);
> > @@ -102,6 +136,11 @@ static inline int jump_label_apply_nops(struct module *mod)
> >  {
> >  	return 0;
> >  }
> > +
> > +static inline void jump_label_inc_once(struct jump_label_key *key)
> > +{
> > +	jump_label_inc(key);
> > +}
> >  #endif	/* HAVE_JUMP_LABEL */
> >  
> >  #endif	/* _LINUX_JUMP_LABEL_H */
> > diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> > index 66ff710..f0e8231 100644
> > --- a/kernel/jump_label.c
> > +++ b/kernel/jump_label.c
> > @@ -134,15 +134,11 @@ static void __jump_label_update(struct jump_label_key *key,
> >  	}
> >  }
> >  
> > -void __init jump_label_init(void)
> > +void __init jump_label_transform_all(struct jump_entry *iter_start,
> > +				     struct jump_entry *iter_stop)
> >  {
> > -	struct jump_entry *iter_start = __start___jump_table;
> > -	struct jump_entry *iter_stop = __stop___jump_table;
> > -	struct jump_label_key *key = NULL;
> >  	struct jump_entry *iter;
> > -
> > -	jump_label_lock();
> > -	jump_label_sort_entries(iter_start, iter_stop);
> > +	struct jump_label_key *key = NULL;
> >  
> >  	for (iter = iter_start; iter < iter_stop; iter++) {
> >  		struct jump_label_key *iterk;
> > @@ -159,6 +155,24 @@ void __init jump_label_init(void)
> >  		key->next = NULL;
> >  #endif
> >  	}
> > +
> > +}
> > +
> > +
> > +void __init jump_label_init(void)
> > +{
> > +	struct jump_entry *iter_start = __start___jump_table;
> > +	struct jump_entry *iter_stop = __stop___jump_table;
> > +
> > +
> > +	jump_label_lock();
> > +	jump_label_sort_entries(iter_start, iter_stop);
> > +	jump_label_transform_all(iter_start, iter_stop);
> 
> Nit, I'd add a space here.
> 

ok. I'll fix.

Thanks,
-Kame

Re: [PATCH] jump_label: jump_label for boot options.

[KAMEZAWA Hiroyuki]

On Thu, 01 Dec 2011 16:05:13 +0100
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Thu, 2011-12-01 at 11:53 +0900, KAMEZAWA Hiroyuki wrote:
> > I tried to use jump_label for handling memcg's boot options which sets
> > global variable true/false and never changes after boot. And found jump_table
> > is larger than expected. This patch is a trial to allow to place jump_table
> > in .init section. How do you think ? 
> 
> I think its much saner to go help hnaz remove this shadow page table and
> thus the need for this stupid boot time param.
> 

Yes, that's an idea. But we also have another stupid shadow swap accounting
table.  This can be disabled at boot, too.

Thanks,
-Kame

Re: [PATCH] jump_label: jump_label for boot options.

[KAMEZAWA Hiroyuki]

On Thu, 1 Dec 2011 10:40:36 -0500
Jason Baron <jbaron@redhat.com> wrote:

> On Thu, Dec 01, 2011 at 11:53:53AM +0900, KAMEZAWA Hiroyuki wrote:
> > I tried to use jump_label for handling memcg's boot options which sets
> > global variable true/false and never changes after boot. And found jump_table
> > is larger than expected. This patch is a trial to allow to place jump_table
> > in .init section. How do you think ?
> > 
> 
> Remeber too, that 'static_branch()' is inherently biased. That is, the
> 'false' path is assumed to be the the most likely path. Thus, the 'true'
> path is move out-of-line. Thus, if the 'true' branch is potentially
> used all the time, we would want to make sure that the savings of not
> having to check a variable is still worth it. I should probably rename
> static_branch() -> 'static_branch_default_false()' to make that clear.
> 
Thank you for pointing out.

My assumption is that it's disabled at boot by few people because sane
people will not config memcg if they don't want. Most of distro users
will not turn off it...



Thanks,
-Kame

Re: [PATCH] jump_label: jump_label for boot options.

[Mike Galbraith]

On Fri, 2011-12-02 at 09:28 +0900, KAMEZAWA Hiroyuki wrote:
> On Thu, 1 Dec 2011 10:40:36 -0500
> Jason Baron <jbaron@redhat.com> wrote:
> 
> > On Thu, Dec 01, 2011 at 11:53:53AM +0900, KAMEZAWA Hiroyuki wrote:
> > > I tried to use jump_label for handling memcg's boot options which sets
> > > global variable true/false and never changes after boot. And found jump_table
> > > is larger than expected. This patch is a trial to allow to place jump_table
> > > in .init section. How do you think ?
> > > 
> > 
> > Remeber too, that 'static_branch()' is inherently biased. That is, the
> > 'false' path is assumed to be the the most likely path. Thus, the 'true'
> > path is move out-of-line. Thus, if the 'true' branch is potentially
> > used all the time, we would want to make sure that the savings of not
> > having to check a variable is still worth it. I should probably rename
> > static_branch() -> 'static_branch_default_false()' to make that clear.
> > 
> Thank you for pointing out.
> 
> My assumption is that it's disabled at boot by few people because sane
> people will not config memcg if they don't want. Most of distro users
> will not turn off it...

I think that's a bad assumption.  I see a lot of cgroup_disable=memory
command lines, and _always_ add it to grub when installing "everything
plus the kitchen sink" distro kernels on my boxen. 

	-Mike

Re: [PATCH] jump_label: jump_label for boot options.

[KAMEZAWA Hiroyuki]

On Fri, 02 Dec 2011 06:34:05 +0100
Mike Galbraith <efault@gmx.de> wrote:

> On Fri, 2011-12-02 at 09:28 +0900, KAMEZAWA Hiroyuki wrote:
> > On Thu, 1 Dec 2011 10:40:36 -0500
> > Jason Baron <jbaron@redhat.com> wrote:
> > 
> > > On Thu, Dec 01, 2011 at 11:53:53AM +0900, KAMEZAWA Hiroyuki wrote:
> > > > I tried to use jump_label for handling memcg's boot options which sets
> > > > global variable true/false and never changes after boot. And found jump_table
> > > > is larger than expected. This patch is a trial to allow to place jump_table
> > > > in .init section. How do you think ?
> > > > 
> > > 
> > > Remeber too, that 'static_branch()' is inherently biased. That is, the
> > > 'false' path is assumed to be the the most likely path. Thus, the 'true'
> > > path is move out-of-line. Thus, if the 'true' branch is potentially
> > > used all the time, we would want to make sure that the savings of not
> > > having to check a variable is still worth it. I should probably rename
> > > static_branch() -> 'static_branch_default_false()' to make that clear.
> > > 
> > Thank you for pointing out.
> > 
> > My assumption is that it's disabled at boot by few people because sane
> > people will not config memcg if they don't want. Most of distro users
> > will not turn off it...
> 
> I think that's a bad assumption.  I see a lot of cgroup_disable=memory
> command lines, and _always_ add it to grub when installing "everything
> plus the kitchen sink" distro kernels on my boxen. 
> 
Ok, I stop this work.

Thanks,
-Kame

Re: [PATCH] jump_label: jump_label for boot options.

[Peter Zijlstra]

On Fri, 2011-12-02 at 09:28 +0900, KAMEZAWA Hiroyuki wrote:
> Most of distro users will not turn off it... 

Most distros carry a patch that changes the default to off.

Re: [PATCH] jump_label: jump_label for boot options.

[Peter Zijlstra]

On Fri, 2011-12-02 at 09:22 +0900, KAMEZAWA Hiroyuki wrote:
> 
> 
> Yes, that's an idea. But we also have another stupid shadow swap accounting
> table.  This can be disabled at boot, too. 

Bah I thought it was just the page frame thing, hnaz any plans to kill
this swap array as well?

Re: [PATCH] jump_label: jump_label for boot options.

[Johannes Weiner]

On Fri, Dec 02, 2011 at 10:24:10AM +0100, Peter Zijlstra wrote:
> On Fri, 2011-12-02 at 09:22 +0900, KAMEZAWA Hiroyuki wrote:
> > 
> > Yes, that's an idea. But we also have another stupid shadow swap accounting
> > table.  This can be disabled at boot, too. 
> 
> Bah I thought it was just the page frame thing, hnaz any plans to kill
> this swap array as well?

I haven't looked at the swap accounting at all yet, sorry.  But where
did this discussion go all of a sudden? :-)

That array is not allocated at all when the memory controller is
disabled at boot-time.

Rather, we have those mem_cgroup_disabled() conditionals everytime we
enter the memory controller from the VM and the idea is to patch them
out during boot, since you can not re-enable the thing anyway.

Re: [PATCH] jump_label: jump_label for boot options.

[Peter Zijlstra]

On Fri, 2011-12-02 at 13:45 +0100, Johannes Weiner wrote:
> On Fri, Dec 02, 2011 at 10:24:10AM +0100, Peter Zijlstra wrote:
> > On Fri, 2011-12-02 at 09:22 +0900, KAMEZAWA Hiroyuki wrote:
> > > 
> > > Yes, that's an idea. But we also have another stupid shadow swap accounting
> > > table.  This can be disabled at boot, too. 
> > 
> > Bah I thought it was just the page frame thing, hnaz any plans to kill
> > this swap array as well?
> 
> I haven't looked at the swap accounting at all yet, sorry.  But where
> did this discussion go all of a sudden? :-)
> 
> That array is not allocated at all when the memory controller is
> disabled at boot-time.
> 
> Rather, we have those mem_cgroup_disabled() conditionals everytime we
> enter the memory controller from the VM and the idea is to patch them
> out during boot, since you can not re-enable the thing anyway.

Yeah, but without those arrays you could.. this boot time switch really
is a wart and if you don't have the shadow page frame and shadow swap
accounting muck stuff you could runtime flip all this..

I've no objection to using jump_labels fwiw, we're looking to do the
same with the cpu controller for the nr_cgroups == 0 case. But boot time
stuff just doesn't make sense to me.

Re: [PATCH] jump_label: jump_label for boot options.

[Johannes Weiner]

On Fri, Dec 02, 2011 at 01:53:27PM +0100, Peter Zijlstra wrote:
> On Fri, 2011-12-02 at 13:45 +0100, Johannes Weiner wrote:
> > On Fri, Dec 02, 2011 at 10:24:10AM +0100, Peter Zijlstra wrote:
> > > On Fri, 2011-12-02 at 09:22 +0900, KAMEZAWA Hiroyuki wrote:
> > > > 
> > > > Yes, that's an idea. But we also have another stupid shadow swap accounting
> > > > table.  This can be disabled at boot, too. 
> > > 
> > > Bah I thought it was just the page frame thing, hnaz any plans to kill
> > > this swap array as well?
> > 
> > I haven't looked at the swap accounting at all yet, sorry.  But where
> > did this discussion go all of a sudden? :-)
> > 
> > That array is not allocated at all when the memory controller is
> > disabled at boot-time.
> > 
> > Rather, we have those mem_cgroup_disabled() conditionals everytime we
> > enter the memory controller from the VM and the idea is to patch them
> > out during boot, since you can not re-enable the thing anyway.
> 
> Yeah, but without those arrays you could.. this boot time switch really
> is a wart and if you don't have the shadow page frame and shadow swap
> accounting muck stuff you could runtime flip all this..

Ah, got you.

The shadow page frame is on its way out.  The extra
swp_entry_t->cgroup_id array is allocated during swap-on in vmap
space, so I don't think we have much of a runtime problem with that
one, either.

The biggest problem I see is that we modify per-page(_cgroup) state
when charging/uncharging against the root_mem_cgroup.  But this sucks
anyway (overhead for distro-kernel users that don't care about memcg),
so we need to ditch that.  And once that is gone, we are much closer
to runtime-toggling the controller.

> I've no objection to using jump_labels fwiw, we're looking to do the
> same with the cpu controller for the nr_cgroups == 0 case. But boot time
> stuff just doesn't make sense to me.

Agreed.