Re: [Xen-devel] [PATCH v2 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Jan Beulich <JBeulich@xxxxxxxx>]

On 26.07.2019 22:32, Andrew Cooper wrote:
> Future changes are going to need to page align some percpu data.
> 
> This means that the percpu area needs suitably aligning in the BSS so CPU0 has
> correctly aligned data.  Shuffle the exact link order of items within the BSS
> to give .bss.percpu.page_aligned appropriate alignment.
> 
> In addition, we need to be able to specify an alignment attribute to
> __DEFINE_PER_CPU().  Rework it so the caller passes in all attributes, and
> adjust DEFINE_PER_CPU{,_READ_MOSTLY}() to match.  This has the added bonus
> that it is now possible to grep for .bss.percpu and find all the users.

And it has the meaningful downside of now every use site needing to get
things right. This is not really a problem solely because
__DEFINE_PER_CPU() is a helper for all the real DEFINE_PER_CPU*(). The
grep-ing argument is not a really meaningful one imo anyway - you could
as well grep for DEFINE_PER_CPU.

Anyway - this is not an objection to the chosen approach, just a remark.
I'd like to note though that you explicitly undo something I had done
(iirc), and I may find odd when running into again down the road,
potentially resulting in an "undo-the-undo" patch. I think we really
need to find a way to avoid re-doing things that were done intentionally
in certain ways, when the difference between variants is merely personal
taste.

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -293,14 +293,15 @@ SECTIONS
>          __bss_start = .;
>          *(.bss.stack_aligned)
>          *(.bss.page_aligned*)
> -       *(.bss)
> -       . = ALIGN(SMP_CACHE_BYTES);
>          __per_cpu_start = .;
> +       *(.bss.percpu.page_aligned)

Now this is a case where I think an explicit ALIGN(PAGE_SIZE) would be
desirable: If the last item in .bss.page_aligned was not a multiple of
PAGE_SIZE in size, then __per_cpu_start would live needlessly early,
possibly increasing our memory overhead by a page per CPU for no gain
at all.

>          *(.bss.percpu)
>          . = ALIGN(SMP_CACHE_BYTES);
>          *(.bss.percpu.read_mostly)
>          . = ALIGN(SMP_CACHE_BYTES);
>          __per_cpu_data_end = .;
> +       *(.bss)
> +       . = ALIGN(SMP_CACHE_BYTES);
>          __bss_end = .;

Why is this last ALIGN() needed?

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel