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.