linux-kernel - Re: [PATCH v2 1/3] fs/file.c: add fast path in alloc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 27 Jun 2024 09:21:40 -0700
From: Tim Chen <tim.c.chen@...ux.intel.com>
To: Jan Kara <jack@...e.cz>
Cc: "Ma, Yu" <yu.ma@...el.com>, viro@...iv.linux.org.uk, brauner@...nel.org,
  mjguzik@...il.com, edumazet@...gle.com, linux-fsdevel@...r.kernel.org, 
 linux-kernel@...r.kernel.org, pan.deng@...el.com, tianyou.li@...el.com, 
 tim.c.chen@...el.com
Subject: Re: [PATCH v2 1/3] fs/file.c: add fast path in alloc_fd()

On Thu, 2024-06-27 at 14:09 +0200, Jan Kara wrote:
> On Wed 26-06-24 09:52:50, Tim Chen wrote:
> > 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. 
> 
> Well, I propose calling find_next_zero_bit() with offset set to 'start' so
> it cannot possibly happen that the returned bit number is smaller than
> start... But maybe I'm missing something?

Yes, you're right. the search begin at start bit so it is okay.

Tim

> 
> 									Honza