Messages in this thread | | | From | Uros Bizjak <> | Date | Wed, 11 Oct 2023 09:27:24 +0200 | Subject | Re: [PATCH v2 -tip] x86/percpu: Use C for arch_raw_cpu_ptr() |
| |
On Tue, Oct 10, 2023 at 8:52 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Tue, 10 Oct 2023 at 11:41, Uros Bizjak <ubizjak@gmail.com> wrote: > > > > Yes, but does it CSE the load from multiple addresses? > > Yes, it should do that just right, because the *asm* itself is > identical, just the offsets (that gcc then adds separately) would be > different.
Indeed. To illustrate the question with an example, foo() and bar() should compile to the same assembly, and there should be only one read form m resp. n:
--cut here-- __seg_gs int m;
int foo (void) { return m + m; }
int n;
static inline int get (int *m) { int res;
asm ("mov %%gs:%1, %0" : "=r"(res) : "m"(*m)); return res; }
int bar (void) { return get (&n) + get (&n); } --cut here--
And they do:
0000000000000000 <foo>: 0: 65 8b 05 00 00 00 00 mov %gs:0x0(%rip),%eax # 7 <foo+0x7> 7: 01 c0 add %eax,%eax 9: c3 retq
0000000000000010 <bar>: 10: 65 8b 05 00 00 00 00 mov %gs:0x0(%rip),%eax # 17 <bar+0x7> 17: 01 c0 add %eax,%eax 19: c3 retq
> > This is not unlike how we depend on gcc CSE'ing the "current" part > when doing multiple accesses of different members off that: > > static __always_inline struct task_struct *get_current(void) > { > return this_cpu_read_stable(pcpu_hot.current_task); > } > > with this_cpu_read_stable() being an inline asm that lacks the memory > component (the same way the fallback hides it by just using > "%%gs:this_cpu_off" directly inside the asm, instead of exposing it as > a memory access to gcc). > > Of course, I think that with the "__seg_gs" patches, we *could* expose > the "%%gs:this_cpu_off" part to gcc, since gcc hopefully then can do > the alias analysis on that side and see that it can CSE the thing > anyway. > > That might be a better choice than __FORCE_ORDER, in fact. > > IOW, something like > > static __always_inline unsigned long new_cpu_offset(void) > { > unsigned long res; > asm(ALTERNATIVE( > "movq " __percpu_arg(1) ",%0", > "rdgsbase %0", > X86_FEATURE_FSGSBASE) > : "=r" (res) > : "m" (this_cpu_off)); > return res; > } > > would presumably work together with your __seg_gs stuff.
I have zero experience with rdgsbase insn, but the above is not dependent on __seg_gs, so (the movq part at least) would also work in the current mainline. To work together with __seg_gs stuff, this_cpu_offset should be enclosed in __my_cpu_var. Also, if rdgsbase is substituted with rdfsbase, it will also work for 32-bit targets.
Uros.
> UNTESTED!! > > Linus
| |