Messages in this thread | | | Subject | Re: [PATCH v2 1/3] mm, oom: move task_will_free_mem up in the file to be used in process_mrelease | From | David Hildenbrand <> | Date | Wed, 21 Jul 2021 18:12:56 +0200 |
| |
On 21.07.21 17:33, Suren Baghdasaryan wrote: > On Wed, Jul 21, 2021 at 12:30 AM David Hildenbrand <david@redhat.com> wrote: >> >> On 21.07.21 01:07, Andrew Morton wrote: >>> On Tue, 20 Jul 2021 14:43:52 +0200 David Hildenbrand <david@redhat.com> 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@google.com> >>>>> --- >>>>> 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
| |