Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

On Thu, Feb 1, 2024 at 5:39 AM Christian Brauner [off-list ref] wrote:

On Thu, Feb 01, 2024 at 02:30:46PM +0100, Christian Brauner wrote:

quoted

On Wed, Jan 31, 2024 at 11:50:23AM -0800, Andy Lutomirski wrote:

quoted

On Wed, Jan 31, 2024 at 11:46 AM Christian Brauner [off-list ref] wrote:

quoted

On Wed, Jan 31, 2024 at 11:24:48AM -0800, Andy Lutomirski wrote:

quoted

quoted

On 01/31, Oleg Nesterov wrote:

quoted

On 01/31, Andy Lutomirski wrote:
Please note

      /* TODO: respect PIDFD_THREAD */

this patch adds into pidfd_send_signal().

See also this part of discussion

      > > +   /* TODO: respect PIDFD_THREAD */
      >
      > So I've been thinking about this at the end of last week. Do we need to
      > give userspace a way to send a thread-group wide signal even when a
      > PIDFD_THREAD pidfd is passed? Or should we just not worry about this
      > right now and wait until someone needs this?

      I don't know. I am fine either way, but I think this needs a separate
      patch and another discussion in any case. Anyway should be trivial,
      pidfd_send_signal() has the "flags" argument.

with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/ (local)

I missed that.  Whoops.

On Wed, Jan 31, 2024 at 11:15 AM Oleg Nesterov [off-list ref] wrote:

quoted

Forgot to mention...

And I agree that pidfd_send_signal(flags => PGID/SID) can make
some sense too.

But this a) doesn't depend on PIDFD_THREAD, and b) needs another
patch/discussion.

But again, I am not sure I understood you correctly.

Hmm.

When one works with regular (non-fd) pids / pgids etc, one specifies
the signal domain at the time that one sends the signal.  I don't know
what pidfds should do.  It seems a bit inefficient for anything that
wants a pidfd and might send a signal in a different mode in the
future to have to hold on to multiple pidfds, so it probably should be
a pidfd_send_signal flag.

Which leaves the question of what the default should be.  Should
pidfd_send_signal with flags = 0 on a PIDFD_THREAD signal the process
or the thread?  I guess there are two reasonable solutions:

1. flags = 0 always means process.  And maybe there's a special flag
to send a signal that matches the pidfd type, or maybe not.

2. flags = 0 does what the pidfd seems to imply, and a new
PIDFD_SIGNAL_PID flag overrides it to signal the whole PID even if the
pidfd is PIDFD_THREAD.

Do any of you have actual use cases in mind where one choice is
clearly better than the other choice?

So conceptually I think having the type of pidfd dictate the default
scope of the signal is the most elegant approach. And then very likely
we should just have:

PIDFD_SIGNAL_THREAD
PIDFD_SIGNAL_THREAD_GROUP
PIDFD_SIGNAL_PROCESS_GROUP

I think for userspace it doesn't really matter as long as we clearly
document what's going on.

This seems reasonable unless we're likely to end up with a pidfd mode
that doesn't actually make sense in a send_signal context.  But I'm
not immediately seeing any reason that that would happen.

Yeah, I think that's very unlikely and we could reject it obased on api
design considerations.

Ah, forgot to ask. Did you intend to send a patch for this?

I can try to get to it tomorrow.  Currently trying to madly line up a
whole bunch of stuff in time for a maintenance window.