> On Oct 12, 2023, at 8:16 PM, Linus Torvalds wrote: > > !! External Email > > On Thu, 12 Oct 2023 at 08:19, Nadav Amit 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