qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v5] osdep: add a qemu_close_all_open_fd() helper


From: Richard Henderson
Subject: Re: [PATCH v5] osdep: add a qemu_close_all_open_fd() helper
Date: Tue, 30 Jul 2024 07:32:26 +1000
User-agent: Mozilla Thunderbird

On 7/30/24 00:20, Clément Léger wrote:


On 29/07/2024 16:00, Philippe Mathieu-Daudé wrote:
Hi Clément,

On 26/7/24 09:54, Clément Léger wrote:
Since commit 03e471c41d8b ("qemu_init: increase NOFILE soft limit on
POSIX"), the maximum number of file descriptors that can be opened are
raised to nofile.rlim_max. On recent debian distro, this yield a maximum
of 1073741816 file descriptors. Now, when forking to start
qemu-bridge-helper, this actually calls close() on the full possible file
descriptor range (more precisely [3 - sysconf(_SC_OPEN_MAX)]) which
takes a considerable amount of time. In order to reduce that time,
factorize existing code to close all open files descriptors in a new
qemu_close_all_open_fd() function. This function uses various methods
to close all the open file descriptors ranging from the most efficient
one to the least one. It also accepts an ordered array of file
descriptors that should not be closed since this is required by the
callers that calls it after forking. Since this function is not used
for Win32, do not implement it to force an error at link time if used.

Signed-off-by: Clément Léger <cleger@rivosinc.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
v5:
   - Move qemu_close_all_open_fd() to oslib-posix.c since it does not
     compile on windows and is not even used on it.
   - v4:
20240717124534.1200735-1-cleger@rivosinc.com/">https://lore.kernel.org/qemu-devel/20240717124534.1200735-1-cleger@rivosinc.com/

v4:
   - Add a comment saying that qemu_close_all_open_fd() can take a NULL
skip
     array and nskip == 0
   - Added an assert in qemu_close_all_open_fd() to check for skip/nskip
     parameters
   - Fix spurious tabs instead of spaces
   - Applied checkpatch
   - v3:
20240716144006.6571-1-cleger@rivosinc.com/">https://lore.kernel.org/qemu-devel/20240716144006.6571-1-cleger@rivosinc.com/

v3:
   - Use STD*_FILENO defines instead of raw values
   - Fix indentation of close_all_fds_after_fork()
   - Check for nksip in fallback code
   - Check for path starting with a '.' in qemu_close_all_open_fd_proc()
   - Use unsigned for cur_skip
   - Move ifdefs inside close_fds functions rather than redefining them
   - Remove uneeded 'if(nskip)' test
   - Add comments to close_range version
   - Reduce range of skip fd as we find them in
   - v2:
https://lore.kernel.org/qemu-devel/20240618111704.63092-1-cleger@rivosinc.com/

v2:
   - Factorize async_teardown.c close_fds implementation as well as
tap.c ones
   - Apply checkpatch
   - v1:
https://lore.kernel.org/qemu-devel/20240617162520.4045016-1-cleger@rivosinc.com/

---
   include/qemu/osdep.h    |   9 +++
   net/tap.c               |  33 +++++-----
   system/async-teardown.c |  37 +-----------
   util/oslib-posix.c      | 131 ++++++++++++++++++++++++++++++++++++++++
   4 files changed, 160 insertions(+), 50 deletions(-)

I'm getting this error on msys2, not sure if related:

WARNING: Failed to terminate process: 1 error occurred:
     * failed to attach the runner process to the console of its parent
process: The handle is invalid.

I find your patch hard to review. Do you mind splitting as trivial
changes? Something like:

- Expose close_all_open_fd() renamed as qemu_close_all_open_fd()
- Rework qemu_close_all_open_fd()
- Factor close_all_fds_after_fork() in net/tap.c
- Use qemu_close_all_open_fd() in net/tap.c

Yes sure, I'll do that.

If you're making updates, I think you should drop the linux ifdef for /proc/self/fd. This is also present on Solaris. Importantly, it'll compile on all POSIX systems, whether or not /proc is available.


r~



reply via email to

[Prev in Thread] Current Thread [Next in Thread]