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

[Clément Léger]

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.

Clément

> 
> Thanks,
> 
> Phil.