mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Jim Newsome <jnewsome@torproject.org>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	"Eric W . Biederman" <ebiederm@xmission.com>,
	Christian Brauner <christian@brauner.io>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] do_wait: make PIDTYPE_PID case O(1) instead of O(n)
Date: Tue, 9 Mar 2021 14:42:00 -0600	[thread overview]
Message-ID: <7f5508ef-dbaa-fe5c-9826-cce0122eb2f8@torproject.org> (raw)
In-Reply-To: <20210309171539.GA32475@redhat.com>

On 3/9/21 11:15, Oleg Nesterov wrote:
> Jim,
>
> Thanks, the patch looks good to me. Yet I think you need to send V3 even
> if I personally do not care ;) Please consider ./scripts/checkpatch.pl,
> it reports all the coding-style problems I was going to mention.

Thanks! I'd thought clang-format with the included configuration would
be sufficient, but apparently not :)

> Both if's use "int retval", to me it would be better to declare this variable
> at the start of do_wait_pid(). But again, I won't insist this is up to you.
My usual inclination is to avoid uninitialized variables and prefer
putting them in tighter scopes. I don't think it's really much of an
issue in this relatively short function though; happy to go with the
prevailing style.
> I am wondering if something like
>
> 	static inline bool is_parent(struct task_struct *tsk, struct task_struct *p, int flags)
> 	{
> 		return	tsk == p || !(flags & __WNOTHREAD)) && same_thread_group(tsk, p);
> 	}
>
> makes any sense to make do_wait_pid() more clear... probably not.

Yeah, I lean slightly towards the extra level of indirection not being
worth the deduplication.

I made a couple other small changes as well:

* No need for do_wait_pid to take the parameter `tsk` since it's only
ever called with `current`

* With that change, the declaration of `tsk` in `do_wait` can be moved
into a tighter scope of where it's used in the loop.

v3: https://lkml.org/lkml/2021/3/9/1134


      reply	other threads:[~2021-03-09 20:43 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-09 16:15 Jim Newsome
2021-03-09 17:15 ` Oleg Nesterov
2021-03-09 20:42   ` Jim Newsome [this message]

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=7f5508ef-dbaa-fe5c-9826-cce0122eb2f8@torproject.org \
    --to=jnewsome@torproject.org \
    --cc=akpm@linux-foundation.org \
    --cc=christian@brauner.io \
    --cc=ebiederm@xmission.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@redhat.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome