On Wed, 2024-06-26 at 09:43 -0700, Tim Chen wrote:
> 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?
Sorry, I mean to say that we could find a bit like 30 that is less than start instead
of the other way round.
Tim
>
