Re: [PATCH v6] mm: Uninline copy_overflow()

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 14/02/2022 à 16:10, David Laight a écrit :
> From: Christophe Leroy
>> Sent: 14 February 2022 14:58
> ...
>>> I make that 3 extra instructions.
>>> Two are needed to load the format string.
>>> Not sure why the third gets added.
>>
>> Third instruction is 'twui', to 'trap' and get the warning oops.
> 
> I wondered what that did :-)
> Although you really want the -- cut here -- to contain the pr_warn().
> Doesn't WARN() do that for you?

I remember some discussion about that in the past. Will dig into it 
tomorrow.

> 
> I was looking at that last week because the 'scheduling while atomic'
> trace is "BUG: xxxx" but doesn't have the '--- cut here --" marker.
> 
>>> Not really significant in the 12-15 the error call actually takes.
>>> Although a lot of those are just generating the stack frame
>>> in order to call the error function - and wouldn't be there in
>>> a less trivial example.
>>
>>
>> Yes, after looking once more, maybe making it __always_inline would be
>> enough.
>>
>> The starting point was that I got almost 50 times copy_overflow() in my
>> vmlinux, each having its own format string as well.
> 
> Didn't the linker merge the format strings?
> They ought to end up in strdata.ro.1 (or whatever it is called)
> and the linker merge the references.
> 
>> So my patch reduced vmlinux size by 3908 bytes.
>>
>> But with __always_inline I get a reduction by 3560 which is almost the same.
>>
>> So if you prefer, I can just make copy_overflow() __always_inline and voila.
> 
> I suspect #define __inline __always_inline is the way to go.

That was the case until 889b3c1245de ("compiler: remove 
CONFIG_OPTIMIZE_INLINING entirely")


> Probable along with -Winline.
> 
> The kernel shouldn't have inline sprinkled where it isn't needed.
> 
>>> More interesting would be changing copy_overflow() to return the size.
>>> So copy_to_user() becomes:
>>>
>>> 	if (size_valid())
>>> 		return _copy_to_user();
>>> 	return copy_overflow()
>>
>> Yes that's something to try, allthough it means changing all callers of
>> check_copy_size().
> 
> You could use a differently named function so they can be changed in stages.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)