On Tue, Dec 24, 2019 at 4:15 AM Andy Lutomirski <l...@amacapital.net> wrote: > > > > > On Dec 24, 2019, at 7:53 PM, christophe leroy <christophe.le...@c-s.fr> > > wrote: > > > > > > > >> Le 24/12/2019 à 03:27, Andy Lutomirski a écrit : > >>> On Mon, Dec 23, 2019 at 6:31 AM Christophe Leroy > >>> <christophe.le...@c-s.fr> wrote: > >>> > >>> On powerpc, __arch_get_vdso_data() clobbers the link register, > >>> requiring the caller to set a stack frame in order to save it. > >>> > >>> As the parent function already has to set a stack frame and save > >>> the link register to call the C vdso function, retriving the > >>> vdso data pointer there is lighter. > >> I'm confused. Can't you inline __arch_get_vdso_data()? Or is the > >> issue that you can't retrieve the program counter on power without > >> clobbering the link register? > > > > Yes it can be inlined (I did it in V1 > > https://patchwork.ozlabs.org/patch/1180571/), but you can't do it without > > clobbering the link register, because the only way to get the program > > counter is to do to as if you were calling another function but you call to > > the address which just follows where you are, so that it sets LR which the > > simulated return address which corresponds to the address following the > > branch. > > > > static __always_inline > > const struct vdso_data *__arch_get_vdso_data(void) > > { > > void *ptr; > > > > asm volatile( > > " bcl 20, 31, .+4;\n" > > " mflr %0;\n" > > " addi %0, %0, __kernel_datapage_offset - (.-4);\n" > > : "=b"(ptr) : : "lr"); > > > > return ptr + *(unsigned long *)ptr; > > } > > > >> I would imagine that this patch generates worse code on any > >> architecture with PC-relative addressing modes (which includes at > >> least x86_64, and I would guess includes most modern architectures). > > > > Why ? Powerpc is also using PC-relative addressing for all calls but > > indirect calls. > > I mean PC-relative access for data. The data page is at a constant, known > offset from the vDSO text. > > I haven’t checked how much x86_64 benefits from this, but at least the > non-array fields ought to be accessible with a PC-relative access. > > It should be possible to refactor a little bit so that the compiler can still > see what’s going on. Maybe your patch actually does this. I’d want to look > at the assembly. This also might not matter much on x86_64 in particular, > since x86_64 can convert a PC-relative address to an absolute address with a > single instruction with no clobbers. > > Does power have PC-relative data access? If so, I wonder if the code can be > arranged so that even the array accesses don’t require computing an absolute > address at any point.
Indeed the x86 code is also suboptimal, but at least the unnecessary absolute address calculation is cheap on x86_64. Ideally we'd pass around offsets into the vdso data instead of passing pointers, and maybe the compiler will figure it out. I can try to play with this in the morning.
