| Message ID | f25eb5c9-7c14-6e23-8535-2c66772b333e@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | move __read_mostly to xen/cache.h | expand |
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 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
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
--- 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 */
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>