Re: Number of arguments in vmalloc.c

[Nadav Amit <nadav.amit@gmail.com>]

> On Dec 3, 2018, at 7:12 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
> 
>> On Dec 3, 2018, at 2:49 PM, Matthew Wilcox <willy@infradead.org> wrote:
>> 
>> On Mon, Dec 03, 2018 at 02:04:41PM -0800, Nadav Amit wrote:
>>> On Dec 3, 2018, at 8:13 AM, Matthew Wilcox <willy@infradead.org> wrote:
>>>> On Mon, Dec 03, 2018 at 02:59:36PM +0100, Vlastimil Babka wrote:
>>>>> On 11/28/18 3:01 PM, Matthew Wilcox wrote:
>>>>>> Some of the functions in vmalloc.c have as many as nine arguments.
>>>>>> So I thought I'd have a quick go at bundling the ones that make sense
>>>>>> into a struct and pass around a pointer to that struct.  Well, it made
>>>>>> the generated code worse,
>>>>> 
>>>>> Worse in which metric?
>>>> 
>>>> More instructions to accomplish the same thing.
>>>> 
>>>>>> so I thought I'd share my attempt so nobody
>>>>>> else bothers (or soebody points out that I did something stupid).
>>>>> 
>>>>> I guess in some of the functions the args parameter could be const?
>>>>> Might make some difference.
>>>>> 
>>>>> Anyway this shouldn't be a fast path, so even if the generated code is
>>>>> e.g. somewhat larger, then it still might make sense to reduce the
>>>>> insane parameter lists.
>>>> 
>>>> It might ... I'm not sure it's even easier to program than the original
>>>> though.
>>> 
>>> My intuition is that if all the fields of vm_args were initialized together
>>> (in the same function), and a 'const struct vm_args *' was provided as
>>> an argument to other functions, code would be better (at least better than
>>> what you got right now).
>>> 
>>> I’m not saying it is easily applicable in this use-case (since I didn’t
>>> check).
>> 
>> Your intuition is wrong ...
>> 
>>  text	   data	    bss	    dec	    hex	filename
>>  9466	     81	     32	   9579	   256b	before.o
>>  9546	     81	     32	   9659	   25bb	.build-tiny/mm/vmalloc.o
>>  9546	     81	     32	   9659	   25bb	const.o
>> 
>> indeed, there's no difference between with or without the const, according
>> to 'cmp'.
>> 
>> Now, only alloc_vmap_area() gets to take a const argument.
>> __get_vm_area_node() intentionally modifies the arguments.  But feel
>> free to play around with this; you might be able to make it do something
>> worthwhile.
> 
> I was playing with it (a bit). What I suggested (modifying
> __get_vm_area_node() so it will not change arguments) helps a bit, but not
> much.
> 
> One insight that I got is that at least part of the overhead comes from the
> the stack protector code that gcc emits.

[ +Peter ]

So I dug some more (I’m still not done), and found various trivial things
(e.g., storing zero extending u32 immediate is shorter for registers,
inlining already takes place).

*But* there is one thing that may require some attention - patch
b59167ac7bafd ("x86/percpu: Fix this_cpu_read()”) set ordering constraints
on the VM_ARGS() evaluation. And this patch also imposes, it appears,
(unnecessary) constraints on other pieces of code.

These constraints are due to the addition of the volatile keyword for
this_cpu_read() by the patch. This affects at least 68 functions in my
kernel build, some of which are hot (I think), e.g., finish_task_switch(),
smp_x86_platform_ipi() and select_idle_sibling().

Peter, perhaps the solution was too big of a hammer? Is it possible instead
to create a separate "this_cpu_read_once()” with the volatile keyword? Such
a function can be used for native_sched_clock() and other seqlocks, etc.