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

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

[Jan Beulich]

>>> On 22.12.11 at 18:13, Arnaud Lacombe <lacombar@gmail.com> wrote:
> On Thu, Dec 22, 2011 at 10:53 AM, 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 op. Put these variables into their own section, outside
>> of half way frequently used memory range.
>>
> how does this relates to kbuild exactly ? I see you did not even CC'ed LKML.

In touching include/asm-generic/vmlinux.lds.h (and not touching
anything outside include/asm-generic/).

And I wasn't aware that any subsystem change needs to go to lkml too;
adding that list now.

> btw, it might be interesting to have numbers backing this change :)

Sort of difficult, as this (obviously) varies by configuration. But as
stated, it's generally a few hundred instances. Additionally, I find it
odd that any small and obvious (at least I view is as such, and it
should have been coded this way from the beginning) improvement
is being asked to be backed by precise numbers.

The one thing I wasn't sure about was whether this shouldn't be done
in a more abstract way, to allow use in other places as well. But such
abstraction can certainly be added incrementally.

Jan

>> --- 3.2-rc6/include/asm-generic/bug.h
>> +++ 3.2-rc6-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.2-rc6/include/asm-generic/vmlinux.lds.h
>> +++ 3.2-rc6-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 */                            \
>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
>> the body of a message to majordomo@vger.kernel.org 
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html 

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

[Jan Beulich]

>>> On 05.01.12 at 22:03, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 05 Jan 2012 11:09:40 +0000
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 05.01.12 at 00:03, Andrew Morton <akpm@linux-foundation.org> wrote:
>> > On Thu, 22 Dec 2011 15:53:49 +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 op. Put these variables into their own section, outside
>> >> of half way frequently used memory range.
>> >> 
>>
>> ...
>>
>> > printk_once() should also be converted.  And ata_print_version_once(),
>> > if it insists on continuing to exist.
>> 
>> I disagree for those (and intentionally didn't touch printk_once();
>> wasn't aware of the other) - at best this could get marked
>> __read_mostly, but that's not the subject of this patch.
> 
> Confused.  It is exactly the subject of the patch?

No - the goal here is to eliminate the wasteful alignment holes
created by the __warned variables in the WARN_...ONCE()
instances. These get accessed past and unlikely() condition,
and hence get moved into a separate data section (so they
would all end up together, with no holes in between).

The printk_once() situation is quite different: The variables get
accessed each time the code in question gets executed, and
hence cannot be considered on an unlikely code path without
additional knowledge of the surrounding code. Consequently
they don't belong into the same data section as the ones that
get moved here.

>> > Also rcu_lockdep_assert().
>> 
>> This one I mostly agree - the access is at least sitting past the
>> debug_lockdep_rcu_enabled() check, albeit not in an unlikely()
>> code path.
> 
> What does "unlikely" have to do with any of this?

See above.

> I'm suspecting that there is some changelog crappiness going on here. 
> What didn't you tell us?

I think the original description says all that it has to. It doesn't
care to say why others don't get converted, but I don't think
that's to be expected of it either (imagine how patch descriptions
would start looking like if everything that wasn't done in patch
was also described).

Jan

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

[Andrew Morton]

On Fri, 06 Jan 2012 07:40:29 +0000 "Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 05.01.12 at 22:03, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Thu, 05 Jan 2012 11:09:40 +0000
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> >>> On 05.01.12 at 00:03, Andrew Morton <akpm@linux-foundation.org> wrote:
> >> > On Thu, 22 Dec 2011 15:53:49 +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 op. Put these variables into their own section, outside
> >> >> of half way frequently used memory range.
> >> >> 
> >>
> >> ...
> >>
> >> > printk_once() should also be converted.  And ata_print_version_once(),
> >> > if it insists on continuing to exist.
> >> 
> >> I disagree for those (and intentionally didn't touch printk_once();
> >> wasn't aware of the other) - at best this could get marked
> >> __read_mostly, but that's not the subject of this patch.
> > 
> > Confused.  It is exactly the subject of the patch?
> 
> No - the goal here is to eliminate the wasteful alignment holes
> created by the __warned variables in the WARN_...ONCE()
> instances.

What are these alignment holes?  I'd assumed (without thinking a lot)
that they were little three or two byte gaps because sizeof(bool)=1 or
2.  But I see that sizeof(bool) is actually 4, so I don't know what
you're talking about.

Apparently there is some gcc behaviour which you know about and I
don't.

> These get accessed past and unlikely() condition,
> and hence get moved into a separate data section (so they
> would all end up together, with no holes in between).
>
>
> ...
>
> > I'm suspecting that there is some changelog crappiness going on here. 
> > What didn't you tell us?
> 
> I think the original description says all that it has to.

If it did that, I wouldn't have had any questions to ask you.

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

[Jan Beulich]

>>> On 06.01.12 at 12:03, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 06 Jan 2012 07:40:29 +0000 "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 05.01.12 at 22:03, Andrew Morton <akpm@linux-foundation.org> wrote:
>> > On Thu, 05 Jan 2012 11:09:40 +0000
>> > "Jan Beulich" <JBeulich@suse.com> wrote:
>> > 
>> >> >>> On 05.01.12 at 00:03, Andrew Morton <akpm@linux-foundation.org> wrote:
>> >> > On Thu, 22 Dec 2011 15:53:49 +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 op. Put these variables into their own section, outside
>> >> >> of half way frequently used memory range.
>> >> >> 
>> >>
>> >> ...
>> >>
>> >> > printk_once() should also be converted.  And ata_print_version_once(),
>> >> > if it insists on continuing to exist.
>> >> 
>> >> I disagree for those (and intentionally didn't touch printk_once();
>> >> wasn't aware of the other) - at best this could get marked
>> >> __read_mostly, but that's not the subject of this patch.
>> > 
>> > Confused.  It is exactly the subject of the patch?
>> 
>> No - the goal here is to eliminate the wasteful alignment holes
>> created by the __warned variables in the WARN_...ONCE()
>> instances.
> 
> What are these alignment holes?  I'd assumed (without thinking a lot)
> that they were little three or two byte gaps because sizeof(bool)=1 or
> 2.

The holes are typically sizeof(long) - 1 bytes (i.e. 7 on 64-bit), with
some cases of smaller/larger ones.

> But I see that sizeof(bool) is actually 4, so I don't know what
> you're talking about.

They're 1-byte variables, and System.map confirms this (with the
patch in place):

ffffffff81638808 d __warned.24033
ffffffff81638809 d __warned.25205
ffffffff8163880a d __warned.30138
ffffffff8163880b d __warned.27363
...

> Apparently there is some gcc behaviour which you know about and I
> don't.

No magic gcc behavior is involved here afaict - this example

typedef _Bool bool;

bool b;

bool test(void) {
	return b;
}

unsigned size(void) {
	return sizeof(bool);
}

unsigned align(void) {
	return __alignof__(bool);
}

confirms that with whatever gcc I try, sizeof and __alignof__
produce 1 on x86.

Jan