Messages in this thread Patch in this message | | | From | Uros Bizjak <> | Date | Wed, 11 Oct 2023 20:42:15 +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. > > 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. > > UNTESTED!!
The attached patch was tested on a target with fsgsbase CPUID and without it. It works!
The patch improves amd_pmu_enable_virt() in the same way as reported in the original patch submission and also reduces the number of percpu offset reads (either from this_cpu_off or with rdgsbase) from 1663 to 1571.
The only drawback is a larger binary size:
text data bss dec hex filename 25546594 4387686 808452 30742732 1d518cc vmlinux-new.o 25515256 4387814 808452 30711522 1d49ee2 vmlinux-old.o
that increases by 31k (0.123%), probably due to 1578 rdgsbase alternatives.
I'll prepare and submit a patch for tip/percpu branch.
Uros.
> > Linus diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h index 34734d730463..8450fe4a2753 100644 --- a/arch/x86/include/asm/percpu.h +++ b/arch/x86/include/asm/percpu.h @@ -31,18 +31,32 @@ #define __percpu_prefix "%%"__stringify(__percpu_seg)":" #define __my_cpu_offset this_cpu_read(this_cpu_off) -/* - * Compared to the generic __my_cpu_offset version, the following - * saves one instruction and avoids clobbering a temp register. - */ -#define arch_raw_cpu_ptr(ptr) \ -({ \ - unsigned long tcp_ptr__; \ - asm ("add " __percpu_arg(1) ", %0" \ - : "=r" (tcp_ptr__) \ - : "m" (this_cpu_off), "0" (ptr)); \ - (typeof(*(ptr)) __kernel __force *)tcp_ptr__; \ +#ifdef CONFIG_X86_64 +#define arch_raw_cpu_ptr(ptr) \ +({ \ + unsigned long tcp_ptr__; \ + asm (ALTERNATIVE("movq " __percpu_arg(1) ", %0", \ + "rdgsbase %0", \ + X86_FEATURE_FSGSBASE) \ + : "=r" (tcp_ptr__) \ + : "m" (this_cpu_off)); \ + \ + tcp_ptr__ += (unsigned long)(ptr); \ + (typeof(*(ptr)) __kernel __force *)tcp_ptr__; \ }) +#else /* CONFIG_X86_64 */ +#define arch_raw_cpu_ptr(ptr) \ +({ \ + unsigned long tcp_ptr__; \ + asm ("movl " __percpu_arg(1) ", %0" \ + : "=r" (tcp_ptr__) \ + : "m" (this_cpu_off)); \ + \ + tcp_ptr__ += (unsigned long)(ptr); \ + (typeof(*(ptr)) __kernel __force *)tcp_ptr__; \ +}) +#endif /* CONFIG_X86_64 */ + #else #define __percpu_prefix "" #endif | |