On Wed, 2024-06-26 at 13:54 +0200, Jan Kara wrote:
>
>
> Indeed, thanks for correcting me! next_fd is just a lower bound for the
> first free fd.
>
> > The conditions
> > should either be like it is in patch or if (!start && !test_bit(0,
> > fdt->full_fds_bits)), the latter should also have the bitmap loading cost,
> > but another point is that a bit in full_fds_bits represents 64 bits in
> > open_fds, no matter fd >64 or not, full_fds_bits should be loaded any way,
> > maybe we can modify the condition to use full_fds_bits ?
>
> So maybe I'm wrong but I think the biggest benefit of your code compared to
> plain find_next_fd() is exactly in that we don't have to load full_fds_bits
> into cache. So I'm afraid that using full_fds_bits in the condition would
> destroy your performance gains. Thinking about this with a fresh head how
> about putting implementing your optimization like:
>
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -490,6 +490,20 @@ static unsigned int find_next_fd(struct fdtable *fdt, unsigned int start)
> unsigned int maxbit = maxfd / BITS_PER_LONG;
> unsigned int bitbit = start / BITS_PER_LONG;
>
> + /*
> + * Optimistically search the first long of the open_fds bitmap. It
> + * saves us from loading full_fds_bits into cache in the common case
> + * and because BITS_PER_LONG > start >= files->next_fd, we have quite
> + * a good chance there's a bit free in there.
> + */
> + if (start < BITS_PER_LONG) {
> + unsigned int bit;
> +
> + bit = find_next_zero_bit(fdt->open_fds, BITS_PER_LONG, start);
Say start is 31 (< BITS_PER_LONG)
bit found here could be 32 and greater than start. Do we care if we return bit > start?
Tim
> + if (bit < BITS_PER_LONG)
> + return bit;
> + }
> +
> bitbit = find_next_zero_bit(fdt->full_fds_bits, maxbit, bitbit) * BITS_PER_LONG;
> if (bitbit >= maxfd)
> return maxfd;
>
> Plus your optimizations with likely / unlikely. This way the code flow in
> alloc_fd() stays more readable, we avoid loading the first open_fds long
> into cache if it is full, and we should get all the performance benefits?
>
> Honza
>
>
> > > > > + goto fastreturn;
> > > > > + }
> > > > > fd = find_next_fd(fdt, fd);
> > > > > + }
> > > > > +
> > > > > + if (unlikely(fd >= fdt->max_fds)) {
> > > > > + error = expand_files(files, fd);
> > > > > + if (error < 0)
> > > > > + goto out;
> > > > > + /*
> > > > > + * If we needed to expand the fs array we
> > > > > + * might have blocked - try again.
> > > > > + */
> > > > > + if (error)
> > > > > + goto repeat;
> > > > > + }
> > > > > +fastreturn:
> > > > > /*
> > > > > * N.B. For clone tasks sharing a files structure, this test
> > > > > * will limit the total number of files that can be opened.
> > > > > */
> > > > > - error = -EMFILE;
> > > > > - if (fd >= end)
> > > > > + if (unlikely(fd >= end))
> > > > > goto out;
> > > > > - error = expand_files(files, fd);
> > > > > - if (error < 0)
> > > > > - goto out;
> > > > > -
> > > > > - /*
> > > > > - * If we needed to expand the fs array we
> > > > > - * might have blocked - try again.
> > > > > - */
> > > > > - if (error)
> > > > > - goto repeat;
> > > > > -
> > > > > if (start <= files->next_fd)
> > > > > files->next_fd = fd + 1;
> > > > > --
> > > > > 2.43.0
> > > > >
> > > > --
> > > > Jan Kara <jack@xxxxxxxx>
> > > > SUSE Labs, CR
> > > >
> >
