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

[David Laight <David.Laight@ACULAB.COM>]

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 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.
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)