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

[Clément Léger]

On 16/07/2024 16:46, Philippe Mathieu-Daudé wrote:
> Hi Clément,
> 
> On 16/7/24 16:39, 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.
>>
>> Signed-off-by: Clément Léger <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    |   8 +++
>>   net/tap.c               |  33 +++++-----
>>   system/async-teardown.c |  37 +-----------
>>   util/osdep.c            | 129 ++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 157 insertions(+), 50 deletions(-)
>>
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index 191916f38e..43a035d756 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -757,6 +757,14 @@ static inline void qemu_reset_optind(void)
>>     int qemu_fdatasync(int fd);
>>   +/**
>> + * Close all open file descriptors except the ones supplied in the
>> @skip array
>> + *
>> + * @skip: ordered array of distinct file descriptors that should not
>> be closed
>> + * @nskip: number of entries in the @skip array.
>> + */
> 
> Can we mention @skip is optional, and @nskip can be 0?
> 
> Also I'd assert(skip != NULL || nskip == 0).

Yes sure, I'll add that.

Thanks,

Clément