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@suse.com>

--- a/xen/arch/arm/include/asm/cache.h
+++ b/xen/arch/arm/include/asm/cache.h
@@ -6,8 +6,6 @@
 #define L1_CACHE_SHIFT  (CONFIG_ARM_L1_CACHE_SHIFT)
 #define L1_CACHE_BYTES  (1 << L1_CACHE_SHIFT)
 
-#define __read_mostly __section(".data.read_mostly")
-
 #endif
 /*
  * Local variables:
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -34,13 +34,13 @@
 #include <xen/lib.h>
 #include <xen/types.h>
 #include <xen/acpi.h>
+#include <xen/cache.h>
 #include <xen/smp.h>
 #include <xen/guest_access.h>
 #include <xen/keyhandler.h>
 #include <xen/param.h>
 #include <xen/trace.h>
 #include <xen/irq.h>
-#include <asm/cache.h>
 #include <asm/io.h>
 #include <asm/iocap.h>
 #include <asm/hpet.h>
--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -7,11 +7,11 @@
  * Copyright (c) 2003-2006, K A Fraser
  */
 
+#include <xen/cache.h>
 #include <xen/paging.h>
 #include <xen/sched.h>
 #include <xen/smp.h>
 #include <xen/softirq.h>
-#include <asm/cache.h>
 #include <asm/flushtlb.h>
 #include <asm/invpcid.h>
 #include <asm/nops.h>
--- a/xen/arch/x86/genapic/probe.c
+++ b/xen/arch/x86/genapic/probe.c
@@ -6,10 +6,10 @@
 #include <xen/cpumask.h>
 #include <xen/string.h>
 #include <xen/kernel.h>
+#include <xen/cache.h>
 #include <xen/ctype.h>
 #include <xen/init.h>
 #include <xen/param.h>
-#include <asm/cache.h>
 #include <asm/fixmap.h>
 #include <asm/mpspec.h>
 #include <asm/apicdef.h>
--- a/xen/arch/x86/guest/hypervisor.c
+++ b/xen/arch/x86/guest/hypervisor.c
@@ -6,11 +6,11 @@
  *
  * Copyright (c) 2019 Microsoft.
  */
+#include <xen/cache.h>
 #include <xen/cpumask.h>
 #include <xen/init.h>
 #include <xen/types.h>
 
-#include <asm/cache.h>
 #include <asm/guest.h>
 
 static struct hypervisor_ops __read_mostly ops;
--- a/xen/arch/x86/include/asm/cache.h
+++ b/xen/arch/x86/include/asm/cache.h
@@ -9,8 +9,6 @@
 #define L1_CACHE_SHIFT	(CONFIG_X86_L1_CACHE_SHIFT)
 #define L1_CACHE_BYTES	(1 << L1_CACHE_SHIFT)
 
-#define __read_mostly __section(".data.read_mostly")
-
 #ifndef __ASSEMBLY__
 
 void cache_flush(const void *addr, unsigned int size);
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -1,3 +1,4 @@
+#include <xen/cache.h>
 #include <xen/compile.h>
 #include <xen/init.h>
 #include <xen/errno.h>
@@ -8,8 +9,6 @@
 #include <xen/elf.h>
 #include <xen/version.h>
 
-#include <asm/cache.h>
-
 const char *xen_compile_date(void)
 {
     return XEN_COMPILE_DATE;
--- a/xen/include/xen/cache.h
+++ b/xen/include/xen/cache.h
@@ -15,6 +15,7 @@
 #define __cacheline_aligned __attribute__((__aligned__(SMP_CACHE_BYTES)))
 #endif
 
+#define __read_mostly   __section(".data.read_mostly")
 #define __ro_after_init __section(".data.ro_after_init")
 
 #endif /* __LINUX_CACHE_H */

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@suse.com>

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.

~Andre

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@suse.com>
> 
> 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). In the absence of this, can we
perhaps deal with this in a 2nd step, thus not blocking this patch and
therefore not needing to then also clean up PPC-specific code?

Jan

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@suse.com>
> > 
> > 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@citrix.com/

Will that work for you?

>  In the absence of this, can we
> perhaps deal with this in a 2nd step, thus not blocking this patch
> and
> therefore not needing to then also clean up PPC-specific code?

~ Oleksii

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@suse.com>
>>>
>>> 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@citrix.com/
> 
> Will that work for you?

I still need to properly look at the suggested options.

Jan

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@suse.com>
> > > > 
> > > > 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@citrix.com/
> > 
> > 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?

~ Oleksii

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@suse.com>
>>>>>
>>>>> 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@citrix.com/
>>>
>>> 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.

Jan

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@suse.com>
> > > > > > 
> > > > > > 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@citrix.com/
> > > > 
> > > > 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

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@suse.com>
>>>>>>>
>>>>>>> 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@citrix.com/
>>>>>
>>>>> 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

On 08.08.2023 12:32, 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@suse.com>
>>
>> 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). In the absence of this, can we
> perhaps deal with this in a 2nd step, thus not blocking this patch and
> therefore not needing to then also clean up PPC-specific code?

Oh, also: I we move them elsewhere, it wouldn't be logical for xen/cache.h
to include that other header as well. Yet without that the risk of build
breakages (perhaps in only exotic configs) is of course quite a bit higher.

Jan