Re: [PATCH] move __read_mostly to xen/cache.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Fri, 2024-03-08 at 09:22 +0100, Jan Beulich wrote:
> On 07.03.2024 18:08, Oleksii wrote:
> > On Fri, 2023-12-22 at 12:09 +0100, Jan Beulich wrote:
> > > On 22.12.2023 10:39, Oleksii wrote:
> > > > On Tue, 2023-08-08 at 12:32 +0200, Jan Beulich wrote:
> > > > > On 08.08.2023 12:18, Andrew Cooper wrote:
> > > > > > On 08/08/2023 10:46 am, Jan Beulich wrote:
> > > > > > > There's no need for every arch to define its own
> > > > > > > identical
> > > > > > > copy.
> > > > > > > If down
> > > > > > > the road an arch needs to customize it, we can add
> > > > > > > #ifndef
> > > > > > > around
> > > > > > > the
> > > > > > > common #define.
> > > > > > > 
> > > > > > > To be on the safe side build-breakage-wise, change a
> > > > > > > couple
> > > > > > > of
> > > > > > > #include
> > > > > > > <asm/cache.h> to the xen/ equivalent.
> > > > > > > 
> > > > > > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > > > 
> > > > > > Could we find a better place to put this?
> > > > > > 
> > > > > > __read_mostly is just a section.  It's relationship to the
> > > > > > cache is
> > > > > > only
> > > > > > microarchitectural, and is not the same kind of information
> > > > > > as
> > > > > > the
> > > > > > rest
> > > > > > of cache.h
> > > > > > 
> > > > > > __ro_after_init is only here because __read_mostly is here,
> > > > > > but
> > > > > > has
> > > > > > absolutely nothing to do with caches whatsoever.
> > > > > > 
> > > > > > If we're cleaning them up, they ought to live elsewhere.
> > > > > 
> > > > > I would be considering init.h (for having most other
> > > > > __section()
> > > > > uses,
> > > > > and for also needing __read_mostly), but that's not a great
> > > > > place
> > > > > to
> > > > > put these either. In fact I see less connection there than
> > > > > for
> > > > > cache.h.
> > > > > So the primary need is a good suggestion (I'm hesitant to
> > > > > suggest
> > > > > to
> > > > > introduce section.h just for this).
> > > > Andrew sent some suggestions here:
> > > > https://lore.kernel.org/xen-devel/3df1dad8-3476-458f-9022-160e0af57d39@xxxxxxxxxx/
> > > > 
> > > > Will that work for you?
> > > 
> > > I still need to properly look at the suggested options.
> > Have you had a chance to review the suggested options?
> 
> I'm sure you've seen
> 
> https://lists.xen.org/archives/html/xen-devel/2024-01/msg00145.html
> 
> To add to that - xen/linkage.h is for assembly code only right now.
> While
> I'd be happy to add C stuff there, there's an (imo) obvious issue
> with
> code churn then: All C files using __read_mostly would then need to
> be
> changed to include xen/linkage.h. And no, I don't view including the
> file
> once in a "central" other header file as a viable solution: That's
> where
> some of our really bad header dependency issues come from. Plus a
> goal
> ought to be (imo) that touching a header like this one would better
> not
> result in a full re-build of everything, when doing incremental
> builds.
> 
> Same obviously goes for the case of introducing xen/sections.h, i.e.
> the
> other proposed alternative.
> 
> Bottom line: Given the state of our tree, I still view my proposed
> placement as the least bad one for the time being. To change my view,
> I'd still expect a _viable_ alternative proposal to be made.
Based on your replies, I can conclude that there is no good place for
__read_mostly and __ro_after_init.

If my conclusion is correct, then an introduction of a new header is
required. I totally agree that the inclusion of the introduced header
to 'central' header only for the reason not to update all C files using
__read_mostly macros is not a good solution, but I don't see an issue
with an update of all C files which use __read_mostly/__ro_after_init
if it is required. I realize that it can be a huge amount of files, but
if the situation requires that, it looks not so bad solution. If to
look at places where <xen/cache.h> is included now, then there wouldn't
be too many places that needed to be updated.

Despite this fact, I don't think that an introduction of xen/section.h
is a bad solution, xen/cache.h can also be a good place for it as my
understanding of __read_mostly is that it is not only meant for just
read-only data, there are performance reasons for using it:
/*
* __read_mostly is used to keep rarely changing variables out of
frequently
* updated cachelines. Its use should be reserved for data that is used
* frequently in hot paths. Performance traces can help decide when to
use
* this. You want __read_mostly data to be tightly packed, so that in
the
* best case multiple frequently read variables for a hot path will be
next
* to each other in order to reduce the number of cachelines needed to
* execute a critical path.
*/

Not related to my words above, here is a little remark about the patch
changes. Does it make sense to wrap the definition of __read_mostly()
by "#ifndef __read_mostly" in case architecture decides to redefine it?

~ Oleksii