* Re: [PATCH for-4.19] xen/arch: Centralise __read_mostly and __ro_after_init
2024-06-14 12:49 [PATCH for-4.19] xen/arch: Centralise __read_mostly and __ro_after_init Andrew Cooper
@ 2024-06-14 13:39 ` Jan Beulich
2024-06-14 13:51 ` Roger Pau Monné
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2024-06-14 13:39 UTC (permalink / raw)
To: Andrew Cooper
Cc: Roger Pau Monné, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Bertrand Marquis, Michal Orzel,
Oleksii Kurochko, Shawn Anastasio, Xen-devel
On 14.06.2024 14:49, Andrew Cooper wrote:
> These being in cache.h is inherited from Linux, but is an inappropriate
> location to live.
>
> __read_mostly is an optimisation related to data placement in order to avoid
> having shared data in cachelines that are likely to be written to, but it
> really is just a section of the linked image separating data by usage
> patterns; it has nothing to do with cache sizes or flushing logic.
>
> Worse, __ro_after_init was only in xen/cache.h because __read_mostly was in
> arch/cache.h, and has literally nothing whatsoever to do with caches.
>
> Move the definitions into xen/sections.h, which in paritcular means that
> RISC-V doesn't need to repeat the problematic pattern. Take the opportunity
> to provide a short descriptions of what these are used for.
>
> For now, leave TODO comments next to the other identical definitions. It
> turns out that unpicking cache.h is more complicated than it appears because a
> number of files use it for transitive dependencies.
I don't particularly mind this approach, so ...
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Yet this (or variants thereof) is precisely what I wouldn't have wanted to do
myself, for leaving garbled state and, if I'm not mistaken, introducing new
Misra violations because of the redundant #define-s.
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH for-4.19] xen/arch: Centralise __read_mostly and __ro_after_init
2024-06-14 12:49 [PATCH for-4.19] xen/arch: Centralise __read_mostly and __ro_after_init Andrew Cooper
2024-06-14 13:39 ` Jan Beulich
@ 2024-06-14 13:51 ` Roger Pau Monné
2024-06-18 23:11 ` Stefano Stabellini
2024-06-14 14:55 ` Oleksii K.
2024-06-20 17:22 ` Shawn Anastasio
3 siblings, 1 reply; 7+ messages in thread
From: Roger Pau Monné @ 2024-06-14 13:51 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Jan Beulich, Stefano Stabellini, Julien Grall,
Volodymyr Babchuk, Bertrand Marquis, Michal Orzel,
Oleksii Kurochko, Shawn Anastasio
On Fri, Jun 14, 2024 at 01:49:50PM +0100, Andrew Cooper wrote:
> These being in cache.h is inherited from Linux, but is an inappropriate
> location to live.
>
> __read_mostly is an optimisation related to data placement in order to avoid
> having shared data in cachelines that are likely to be written to, but it
> really is just a section of the linked image separating data by usage
> patterns; it has nothing to do with cache sizes or flushing logic.
>
> Worse, __ro_after_init was only in xen/cache.h because __read_mostly was in
> arch/cache.h, and has literally nothing whatsoever to do with caches.
>
> Move the definitions into xen/sections.h, which in paritcular means that
> RISC-V doesn't need to repeat the problematic pattern. Take the opportunity
> to provide a short descriptions of what these are used for.
>
> For now, leave TODO comments next to the other identical definitions. It
> turns out that unpicking cache.h is more complicated than it appears because a
> number of files use it for transitive dependencies.
I assume that including sections.h from cache.h (in the meantime)
creates a circular header dependency?
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> CC: Shawn Anastasio <sanastasio@raptorengineering.com>
>
> For 4.19. This is to help the RISC-V "full build of Xen" effort without
> introducing a pattern that's going to require extra effort to undo after the
> fact.
> ---
> xen/arch/arm/include/asm/cache.h | 1 +
> xen/arch/ppc/include/asm/cache.h | 1 +
> xen/arch/x86/include/asm/cache.h | 1 +
> xen/include/xen/cache.h | 1 +
> xen/include/xen/sections.h | 21 +++++++++++++++++++++
> 5 files changed, 25 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/cache.h b/xen/arch/arm/include/asm/cache.h
> index 240b6ae0eaa3..029b2896fb3e 100644
> --- a/xen/arch/arm/include/asm/cache.h
> +++ b/xen/arch/arm/include/asm/cache.h
> @@ -6,6 +6,7 @@
> #define L1_CACHE_SHIFT (CONFIG_ARM_L1_CACHE_SHIFT)
> #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
>
> +/* TODO: Phase out the use of this via cache.h */
> #define __read_mostly __section(".data.read_mostly")
>
> #endif
> diff --git a/xen/arch/ppc/include/asm/cache.h b/xen/arch/ppc/include/asm/cache.h
> index 0d7323d7892f..13c0bf3242c8 100644
> --- a/xen/arch/ppc/include/asm/cache.h
> +++ b/xen/arch/ppc/include/asm/cache.h
> @@ -3,6 +3,7 @@
> #ifndef _ASM_PPC_CACHE_H
> #define _ASM_PPC_CACHE_H
>
> +/* TODO: Phase out the use of this via cache.h */
> #define __read_mostly __section(".data.read_mostly")
>
> #endif /* _ASM_PPC_CACHE_H */
> diff --git a/xen/arch/x86/include/asm/cache.h b/xen/arch/x86/include/asm/cache.h
> index e4770efb22b9..956c05493e23 100644
> --- a/xen/arch/x86/include/asm/cache.h
> +++ b/xen/arch/x86/include/asm/cache.h
> @@ -9,6 +9,7 @@
> #define L1_CACHE_SHIFT (CONFIG_X86_L1_CACHE_SHIFT)
> #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
>
> +/* TODO: Phase out the use of this via cache.h */
> #define __read_mostly __section(".data.read_mostly")
>
> #ifndef __ASSEMBLY__
> diff --git a/xen/include/xen/cache.h b/xen/include/xen/cache.h
> index f52a0aedf768..55456823c543 100644
> --- 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
>
> +/* TODO: Phase out the use of this via cache.h */
> #define __ro_after_init __section(".data.ro_after_init")
>
> #endif /* __LINUX_CACHE_H */
> diff --git a/xen/include/xen/sections.h b/xen/include/xen/sections.h
> index b6cb5604c285..6d4db2b38f0f 100644
> --- a/xen/include/xen/sections.h
> +++ b/xen/include/xen/sections.h
> @@ -3,9 +3,30 @@
> #ifndef __XEN_SECTIONS_H__
> #define __XEN_SECTIONS_H__
>
> +#include <xen/compiler.h>
> +
> /* SAF-0-safe */
> extern char __init_begin[], __init_end[];
>
> +/*
> + * Some data is expected to be written very rarely (if at all).
> + *
> + * For performance reasons is it helpful to group such data in the build, to
> + * avoid the linker placing it adjacent to often-written data.
> + */
> +#define __read_mostly __section(".data.read_mostly")
> +
> +/*
> + * Some data should be chosen during boot and be immutable thereafter.
> + *
> + * Variables annotated with __ro_after_init will become read-only after boot
> + * and suffer a runtime access fault if modified.
> + *
> + * For architectures/platforms which haven't implemented support, these
> + * variables will be treated as regular mutable data.
> + */
> +#define __ro_after_init __section(".data.ro_after_init")
Is it fine for MISRA to have possibly two identical defines in the
same translation unit?
Thanks, Roger.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH for-4.19] xen/arch: Centralise __read_mostly and __ro_after_init
2024-06-14 13:51 ` Roger Pau Monné
@ 2024-06-18 23:11 ` Stefano Stabellini
0 siblings, 0 replies; 7+ messages in thread
From: Stefano Stabellini @ 2024-06-18 23:11 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Andrew Cooper, Xen-devel, Jan Beulich, Stefano Stabellini,
Julien Grall, Volodymyr Babchuk, Bertrand Marquis, Michal Orzel,
Oleksii Kurochko, Shawn Anastasio
[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]
On Fri, 13 Jun 2024, Roger Pau Monné wrote:
> On Fri, Jun 14, 2024 at 01:49:50PM +0100, Andrew Cooper wrote:
> > These being in cache.h is inherited from Linux, but is an inappropriate
> > location to live.
> >
> > __read_mostly is an optimisation related to data placement in order to avoid
> > having shared data in cachelines that are likely to be written to, but it
> > really is just a section of the linked image separating data by usage
> > patterns; it has nothing to do with cache sizes or flushing logic.
> >
> > Worse, __ro_after_init was only in xen/cache.h because __read_mostly was in
> > arch/cache.h, and has literally nothing whatsoever to do with caches.
> >
> > Move the definitions into xen/sections.h, which in paritcular means that
> > RISC-V doesn't need to repeat the problematic pattern. Take the opportunity
> > to provide a short descriptions of what these are used for.
> >
> > For now, leave TODO comments next to the other identical definitions. It
> > turns out that unpicking cache.h is more complicated than it appears because a
> > number of files use it for transitive dependencies.
>
> I assume that including sections.h from cache.h (in the meantime)
> creates a circular header dependency?
Assuming this patch doesn't introduce ECLAIR regressions:
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH for-4.19] xen/arch: Centralise __read_mostly and __ro_after_init
2024-06-14 12:49 [PATCH for-4.19] xen/arch: Centralise __read_mostly and __ro_after_init Andrew Cooper
2024-06-14 13:39 ` Jan Beulich
2024-06-14 13:51 ` Roger Pau Monné
@ 2024-06-14 14:55 ` Oleksii K.
2024-06-20 17:22 ` Shawn Anastasio
3 siblings, 0 replies; 7+ messages in thread
From: Oleksii K. @ 2024-06-14 14:55 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Jan Beulich, Roger Pau Monné, Stefano Stabellini,
Julien Grall, Volodymyr Babchuk, Bertrand Marquis, Michal Orzel,
Shawn Anastasio
On Fri, 2024-06-14 at 13:49 +0100, Andrew Cooper wrote:
> These being in cache.h is inherited from Linux, but is an
> inappropriate
> location to live.
>
> __read_mostly is an optimisation related to data placement in order
> to avoid
> having shared data in cachelines that are likely to be written to,
> but it
> really is just a section of the linked image separating data by usage
> patterns; it has nothing to do with cache sizes or flushing logic.
>
> Worse, __ro_after_init was only in xen/cache.h because __read_mostly
> was in
> arch/cache.h, and has literally nothing whatsoever to do with caches.
>
> Move the definitions into xen/sections.h, which in paritcular means
> that
> RISC-V doesn't need to repeat the problematic pattern. Take the
> opportunity
> to provide a short descriptions of what these are used for.
>
> For now, leave TODO comments next to the other identical
> definitions. It
> turns out that unpicking cache.h is more complicated than it appears
> because a
> number of files use it for transitive dependencies.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-Acked-By: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> CC: Shawn Anastasio <sanastasio@raptorengineering.com>
>
> For 4.19. This is to help the RISC-V "full build of Xen" effort
> without
> introducing a pattern that's going to require extra effort to undo
> after the
> fact.
> ---
> xen/arch/arm/include/asm/cache.h | 1 +
> xen/arch/ppc/include/asm/cache.h | 1 +
> xen/arch/x86/include/asm/cache.h | 1 +
> xen/include/xen/cache.h | 1 +
> xen/include/xen/sections.h | 21 +++++++++++++++++++++
> 5 files changed, 25 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/cache.h
> b/xen/arch/arm/include/asm/cache.h
> index 240b6ae0eaa3..029b2896fb3e 100644
> --- a/xen/arch/arm/include/asm/cache.h
> +++ b/xen/arch/arm/include/asm/cache.h
> @@ -6,6 +6,7 @@
> #define L1_CACHE_SHIFT (CONFIG_ARM_L1_CACHE_SHIFT)
> #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
>
> +/* TODO: Phase out the use of this via cache.h */
> #define __read_mostly __section(".data.read_mostly")
>
> #endif
> diff --git a/xen/arch/ppc/include/asm/cache.h
> b/xen/arch/ppc/include/asm/cache.h
> index 0d7323d7892f..13c0bf3242c8 100644
> --- a/xen/arch/ppc/include/asm/cache.h
> +++ b/xen/arch/ppc/include/asm/cache.h
> @@ -3,6 +3,7 @@
> #ifndef _ASM_PPC_CACHE_H
> #define _ASM_PPC_CACHE_H
>
> +/* TODO: Phase out the use of this via cache.h */
> #define __read_mostly __section(".data.read_mostly")
>
> #endif /* _ASM_PPC_CACHE_H */
> diff --git a/xen/arch/x86/include/asm/cache.h
> b/xen/arch/x86/include/asm/cache.h
> index e4770efb22b9..956c05493e23 100644
> --- a/xen/arch/x86/include/asm/cache.h
> +++ b/xen/arch/x86/include/asm/cache.h
> @@ -9,6 +9,7 @@
> #define L1_CACHE_SHIFT (CONFIG_X86_L1_CACHE_SHIFT)
> #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
>
> +/* TODO: Phase out the use of this via cache.h */
> #define __read_mostly __section(".data.read_mostly")
>
> #ifndef __ASSEMBLY__
> diff --git a/xen/include/xen/cache.h b/xen/include/xen/cache.h
> index f52a0aedf768..55456823c543 100644
> --- 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
>
> +/* TODO: Phase out the use of this via cache.h */
> #define __ro_after_init __section(".data.ro_after_init")
>
> #endif /* __LINUX_CACHE_H */
> diff --git a/xen/include/xen/sections.h b/xen/include/xen/sections.h
> index b6cb5604c285..6d4db2b38f0f 100644
> --- a/xen/include/xen/sections.h
> +++ b/xen/include/xen/sections.h
> @@ -3,9 +3,30 @@
> #ifndef __XEN_SECTIONS_H__
> #define __XEN_SECTIONS_H__
>
> +#include <xen/compiler.h>
> +
> /* SAF-0-safe */
> extern char __init_begin[], __init_end[];
>
> +/*
> + * Some data is expected to be written very rarely (if at all).
> + *
> + * For performance reasons is it helpful to group such data in the
> build, to
> + * avoid the linker placing it adjacent to often-written data.
> + */
> +#define __read_mostly __section(".data.read_mostly")
> +
> +/*
> + * Some data should be chosen during boot and be immutable
> thereafter.
> + *
> + * Variables annotated with __ro_after_init will become read-only
> after boot
> + * and suffer a runtime access fault if modified.
> + *
> + * For architectures/platforms which haven't implemented support,
> these
> + * variables will be treated as regular mutable data.
> + */
> +#define __ro_after_init __section(".data.ro_after_init")
> +
> #endif /* !__XEN_SECTIONS_H__ */
> /*
> * Local variables:
>
> base-commit: 8b4243a9b560c89bb259db5a27832c253d4bebc7
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH for-4.19] xen/arch: Centralise __read_mostly and __ro_after_init
2024-06-14 12:49 [PATCH for-4.19] xen/arch: Centralise __read_mostly and __ro_after_init Andrew Cooper
` (2 preceding siblings ...)
2024-06-14 14:55 ` Oleksii K.
@ 2024-06-20 17:22 ` Shawn Anastasio
2024-06-20 17:25 ` Andrew Cooper
3 siblings, 1 reply; 7+ messages in thread
From: Shawn Anastasio @ 2024-06-20 17:22 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: Jan Beulich, Roger Pau Monné, Stefano Stabellini,
Julien Grall, Volodymyr Babchuk, Bertrand Marquis, Michal Orzel,
Oleksii Kurochko
On 6/14/24 7:49 AM, Andrew Cooper wrote:
> These being in cache.h is inherited from Linux, but is an inappropriate
> location to live.
>
> __read_mostly is an optimisation related to data placement in order to avoid
> having shared data in cachelines that are likely to be written to, but it
> really is just a section of the linked image separating data by usage
> patterns; it has nothing to do with cache sizes or flushing logic.
>
> Worse, __ro_after_init was only in xen/cache.h because __read_mostly was in
> arch/cache.h, and has literally nothing whatsoever to do with caches.
>
> Move the definitions into xen/sections.h, which in paritcular means that
> RISC-V doesn't need to repeat the problematic pattern. Take the opportunity
> to provide a short descriptions of what these are used for.
>
> For now, leave TODO comments next to the other identical definitions. It
> turns out that unpicking cache.h is more complicated than it appears because a
> number of files use it for transitive dependencies.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
This seems like a reasonable approach, and removing usage of the old
cache.h __read_mostly from the PPC tree should be a relatively simple
follow up patch from my end.
Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
Thanks,
Shawn
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH for-4.19] xen/arch: Centralise __read_mostly and __ro_after_init
2024-06-20 17:22 ` Shawn Anastasio
@ 2024-06-20 17:25 ` Andrew Cooper
0 siblings, 0 replies; 7+ messages in thread
From: Andrew Cooper @ 2024-06-20 17:25 UTC (permalink / raw)
To: Shawn Anastasio, Xen-devel
Cc: Jan Beulich, Roger Pau Monné, Stefano Stabellini,
Julien Grall, Volodymyr Babchuk, Bertrand Marquis, Michal Orzel,
Oleksii Kurochko
On 20/06/2024 6:22 pm, Shawn Anastasio wrote:
> On 6/14/24 7:49 AM, Andrew Cooper wrote:
>> These being in cache.h is inherited from Linux, but is an inappropriate
>> location to live.
>>
>> __read_mostly is an optimisation related to data placement in order to avoid
>> having shared data in cachelines that are likely to be written to, but it
>> really is just a section of the linked image separating data by usage
>> patterns; it has nothing to do with cache sizes or flushing logic.
>>
>> Worse, __ro_after_init was only in xen/cache.h because __read_mostly was in
>> arch/cache.h, and has literally nothing whatsoever to do with caches.
>>
>> Move the definitions into xen/sections.h, which in paritcular means that
>> RISC-V doesn't need to repeat the problematic pattern. Take the opportunity
>> to provide a short descriptions of what these are used for.
>>
>> For now, leave TODO comments next to the other identical definitions. It
>> turns out that unpicking cache.h is more complicated than it appears because a
>> number of files use it for transitive dependencies.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> This seems like a reasonable approach, and removing usage of the old
> cache.h __read_mostly from the PPC tree should be a relatively simple
> follow up patch from my end.
>
> Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
Thanks.
And funnily enough, I have a patch doing that which I'm just about to
post, because RISC-V needs exactly the same treatment. x86 and ARM are
a different story.
~Andrew
^ permalink raw reply [flat|nested] 7+ messages in thread