LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: David Hildenbrand <david@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Suren Baghdasaryan <surenb@google.com>,
	mhocko@kernel.org, mhocko@suse.com, rientjes@google.com,
	willy@infradead.org, hannes@cmpxchg.org, guro@fb.com,
	riel@surriel.com, minchan@kernel.org, christian@brauner.io,
	hch@infradead.org, oleg@redhat.com, jannh@google.com,
	shakeelb@google.com, luto@kernel.org,
	christian.brauner@ubuntu.com, fweimer@redhat.com,
	jengelh@inai.de, timmurray@google.com, linux-api@vger.kernel.org,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	kernel-team@android.com
Subject: Re: [PATCH v2 1/3] mm, oom: move task_will_free_mem up in the file to be used in process_mrelease
Date: Wed, 21 Jul 2021 09:30:50 +0200	[thread overview]
Message-ID: <ba763ed3-9ad1-9502-1ffc-3175446570d1@redhat.com> (raw)
In-Reply-To: <20210720160707.2332738708948f7d865d67c1@linux-foundation.org>

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.

Anyhow, just my 2 cents.

-- 
Thanks,

David / dhildenb


  reply	other threads:[~2021-07-21  7:33 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-18 21:41 Suren Baghdasaryan
2021-07-18 21:41 ` [PATCH v2 2/3] mm: introduce process_mrelease system call Suren Baghdasaryan
2021-07-21  8:02   ` David Hildenbrand
2021-07-21 15:43     ` Suren Baghdasaryan
2021-07-21 22:59       ` Suren Baghdasaryan
2021-07-22  7:45         ` David Hildenbrand
2021-07-18 21:41 ` [PATCH v2 3/3] mm: wire up syscall process_mrelease Suren Baghdasaryan
2021-07-20 12:43 ` [PATCH v2 1/3] mm, oom: move task_will_free_mem up in the file to be used in process_mrelease David Hildenbrand
2021-07-20 16:18   ` Suren Baghdasaryan
2021-07-20 23:07   ` Andrew Morton
2021-07-21  7:30     ` David Hildenbrand [this message]
2021-07-21 15:33       ` Suren Baghdasaryan
2021-07-21 16:12         ` David Hildenbrand
2021-07-21 20:19           ` Suren Baghdasaryan
2021-07-21 20:50             ` Andrew Morton
2021-07-21 20:59               ` Suren Baghdasaryan
2021-07-23  1:15                 ` Suren Baghdasaryan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ba763ed3-9ad1-9502-1ffc-3175446570d1@redhat.com \
    --to=david@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=christian.brauner@ubuntu.com \
    --cc=christian@brauner.io \
    --cc=fweimer@redhat.com \
    --cc=guro@fb.com \
    --cc=hannes@cmpxchg.org \
    --cc=hch@infradead.org \
    --cc=jannh@google.com \
    --cc=jengelh@inai.de \
    --cc=kernel-team@android.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=luto@kernel.org \
    --cc=mhocko@kernel.org \
    --cc=mhocko@suse.com \
    --cc=minchan@kernel.org \
    --cc=oleg@redhat.com \
    --cc=riel@surriel.com \
    --cc=rientjes@google.com \
    --cc=shakeelb@google.com \
    --cc=surenb@google.com \
    --cc=timmurray@google.com \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH v2 1/3] mm, oom: move task_will_free_mem up in the file to be used in process_mrelease' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).