On Tue, Oct 01, 2024 at 12:21:32PM GMT, Christian Brauner wrote:
On Mon, Sep 30, 2024 at 03:32:25PM GMT, Lorenzo Stoakes wrote:
On Mon, Sep 30, 2024 at 04:21:23PM GMT, Aleksa Sarai wrote:
[snip]
Sorry to bike-shed, but to match /proc/self and /proc/thread-self, maybe they should be called PIDFD_SELF (for tgid) and PIDFD_THREAD_SELF (for current's tid)? In principle I guess users might use PIDFD_SELF by accident but if we mirror the naming with /proc/{,thread-}self that might not be that big of an issue?
Lol, you know I wasn't even aware /proc/thread-self existed...
Wait until you learn that /proc/$TID thread entries exist but aren't shown when you do ls -al /proc, only when you explicitly access them.
My God... You're right, that's crazy... :)
Yeah, that actually makes sense and is consistent, though obviously the concern is people will reflexively use PIDFD_SELF and end up with potentially confusing results.
I will obviously be doing a manpage patch for this so we can spell it out there very clearly and also in the header - so I'd actually lean towards doing this myself.
Christian, Florian? Thoughts?
I think adding both would be potentially useful. How about:
#define PIDFD_SELF -100 #define PIDFD_THREAD_GROUP -200
Sure, makes sense to add both.
This will make PIDFD_SELF mean current and PIDFD_THREAD_GROUP mean current->pid_links[PIDTYPE_TGID]. I don't think we need to or should mirror procfs in any way. pidfds are intended to be usable without procfs at all.
Yeah, I think it's important to ensure the _default_ choice, so in this case PIDFD_SELF clearly, is one that will be least surprising.
The proc thing is sort of pleasing from an aesthetic point of view, but if you followed it you'd have to _clearly_ document PIDFD_THREAD_SELF as the default.
Happy to go along with this. PIDFD_THREAD_GROUP is also clearer as it is distinct from PIDFD_SELF (doesn't reference 'self' at all).
I want to leave one comment on a bit of quirkiness in the api when we add this. I don't consider it a big deal but it should be pointed out.
It can be useful to allow PIDFD_SELF or PIDFD_THREAD_GROUP to refer to the calling thread even for pidfd_open() to avoid an additional getpid() system call:
(1) pidfd_open(PIDFD_SELF, PIDFD_THREAD) (2) pidfd_open(PIDFD_SELF, 0)
Hm, this is a bit weird, as these are pid_t's and PIDFD_SELF and PIDFD_THREAD_GROUP are otherwise (pid)fd's.
Being dummy values sort of allows us to put them into service here also, but it is just weird, we pass what is usually a pidfd to receive a pidfd, only this time it's an actually concrete one?
I'm not sure I like this, even though as you say it avoids a getpid().
If we did this I'd prefer it to be a separate name, even if it has the same numeric value (I guess we also might want to check for anything that uses a negative pid_t to refer to an error or something else too).
Perhaps PID_SELF and PID_THREAD_GROUP?
So if we allow this (Should we allow it?) then (1) is just redundant but whathever. But (2) is at least worth discussing: Should we reject (2) on the grounds of contradictory requests or allow it and document that it's equivalent to pidfd_open(getpid(), PIDFD_THREAD)? It feels like the latter would be ok.
Similar for pidfd_send_signal():
// redundant but ok: (1) pidfd_send_signal(PIDFD_SELF, PIDFD_SIGNAL_THREAD)
// redundant but ok: (2) pidfd_send_signal(PIDFD_THREAD_GROUP, PIDFD_SIGNAL_THREAD_GROUP)
// weird way to spell pidfd_send_signal(PIDFD_THREAD_GROUP, 0) (3) pidfd_send_signal(PIDFD_SELF, PIDFD_SIGNAL_THREAD_GROUP)
// weird way to spell pidfd_send_signal(PIDFD_SELF, 0) (4) pidfd_send_signal(PIDFD_THREAD_GROUP, PIDFD_SIGNAL_THREAD)
I think all of this is ok but does anyone else have a strong opinion?
I think it's fine to allow all 4 and we should get this behaviour by default (if we have no flags we use the f_flags as a hint, which will be set correctly).
I think people might find contradictory ones, i.e. 3 and 4, strange, but it makes sense for the flags to override the pidfd (as they would for a non-sentinel pidfd) and it makes everything consistent vs. if you were not using a sentinel value.
So yes I think that's fine.