* Re: [PATCH] consolidate WARN_...ONCE() static variables [not found] ` <CACqU3MWahjXy3q6AqDARCfPx5r3WF+uoHXZA6mkkpNKDjWcSMA@mail.gmail.com> @ 2011-12-23 7:52 ` Jan Beulich 0 siblings, 0 replies; 4+ messages in thread From: Jan Beulich @ 2011-12-23 7:52 UTC (permalink / raw) To: Arnaud Lacombe; +Cc: akpm, Michal Marek, linux-kbuild, linux-kernel >>> 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <20120104150305.8b2ab00c.akpm@linux-foundation.org>]
[parent not found: <4F059304020000780006A906@nat28.tlf.novell.com>]
[parent not found: <20120105130324.44949af9.akpm@linux-foundation.org>]
* Re: [PATCH] consolidate WARN_...ONCE() static variables [not found] ` <20120105130324.44949af9.akpm@linux-foundation.org> @ 2012-01-06 7:40 ` Jan Beulich 2012-01-06 11:03 ` Andrew Morton 0 siblings, 1 reply; 4+ messages in thread From: Jan Beulich @ 2012-01-06 7:40 UTC (permalink / raw) To: Andrew Morton; +Cc: Michal Marek, linux-kbuild, linux-kernel >>> 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] consolidate WARN_...ONCE() static variables 2012-01-06 7:40 ` Jan Beulich @ 2012-01-06 11:03 ` Andrew Morton 2012-01-06 12:38 ` Jan Beulich 0 siblings, 1 reply; 4+ messages in thread From: Andrew Morton @ 2012-01-06 11:03 UTC (permalink / raw) To: Jan Beulich; +Cc: Michal Marek, linux-kbuild, linux-kernel 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] consolidate WARN_...ONCE() static variables 2012-01-06 11:03 ` Andrew Morton @ 2012-01-06 12:38 ` Jan Beulich 0 siblings, 0 replies; 4+ messages in thread From: Jan Beulich @ 2012-01-06 12:38 UTC (permalink / raw) To: Andrew Morton; +Cc: Michal Marek, linux-kbuild, linux-kernel >>> 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-01-06 12:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <4EF3609D0200007800069A30@nat28.tlf.novell.com>
[not found] ` <CACqU3MWahjXy3q6AqDARCfPx5r3WF+uoHXZA6mkkpNKDjWcSMA@mail.gmail.com>
2011-12-23 7:52 ` [PATCH] consolidate WARN_...ONCE() static variables Jan Beulich
[not found] ` <20120104150305.8b2ab00c.akpm@linux-foundation.org>
[not found] ` <4F059304020000780006A906@nat28.tlf.novell.com>
[not found] ` <20120105130324.44949af9.akpm@linux-foundation.org>
2012-01-06 7:40 ` Jan Beulich
2012-01-06 11:03 ` Andrew Morton
2012-01-06 12:38 ` Jan Beulich
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox