[PATCH] rcu: avoid checking for constant

[PATCH] rcu: avoid checking for constant

[Jan Engelhardt]

When compiling kernel or module code with -O0, "offset" is no longer
considered a constant, and therefore always triggers the build error
that BUILD_BUG_ON is defined to yield.

What is the rationale between the forced constant check,
introduced in 9ab1544eb4196ca8d05c433b2eb56f74496b1ee3?

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 include/linux/rcupdate.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 2cf4226..38c5ba5 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -805,8 +805,6 @@ void __kfree_rcu(struct rcu_head *head, unsigned long offset)
 {
 	typedef void (*rcu_callback)(struct rcu_head *);
 
-	BUILD_BUG_ON(!__builtin_constant_p(offset));
-
 	/* See the kfree_rcu() header comment. */
 	BUILD_BUG_ON(!__is_kfree_rcu_offset(offset));
 
-- 
1.7.7

Re: [PATCH] rcu: avoid checking for constant

[Josh Triplett]

On Thu, Jan 12, 2012 at 06:11:44AM +0100, Jan Engelhardt wrote:
> When compiling kernel or module code with -O0, "offset" is no longer
> considered a constant, and therefore always triggers the build error
> that BUILD_BUG_ON is defined to yield.
> 
> What is the rationale between the forced constant check,
> introduced in 9ab1544eb4196ca8d05c433b2eb56f74496b1ee3?
[...]
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -805,8 +805,6 @@ void __kfree_rcu(struct rcu_head *head, unsigned long offset)
>  {
>  	typedef void (*rcu_callback)(struct rcu_head *);
>  
> -	BUILD_BUG_ON(!__builtin_constant_p(offset));
> -
>  	/* See the kfree_rcu() header comment. */
>  	BUILD_BUG_ON(!__is_kfree_rcu_offset(offset));

The very next check after the one you removed will break if the
compiler can't check the offset at compile time.

- Josh Triplett

Re: [PATCH] rcu: avoid checking for constant

[Jan Engelhardt]

On Thursday 2012-01-12 08:14, Josh Triplett wrote:

>On Thu, Jan 12, 2012 at 06:11:44AM +0100, Jan Engelhardt wrote:
>> When compiling kernel or module code with -O0, "offset" is no longer
>> considered a constant, and therefore always triggers the build error
>> that BUILD_BUG_ON is defined to yield.
>[...]
>> --- a/include/linux/rcupdate.h
>> +++ b/include/linux/rcupdate.h
>> @@ -805,8 +805,6 @@ void __kfree_rcu(struct rcu_head *head, unsigned long offset)
>>  {
>>  	typedef void (*rcu_callback)(struct rcu_head *);
>>  
>> -	BUILD_BUG_ON(!__builtin_constant_p(offset));
>> -
>>  	/* See the kfree_rcu() header comment. */
>>  	BUILD_BUG_ON(!__is_kfree_rcu_offset(offset));
>
>The very next check after the one you removed will break if the
>compiler can't check the offset at compile time.

Thanks for catching that. How about the following approach? :

parent f9fab10bbd768b0e5254e53a4a8477a94bfc4b96 (v3.2-rc7-86-gf9fab10)
commit a4145604f1e40ce93a25a053f0f49341324b0077
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Wed Jan 11 10:05:00 2012 +0100

rcu: avoid checking for constant

When compiling kernel or module code with -O0, "offset" is no longer
considered a constant, and therefore always triggers the build error
that BUILD_BUG_ON is defined to yield.

What is the rationale between the forced constant check,
introduced in 9ab1544eb4196ca8d05c433b2eb56f74496b1ee3?

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 include/linux/rcupdate.h |   22 ++++------------------
 1 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 2cf4226..5e7286d 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -795,24 +795,6 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
 #define RCU_INIT_POINTER(p, v) \
 		p = (typeof(*v) __force __rcu *)(v)
 
-static __always_inline bool __is_kfree_rcu_offset(unsigned long offset)
-{
-	return offset < 4096;
-}
-
-static __always_inline
-void __kfree_rcu(struct rcu_head *head, unsigned long offset)
-{
-	typedef void (*rcu_callback)(struct rcu_head *);
-
-	BUILD_BUG_ON(!__builtin_constant_p(offset));
-
-	/* See the kfree_rcu() header comment. */
-	BUILD_BUG_ON(!__is_kfree_rcu_offset(offset));
-
-	call_rcu(head, (rcu_callback)offset);
-}
-
 /**
  * kfree_rcu() - kfree an object after a grace period.
  * @ptr:	pointer to kfree
@@ -836,6 +818,10 @@ void __kfree_rcu(struct rcu_head *head, unsigned long offset)
  * Note that the allowable offset might decrease in the future, for example,
  * to allow something like kmem_cache_free_rcu().
  */
+#define __kfree_rcu(head, offset) \
+	call_rcu(head, (void (*)(struct rcu_head *))(unsigned long)(offset) + \
+		 BUILD_BUG_ON_ZERO((offset) >= 4096))
+
 #define kfree_rcu(ptr, rcu_head)					\
 	__kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
 
-- 
# Created with git-export-patch

Re: [PATCH] rcu: avoid checking for constant

[Josh Triplett]

On Thu, Jan 12, 2012 at 10:25:23AM +0100, Jan Engelhardt wrote:
> rcu: avoid checking for constant
> 
> When compiling kernel or module code with -O0, "offset" is no longer
> considered a constant, and therefore always triggers the build error
> that BUILD_BUG_ON is defined to yield.
> 
> What is the rationale between the forced constant check,
> introduced in 9ab1544eb4196ca8d05c433b2eb56f74496b1ee3?

This question shouldn't live in the commit message.  Please replace it
with an explanation of the change instead.

> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
> ---
>  include/linux/rcupdate.h |   22 ++++------------------
>  1 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 2cf4226..5e7286d 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -795,24 +795,6 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
>  #define RCU_INIT_POINTER(p, v) \
>  		p = (typeof(*v) __force __rcu *)(v)
>  
> -static __always_inline bool __is_kfree_rcu_offset(unsigned long offset)
> -{
> -	return offset < 4096;
> -}
> -
> -static __always_inline
> -void __kfree_rcu(struct rcu_head *head, unsigned long offset)
> -{
> -	typedef void (*rcu_callback)(struct rcu_head *);
> -
> -	BUILD_BUG_ON(!__builtin_constant_p(offset));
> -
> -	/* See the kfree_rcu() header comment. */
> -	BUILD_BUG_ON(!__is_kfree_rcu_offset(offset));
> -
> -	call_rcu(head, (rcu_callback)offset);
> -}
> -
>  /**
>   * kfree_rcu() - kfree an object after a grace period.
>   * @ptr:	pointer to kfree
> @@ -836,6 +818,10 @@ void __kfree_rcu(struct rcu_head *head, unsigned long offset)
>   * Note that the allowable offset might decrease in the future, for example,
>   * to allow something like kmem_cache_free_rcu().
>   */
> +#define __kfree_rcu(head, offset) \
> +	call_rcu(head, (void (*)(struct rcu_head *))(unsigned long)(offset) + \
> +		 BUILD_BUG_ON_ZERO((offset) >= 4096))
> +

I had to stare at this for a while, and look up the definition of
BUILD_BUG_ON_ZERO.  Naturally I assumed that BUILD_BUG_ON_ZERO(arg)
meant BUILT_BUG_ON((arg) == 0), which would have made the logic
backwards here.  However, per the definition it just provides a
zero-returning version of BUILD_BUG_ON.  Ow.

In any case, __kfree_rcu has void return type, so how about just using
do { ... } while(0) and BUILD_BUG_ON instead?  That seems significantly
clearer.

Apart from that, I can live with this, though it seems horribly
backwards to replace an inline with a macro rather than the other way
around.

- Josh Triplett

Re: [PATCH] rcu: avoid checking for constant

[Jan Engelhardt]

On Thursday 2012-01-12 10:52, Josh Triplett wrote:
>> +#define __kfree_rcu(head, offset) \
>> +	call_rcu(head, (void (*)(struct rcu_head *))(unsigned long)(offset) + \
>> +		 BUILD_BUG_ON_ZERO((offset) >= 4096))
>> +
>
>I had to stare at this for a while, and look up the definition of
>BUILD_BUG_ON_ZERO.  Naturally I assumed that BUILD_BUG_ON_ZERO(arg)
>meant BUILT_BUG_ON((arg) == 0), which would have made the logic
>backwards here.  However, per the definition it just provides a
>zero-returning version of BUILD_BUG_ON.  Ow.

Same impression here. BUILD_BUG_ON_ZERO was introduced by

commit 4552d5dc08b79868829b4be8951b29b07284753f
Author: Jan Beulich <jbeulich@novell.com>
Date:   Mon Jun 26 13:57:28 2006 +0200

while Rusty's CCAN archive calls it "BUILD_BUG_OR_ZERO" (since either 
it's a bug, or returning neutral zero).

>In any case, __kfree_rcu has void return type, so how about just using
>do { ... } while(0) and BUILD_BUG_ON instead?  That seems significantly
>clearer.

Ok. In that case, I'll allow myself to reintroduce the extra temporal 
typedef line that was there previously:

>Apart from that, I can live with this, though it seems horribly
>backwards to replace an inline with a macro rather than the other way
>around.

parent f9fab10bbd768b0e5254e53a4a8477a94bfc4b96 (v3.2-rc7-86-gf9fab10)
commit 041ac1b39390b93d8db91075aefc56b585db75cd
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Wed Jan 11 10:05:00 2012 +0100

rcu: avoid checking for constant

When compiling kernel or module code with -O0, "offset" is no longer
considered a constant, and therefore always triggers the build error
that BUILD_BUG_ON is defined to yield.

Therefore, change the innards of kfree_rcu so that the offset is not
tunneled through a function argument before checking it.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 include/linux/rcupdate.h |   31 +++++++++++++------------------
 1 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 2cf4226..7395ab6 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -795,24 +795,6 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
 #define RCU_INIT_POINTER(p, v) \
 		p = (typeof(*v) __force __rcu *)(v)
 
-static __always_inline bool __is_kfree_rcu_offset(unsigned long offset)
-{
-	return offset < 4096;
-}
-
-static __always_inline
-void __kfree_rcu(struct rcu_head *head, unsigned long offset)
-{
-	typedef void (*rcu_callback)(struct rcu_head *);
-
-	BUILD_BUG_ON(!__builtin_constant_p(offset));
-
-	/* See the kfree_rcu() header comment. */
-	BUILD_BUG_ON(!__is_kfree_rcu_offset(offset));
-
-	call_rcu(head, (rcu_callback)offset);
-}
-
 /**
  * kfree_rcu() - kfree an object after a grace period.
  * @ptr:	pointer to kfree
@@ -835,7 +817,20 @@ void __kfree_rcu(struct rcu_head *head, unsigned long offset)
  *
  * Note that the allowable offset might decrease in the future, for example,
  * to allow something like kmem_cache_free_rcu().
+ *
+ * The BUILD_BUG_ON check must not involve any function calls, hence the
+ * checks are done in macros here. __is_kfree_rcu_offset is also used by
+ * kernel/rcu.h.
  */
+#define __is_kfree_rcu_offset(offset) ((offset) < 4096)
+
+#define __kfree_rcu(head, offset) \
+	do { \
+		typedef void (*rcu_callback)(struct rcu_head *); \
+		BUILD_BUG_ON(!__is_kfree_rcu_offset(offset)); \
+		call_rcu(head, (rcu_callback)(unsigned long)(offset)); \
+	} while (0)
+
 #define kfree_rcu(ptr, rcu_head)					\
 	__kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
 
-- 

Re: [PATCH] rcu: avoid checking for constant

[Eric Dumazet]

Le jeudi 12 janvier 2012 à 11:34 +0100, Jan Engelhardt a écrit :

> When compiling kernel or module code with -O0, "offset" is no longer
> considered a constant, and therefore always triggers the build error
> that BUILD_BUG_ON is defined to yield.


Why compiling kernel with -O0 is even considered ?

A lot of patches are needed because gcc -O0 is unable to understand some
code is dead code.

  LD      .tmp_vmlinux1
mm/built-in.o: In function `do_swap_page':
memory.c:(.text+0x44dbd): undefined reference to `ksm_does_need_to_copy'
net/built-in.o: In function `ieee80211_tx_status':
(.text+0x221ac1): undefined reference to `ieee80211s_update_metric'
net/built-in.o: In function `sta_info_insert_finish':
sta_info.c:(.text+0x224c62): undefined reference to
`mesh_accept_plinks_update'
net/built-in.o: In function `ieee80211_teardown_sdata':
iface.c:(.text+0x242773): undefined reference to `mesh_rmc_free'
net/built-in.o: In function `ieee80211_iface_work':
iface.c:(.text+0x242cfa): undefined reference to
`ieee80211_mesh_rx_queued_mgmt'
iface.c:(.text+0x242dc2): undefined reference to `ieee80211_mesh_work'
net/built-in.o: In function `ieee80211_setup_sdata':
iface.c:(.text+0x242ffc): undefined reference to
`ieee80211_mesh_init_sdata'
net/built-in.o: In function `ieee80211_if_remove':
(.text+0x243d1a): undefined reference to `mesh_path_flush_by_iface'
net/built-in.o: In function `ieee80211_remove_interfaces':
(.text+0x243df5): undefined reference to `mesh_path_flush_by_iface'
net/built-in.o: In function `ieee80211_rx_h_action':
rx.c:(.text+0x253ba6): undefined reference to `mesh_action_is_path_sel'
net/built-in.o: In function `ieee80211_xmit':
(.text+0x25a254): undefined reference to `mesh_nexthop_resolve'
net/built-in.o: In function `ieee80211_beacon_get_tim':
(.text+0x25c250): undefined reference to `mesh_add_ds_params_ie'
net/built-in.o: In function `ieee80211_beacon_get_tim':
(.text+0x25c28d): undefined reference to `mesh_add_rsn_ie'
net/built-in.o: In function `ieee80211_beacon_get_tim':
(.text+0x25c2a4): undefined reference to `mesh_add_ht_cap_ie'
net/built-in.o: In function `ieee80211_beacon_get_tim':
(.text+0x25c2bb): undefined reference to `mesh_add_ht_info_ie'
net/built-in.o: In function `ieee80211_beacon_get_tim':
(.text+0x25c2d2): undefined reference to `mesh_add_meshid_ie'
net/built-in.o: In function `ieee80211_beacon_get_tim':
(.text+0x25c2e9): undefined reference to `mesh_add_meshconf_ie'
net/built-in.o: In function `ieee80211_beacon_get_tim':
(.text+0x25c300): undefined reference to `mesh_add_vendor_ies'
make: *** [.tmp_vmlinux1] Erreur 1

Re: [PATCH] rcu: avoid checking for constant

[Jan Engelhardt]

On Thursday 2012-01-12 11:54, Eric Dumazet wrote:

>Le jeudi 12 janvier 2012 à 11:34 +0100, Jan Engelhardt a écrit :
>
>> When compiling kernel or module code with -O0, "offset" is no longer
>> considered a constant, and therefore always triggers the build error
>> that BUILD_BUG_ON is defined to yield.
>
>Why compiling kernel with -O0 is even considered ?

The compile error was observed when trying to compile an out-of-tree 
module with -O0.

Re: [PATCH] rcu: avoid checking for constant

[Josh Triplett]

On Thu, Jan 12, 2012 at 11:34:23AM +0100, Jan Engelhardt wrote:
> On Thursday 2012-01-12 10:52, Josh Triplett wrote:
> >> +#define __kfree_rcu(head, offset) \
> >> +	call_rcu(head, (void (*)(struct rcu_head *))(unsigned long)(offset) + \
> >> +		 BUILD_BUG_ON_ZERO((offset) >= 4096))
> >> +
> >
> >I had to stare at this for a while, and look up the definition of
> >BUILD_BUG_ON_ZERO.  Naturally I assumed that BUILD_BUG_ON_ZERO(arg)
> >meant BUILT_BUG_ON((arg) == 0), which would have made the logic
> >backwards here.  However, per the definition it just provides a
> >zero-returning version of BUILD_BUG_ON.  Ow.
> 
> Same impression here. BUILD_BUG_ON_ZERO was introduced by
> 
> commit 4552d5dc08b79868829b4be8951b29b07284753f
> Author: Jan Beulich <jbeulich@novell.com>
> Date:   Mon Jun 26 13:57:28 2006 +0200
> 
> while Rusty's CCAN archive calls it "BUILD_BUG_OR_ZERO" (since either 
> it's a bug, or returning neutral zero).

Sounds like a good target for a fix at some point.

> rcu: avoid checking for constant
> 
> When compiling kernel or module code with -O0, "offset" is no longer
> considered a constant, and therefore always triggers the build error
> that BUILD_BUG_ON is defined to yield.
> 
> Therefore, change the innards of kfree_rcu so that the offset is not
> tunneled through a function argument before checking it.

The commit message looks good now.

> @@ -835,7 +817,20 @@ void __kfree_rcu(struct rcu_head *head, unsigned long offset)
>   *
>   * Note that the allowable offset might decrease in the future, for example,
>   * to allow something like kmem_cache_free_rcu().
> + *
> + * The BUILD_BUG_ON check must not involve any function calls, hence the
> + * checks are done in macros here. __is_kfree_rcu_offset is also used by
> + * kernel/rcu.h.

The first sentence of that paragraph seems like a worthwhile addition.
Please drop the second, though, since it'll inevitably become outdated.

> +#define __is_kfree_rcu_offset(offset) ((offset) < 4096)
> +
> +#define __kfree_rcu(head, offset) \
> +	do { \
> +		typedef void (*rcu_callback)(struct rcu_head *); \
> +		BUILD_BUG_ON(!__is_kfree_rcu_offset(offset)); \
> +		call_rcu(head, (rcu_callback)(unsigned long)(offset)); \
> +	} while (0)

No, you can't define that typedef here with that name.  Unlike in the
inline function, in a macro you could introduce a name conflict.

- Josh Triplett

Re: [PATCH] rcu: avoid checking for constant

[Paul E. McKenney]

On Thu, Jan 12, 2012 at 11:57:10AM +0100, Jan Engelhardt wrote:
> On Thursday 2012-01-12 11:54, Eric Dumazet wrote:
> 
> >Le jeudi 12 janvier 2012 à 11:34 +0100, Jan Engelhardt a écrit :
> >
> >> When compiling kernel or module code with -O0, "offset" is no longer
> >> considered a constant, and therefore always triggers the build error
> >> that BUILD_BUG_ON is defined to yield.
> >
> >Why compiling kernel with -O0 is even considered ?
> 
> The compile error was observed when trying to compile an out-of-tree 
> module with -O0.

Hmmm...  Why not have a glue .c file in your out-of-tree module
that contains function like:

	void kfree_rcu_foo(struct foo *fp)
	{
		kfree_rcu(fp, foo_rcu);
	}

Compile this .c file normally, then apply -O0 only to the other
files in your module.

							Thanx, Paul

Re: [PATCH] rcu: avoid checking for constant

[Jan Engelhardt]

On Thursday 2012-01-12 12:58, Josh Triplett wrote:
>> Same impression here. BUILD_BUG_ON_ZERO was introduced by
>> 
>> commit 4552d5dc08b79868829b4be8951b29b07284753f
>> Author: Jan Beulich <jbeulich@novell.com>
>> Date:   Mon Jun 26 13:57:28 2006 +0200
>> 
>> while Rusty's CCAN archive calls it "BUILD_BUG_OR_ZERO" (since either 
>> it's a bug, or returning neutral zero).
>
>Sounds like a good target for a fix at some point.

What names do you propose? I have BUILD_BUG_ON_EXPR for some of my code,
though in the kernel, it has both _ON_ZERO and _ON_NULL.

>> Therefore, change the innards of kfree_rcu so that the offset is not
>> tunneled through a function argument before checking it.
>
>The commit message looks good now.
>
>> + * The BUILD_BUG_ON check must not involve any function calls, hence the
>> + * checks are done in macros here. __is_kfree_rcu_offset is also used by
>> + * kernel/rcu.h.
>
>The first sentence of that paragraph seems like a worthwhile addition.
>Please drop the second, though, since it'll inevitably become outdated.
>
>> +		typedef void (*rcu_callback)(struct rcu_head *); \
>
>No, you can't define that typedef here with that name.  Unlike in the
>inline function, in a macro you could introduce a name conflict.

parent f9fab10bbd768b0e5254e53a4a8477a94bfc4b96 (v3.2-rc7-86-gf9fab10)
commit 2d6e2f3244513ec05953383886ef0556f64038b1
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Wed Jan 11 10:05:00 2012 +0100

rcu: avoid checking for constant

When compiling kernel or module code with -O0, "offset" is no longer
considered a constant, and therefore always triggers the build error
that BUILD_BUG_ON is defined to yield.

Therefore, change the innards of kfree_rcu so that the offset is not
tunneled through a function argument before checking it.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 include/linux/rcupdate.h |   29 +++++++++++------------------
 1 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 2cf4226..c693e4a 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -795,24 +795,6 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
 #define RCU_INIT_POINTER(p, v) \
 		p = (typeof(*v) __force __rcu *)(v)
 
-static __always_inline bool __is_kfree_rcu_offset(unsigned long offset)
-{
-	return offset < 4096;
-}
-
-static __always_inline
-void __kfree_rcu(struct rcu_head *head, unsigned long offset)
-{
-	typedef void (*rcu_callback)(struct rcu_head *);
-
-	BUILD_BUG_ON(!__builtin_constant_p(offset));
-
-	/* See the kfree_rcu() header comment. */
-	BUILD_BUG_ON(!__is_kfree_rcu_offset(offset));
-
-	call_rcu(head, (rcu_callback)offset);
-}
-
 /**
  * kfree_rcu() - kfree an object after a grace period.
  * @ptr:	pointer to kfree
@@ -835,7 +817,18 @@ void __kfree_rcu(struct rcu_head *head, unsigned long offset)
  *
  * Note that the allowable offset might decrease in the future, for example,
  * to allow something like kmem_cache_free_rcu().
+ *
+ * The BUILD_BUG_ON check must not involve any function calls, hence the
+ * checks are done in macros here.
  */
+#define __is_kfree_rcu_offset(offset) ((offset) < 4096)
+
+#define __kfree_rcu(head, offset) \
+	do { \
+		BUILD_BUG_ON(!__is_kfree_rcu_offset(offset)); \
+		call_rcu(head, (void (*)(struct rcu_head *))(unsigned long)(offset)); \
+	} while (0)
+
 #define kfree_rcu(ptr, rcu_head)					\
 	__kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
 
-- 
# Created with git-export-patch

Re: [PATCH] rcu: avoid checking for constant

[Jan Engelhardt]

On Thursday 2012-01-12 16:29, Paul E. McKenney wrote:

>On Thu, Jan 12, 2012 at 11:57:10AM +0100, Jan Engelhardt wrote:
>> On Thursday 2012-01-12 11:54, Eric Dumazet wrote:
>> 
>> >Le jeudi 12 janvier 2012 à 11:34 +0100, Jan Engelhardt a écrit :
>> >
>> >> When compiling kernel or module code with -O0, "offset" is no longer
>> >> considered a constant, and therefore always triggers the build error
>> >> that BUILD_BUG_ON is defined to yield.
>> >
>> >Why compiling kernel with -O0 is even considered ?
>> 
>> The compile error was observed when trying to compile an out-of-tree 
>> module with -O0.
>
>Hmmm...  Why not have a glue .c file in your out-of-tree module
>that contains function like[...]

Maybe, maybe naah. The kernel relying on gcc's -O2 mode to do constant 
value analysis through and into inline functions does not sound too 
great. The compiler could change on the next moonphase. In fact, one 
can't use clang -O2 because of this. With my patch, one can again use 
clang -O2 [with respect to kfree_rcu].

Re: [PATCH] rcu: avoid checking for constant

[Eric Dumazet]

Le jeudi 12 janvier 2012 à 17:08 +0100, Jan Engelhardt a écrit :

> Maybe, maybe naah. The kernel relying on gcc's -O2 mode to do constant 
> value analysis through and into inline functions does not sound too 
> great. The compiler could change on the next moonphase. In fact, one 
> can't use clang -O2 because of this. With my patch, one can again use 
> clang -O2 [with respect to kfree_rcu].

Unless whole kernel can be compiled with -O0, its really some noise.

Re: [PATCH] rcu: avoid checking for constant

[Josh Triplett]

On Thu, Jan 12, 2012 at 04:38:52PM +0100, Jan Engelhardt wrote:
> On Thursday 2012-01-12 12:58, Josh Triplett wrote:
> >> Same impression here. BUILD_BUG_ON_ZERO was introduced by
> >> 
> >> commit 4552d5dc08b79868829b4be8951b29b07284753f
> >> Author: Jan Beulich <jbeulich@novell.com>
> >> Date:   Mon Jun 26 13:57:28 2006 +0200
> >> 
> >> while Rusty's CCAN archive calls it "BUILD_BUG_OR_ZERO" (since either 
> >> it's a bug, or returning neutral zero).
> >
> >Sounds like a good target for a fix at some point.
> 
> What names do you propose? I have BUILD_BUG_ON_EXPR for some of my code,
> though in the kernel, it has both _ON_ZERO and _ON_NULL.

BUILD_BUG_OR_ZERO (and BUILD_BUG_OR_NULL) seem like improvements over
_ON_ZERO and _ON_NULL; that would remove the ambiguity we both tripped
over.

> rcu: avoid checking for constant
> 
> When compiling kernel or module code with -O0, "offset" is no longer
> considered a constant, and therefore always triggers the build error
> that BUILD_BUG_ON is defined to yield.
> 
> Therefore, change the innards of kfree_rcu so that the offset is not
> tunneled through a function argument before checking it.
> 
> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -795,24 +795,6 @@ static inline notrace void rcu_read_unlock_sched_notrace(void)
>  #define RCU_INIT_POINTER(p, v) \
>  		p = (typeof(*v) __force __rcu *)(v)
>  
> -static __always_inline bool __is_kfree_rcu_offset(unsigned long offset)
> -{
> -	return offset < 4096;
> -}
> -
> -static __always_inline
> -void __kfree_rcu(struct rcu_head *head, unsigned long offset)
> -{
> -	typedef void (*rcu_callback)(struct rcu_head *);
> -
> -	BUILD_BUG_ON(!__builtin_constant_p(offset));
> -
> -	/* See the kfree_rcu() header comment. */
> -	BUILD_BUG_ON(!__is_kfree_rcu_offset(offset));
> -
> -	call_rcu(head, (rcu_callback)offset);
> -}
> -
>  /**
>   * kfree_rcu() - kfree an object after a grace period.
>   * @ptr:	pointer to kfree
> @@ -835,7 +817,18 @@ void __kfree_rcu(struct rcu_head *head, unsigned long offset)
>   *
>   * Note that the allowable offset might decrease in the future, for example,
>   * to allow something like kmem_cache_free_rcu().
> + *
> + * The BUILD_BUG_ON check must not involve any function calls, hence the
> + * checks are done in macros here.
>   */
> +#define __is_kfree_rcu_offset(offset) ((offset) < 4096)
> +
> +#define __kfree_rcu(head, offset) \
> +	do { \
> +		BUILD_BUG_ON(!__is_kfree_rcu_offset(offset)); \
> +		call_rcu(head, (void (*)(struct rcu_head *))(unsigned long)(offset)); \
> +	} while (0)
> +
>  #define kfree_rcu(ptr, rcu_head)					\
>  	__kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))

- Josh Triplett

Re: [PATCH] rcu: avoid checking for constant

[Paul E. McKenney]

On Thu, Jan 12, 2012 at 10:41:50AM -0800, Josh Triplett wrote:
> On Thu, Jan 12, 2012 at 04:38:52PM +0100, Jan Engelhardt wrote:
> > On Thursday 2012-01-12 12:58, Josh Triplett wrote:
> > >> Same impression here. BUILD_BUG_ON_ZERO was introduced by
> > >> 
> > >> commit 4552d5dc08b79868829b4be8951b29b07284753f
> > >> Author: Jan Beulich <jbeulich@novell.com>
> > >> Date:   Mon Jun 26 13:57:28 2006 +0200
> > >> 
> > >> while Rusty's CCAN archive calls it "BUILD_BUG_OR_ZERO" (since either 
> > >> it's a bug, or returning neutral zero).
> > >
> > >Sounds like a good target for a fix at some point.
> > 
> > What names do you propose? I have BUILD_BUG_ON_EXPR for some of my code,
> > though in the kernel, it has both _ON_ZERO and _ON_NULL.
> 
> BUILD_BUG_OR_ZERO (and BUILD_BUG_OR_NULL) seem like improvements over
> _ON_ZERO and _ON_NULL; that would remove the ambiguity we both tripped
> over.
> 
> > rcu: avoid checking for constant
> > 
> > When compiling kernel or module code with -O0, "offset" is no longer
> > considered a constant, and therefore always triggers the build error
> > that BUILD_BUG_ON is defined to yield.
> > 
> > Therefore, change the innards of kfree_rcu so that the offset is not
> > tunneled through a function argument before checking it.
> > 
> > Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
> 
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>

OK, a similar run-in with copy_from_user() convince me that we need
this.  (And I was even using default optimization levels!)

Queue for 3.5, with reworked comments and commit log.

						Thanx, Paul

------------------------------------------------------------------------

rcu: Make __kfree_rcu() less dependent on compiler choices

Currently, __kfree_rcu() is implemented as an inline function, and
contains a BUILD_BUG_ON() that malfunctions if __kfree_rcu() is compiled
as an out-of-line function.  Unfortunately, there are compiler settings
(e.g., -O0) that can result in __kfree_rcu() being compiled out of line,
resulting in annoying build breakage.  This commit therefore converts
both __kfree_rcu() and __is_kfree_rcu_offset() from inline functions to
macros to prevent such misbehavior on the part of the compiler.

Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 20fb776..d5dfb10 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -922,6 +922,21 @@ void __kfree_rcu(struct rcu_head *head, unsigned long offset)
 	kfree_call_rcu(head, (rcu_callback)offset);
 }
 
+/*
+ * Does the specified offset indicate that the corresponding rcu_head
+ * structure can be handled by kfree_rcu()?
+ */
+#define __is_kfree_rcu_offset(offset) ((offset) < 4096)
+
+/*
+ * Helper macro for kfree_rcu() to prevent argument-expansion eyestrain.
+ */
+#define __kfree_rcu(head, offset) \
+	do { \
+		BUILD_BUG_ON(!__is_kfree_rcu_offset(offset)); \
+		call_rcu(head, (void (*)(struct rcu_head *))(unsigned long)(offset)); \
+	} while (0)
+
 /**
  * kfree_rcu() - kfree an object after a grace period.
  * @ptr:	pointer to kfree
@@ -944,6 +959,9 @@ void __kfree_rcu(struct rcu_head *head, unsigned long offset)
  *
  * Note that the allowable offset might decrease in the future, for example,
  * to allow something like kmem_cache_free_rcu().
+ *
+ * The BUILD_BUG_ON check must not involve any function calls, hence the
+ * checks are done in macros here.
  */
 #define kfree_rcu(ptr, rcu_head)					\
 	__kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))