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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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