On 12/07/2024 17:12, Daniel P. Berrangé wrote:
> On Tue, Jun 18, 2024 at 01:17:03PM +0200, 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 <cle...@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-cle...@rivosinc.com/
>>
>> ---
>> include/qemu/osdep.h | 8 +++
>> net/tap.c | 31 ++++++-----
>> system/async-teardown.c | 37 +------------
>> util/osdep.c | 115 ++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 141 insertions(+), 50 deletions(-)
>>
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index f61edcfdc2..9369a97d3d 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -755,6 +755,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.
>> + */
>> +void qemu_close_all_open_fd(const int *skip, unsigned int nskip);
>> +
>> /**
>> * Sync changes made to the memory mapped file back to the backing
>> * storage. For POSIX compliant systems this will fallback
>> diff --git a/net/tap.c b/net/tap.c
>> index 51f7aec39d..6fc3939078 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -385,6 +385,21 @@ static TAPState *net_tap_fd_init(NetClientState *peer,
>> return s;
>> }
>>
>> +static void close_all_fds_after_fork(int excluded_fd)
>> +{
>> + const int skip_fd[] = {0, 1, 2, 3, excluded_fd};
>> + unsigned int nskip = ARRAY_SIZE(skip_fd);
>> +
>> + /*
>> + * skip_fd must be an ordered array of distinct fds, exclude
>> + * excluded_fd if already included in the [0 - 3] range
>> + */
>> + if (excluded_fd <= 3) {
>> + nskip--;
>> + }
>> + qemu_close_all_open_fd(skip_fd, nskip);
>> +}
>
> This is slightly over-indented - 4 space is QEMU normal style.
Indeed, my bad.
>
>> diff --git a/util/osdep.c b/util/osdep.c
>> index 5d23bbfbec..f3710710e3 100644
>> --- a/util/osdep.c
>> +++ b/util/osdep.c
>> @@ -625,3 +625,118 @@ int qemu_fdatasync(int fd)
>> return fsync(fd);
>> #endif
>> }
>> +
>> +#ifdef CONFIG_LINUX
>> +static bool qemu_close_all_open_fd_proc(const int *skip, unsigned int nskip)
>> +{
>> + struct dirent *de;
>> + int fd, dfd;
>> + bool close_fd;
>> + DIR *dir;
>> + int i;
>> +
>> + dir = opendir("/proc/self/fd");
>> + if (!dir) {
>> + /* If /proc is not mounted, there is nothing that can be done. */
>> + return false;
>> + }
>> + /* Avoid closing the directory. */
>> + dfd = dirfd(dir);
>> +
>> + for (de = readdir(dir); de; de = readdir(dir)) {
>
> Don't we need
>
> if (de->d_name[0] == '.') {
> continue;
> }
>
> otherwise atoi will fail and we'll try to close(0) multiple
> times.
Yes, that seems like it was the case for the previous code. I'll fix that.
Thanks,
Clément
>
>> + fd = atoi(de->d_name);
>> + close_fd = true;
>> + if (fd == dfd) {
>> + close_fd = false;
>> + } else {
>> + for (i = 0; i < nskip; i++) {
>> + if (fd == skip[i]) {
>> + close_fd = false;
>> + break;
>> + }
>> + }
>> + }
>> + if (close_fd) {
>> + close(fd);
>> + }
>> + }
>> + closedir(dir);
>> +
>> + return true;
>> +}
>
> With regards,
> Daniel
