Re: [PATCH v2 1/3] fs/file.c: add fast path in alloc_fd()

[Tim Chen <tim.c.chen@linux.intel.com>]

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@suse.com>
> > > > SUSE Labs, CR
> > > > 
> >