Re: [PATCH v2 -tip] x86/percpu: Use C for arch_raw_cpu_ptr()

[Nadav Amit]

> On Oct 12, 2023, at 8:16 PM, Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> 
> !! External Email
> 
> On Thu, 12 Oct 2023 at 08:19, Nadav Amit <namit@xxxxxxxxxx> wrote:
>> 
>> +/*
>> + * Hold a constant alias for current_task, which would allow to avoid caching of
>> + * current task.
>> + *
>> + * We must mark const_current_task with the segment qualifiers, as otherwise gcc
>> + * would do redundant reads of const_current_task.
>> + */
>> +DECLARE_PER_CPU(struct pcpu_hot const __percpu_seg_override, const_pcpu_hot);
> 
> Hmm. The only things I'm not super-happy about with your patch is
> 
> (a) it looks like this depends on the alias analysis knowing that the
> __seg_gs isn't affected by normal memory ops. That implies that this
> will not work well with compiler versions that don't do that?
> 
> (b) This declaration doesn't match the other one. So now there are
> two *different* declarations for const_pcpu_hot, which I really don't
> like.
> 
> That second one would seem to be trivial to just fix (or maybe not,
> and you do it that way for some horrible reason).

If you refer to the difference between DECLARE_PER_CPU_ALIGNED() and
DECLARE_PER_CPU() - that’s just a silly mistake that I made porting my
old patch (I also put “const” in the wrong place of the declaration, sorry).

> 
> The first one sounds bad to me - basically making the *reason* for
> this patch go away - but maybe the compilers that don't support
> address spaces are so rare that we can ignore it.

As far as I understand it has nothing to do with the address spaces, and IIRC
the compiler does not regard gs/fs address spaces as independent from the main
one. That’s the reason a compiler barrier affects regular loads with __seg_gs.

The “trick” that the patch does is to expose a new const_pcpu_hot symbol that has
a “const” qualifier. For compilation units from which the symbol is effectively
constant, we use const_pcpu_hot. The compiler then knows that the value would not
change.

Later, when we actually define the const_pcpu_hot, we tell the compiler using
__attribute__((alias("pcpu_hot”)) that this symbol is actually an alias to pcpu_hot.

Although it is a bit of a trick that I have never seen elsewhere, I don’t see it
violating GCC specifications (“except for top-level qualifiers the alias target
must have the same type as the alias” [1]), and there is nothing that is specific
to the gs address-space. I still have the concern of its interaction with LTO
though, and perhaps using “-fno-lto” when compiling compilation units that
modify current (e.g., arch/x86/kernel/process_64.o) is necessary.

I hope it makes sense.

[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html

Attachment: signature.asc
Description: Message signed with OpenPGP