Messages in this thread | | | From | Uros Bizjak <> | Date | Thu, 12 Oct 2023 18:55:06 +0200 | Subject | Re: [PATCH v2 -tip] x86/percpu: Use C for arch_raw_cpu_ptr() |
| |
On Thu, Oct 12, 2023 at 6:33 PM Uros Bizjak <ubizjak@gmail.com> wrote: > > On Thu, Oct 12, 2023 at 5:19 PM Nadav Amit <namit@vmware.com> wrote: > > > > > > > On Oct 12, 2023, at 12:54 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > > > > !! External Email > > > > > > On Wed, 11 Oct 2023 at 14:33, Uros Bizjak <ubizjak@gmail.com> wrote: > > >> > > >> Reading the above, it looks to me that we don't want to play games > > >> with "const aliased" versions of current_task [1], as proposed by > > >> Nadav in his patch series. > > > > > > Well, maybe I'd like it if I saw what the effect of it was, but that > > > patch mentions "sync_mm_rss()" which doesn't actually exist > > > (SPLIT_RSS_COUNTING is never defined, the split version is gone and > > > hasn't existed since commit f1a7941243c1 "mm: convert mm's rss stats > > > into percpu_counter") > > > > So I added a new version of the current aliasing (well, actually pcpu_hot > > in the new version) on top of Uros’s patches, and the effect can be seen > > in many functions. I don’t want to bother with many examples so here is > > a common and simple one: > > > > Currently syscall_exit_work() that starts with: > > > > 0xffffffff8111e120 <+0>: push %rbp > > 0xffffffff8111e121 <+1>: mov %rdi,%rbp > > 0xffffffff8111e124 <+4>: push %rbx > > 0xffffffff8111e125 <+5>: mov %rsi,%rbx > > 0xffffffff8111e128 <+8>: and $0x20,%esi > > 0xffffffff8111e12b <+11>: je 0xffffffff8111e143 <syscall_exit_work+35> > > 0xffffffff8111e12d <+13>: mov %gs:0x2ac80,%rax > > 0xffffffff8111e136 <+22>: cmpb $0x0,0x800(%rax) > > 0xffffffff8111e13d <+29>: jne 0xffffffff8111e22a <syscall_exit_work+266> > > 0xffffffff8111e143 <+35>: mov %gs:0x2ac80,%rax > > 0xffffffff8111e14c <+44>: cmpq $0x0,0x7c8(%rax) > > > > Using the const-alias changes the beginning of syscall_exit_work to: > > > > 0xffffffff8111cb80 <+0>: push %r12 > > 0xffffffff8111cb82 <+2>: mov %gs:0x7ef0e0f6(%rip),%r12 # 0x2ac80 <pcpu_hot> > > 0xffffffff8111cb8a <+10>: push %rbp > > 0xffffffff8111cb8b <+11>: mov %rdi,%rbp > > 0xffffffff8111cb8e <+14>: push %rbx > > 0xffffffff8111cb8f <+15>: mov %rsi,%rbx > > 0xffffffff8111cb92 <+18>: and $0x20,%esi > > 0xffffffff8111cb95 <+21>: je 0xffffffff8111cba6 <syscall_exit_work+38> > > 0xffffffff8111cb97 <+23>: cmpb $0x0,0x800(%r12) > > 0xffffffff8111cba0 <+32>: jne 0xffffffff8111cc7a <syscall_exit_work+250> > > 0xffffffff8111cba6 <+38>: cmpq $0x0,0x7c8(%r12) > > > > So we both see RIP-relative addressing is being used (hence the instruction is > > one byte shorter) and the reload going away. > > Just a quick remark here: > > For some reason existing percpu_stable_op asm uses %P operand > modifier. This will drop all syntax-specific prefixes and issue the > bare constant. It will also remove the (%rip) suffix. What we want > here is a generic %a modifier (See 6.47.2.8 Generic Operand Modifiers > [1]) that will substitute a memory reference, with the actual operand > treated as the address. In combination with "p" constraint will DTRT > and will emit symbol with the (%rip) suffix when available, also when > -fpie is in effect.
An example:
--cut here-- int m;
int foo (void) { asm ("# %0 %P0 %a0" :: "p" (&m)); } --cut here--
gcc -O2 -S:
# $m m m(%rip)
gcc -O2 -fpie -S:
# $m m m(%rip)
gcc -O2 -m32 -S:
# $m m m
Uros.
| |