Re: [GIT PULL] Additional x86 fixes for 2.6.31-rc5

Tejun Heo
2009-08-03T01:49:57Z

Hello, Linus.

Linus Torvalds wrote:
> I just noticed another issue on x86 code generation, since I was looking
> at assembly language generation due to the do_sigaltstack() kernel stack
> info leak thing.
>
> Our "get_current()" seriously sucks now that it's a per-cpu variable.
>
> Look at the code generated for something like
>
> current->sas_ss_sp = (unsigned long) ss_sp;
> current->sas_ss_size = ss_size;
>
> and notice how the code really really sucks:
>
> movq %gs:per_cpu__current_task,%rcx
> movq %rdx, 1152(%rcx)
> movq %gs:per_cpu__current_task,%rdx
> movq %rax, 1160(%rdx)

Right. This isn't new tho. In practive, the current percpu_read()
thingies behave identically to the original x86_read_percpu(). 2.6.29
generates about the same code.

(This is from the object file so the offsets haven't been set yet)

(gdb) disassemble block_all_signals
Dump of assembler code for function block_all_signals:
...
0x000000000000168a <block_all_signals+74>: mov %gs:0x0,%rax
0x0000000000001693 <block_all_signals+83>: mov %r12,0x4e0(%rax)
0x000000000000169a <block_all_signals+90>: mov %gs:0x0,%rax
0x00000000000016a3 <block_all_signals+99>: mov %r13,0x4d8(%rax)
0x00000000000016aa <block_all_signals+106>: mov %gs:0x0,%rax
0x00000000000016b3 <block_all_signals+115>: mov %r14,0x4d0(%rax)
0x00000000000016ba <block_all_signals+122>: mov %gs:0x0,%rax
0x00000000000016c3 <block_all_signals+131>: mov 0x488(%rax),%rdi
...

...
> End result: the above horror becomes a more reasonable
>
> movq %gs:per_cpu__current_task,%rax
> movq %rcx, 1152(%rax)
> movq %rdx, 1160(%rax)
>
> instead (it still doesn't cache it over the whole function, but it's
> certainly better).
>
> NOTE! I did not test that it all worked. I only looked at the asm, and
> checked out the improvements. All the ones I looked at looked reasonable.

Cool. I'm currently testing the kernel. It has booted fine on 8-core
2-way NUMA machine and repetetively compiling kernel w/ -j16.
Everything seems to work fine till now.

> Worthwhile? You be the judge.

I think it's definitely worthwhile. The gain is significant compared
to the added miniscule complexity.

"current" is a per-thread variable implemented as a per-cpu variable.
Given that we don't have many of them yet and they're accessed via
arch-specific accessors, percpu_read_stable() seems like a pretty good
solution to me.

If we ever need more thread variables, we might venture into using %fs
and maybe __thread if other archs can be converted similarly but I
think we're better off with packing such things in task_struct -
per-thread is far scarier than per-cpu scalability-wise.

> There's another detail that may be worth looking at: we often get
> 'current' and 'thread_info' together, and they are _not_ in the same
> cache-line. It might be worth defining them together in the per-cpu data,
> and making sure they are in the same cacheline too. In general, we should
> probably look at which per-pcu variables are hot and read-only, and try to
> gathe them all together.

Yeap, putting hot ones together would be great. I'm a bit curious why
it should be hot _and_ read-only tho. For variables which aren't
accessed too often by other cpus, read-onlyness shouldn't matter too
much, right?

> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Sat, 1 Aug 2009 11:50:54 -0700
> Subject: [PATCH] x86-64: Add 'percpu_read_stable()' interface for cacheable accesses
>
> This is very useful for some common things like 'get_current()' and
> 'get_thread_info()', which can be used multiple times in a function, and
> where the result is cacheable.
>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

Do you want to queue it for 2.6.31? Given that the generated code
changes compared to the previous kernels (both pre and post percpu
stuff), I think it would be safer to queue it for 2.6.32 window. I
would be happy to carry it in the percpu tree.

Thanks.

--
tejun