Messages in this thread | | | From | Uros Bizjak <> | Date | Thu, 12 Oct 2023 18:33:22 +0200 | Subject | Re: [PATCH v2 -tip] x86/percpu: Use C for arch_raw_cpu_ptr() |
| |
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.
[1] https://gcc.gnu.org/onlinedocs/gcc-13.2.0/gcc.pdf
Uros.
| |