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

[Clément Léger]

On 29/07/2024 23:32, Richard Henderson wrote:
> 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.

Acked

Thanks,

Clément

> 
> 
> r~