Re: [PATCH v6 2/5] pidfd: add PIDFD_SELF_* sentinels to refer to own thread/process

[Shakeel Butt <shakeel.butt@linux.dev>]

On Mon, Dec 02, 2024 at 10:52:13AM +0000, Lorenzo Stoakes wrote:
> On Fri, Nov 08, 2024 at 02:28:14PM +0000, Lorenzo Stoakes wrote:
> > On Wed, Oct 30, 2024 at 04:37:37PM +0000, Lorenzo Stoakes wrote:
> > > On Mon, Oct 28, 2024 at 04:06:07PM +0000, Lorenzo Stoakes wrote:
> > > > I guess I'll try to adapt that and respin a v7 when I get a chance.
> > >
> > > Hm looking at this draft patch, it seems like a total rework of pidfd's
> > > across the board right (now all pidfd's will need to be converted to
> > > pid_fd)? Correct me if I'm wrong.
> > >
> > > If only for the signal case, it seems like overkill to define a whole
> > > pid_fd and to use this CLASS() wrapper just for this one instance.
> > >
> > > If the intent is to convert _all_ pidfd's to use this type, it feels really
> > > out of scope for this series and I think we'd probably instead want to go
> > > off and do that as a separate series and put this on hold until that is
> > > done.
> > >
> > > If instead you mean that we ought to do something like this just for the
> > > signal case, it feels like it'd be quite a bit of extra abstraction just
> > > used in this one case but nowhere else, I think if you did an abstraction
> > > like this it would _have_ to be across the board right?
> > >
> > > I agree that the issue is with this one signal case that pins only the fd
> > > (rather than this pid) where this 'pinning' doesn't _necessary_ mess around
> > > with reference counts.
> > >
> > > So we definitely must address this, but the issue you had with the first
> > > approach was that I think (correct me if I'm wrong) I was passing a pointer
> > > to a struct fd which is not permitted right?
> > >
> > > Could we pass the struct fd by value to avoid this? I think we'd have to
> > > unfortunately special-case this and probably duplicate some code which is a
> > > pity as I liked the idea of abstracting everything to one place, but we can
> > > obviously do that.
> > >
> > > So I guess to TL;DR it, the options are:
> > >
> > > 1. Implement pid_fd everywhere, in which case I will leave off on
> > >    this series and I guess, if I have time I could look at trying to
> > >    implement that or perhaps you'd prefer to?
> > >
> > > 2. We are good for the sake of this series to special-case a pidfd_to_pid()
> > >    implementation (used only by the pidfd_send_signal() syscall)
> > >
> > > 3. Something else, or I am misunderstanding your point :)
> > >
> > > Let me know how you want me to proceed on this as we're at v6 already and I
> > > want to be _really_ sure I'm doing what you want here.
> > >
> > > Thanks!
> >
> > Hi Christian,
> >
> > Just a gentle nudge on this - as I need some guidance in order to know how
> > to move the series forwards.
> >
> > Obviously no rush if your workload is high at the moment as this is pretty
> > low priority, but just in case you missed it :)
> >
> > Thanks, Lorenzo
> 
> Hi Christian,
> 
> Just a ping on this now we're past the merge window and it's been over a
> month.
> 
> It'd be good to at least get a polite ack to indicate you're aware even if
> you don't have the time to respond right now.
> 
> If you'd prefer this series not to go ahead just let me know, but
> unfortunately I really require your input to know how to move forward
> otherwise I risk doing work that you might then reject.
> 

Hey Lorenzo & Christian, what's the latest here? I see Christian has
code suggestions at [1] which just needs to be addressed. Any thing
else? I am hoping we can get this merged in the coming open window.

[1] https://lore.kernel.org/linux-mm/20241202-wahrnehmen-mitten-e330cbd1eaf0@brauner/