Re: [PATCH 3/5] mm: abstract get_arg_page() stack expansion and mmap read lock

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Sun, Dec 08, 2024 at 11:27:06AM +0000, Wei Yang wrote:
> On Thu, Dec 05, 2024 at 07:01:14AM +0000, Lorenzo Stoakes wrote:
> [...]
> >>
> >> Maybe we just leave this done in one place is enough?
> >
> >Wei, I feel like I have repeated myself about 'mathematically smallest
> >code' rather too many times at this stage. Doing an unsolicited drive-by
> >review applying this concept, which I have roundly and clearly rejected, is
> >not appreciated.
> >
>
> Hi, Lorenzo
>
> I would apologize for introducing this un-pleasant mail. Would be more
> thoughtful next time.

It wasn't unpleasant, don't worry! :) I'm just trying to be as clear as I
can about this so as to avoid you spending time on things that aren't
useful.

On occasion I think, for clarity, it's important to be firm - otherwise if
I were receptive even on things that I thought not worthwhile - you'd end
up wasting your time doing work that might end up not being used.

>
> >At any rate, we are checking this _before the mmap lock is acquired_. It is
> >also self-documenting.
> >
> >Please try to take on board the point that there are many factors when it
> >comes to writing kernel code, aversion to possibly generated branches being
> >only one of them.
> >
>
> Thanks for this suggestion.
>
> I am trying to be as professional as you are. In case you have other
> suggestions, they are welcome.

Thanks, what I would say is that simply observing that we might duplicate
some logic in a non-harmful way does not necessarily indicate that this
should be changed.

Obviously if you can evidence a _statistically significant_ performance
impact then OF COURSE you should report something like this and send a
patch for it (along side the evidence of the perf regression).

But in general, if you feel the need to refactor just for the sake of
eliminating branches or grouping code like this, it isn't helpful or
useful.

Refactorings can be very useful _in general_ (I have done a lot of work on
things like this myself obviously), but it's important to assess the RoI -
is the churn worth the benefit - does it make significant enough of an
impact - and is it 'tasteful'?

These things are at least somewhat subjective obviously.

What I would suggest you focus on instead is in finding bugs - your help in
finding the bug where I (ugh) set a pointer to an error value in a case
where, if you were unlucky, it might be dereferenced - was a really helpful
contribution, as you can tell from how quickly we approved it and arranged
for backporting.

I'd say this ought to be your focus. For instance [0] was a horrid mistake
on my part, and ripe for being discovered. Having a second pair of eyes
checking for this kind of thing would be very useful, and discovering this
kind of bug as early as possible would be hugely valued.

So yeah TL;DR my advice is at the moment - focus on finding bugs above all
else :)

Cheers, Lorenzo

[0]:https://lore.kernel.org/linux-mm/20241206215229.244413-1-lorenzo.stoakes@oracle.com/

>
>
> --
> Wei Yang
> Help you, Help me