Re: [PATCH v2 1/3] mm, oom: move task_will_free_mem up in the file to be used in process_mrelease

[David Hildenbrand]

On 21.07.21 17:33, Suren Baghdasaryan wrote:
> On Wed, Jul 21, 2021 at 12:30 AM David Hildenbrand <david@xxxxxxxxxx> wrote:
> > On 21.07.21 01:07, Andrew Morton wrote:
> > > On Tue, 20 Jul 2021 14:43:52 +0200 David Hildenbrand <david@xxxxxxxxxx> wrote:
> > >
> > > > On 18.07.21 23:41, Suren Baghdasaryan wrote:
> > > > > process_mrelease needs to be added in the CONFIG_MMU-dependent block which
> > > > > comes before __task_will_free_mem and task_will_free_mem. Move these
> > > > > functions before this block so that new process_mrelease syscall can use
> > > > > them.
> > > > >
> > > > > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > > > > ---
> > > > > changes in v2:
> > > > > - Fixed build error when CONFIG_MMU=n, reported by kernel test robot. This
> > > > > required moving task_will_free_mem implemented in the first patch
> > > > > - Renamed process_reap to process_mrelease, per majority of votes
> > > > > - Replaced "dying process" with "process which was sent a SIGKILL signal" in
> > > > > the manual page text, per Florian Weimer
> > > > > - Added ERRORS section in the manual page text
> > > > > - Resolved conflicts in syscall numbers caused by the new memfd_secret syscall
> > > > > - Separated boilerplate code wiring-up the new syscall into a separate patch
> > > > > to facilitate the review process
> > > > >
> > > > >     mm/oom_kill.c | 150 +++++++++++++++++++++++++-------------------------
> > > > >     1 file changed, 75 insertions(+), 75 deletions(-)
> > > >
> > > > TBH, I really dislike this move as it makes git blame a lot harder with
> > > > any real benefit.
> > > >
> > > > Can't you just use prototypes to avoid the move for now in patch #2?
> > > >
> > > > static bool task_will_free_mem(struct task_struct *task);
> > >
> > > This change makes the code better - it's silly to be adding forward
> > > declarations just because the functions are in the wrong place.
> >
> > I'd really love to learn what "better" here means and if it's rather
> > subjective. When it comes to navigating the code, we do have established
> > tools for that (ctags), and personally I couldn't care less where
> > exactly in a file the code is located.
> >
> > Sure, ending up with a forward-declaration for every function might not
> > be what we want ;)
> >
> > > If that messes up git-blame then let's come up with better tooling
> > > rather than suffering poorer kernel code because the tools aren't doing
> > > what we want of them.  Surely?
> >
> > I don't agree that what we get is "poorer kernel code" in this very
> > instance; I can understand that we avoid forward-declarations when
> > moving smallish functions. But moving two functions with 75 LOC is a bit
> > too much for my taste at least -- speaking as someone who cares about
> > easy backports and git-blame.
>
> There is a third alternative here to have process_mrelease() at the
> end of the file with its own #ifdef CONFIG_MMU block, maybe even
> embedded in the function like this:
>
>   int process_mrelease(int pidfd, unsigned int flags)
> {
> #ifdef CONFIG_MMU
>          ...
> #else
>          return ENOSYS;
> #endif
> }
>
> This would not require moving other functions.
> Would that be better than the current approach or the forward declaration?

IMHO that could be an easy, possible alternative.

--
Thanks,

David / dhildenb