| Message ID | 20240713023917.3967269-4-yu.ma@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fs/file.c: optimize the critical section of file_lock in | expand |
On Fri 12-07-24 22:39:17, Yu Ma wrote: > Skip 2-levels searching via find_next_zero_bit() when there is free slot in the > word contains next_fd, as: > (1) next_fd indicates the lower bound for the first free fd. > (2) There is fast path inside of find_next_zero_bit() when size<=64 to speed up > searching. > (3) After fdt is expanded (the bitmap size doubled for each time of expansion), > it would never be shrunk. The search size increases but there are few open fds > available here. > > This fast path is proposed by Mateusz Guzik <mjguzik@gmail.com>, and agreed by > Jan Kara <jack@suse.cz>, which is more generic and scalable than previous > versions. And on top of patch 1 and 2, it improves pts/blogbench-1.1.0 read by > 8% and write by 4% on Intel ICX 160 cores configuration with v6.10-rc7. > > Reviewed-by: Tim Chen <tim.c.chen@linux.intel.com> > Signed-off-by: Yu Ma <yu.ma@intel.com> Looks good. Just some code style nits below. > diff --git a/fs/file.c b/fs/file.c > index 1be2a5bcc7c4..a3ce6ba30c8c 100644 > --- a/fs/file.c > +++ b/fs/file.c > @@ -488,9 +488,20 @@ struct files_struct init_files = { > > static unsigned int find_next_fd(struct fdtable *fdt, unsigned int start) > { > + unsigned int bitbit = start / BITS_PER_LONG; > + unsigned int bit; > + > + /* > + * Try to avoid looking at the second level bitmap > + */ > + bit = find_next_zero_bit(&fdt->open_fds[bitbit], BITS_PER_LONG, > + start & (BITS_PER_LONG -1)); ^^ Either (BITS_PER_LONG-1) or (BITS_PER_LONG - 1) please. Your combination looks particularly weird :) > + if (bit < BITS_PER_LONG) { > + return bit + bitbit * BITS_PER_LONG; > + } No need for braces around the above block. > unsigned int maxfd = fdt->max_fds; /* always multiple of BITS_PER_LONG */ > unsigned int maxbit = maxfd / BITS_PER_LONG; We keep declarations at the beginning of the block. Usually it keeps the code more readable and the compiler should be clever enough to perform the loads & arithmetics only when needed. After fixing these style nits feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza
On 7/16/2024 7:19 PM, Jan Kara wrote: > On Fri 12-07-24 22:39:17, Yu Ma wrote: >> Skip 2-levels searching via find_next_zero_bit() when there is free slot in the >> word contains next_fd, as: >> (1) next_fd indicates the lower bound for the first free fd. >> (2) There is fast path inside of find_next_zero_bit() when size<=64 to speed up >> searching. >> (3) After fdt is expanded (the bitmap size doubled for each time of expansion), >> it would never be shrunk. The search size increases but there are few open fds >> available here. >> >> This fast path is proposed by Mateusz Guzik <mjguzik@gmail.com>, and agreed by >> Jan Kara <jack@suse.cz>, which is more generic and scalable than previous >> versions. And on top of patch 1 and 2, it improves pts/blogbench-1.1.0 read by >> 8% and write by 4% on Intel ICX 160 cores configuration with v6.10-rc7. >> >> Reviewed-by: Tim Chen <tim.c.chen@linux.intel.com> >> Signed-off-by: Yu Ma <yu.ma@intel.com> > Looks good. Just some code style nits below. Copy that, thanks Honza, I'll revise and send out updated version soon. > >> diff --git a/fs/file.c b/fs/file.c >> index 1be2a5bcc7c4..a3ce6ba30c8c 100644 >> --- a/fs/file.c >> +++ b/fs/file.c >> @@ -488,9 +488,20 @@ struct files_struct init_files = { >> >> static unsigned int find_next_fd(struct fdtable *fdt, unsigned int start) >> { >> + unsigned int bitbit = start / BITS_PER_LONG; >> + unsigned int bit; >> + >> + /* >> + * Try to avoid looking at the second level bitmap >> + */ >> + bit = find_next_zero_bit(&fdt->open_fds[bitbit], BITS_PER_LONG, >> + start & (BITS_PER_LONG -1)); > ^^ Either > (BITS_PER_LONG-1) or (BITS_PER_LONG - 1) please. Your combination looks > particularly weird :) > >> + if (bit < BITS_PER_LONG) { >> + return bit + bitbit * BITS_PER_LONG; >> + } > No need for braces around the above block. > >> unsigned int maxfd = fdt->max_fds; /* always multiple of BITS_PER_LONG */ >> unsigned int maxbit = maxfd / BITS_PER_LONG; > We keep declarations at the beginning of the block. Usually it keeps the > code more readable and the compiler should be clever enough to perform the > loads & arithmetics only when needed. > > After fixing these style nits feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > > Honza Yes, I'll polish this part of code accordingly, thanks for all the comments here :)
diff --git a/fs/file.c b/fs/file.c index 1be2a5bcc7c4..a3ce6ba30c8c 100644 --- a/fs/file.c +++ b/fs/file.c @@ -488,9 +488,20 @@ struct files_struct init_files = { static unsigned int find_next_fd(struct fdtable *fdt, unsigned int start) { + unsigned int bitbit = start / BITS_PER_LONG; + unsigned int bit; + + /* + * Try to avoid looking at the second level bitmap + */ + bit = find_next_zero_bit(&fdt->open_fds[bitbit], BITS_PER_LONG, + start & (BITS_PER_LONG -1)); + if (bit < BITS_PER_LONG) { + return bit + bitbit * BITS_PER_LONG; + } + unsigned int maxfd = fdt->max_fds; /* always multiple of BITS_PER_LONG */ unsigned int maxbit = maxfd / BITS_PER_LONG; - unsigned int bitbit = start / BITS_PER_LONG; bitbit = find_next_zero_bit(fdt->full_fds_bits, maxbit, bitbit) * BITS_PER_LONG; if (bitbit >= maxfd)