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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.03.2024 13:01, Oleksii wrote:
> 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.

No, no, I'd be happy with xen/linkage.h, if there wasn't the need to
then add perhaps many dozens of #include-s throughout the tree.

> 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?

I'd say not ahead of there actually arising such a need.

Jan