Mailing List Archive

Alexey Izbyshev <izbyshev@ispras.ru> added the comment:

I've updated my PR.

* After a discussion with Alexander Monakov (a GCC developer), moved vfork() into a small function to isolate it from both subprocess_fork_exec() and child_exec(). This appears to be the best strategy to avoid -Wclobbered false positives as well as potential compiler bugs.

* Got rid of VFORK_USABLE checks in function parameter lists. Now `child_sigmask` parameter is always present, but is NULL if vfork() is not available or usable. The type is `void *` to avoid hard dependence on sigset_t, which other CPython source files appear to avoid.

* Disabled vfork() in case setuid()/setgid()/setgroups() needed.

* Added more comments explaining vfork()-related stuff.

* Added commit message and NEWS entry.

Potential improvements:

* To avoid repeating long parameter lists in several functions, we can move them to a struct. The downside is that we'd need to convert child_exec() to use that struct all over the place. I don't have a strong preference here.

* Are any documentation changes needed? We don't change any interfaces, so I'm not sure.

Potential future directions:

* If desired, a vfork() opt-out may be implemented. But it'd need to disable posix_spawn() on Linux as well, since it might be implemented via vfork(), and we have no good way to check that.

----------

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Gregory P. Smith <greg@krypto.org> added the comment:


New changeset 976da903a746a5455998e9ca45fbc4d3ad3479d8 by Alexey Izbyshev in branch 'master':
bpo-35823: subprocess: Use vfork() instead of fork() on Linux when safe (GH-11671)
https://github.com/python/cpython/commit/976da903a746a5455998e9ca45fbc4d3ad3479d8


----------

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Gregory P. Smith <greg@krypto.org> added the comment:

now waiting to see how happy all of the buildbots are...

We currently have a `__linux__` check in the code deciding VFORK_USABLE.

>From what I can tell, vfork probably also works on macOS (darwin).

Lets let this run for a bit on Linux and it can be a separate issue to open vfork usage up to other platforms.

----------
stage: patch review -> commit review

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Gregory P. Smith <greg@krypto.org> added the comment:

> * To avoid repeating long parameter lists in several functions, we can move them to a struct. The downside is that we'd need to convert child_exec() to use that struct all over the place. I don't have a strong preference here.

Agreed that the long parameter lists are annoying. I don't like shoving that much on the stack and by adding an additional call with so many params we've increase stack usage. I consider this refactoring work that can be done on its own outside of this issue though.

> * Are any documentation changes needed? We don't change any interfaces, so I'm not sure.

I don't think so. I looked things over and I think all that is needed is the existing Misc/NEWS entry.

> Potential future directions:
>
> * If desired, a vfork() opt-out may be implemented. But it'd need to disable posix_spawn() on Linux as well, since it might be implemented via vfork(), and we have no good way to check that.

We have such an opt-out capabilty for posix_spawn via `subprocess._USE_POSIX_SPAWN` https://github.com/python/cpython/blob/master/Lib/subprocess.py#L651
along with code in there that determines the default value based on the detected runtime environment.

I'm not sure we'll need that for vfork() as it seems to pretty much be a low level raw system call wrapper rather than something to be implemented via libc that can have its own bugs. If we do wind up wanting one, I'd structure it the same way.

----------

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Gregory P. Smith <greg@krypto.org> added the comment:

Thank you for taking this on! I'm calling it fixed for now as the buildbots are looking happy with it. If issues with it arise we can address them.

----------
resolution: -> fixed
stage: commit review -> resolved
status: open -> closed

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Ronald Oussoren <ronaldoussoren@mac.com> added the comment:

> From what I can tell, vfork probably also works on macOS (darwin).
>
> Lets let this run for a bit on Linux and it can be a separate issue to
> open vfork usage up to other platforms.

I'd prefer to not use vfork on macOS. For one I don't particularly trust that vfork would work reliably when using higher level APIs, but more importantly posix_spawn on macOS has some options that are hard to achieve otherwise and might be useful to expose in subprocess. An example of this is selecting the CPU architecture to use for the launched process when using fat binaries and a system that supports CPU emulation, such as the intel emulation on the upcoming Apple Silicon systems.

----------

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Change by Alexey Izbyshev <izbyshev@ispras.ru>:


----------
pull_requests: +21862
pull_request: https://github.com/python/cpython/pull/22944

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Alexey Izbyshev <izbyshev@ispras.ru> added the comment:

> Thank you for taking this on! I'm calling it fixed for now as the buildbots are looking happy with it. If issues with it arise we can address them.

Thank you for reviewing and merging!

Using POSIX_CALL for pthread_sigmask() is incorrect, however, since it *returns* the error instead of setting errno. I've opened a PR with a fix and additional handling of the first pthread_sigmask() call in the parent (which can be done easily).

I've also noticed a memory leak why doing the above: cwd_obj2 is not released on the error path. It probably slipped with patches adding user/groups support. I'll file a separate issue.

Would you also clarify why you disallowed vfork() in case setsid() is needed? Have you discovered any potential issues with setsid()? Note that setsid() doesn't suffer from thread sync issues like setuid()/setgid()/etc.

----------

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Gregory P. Smith <greg@krypto.org> added the comment:


New changeset 473db47747bb8bc986d88ad81799bcbd88153ac5 by Alexey Izbyshev in branch 'master':
bpo-35823: subprocess: Fix handling of pthread_sigmask() errors (GH-22944)
https://github.com/python/cpython/commit/473db47747bb8bc986d88ad81799bcbd88153ac5


----------

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Gregory P. Smith <greg@krypto.org> added the comment:

regarding excluding the setsid() case: I was being conservative as I couldn't find a reference of what was and wasn't allowed after vfork.

I found one thing suggesting that on macOS setsid() was not safe after vfork(). But that appeared to be a Darwin-ism. I expect that is not true on Linux as it should just be a syscall updating a couple of fields in the process info. Confirming, in glibc is appears to be a shim for the setsid syscall (based on not finding any code implementing anything special for it) and in uclibc (*much* easier to read) it is clearly just a setsid syscall shim.

I'll make a PR to undo the setsid restriction given we're Linux only.

----------

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Change by Gregory P. Smith <greg@krypto.org>:


----------
pull_requests: +21863
pull_request: https://github.com/python/cpython/pull/22945

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Gregory P. Smith <greg@krypto.org> added the comment:


New changeset be3c3a0e468237430ad7d19a33c60d306199a7f2 by Gregory P. Smith in branch 'master':
bpo-35823: Allow setsid() after vfork() on Linux. (GH-22945)
https://github.com/python/cpython/commit/be3c3a0e468237430ad7d19a33c60d306199a7f2


----------

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Alexey Izbyshev <izbyshev@ispras.ru> added the comment:

> regarding excluding the setsid() case: I was being conservative as I couldn't find a reference of what was and wasn't allowed after vfork.

Yes, there is no list of functions allowed after vfork(), except for the conservative POSIX.1 list consisting only of _exit() and execve(), so we can only take async-signal-safe functions as a first approximation and work from there. Thankfully, on Linux, C libraries don't do anything fancy in most cases. But, for example, it appears that on FreeBSD we wouldn't be able to use sigprocmask()/sigaction()[1]. BTW, commit[2] and the linked review are an interesting reading for anybody who would like to support posix_spawn() and/or vfork() in subprocess on FreeBSD.

> Confirming, in glibc is appears to be a shim for the setsid syscall (based on not finding any code implementing anything special for it) and in uclibc (*much* easier to read) it is clearly just a setsid syscall shim.

I also recommend musl[3] when simplicity (and correctness) is required :)

[1] https://svnweb.freebsd.org/base/head/lib/libc/gen/posix_spawn.c?view=markup&pathrev=362111#l126
[2] https://svnweb.freebsd.org/base?view=revision&revision=352712
[3] https://git.musl-libc.org/cgit/musl/tree/src/unistd/setsid.c?id=a5aff1972c9e3981566414b09a28e331ccd2be5d

----------

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Alexey Izbyshev <izbyshev@ispras.ru> added the comment:

@ronaldoussoren

> I'd prefer to not use vfork on macOS. For one I don't particularly trust that vfork would work reliably when using higher level APIs, but more importantly posix_spawn on macOS has some options that are hard to achieve otherwise and might be useful to expose in subprocess.

I can't comment on vfork() usability on macOS myself, but given the number of issues/considerations described here, I expect that significant research would be needed to check that.

Regarding your second point about extra posix_spawn() options, I actually don't see why it would be an argument against vfork(). Even on Linux, subprocess prefers posix_spawn() to vfork()/fork() when possible, so vfork() support doesn't hinder posix_spawn().

----------

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Gregory P. Smith <greg@krypto.org> added the comment:

Nice links. LOL, yes, musl source was going to be my next stop if the larger libc sources proved impossible for a mere mortal to reason about. :)

regarding macOS, agreed. If someone needs vfork() to work there, I believe it could be made to.

Options like selecting the architecture of the child process could be higher level options to the subprocess API. Hiding the platform specific details of how that happens and deciding which underlying approach to use based on the flags.

multi-arch systems are a thing. It is conceivable that we may even see non-esoteric multi-arch hardware at some point.

----------

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com

Gregory P. Smith <greg@krypto.org> added the comment:

Performance improvement measured on a 1.4Ghz quad aarch64 A57 (nvidia jetson nano):

#define VFORK_USABLE 1
test_subprocess: 36.5 seconds

#undef VFORK_USABLE
test_subprocess: 45 seconds

Nice. I really didn't expect a 20% speedup on its testsuite alone!

Lets dive into that with a microbenchmark:

$ ./build-novfork/python ten_seconds_of_truth.py
Launching /bin/true for 10 seconds in a loop.
Launched 2713 subprocesses in 10.00194378796732 seconds.
271.247275281014 subprocesses/second.
Increased our mapped pages by 778240KiB.
Launching /bin/true for 10 seconds in a loop.
Launched 212 subprocesses in 10.006392606999725 seconds.
21.186456331095847 subprocesses/second.

$ ./build/python ten_seconds_of_truth.py
Launching /bin/true for 10 seconds in a loop.
Launched 3310 subprocesses in 10.001623224001378 seconds.
330.94628000551285 subprocesses/second.
Increased our mapped pages by 778240KiB.
Launching /bin/true for 10 seconds in a loop.
Launched 3312 subprocesses in 10.001519071985967 seconds.
331.1496959773679 subprocesses/second.

Demonstrating perfectly the benefit of vfork(). The more mapped pages, the slower regular fork() becomes. With vfork() the size of the process's memory map does not matter.

ten_seconds_of_truth.py:

```python
from time import monotonic as now
import subprocess

def benchmark_for_ten():
print('Launching /bin/true for 10 seconds in a loop.')

count = 0
start = now()
while now() - start < 10.:
subprocess.run(['/bin/true'])
count += 1
end = now()
duration = end-start

print(f'Launched {count} subprocesses in {duration} seconds.')
print(f'{count/duration} subprocesses/second.')

benchmark_for_ten()

map_a_bunch_of_pages = '4agkahglahaa^#\0ag3\3'*1024*1024*40

print(f'Increased our mapped pages by {len(map_a_bunch_of_pages)//1024}KiB.')

benchmark_for_ten()
```

----------

_______________________________________
Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue35823>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/list-python-bugs%40lists.gossamer-threads.com