linux-api.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Tycho Andersen <tycho@tycho.pizza>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Christian Brauner <brauner@kernel.org>,
	linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
	Tycho Andersen <tandersen@netflix.com>,
	"Eric W. Biederman" <ebiederm@xmission.com>
Subject: Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
Date: Thu, 25 Jan 2024 11:03:44 -0700	[thread overview]
Message-ID: <ZbKigMNQM0Yklc/5@tycho.pizza> (raw)
In-Reply-To: <20240125175113.GC5513@redhat.com>

On Thu, Jan 25, 2024 at 06:51:14PM +0100, Oleg Nesterov wrote:
> > > What if we add the new PIDFD_THREAD flag? With this flag
> > >
> > > 	- sys_pidfd_open() doesn't require the must be a group leader
> >
> > Yes.
> >
> > >
> > > 	- pidfd_poll() succeeds when the task passes exit_notify() and
> > > 	  becomes a zombie, even if it is a leader and has other threads.
> >
> > Iiuc, if an existing user creates a pidfd for a thread-group leader and
> > then polls that pidfd they would currently only get notified if the
> > thread-group is empty and the leader has exited.
> >
> > If we now start notifying when the thread-group leader exits but the
> > thread-group isn't empty then this would be a fairly big api change
> 
> Hmm... again, this patch doesn't (shouldn't) change the current behavior.
> 
> Please note "with this flag" above. If sys_pidfd_open() was called
> without PIDFD_THREAD, then sys_pidfd_open() still requires that the
> target task must be a group leader, and pidfd_poll() won't succeed
> until the leader exits and thread_group_empty() is true.

Thanks for sending your patch, I'll take a look at it (probably
tomorrow at this rate).

One of the things I don't like about PIDFD_THREAD is that it's hard to
tell whether an arbitrary thread is a leader or not. Right now we do
it by parsing /proc/pid/status, which shows all the stuff from
do_task_stat() that we don't care about but which is quite expensive
to compute. (Maybe there's a better way?)

With PIDFD_THREAD we could could do it twice, once with the flag, get
EINVAL, and then do it again. But ideally we wouldn't have to.

Still, if that's the only way that makes sense, that's fine.

Tycho

  reply	other threads:[~2024-01-25 18:03 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-23 15:34 [PATCH v3 0/3] pidfds for non thread group leaders Tycho Andersen
2024-01-23 15:34 ` [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders Tycho Andersen
2024-01-23 19:56   ` Oleg Nesterov
2024-01-23 21:10     ` Tycho Andersen
2024-01-23 22:22       ` Oleg Nesterov
2024-01-24  1:25         ` Oleg Nesterov
2024-01-25 14:08       ` Oleg Nesterov
2024-01-25 17:17         ` Christian Brauner
2024-01-25 17:51           ` Oleg Nesterov
2024-01-25 18:03             ` Tycho Andersen [this message]
2024-01-25 18:25               ` Oleg Nesterov
2024-01-25 18:30                 ` Oleg Nesterov
2024-01-25 18:36                   ` Tycho Andersen
2024-01-26  9:49                     ` Christian Brauner
2024-01-26  9:42             ` Christian Brauner
2024-01-26 14:33               ` Oleg Nesterov
2024-01-26  9:47         ` Christian Brauner
2024-01-26 14:33           ` Oleg Nesterov
2024-01-27 14:26             ` Christian Brauner
2024-01-26 21:50         ` Tycho Andersen
2024-01-27 10:54           ` Oleg Nesterov
2024-01-27 14:33             ` Christian Brauner
2024-01-27 15:55             ` Tycho Andersen
2024-01-27 16:31               ` Oleg Nesterov
2024-01-27 17:20                 ` Tycho Andersen
2024-01-27 19:31                   ` Oleg Nesterov
2024-01-27 20:44                     ` Tycho Andersen
2024-01-27 21:10                       ` Oleg Nesterov
2024-01-29 11:23                         ` [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open() Oleg Nesterov
2024-01-29 13:41                           ` Christian Brauner
2024-01-29 14:31                             ` Tycho Andersen
2024-01-29 15:14                               ` Christian Brauner
2024-01-30 11:21                             ` Oleg Nesterov
2024-01-31 18:11                           ` Andy Lutomirski
2024-01-31 18:48                             ` Oleg Nesterov
2024-01-31 19:14                               ` Oleg Nesterov
2024-01-31 19:24                                 ` Andy Lutomirski
2024-01-31 19:46                                   ` Christian Brauner
2024-01-31 19:50                                     ` Andy Lutomirski
2024-02-01 13:30                                       ` Christian Brauner
2024-02-01 13:39                                         ` Christian Brauner
2024-02-01 19:33                                           ` Andy Lutomirski
2024-01-23 15:34 ` [PATCH v3 2/3] selftests/pidfd: add non-thread-group leader tests Tycho Andersen
2024-01-23 15:34 ` [PATCH v3 3/3] clone: allow CLONE_THREAD | CLONE_PIDFD together Tycho Andersen

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=ZbKigMNQM0Yklc/5@tycho.pizza \
    --to=tycho@tycho.pizza \
    --cc=brauner@kernel.org \
    --cc=ebiederm@xmission.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@redhat.com \
    --cc=tandersen@netflix.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;
as well as URLs for NNTP newsgroup(s).