| Message ID | cover.1738268370.git.lorenzo.stoakes@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | introduce PIDFD_SELF* sentinels | expand |
On Thu, 30 Jan 2025 20:40:25 +0000 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > If you wish to utilise a pidfd interface to refer to the current process or > thread it is rather cumbersome, requiring something like: > > int pidfd = pidfd_open(getpid(), 0 or PIDFD_THREAD); > > ... > > close(pidfd); > > Or the equivalent call opening /proc/self. It is more convenient to use a > sentinel value to indicate to an interface that accepts a pidfd that we > simply wish to refer to the current process thread. > The above code sequence doesn't seem at all onerous. I'm not understanding why it's worth altering the kernel to permit this little shortcut?
On Thu, Jan 30, 2025 at 02:37:54PM -0800, Andrew Morton wrote: > On Thu, 30 Jan 2025 20:40:25 +0000 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > > If you wish to utilise a pidfd interface to refer to the current process or > > thread it is rather cumbersome, requiring something like: > > > > int pidfd = pidfd_open(getpid(), 0 or PIDFD_THREAD); > > > > ... > > > > close(pidfd); > > > > Or the equivalent call opening /proc/self. It is more convenient to use a > > sentinel value to indicate to an interface that accepts a pidfd that we > > simply wish to refer to the current process thread. > > > > The above code sequence doesn't seem at all onerous. I'm not > understanding why it's worth altering the kernel to permit this little > shortcut? In practice it adds quite a bit of overhead for something that whatever mechanism is using the pidfd can avoid. It was specifically intended for a real case of utilising process_madvise(), using the newly extended ability to batch _any_ madvise() operations for the current process, like: if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) { ... error handling ... } vs. pid_t pid = getpid(); int pidfd = pidfd_open(pid, PIDFD_THREAD); if (pidfd < 0) { ... error handling ... } if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) { ... cleanup pidfd ... ... error handling ... } ... ... cleanup pidfd ... So in practice, it's actually a lot more ceremony and noise. Suren has been working with this code in practice and found this to be useful. The suggestion to embed it as PIDFD_SELF rather than to pass it as a process_madvise() flag was made on the original series where I extended its functionality. So in practice I think it's onerous enough to justify this, plus it allows for a more fluent use of pidfd's in other cases where one is referring to the same process/thread, to the extent that I've seen people commenting on supporting it while sending series relating to pidfd. Also Christian and others appear to support this idea.
On Thu, Jan 30, 2025 at 10:53 PM Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > On Thu, Jan 30, 2025 at 02:37:54PM -0800, Andrew Morton wrote: > > On Thu, 30 Jan 2025 20:40:25 +0000 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > > > > If you wish to utilise a pidfd interface to refer to the current process or > > > thread it is rather cumbersome, requiring something like: > > > > > > int pidfd = pidfd_open(getpid(), 0 or PIDFD_THREAD); > > > > > > ... > > > > > > close(pidfd); > > > > > > Or the equivalent call opening /proc/self. It is more convenient to use a > > > sentinel value to indicate to an interface that accepts a pidfd that we > > > simply wish to refer to the current process thread. > > > > > > > The above code sequence doesn't seem at all onerous. I'm not > > understanding why it's worth altering the kernel to permit this little > > shortcut? > > In practice it adds quite a bit of overhead for something that whatever > mechanism is using the pidfd can avoid. > > It was specifically intended for a real case of utilising > process_madvise(), using the newly extended ability to batch _any_ > madvise() operations for the current process, like: > > if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) { > ... error handling ... > } > > vs. > > pid_t pid = getpid(); > int pidfd = pidfd_open(pid, PIDFD_THREAD); > > if (pidfd < 0) { > ... error handling ... > } > > if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) { > ... cleanup pidfd ... > ... error handling ... > } > > ... > > ... cleanup pidfd ... > > So in practice, it's actually a lot more ceremony and noise. Suren has been > working with this code in practice and found this to be useful. It's also nice to add that people on the libc/allocator side should also appreciate skipping pidfd_open's reliability concerns (mostly, that RLIMIT_NOFILE Should Not(tm) ever affect thread spawning or a malloc[1]). Besides the big syscall reduction and nice speedup, that is. [1] whether this is the already case is an exercise left to the reader, but at the very least we should not add onto existing problems
On Thu, 30 Jan 2025 23:10:53 +0000 Pedro Falcato <pedro.falcato@gmail.com> wrote: > On Thu, Jan 30, 2025 at 10:53 PM Lorenzo Stoakes > <lorenzo.stoakes@oracle.com> wrote: > > > > > The above code sequence doesn't seem at all onerous. I'm not > > > understanding why it's worth altering the kernel to permit this little > > > shortcut? > > > > In practice it adds quite a bit of overhead for something that whatever > > mechanism is using the pidfd can avoid. > > > > It was specifically intended for a real case of utilising > > process_madvise(), using the newly extended ability to batch _any_ > > madvise() operations for the current process, like: > > > > if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) { > > ... error handling ... > > } > > > > vs. > > > > pid_t pid = getpid(); > > int pidfd = pidfd_open(pid, PIDFD_THREAD); > > > > if (pidfd < 0) { > > ... error handling ... > > } > > > > if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) { > > ... cleanup pidfd ... > > ... error handling ... > > } > > > > ... > > > > ... cleanup pidfd ... > > > > So in practice, it's actually a lot more ceremony and noise. Suren has been > > working with this code in practice and found this to be useful. > > It's also nice to add that people on the libc/allocator side should > also appreciate skipping pidfd_open's reliability concerns (mostly, > that RLIMIT_NOFILE Should Not(tm) ever affect thread spawning or a > malloc[1]). Besides the big syscall reduction and nice speedup, that > is. > > [1] whether this is the already case is an exercise left to the > reader, but at the very least we should not add onto existing problems Thanks. Could we please get all the above spelled out much more thoroughly in the [0/n] description (aka Patch Series Sales Brochure)?