Bug 221700
| Summary: | lang/python??: Add closefrom(2) support to subprocess on FreeBSD | ||
|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Ed Maste <emaste> |
| Component: | Individual Port(s) | Assignee: | Kubilay Kocak <koobs> |
| Status: | Closed FIXED | ||
| Severity: | Affects Many People | CC: | cem, dimpase+freebsd, emaste, jbeich, john, jonaspalm, koobs, victor.stinner, ygy |
| Priority: | Normal | Keywords: | performance |
| Version: | Latest | Flags: | koobs:
maintainer-feedback+
|
| Hardware: | Any | ||
| OS: | Any | ||
| URL: | https://bugs.python.org/issue38061 | ||
| Bug Depends on: | 242274 | ||
| Bug Blocks: | 221696 | ||
Related python bugs: https://bugs.python.org/issue8052 https://bugs.python.org/issue11284 https://bugs.python.org/issue1663329 A minimum viable patch to address this:
diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c
index 8c8777cfe3..d390ec8ee6 100644
--- a/Modules/_posixsubprocess.c
+++ b/Modules/_posixsubprocess.c
@@ -232,9 +232,13 @@ _close_fds_by_brute_force(long start_fd, PyObject *py_fds_to_keep)
start_fd = keep_fd + 1;
}
if (start_fd <= end_fd) {
+#if defined(__FreeBSD__)
+ closefrom(start_fd);
+#else
for (fd_num = start_fd; fd_num < end_fd; ++fd_num) {
close(fd_num);
}
+#endif
}
}
You might also consider closefrom() in posix_closerange() of Modules/posixmodule.c:
--- ./Modules/posixmodule.c 2015-05-23 09:09:20.000000000 -0700
+++ ./Modules/posixmodule.c 2015-07-17 15:00:38.784909475 -0700
@@ -6668,9 +6668,12 @@
if (!PyArg_ParseTuple(args, "ii:closerange", &fd_from, &fd_to))
return NULL;
Py_BEGIN_ALLOW_THREADS
- for (i = fd_from; i < fd_to; i++)
- if (_PyVerify_fd(i))
- close(i);
+ if (fd_to >= sysconf(_SC_OPEN_MAX)) {
+ closefrom(fd_from);
+ } else
+ for (i = fd_from; i < fd_to; i++)
+ if (_PyVerify_fd(i))
+ close(i);
Py_END_ALLOW_THREADS
Py_RETURN_NONE;
}
(In Python3, posix_closerange() is renamed to os_closerange_impl().) Is the behaviour (not using closefrom(2)) consistent across all Python versions, including 3.x up to 3.6 and default? Is closefrom(2) available on all (supported) FreeBSD versions? (In reply to Kubilay Kocak from comment #5) Yes; I don't believe any current version of Python uses closefrom(). closefrom() is available on all supported FreeBSD releases. HISTORY The closefrom() function first appeared in FreeBSD 8.0. For explicitness, this is effectively "Use closefrom(2) on FreeBSD", correct? We should create an issue upstream if one hasn't already been created, and any changes created should be appropriate for upstream inclusion, in particular closefrom(2) detection (using any existing mechanisms) We can carry local ports patches until the changes are merged upstream and included in release versions This is an issue who's resolution requires an upstream patch and issue (bug report) such that it can be correctly and permanently resolved there in all relevant branches. Reassign to reporter accordingly to coordinate that. Once a patch against upstream sources has been created and submitted, please create new separate issue blocking this one: lang/python??: Backport use closefrom(2) on FreeBSD and assign it to python@ We can carry it in the respective lang/python?? ports until new releases land. A twitter discussion referenced this PR and the question of async-signal-saftiness of closefrom: https://twitter.com/koobs/status/1046674765585367041 Note that closefrom is in sigaction(2)'s list of async-signal-safe functions, in the "Base Interfaces not specified as async-signal safe by POSIX, but planned to be:" section. These are interfaces that are async-signal-safe in FreeBSD, are not currently required to be so by POSIX, but are expected to be so in the future. That said, I am not sure why closefrom is in that list and not 'Extension Interfaces' as POSIX does not specify closefrom. (In reply to Ed Maste from comment #10) Oh, I was mistaken in the previous comment - closefrom is indeed in the "Extension Interfaces" section as it should be. See Also: base r194262 (original closefrom(2) syscall commit) (In reply to Ed Maste from comment #10) @Koobs (re: twitter), why would it not be async signal safe? In general signal safety is more about interacting with userspace memory state of ordinary non-signal contexts given arbitrary signal interruption. I.e., classic example is stdio FILE objects, which may be locked or have corrupt, half-modified state. closefrom(2) is just a plain system call. The original commit, r194262, just says: > Note that this implementation of closefrom(2) does not make any effort to > resolve userland races with open(2) in other threads. As such, it is not > multithread safe. That is a comment on threading, not signals. Signaled, single-threaded programs are still single-threaded; the signal context and ordinary thread context do not run concurrently. And by definition, a user process cannot run code of any kind — signal-handler or ordinary — while that user process is in the kernel. So closefrom(2) in signal handler cannot race with open(2) or other syscalls that create fds in single-threaded programs. The scenario the comment refers to is something like this: - The program has two threads and fds 0-5 allocated. - Concurrently, thread A and B attempt to: A: open() a new fd B: closefrom(3) - The behavior is unpredictable and depends on which thread acquires the fdesctable lock first. [r194262]: https://svnweb.freebsd.org/base?view=revision&revision=194262 (In reply to Ed Maste from comment #1) Note that upstream Python has closed all of these issues after fixing only Linux. Also, I believe that the concerns about async signal safety come from a link to the *Linux* closefrom manual page. Anyone have a python bugs account and want to try banging their head against that wall another time? Unassign, I won't be able to work on this in the near future. I just want to avoid the situation we have now where python is perceived to have terrible performance on FreeBSD. Hopefully one of the FreeBSD python maintainers has a good relationship with upstream and can coordinate, otherwise I will pick this up when I can. For explicitness and reference to future readers, current state is: 1) Yep, we're now comfortable that our closefrom(2) is async-signal safe (thanks emaste, conrad, others) 2) Yep, we want to resolve this issue, it has high value. I'm happy to coordinate and usher code into ports/upstream. 3) We want to resolve this in a manner upstream will accept. I think this will be in the form of something like a configure check for closefrom(2) and conditionals in code with HAVE_CLOSEFROM. If other platforms have closefrom(2) but aren't signal safe (or there's no positive evidence they are), we could additionally if && defined(__FreeBSD__) to be conservative in the first instance. We need patches for each upstream supported Python version (currently 2.7, 3.6, 3.7, default (3.8)) 4) I have a python bugs account, and have a good relationship with some core devs, who I'd like to get input/direction from (both in general and for patches we create). 5) At present we need a patch in a form similar to (3) that I can get upstream feedback on. With a patch reasonably close to (3) we can also consider carrying it locally in ports until upstream merges and releases future versions with support. I need (C) help with this if the diffs in comments are only examples and not 'complete/ready/finished'. Open Questions: Are there any places other than _posixsubprocess.c:_close_fds_by_brute_force and posix_closerange() that might benefit from this? (In reply to Kubilay Kocak from comment #16) Re (3), I think #ifdef __FreeBSD__ or maybe at least #ifndef __glibc__ or __linux__ are going to be a requirement from the Python community, given prior treatments of the issue. > Are there any places other than _posixsubprocess.c:_close_fds_by_brute_force > and posix_closerange() that might benefit from this? I don't know the answer to that, but I would encourage tackling the low hanging fruit first without even looking for other places, to reduce scope creep, risk, and scale of change. (In reply to Conrad Meyer from comment #17) Yep, just wanted the question there explicitly asked instead of checking for __FreeBSD__, there should be a call to AC_SEARCH_LIB() in Python's configure.ac, to look for `closefrom()`. (Note that `closefrom()` exists on many BSD's including Darwin. Any news on this? Python 2.7 EOL is in about four months and upgrading to 3.6 is a pain as i.e. Ansible plays take a lot longer: ansible-playbook-2.7: 57.987u 57.349s 0:42.26 272.8% 22+203k 9026+4552io 139pf+0w ansible-playbook-3.6 doing exactly the same thing: 315.849u 422.997s 4:12.47 292.6% 7+368k 14328+4540io 273pf+0w (In reply to Jonas Palm from comment #20) Try "mount -t fdescfs none /dev/fd" per https://github.com/python/cpython/commit/4842efcf972e. (In reply to Dima Pasechnik from comment #19) Sure, except Python has previously rejected some implementations of closefrom() (Linux's?). So the configure.ac check is not sufficient anyway. (In reply to Jonas Palm from comment #20) Here's a secret: nothing happens in 2020. Python 2.7.X works as well as it ever did; Py3.x will continue to diverge as a language. Tauthon is a 2.7 fork ("2.8") that aims to maintain 2 compatibility, while backporting some of 3.x. (In reply to Jan Beich from comment #21) Seems like they still haven't fixed _close_fds_by_brute_force(), though. It is unfortunate we need a special linux filesystem to have working Python when it's absolutely not needed. They try and keep some special list of fds open. But it's very likely those special few are low numbers (since fds always open minimum possible value and most programs do not reach fd limit). So special casing just the end of range would likely help significantly: Just replacing this bit with closefrom(): https://github.com/python/cpython/blob/4842efcf972eee8acef0840ecca34a88945a99a7/Modules/_posixsubprocess.c#L157-L161 Or we could add a closerange() with similar semantics. I'm not sure why we didn't. (In reply to Conrad Meyer from comment #23) Probably not, but I doubt that Ansible will support Tauthon or similar projects and will rather drop support for Python 2.7 in a couple of years. With all support dropping for Python 2.7, I'd rather be ahead of the time and migrate to a supported version right now. All the software I'm using is supporting Python 3.6, so the only reason for not migrating is this problem. (In reply to Jan Beich from comment #21) Thanks for the suggestion. If there isn't any progress on this bug I'll probably try this soon. A commit references this bug: Author: koobs Date: Fri Nov 29 10:55:03 UTC 2019 New revision: 518640 URL: https://svnweb.freebsd.org/changeset/ports/518640 Log: lang/python{27,35,36,37,38}: Add closefrom(2) support A single close(fd) syscall is cheap, but when MAXFDS (maximum file descriptor number) is high, the loop calling close(fd) on each file descriptor can take several milliseconds. The default value of subprocess.Popen "close_fds" parameter changed to True in Python 3. Compared to Python 2, close_fds=True can make Popen 10x slower: see bpo-37790 [1] The present workaround on FreeBSD to improve performance is to load and mount the fdescfs kernel module, but this is not enabled by default. This change adds minimum viable (and upstreamable) closefrom(2) syscall support to Python's subprocess and posix modules, improving performance significantly for loads that involve working with many processes, such as diffoscope, ansible, and many others. For additional optimizations, upstream recently (3.8) landed posix_spawn(2) support [3] and has stated that they will adopt close_range(2) after Linux merges it [4]. Linux/FreeBSD developers are already collaborating on ensuring compatible implementations, with FreeBSD's implementation pending in D21627. [5] Thank you emaste, cem, kevans for providing analysis, input, clarifications, comms/upstream support and patches. [1] https://bugs.python.org/issue37790 [2] https://bugs.python.org/issue38061 [3] https://bugs.python.org/issue35537 [4] https://lwn.net/Articles/789023/ [5] https://reviews.freebsd.org/D21627 Additional References: https://bugs.python.org/issue8052 https://bugs.python.org/issue11284 https://bugs.python.org/issue13788 https://bugs.python.org/issue1663329 https://www.python.org/dev/peps/pep-0446/ PR: 242274, 221700 Submitted by: kevans (emaste, cem) Approved by: koobs (python (maintainer), santa) Changes: head/lang/python27/Makefile head/lang/python27/files/patch-Modules_posixmodule.c head/lang/python35/Makefile head/lang/python35/files/patch-Modules___posixsubprocess.c head/lang/python35/files/patch-Modules_posixmodule.c head/lang/python36/Makefile head/lang/python36/files/patch-Modules___posixsubprocess.c head/lang/python36/files/patch-Modules_posixmodule.c head/lang/python37/Makefile head/lang/python37/files/patch-Modules___posixsubprocess.c head/lang/python37/files/patch-Modules_posixmodule.c head/lang/python38/Makefile head/lang/python38/files/patch-Modules___posixsubprocess.c head/lang/python38/files/patch-Modules_posixmodule.c Thank you everyone for your support and consistent encouragement and reminders to get this done. Thank you Kyle for the final QA'd patch and Ed/Conrad for the original implementations. A commit references this bug: Author: jbeich Date: Fri Nov 29 12:45:12 UTC 2019 New revision: 518646 URL: https://svnweb.freebsd.org/changeset/ports/518646 Log: gecko: drop close_fds workaround after r518640 PR: 221700 Changes: head/mail/thunderbird/files/patch-bug1507655 head/www/cliqz/files/patch-bug1507655 head/www/firefox/files/patch-bug1507655 head/www/firefox-esr/files/patch-bug1507655 |
% python --version Python 2.7.13 % pkg info python | grep Version Version : 2.7_3,2 % ktrace -i python -c 'import subprocess; subprocess.call("/usr/bin/true", close_fds=True);' % kdump -s | egrep -c 'CALL.*close' 939647 excerpt from kdump: 1958 python2.7 CALL close(0x3) 1958 python2.7 RET close -1 errno 9 Bad file descriptor 1958 python2.7 CALL close(0x5) 1958 python2.7 RET close -1 errno 9 Bad file descriptor 1958 python2.7 CALL close(0x6) 1958 python2.7 RET close -1 errno 9 Bad file descriptor 1958 python2.7 CALL close(0x7) 1958 python2.7 RET close -1 errno 9 Bad file descriptor 1958 python2.7 CALL close(0x8) 1958 python2.7 RET close -1 errno 9 Bad file descriptor 1958 python2.7 CALL close(0x9) 1958 python2.7 RET close -1 errno 9 Bad file descriptor ... 1958 python2.7 CALL close(0xe5628) 1958 python2.7 RET close -1 errno 9 Bad file descriptor 1958 python2.7 CALL close(0xe5629) 1958 python2.7 RET close -1 errno 9 Bad file descriptor 1958 python2.7 CALL close(0xe562a) 1958 python2.7 RET close -1 errno 9 Bad file descriptor 1958 python2.7 CALL close(0xe562b) 1958 python2.7 RET close -1 errno 9 Bad file descriptor This has both a performance impact and an impact on the ability to debug problems with python programs.