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
