[PATCH v2] consolidate WARN_...ONCE() static variables

[PATCH v2] consolidate WARN_...ONCE() static variables

[Jan Beulich]

Due to the alignment of following variables, these typically consume
more than just the single byte that 'bool' requires, and as there are
a few hundred instances, the cache pollution (not so much the waste of
memory) sums up. Put these variables into their own section, outside
of any half way frequently used memory range.

v2: Do the same also to the __warned variable of rcu_lockdep_assert().
(Don't, however, include the ones used by printk_once() and alike, as
they can potentially be hot.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>

---
 include/asm-generic/bug.h         |    6 +++---
 include/asm-generic/vmlinux.lds.h |    1 +
 include/linux/rcupdate.h          |    2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

--- 3.3-rc5/include/asm-generic/bug.h
+++ 3.3-rc5-warn-once-flag-cold/include/asm-generic/bug.h
@@ -134,7 +134,7 @@ extern void warn_slowpath_null(const cha
 #endif
 
 #define WARN_ON_ONCE(condition)	({				\
-	static bool __warned;					\
+	static bool __section(.data.unlikely) __warned;		\
 	int __ret_warn_once = !!(condition);			\
 								\
 	if (unlikely(__ret_warn_once))				\
@@ -144,7 +144,7 @@ extern void warn_slowpath_null(const cha
 })
 
 #define WARN_ONCE(condition, format...)	({			\
-	static bool __warned;					\
+	static bool __section(.data.unlikely) __warned;		\
 	int __ret_warn_once = !!(condition);			\
 								\
 	if (unlikely(__ret_warn_once))				\
@@ -154,7 +154,7 @@ extern void warn_slowpath_null(const cha
 })
 
 #define WARN_TAINT_ONCE(condition, taint, format...)	({	\
-	static bool __warned;					\
+	static bool __section(.data.unlikely) __warned;		\
 	int __ret_warn_once = !!(condition);			\
 								\
 	if (unlikely(__ret_warn_once))				\
--- 3.3-rc5/include/asm-generic/vmlinux.lds.h
+++ 3.3-rc5-warn-once-flag-cold/include/asm-generic/vmlinux.lds.h
@@ -167,6 +167,7 @@
 	CPU_KEEP(exit.data)						\
 	MEM_KEEP(init.data)						\
 	MEM_KEEP(exit.data)						\
+	*(.data.unlikely)						\
 	STRUCT_ALIGN();							\
 	*(__tracepoints)						\
 	/* implement dynamic printk debug */				\
--- 3.3-rc5/include/linux/rcupdate.h
+++ 3.3-rc5-warn-once-flag-cold/include/linux/rcupdate.h
@@ -374,7 +374,7 @@ extern int rcu_my_thread_group_empty(voi
  */
 #define rcu_lockdep_assert(c, s)					\
 	do {								\
-		static bool __warned;					\
+		static bool __section(.data.unlikely) __warned;		\
 		if (debug_lockdep_rcu_enabled() && !__warned && !(c)) {	\
 			__warned = true;				\
 			lockdep_rcu_suspicious(__FILE__, __LINE__, s);	\

Re: [PATCH v2] consolidate WARN_...ONCE() static variables

[Andrew Morton]

On Mon, 27 Feb 2012 15:10:34 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> Due to the alignment of following variables, these typically consume
> more than just the single byte that 'bool' requires, and as there are
> a few hundred instances, the cache pollution (not so much the waste of
> memory) sums up. Put these variables into their own section, outside
> of any half way frequently used memory range.
> 
> v2: Do the same also to the __warned variable of rcu_lockdep_assert().
> (Don't, however, include the ones used by printk_once() and alike, as
> they can potentially be hot.)

I have a bad feeling that I still don't understand this patch.  Ho hum.

What are the rules for the new .data.unlikely section?  When should
people put variables into this section?  Perhaps we can document this
somewhere?

Re: [PATCH v2] consolidate WARN_...ONCE() static variables

[Jan Beulich]

>>> On 28.02.12 at 01:03, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Mon, 27 Feb 2012 15:10:34 +0000
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> Due to the alignment of following variables, these typically consume
>> more than just the single byte that 'bool' requires, and as there are
>> a few hundred instances, the cache pollution (not so much the waste of
>> memory) sums up. Put these variables into their own section, outside
>> of any half way frequently used memory range.
>> 
>> v2: Do the same also to the __warned variable of rcu_lockdep_assert().
>> (Don't, however, include the ones used by printk_once() and alike, as
>> they can potentially be hot.)
> 
> I have a bad feeling that I still don't understand this patch.  Ho hum.
> 
> What are the rules for the new .data.unlikely section?  When should
> people put variables into this section?  Perhaps we can document this
> somewhere?

If I knew the "where" part of this, I could put together a few sentences.
I just grep-ed through Documentation/, without finding e.g. any rules
or guidelines for using {,un}likely()...

Jan

Re: [PATCH v2] consolidate WARN_...ONCE() static variables

[Andrew Morton]

On Tue, 28 Feb 2012 07:41:54 +0000 "Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 28.02.12 at 01:03, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Mon, 27 Feb 2012 15:10:34 +0000
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> Due to the alignment of following variables, these typically consume
> >> more than just the single byte that 'bool' requires, and as there are
> >> a few hundred instances, the cache pollution (not so much the waste of
> >> memory) sums up. Put these variables into their own section, outside
> >> of any half way frequently used memory range.
> >> 
> >> v2: Do the same also to the __warned variable of rcu_lockdep_assert().
> >> (Don't, however, include the ones used by printk_once() and alike, as
> >> they can potentially be hot.)
> > 
> > I have a bad feeling that I still don't understand this patch.  Ho hum.
> > 
> > What are the rules for the new .data.unlikely section?  When should
> > people put variables into this section?  Perhaps we can document this
> > somewhere?
> 
> If I knew the "where" part of this, I could put together a few sentences.
> I just grep-ed through Documentation/, without finding e.g. any rules
> or guidelines for using {,un}likely()...
>

At the definition site in vmlinux.lds?

Re: [PATCH v2] consolidate WARN_...ONCE() static variables

[Jan Beulich]

>>> On 28.02.12 at 08:44, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 28 Feb 2012 07:41:54 +0000 "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 28.02.12 at 01:03, Andrew Morton <akpm@linux-foundation.org> wrote:
>> > On Mon, 27 Feb 2012 15:10:34 +0000
>> > "Jan Beulich" <JBeulich@suse.com> wrote:
>> > 
>> >> Due to the alignment of following variables, these typically consume
>> >> more than just the single byte that 'bool' requires, and as there are
>> >> a few hundred instances, the cache pollution (not so much the waste of
>> >> memory) sums up. Put these variables into their own section, outside
>> >> of any half way frequently used memory range.
>> >> 
>> >> v2: Do the same also to the __warned variable of rcu_lockdep_assert().
>> >> (Don't, however, include the ones used by printk_once() and alike, as
>> >> they can potentially be hot.)
>> > 
>> > I have a bad feeling that I still don't understand this patch.  Ho hum.
>> > 
>> > What are the rules for the new .data.unlikely section?  When should
>> > people put variables into this section?  Perhaps we can document this
>> > somewhere?
>> 
>> If I knew the "where" part of this, I could put together a few sentences.
>> I just grep-ed through Documentation/, without finding e.g. any rules
>> or guidelines for using {,un}likely()...
>>
> 
> At the definition site in vmlinux.lds?

Sorry, Andrew, but this makes no sense to me. For one, vmlinux.lds{,h}
don't define anything, they merely gather together all input sections.
Second, the very similar use of .text.unlikely there isn't being explained
in any way either (which makes sense given that that's a compiler
generated section, which I just derived the new section's name from).

If anything, I would expect a place where all the special sections (.init*,
.exit*, etc) are being explained, but this (if existing at all) is scattered
around. While I don't mind adding a few words to something that
already exists, I'm not really eager to write something from scratch.

Finally, if we're really looking forward to having this used in a broader
manner (and particularly with objects wider than bool), then the
section name should get changed again anyway (and perhaps some
further abstraction be created): The goal of the patch is not only to
get the symbols out of potentially hot cache lines, but (secondary,
but nevertheless) also to avoid the non-negligible waste of space due
to padding (taking into consideration that the compiler doesn't really
do a good job in this regard, nor does it make meaningful attempts
at providing the necessary infrastructure for doing so manually).

Jan

Re: [PATCH v2] consolidate WARN_...ONCE() static variables

[Andrew Morton]

On Tue, 28 Feb 2012 08:02:40 +0000 "Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 28.02.12 at 08:44, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Tue, 28 Feb 2012 07:41:54 +0000 "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> >>> On 28.02.12 at 01:03, Andrew Morton <akpm@linux-foundation.org> wrote:
> >> > On Mon, 27 Feb 2012 15:10:34 +0000
> >> > "Jan Beulich" <JBeulich@suse.com> wrote:
> >> > 
> >> >> Due to the alignment of following variables, these typically consume
> >> >> more than just the single byte that 'bool' requires, and as there are
> >> >> a few hundred instances, the cache pollution (not so much the waste of
> >> >> memory) sums up. Put these variables into their own section, outside
> >> >> of any half way frequently used memory range.
> >> >> 
> >> >> v2: Do the same also to the __warned variable of rcu_lockdep_assert().
> >> >> (Don't, however, include the ones used by printk_once() and alike, as
> >> >> they can potentially be hot.)
> >> > 
> >> > I have a bad feeling that I still don't understand this patch.  Ho hum.
> >> > 
> >> > What are the rules for the new .data.unlikely section?  When should
> >> > people put variables into this section?  Perhaps we can document this
> >> > somewhere?
> >> 
> >> If I knew the "where" part of this, I could put together a few sentences.
> >> I just grep-ed through Documentation/, without finding e.g. any rules
> >> or guidelines for using {,un}likely()...
> >>
> > 
> > At the definition site in vmlinux.lds?
> 
> Sorry, Andrew, but this makes no sense to me.

I really don't care - anywhere you like.  Just the darn changelog, if
nowhere else.

I still don't have an answer to my question :( Some statement
describing what the new section is *for*.  Seems a pretty important
thing?

Re: [PATCH v2] consolidate WARN_...ONCE() static variables

[Jan Beulich]

>>> On 28.02.12 at 09:12, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 28 Feb 2012 08:02:40 +0000 "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 28.02.12 at 08:44, Andrew Morton <akpm@linux-foundation.org> wrote:
>> > On Tue, 28 Feb 2012 07:41:54 +0000 "Jan Beulich" <JBeulich@suse.com> wrote:
>> > 
>> >> >>> On 28.02.12 at 01:03, Andrew Morton <akpm@linux-foundation.org> wrote:
>> >> > On Mon, 27 Feb 2012 15:10:34 +0000
>> >> > "Jan Beulich" <JBeulich@suse.com> wrote:
>> >> > 
>> >> >> Due to the alignment of following variables, these typically consume
>> >> >> more than just the single byte that 'bool' requires, and as there are
>> >> >> a few hundred instances, the cache pollution (not so much the waste of
>> >> >> memory) sums up. Put these variables into their own section, outside
>> >> >> of any half way frequently used memory range.
>> >> >> 
>> >> >> v2: Do the same also to the __warned variable of rcu_lockdep_assert().
>> >> >> (Don't, however, include the ones used by printk_once() and alike, as
>> >> >> they can potentially be hot.)
>> >> > 
>> >> > I have a bad feeling that I still don't understand this patch.  Ho hum.
>> >> > 
>> >> > What are the rules for the new .data.unlikely section?  When should
>> >> > people put variables into this section?  Perhaps we can document this
>> >> > somewhere?
>> >> 
>> >> If I knew the "where" part of this, I could put together a few sentences.
>> >> I just grep-ed through Documentation/, without finding e.g. any rules
>> >> or guidelines for using {,un}likely()...
>> >>
>> > 
>> > At the definition site in vmlinux.lds?
>> 
>> Sorry, Andrew, but this makes no sense to me.
> 
> I really don't care - anywhere you like.  Just the darn changelog, if
> nowhere else.
> 
> I still don't have an answer to my question :( Some statement
> describing what the new section is *for*.  Seems a pretty important
> thing?

Oh, sorry - to carry static data the accesses to which are unlikely
(i.e., as in the case given, fully contained in code sections inside
conditionals which themselves use unlikely() on their primary/only
clause - in other words, something that the compiler really could
do on its own).

Jan

Re: [PATCH v2] consolidate WARN_...ONCE() static variables

[Andrew Morton]

On Tue, 28 Feb 2012 08:16:40 +0000 "Jan Beulich" <JBeulich@suse.com> wrote:

> Oh, sorry - to carry static data the accesses to which are unlikely
> (i.e., as in the case given, fully contained in code sections inside
> conditionals which themselves use unlikely() on their primary/only
> clause - in other words, something that the compiler really could
> do on its own).

I think I just learned more about this patch than at any time since we
started discussing it.

Why add a new section, rather than using __read_mostly?

I suppose we should add and use a #define for this, like __read_mostly.
That would be a good site for documenting it ;)

And I come back to my old friend printk_once().  If I'm understanding
things correctly, we can/should make that test unlikely, then mark
__print_once as __this_new_section?  Otherwise... help!

btw, I don't think there's a significant performance benefit here - if
the kernel is ever executing WARN_ON_ONCE(), WARN_ONCE() or
printk_once() with any frequency then it is already badly broken.

Which brings us down to saving a bit of space.  And I don't think I see
how this saves space?

Re: [PATCH v2] consolidate WARN_...ONCE() static variables

[Jan Beulich]

>>> On 28.02.12 at 09:32, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Tue, 28 Feb 2012 08:16:40 +0000 "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> Oh, sorry - to carry static data the accesses to which are unlikely
>> (i.e., as in the case given, fully contained in code sections inside
>> conditionals which themselves use unlikely() on their primary/only
>> clause - in other words, something that the compiler really could
>> do on its own).
> 
> I think I just learned more about this patch than at any time since we
> started discussing it.
> 
> Why add a new section, rather than using __read_mostly?

Because __read_mostly data frequently is hot.

> I suppose we should add and use a #define for this, like __read_mostly.
> That would be a good site for documenting it ;)

That's a possibility of course. I'll do so in an eventual v3.

> And I come back to my old friend printk_once().  If I'm understanding
> things correctly, we can/should make that test unlikely, then mark
> __print_once as __this_new_section?  Otherwise... help!

No. The variable itself serves for the tested condition here, and
hence a priori you can't say whether the whole construct sits in
a hot path (whether putting it in a hot path is a good idea is
another question, but the print-this-one-time-only nature of it
may be the very reason why someone considers this reasonable
for his code).

As opposed to that, the access to the static variable in WARN_ONCE()
is *after* an unlikely() condition was already evaluated, i.e. would
generally sit on a code path that we hope doesn't get speculated
along (often), by means of the compiler suitably arranging branch
targets.

> btw, I don't think there's a significant performance benefit here - if
> the kernel is ever executing WARN_ON_ONCE(), WARN_ONCE() or
> printk_once() with any frequency then it is already badly broken.

The intended performance benefit isn't with the WARN_ONCE()
constructs - we certainly don't care much about them being executed
efficiently - but instead with code path accessing other data in the
same cache line (or spilled across multiple cache lines just because
of the __warned variable sitting in the middle).

> Which brings us down to saving a bit of space.  And I don't think I see
> how this saves space?

The space saving results from grouping (many) 1-byte entities
together, which (when emitted normally) will generally require
padding to 4 or 8 bytes (as being adjacent with other static data
in the same or next compilation unit). That padding won't occur
if all of the items in a given section are of the same size (and
alignment).

Jan

Re: [PATCH v2] consolidate WARN_...ONCE() static variables

[Andrew Morton]

I think I now understand this patch.  This was far, far too hard!

On Tue, 28 Feb 2012 08:58:58 +0000
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 28.02.12 at 09:32, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Tue, 28 Feb 2012 08:16:40 +0000 "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> Oh, sorry - to carry static data the accesses to which are unlikely
> >> (i.e., as in the case given, fully contained in code sections inside
> >> conditionals which themselves use unlikely() on their primary/only
> >> clause - in other words, something that the compiler really could
> >> do on its own).
> > 
> > I think I just learned more about this patch than at any time since we
> > started discussing it.
> > 
> > Why add a new section, rather than using __read_mostly?
> 
> Because __read_mostly data frequently is hot.

I seem to remember discussing this last time.  That I had to ask was a
sign that it should have been changelogged!

> > I suppose we should add and use a #define for this, like __read_mostly.
> > That would be a good site for documenting it ;)
> 
> That's a possibility of course. I'll do so in an eventual v3.
> 
> > And I come back to my old friend printk_once().  If I'm understanding
> > things correctly, we can/should make that test unlikely, then mark
> > __print_once as __this_new_section?  Otherwise... help!
> 
> No. The variable itself serves for the tested condition here, and
> hence a priori you can't say whether the whole construct sits in
> a hot path (whether putting it in a hot path is a good idea is
> another question, but the print-this-one-time-only nature of it
> may be the very reason why someone considers this reasonable
> for his code).
>
> As opposed to that, the access to the static variable in WARN_ONCE()
> is *after* an unlikely() condition was already evaluated, i.e. would
> generally sit on a code path that we hope doesn't get speculated
> along (often), by means of the compiler suitably arranging branch
> targets.

OK, so there's a distinction here between storage which is frequently
touched and storage which is infrequently touched, because it is inside
an unlikely() block.

So the idea behind the patch is to use the "unlikely" as a sign that
the data is rarely touched, so we can move it into its own section to
prevent it from adding sparseness to data which is more frequently
touched.

Correct?  If so, that's key: please copy-n-paste this into the
changelog.

> > btw, I don't think there's a significant performance benefit here - if
> > the kernel is ever executing WARN_ON_ONCE(), WARN_ONCE() or
> > printk_once() with any frequency then it is already badly broken.
> 
> The intended performance benefit isn't with the WARN_ONCE()
> constructs - we certainly don't care much about them being executed
> efficiently - but instead with code path accessing other data in the
> same cache line (or spilled across multiple cache lines just because
> of the __warned variable sitting in the middle).

Right.

> > Which brings us down to saving a bit of space.  And I don't think I see
> > how this saves space?
> 
> The space saving results from grouping (many) 1-byte entities
> together, which (when emitted normally) will generally require
> padding to 4 or 8 bytes (as being adjacent with other static data
> in the same or next compilation unit). That padding won't occur
> if all of the items in a given section are of the same size (and
> alignment).

OK.  So the new section should only be used for static bool (or static
char)?

Re: [PATCH v2] consolidate WARN_...ONCE() static variables

[Jan Beulich]

>>> On 28.02.12 at 22:19, Andrew Morton <akpm@linux-foundation.org> wrote:
> So the idea behind the patch is to use the "unlikely" as a sign that
> the data is rarely touched, so we can move it into its own section to
> prevent it from adding sparseness to data which is more frequently
> touched.
> 
> Correct?  If so, that's key: please copy-n-paste this into the
> changelog.

Done so for the next revision.

>> The space saving results from grouping (many) 1-byte entities
>> together, which (when emitted normally) will generally require
>> padding to 4 or 8 bytes (as being adjacent with other static data
>> in the same or next compilation unit). That padding won't occur
>> if all of the items in a given section are of the same size (and
>> alignment).
> 
> OK.  So the new section should only be used for static bool (or static
> char)?

That's a little too strong, but yes, the change would be less
efficient when objects with an alignment requirement of more than a
byte would get added to that same section.

Avoiding this is difficult though: gcc doesn't allow section name
templates (where e.g. the alignment could be embedded in the
section name by the compiler), nor does it allow any other means
to generate the section name on the fly in all possible cases (it
would only be possible when the object has function scope, as
there we could use an asm() for doing this). And requiring to
_literally_ (i.e. without any alignof() or sizeof()) specify the
respective number would be rather ugly (and calling for mistakes).
I can't think of other alternatives.

Jan