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@xxxxxxxx> wrote:

> >>> On 28.02.12 at 09:32, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> > On Tue, 28 Feb 2012 08:16:40 +0000 "Jan Beulich" <JBeulich@xxxxxxxx> 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)?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/