On 6/17/2024 7:23 PM, Mateusz Guzik wrote:
On Sun, Jun 16, 2024 at 11:47:40AM +0800, Ma, Yu wrote:
On 6/15/2024 12:41 PM, Mateusz Guzik wrote:
So you are moving this to another locked area, but one which does not
execute in the benchmark?
The consideration here as mentioned is to reduce the performance impact (if
to reserve the sanity check, and have the same functionality) by moving it
from critical path to non-critical, as put_unused_fd() is mostly used for
error handling when fd is allocated successfully but struct file failed to
obtained in the next step.
As mentioned by Christian in his mail this check can just be removed.
Yes, that's great, I'll update in v2
error = -EMFILE;
if (fd < fdt->max_fds) {
I would likely() on it.
That's better :)
if (~fdt->open_fds[0]) {
fd = find_next_zero_bit(fdt->open_fds,
BITS_PER_LONG, fd);
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 (error)
goto repeat;
}
fastreturn:
if (unlikely(fd >= end))
goto out;
if (start <= files->next_fd)
files->next_fd = fd + 1;
....
This is not a review, but it does read fine.
LTP (https://github.com/linux-test-project/ltp.git) has a bunch of fd
tests, I would make sure they still pass before posting a v2.
Got it, thanks for the kind reminder.
I would definitely try moving out the lock to its own cacheline --
currently it resides with the bitmaps (and first 4 fds of the embedded
array).
I expect it to provide some win on top of the current patchset, but
whether it will be sufficient to justify it I have no idea.
Something of this sort:
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 2944d4aa413b..423cb599268a 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -50,11 +50,11 @@ struct files_struct {
* written part on a separate cache line in SMP
*/
spinlock_t file_lock ____cacheline_aligned_in_smp;
- unsigned int next_fd;
+ unsigned int next_fd ____cacheline_aligned_in_smp;
unsigned long close_on_exec_init[1];
unsigned long open_fds_init[1];
unsigned long full_fds_bits_init[1];
- struct file __rcu * fd_array[NR_OPEN_DEFAULT];
+ struct file __rcu * fd_array[NR_OPEN_DEFAULT] ____cacheline_aligned_in_smp;
};
struct file_operations;
(feel free to just take)
Thanks Guzik for the thoughtful suggestions, seems we've ever tried to
separate file_lock and next_fd to different cachelines, but no obvious
improvement observed, we'll double check and verify these 2 tips to see
how it goes.
All that said, I have to note blogbench has numerous bugs. For example
it collects stats by merely bumping a global variable (not even with
atomics), so some updates are straight up lost.
To my understanding it was meant to test filesystems and I'm guessing
threading (instead of separate processes) was only used to make it
easier to share statistics. Given the current scales this
unintentionally transitioned into bottlenecking on the file_lock
instead.
There were scalability changes made about 9 years ago and according to
git log they were benchmarked by Eric Dumazet, while benchmark code was
not shared. I suggest CC'ing the guy with your v2 and asking if he can
bench. Even if he is no longer in position to do it perhaps he can rope
someone in (or even better share his benchmark).
Good advice, we also noticed the problem for blogbench, and current data
in commits is collected based on revision to make sure the variables for
score statistic/reporting are safe, will submit patch to blogbench for
update. We'll copy Eric for more information on his benchmark and check
details if it is available.
