On Thu 11-04-24 19:22:29, Amir Goldstein wrote: > On Thu, Apr 11, 2024 at 2:54 PM Jan Kara <jack@xxxxxxx> wrote: > > > > On Thu 11-04-24 12:23:34, Amir Goldstein wrote: > > > On Thu, Apr 11, 2024 at 4:42 AM kernel test robot <oliver.sang@xxxxxxxxx> wrote: > > > > for "[amir73il:fsnotify-sbconn] [fsnotify] 629f30e073: unixbench.throughput 5.8% improvement" > > > > (https://lore.kernel.org/all/202403141505.807a722b-oliver.sang@xxxxxxxxx/) > > > > you requested us to test unixbench for this commit on different branches and we > > > > observed consistent performance improvement. > > > > > > > > now we noticed this commit is merged into linux-next/master, we still > > > > observed similar unixbench improvement, however, we also captured a > > > > stress-ng regression now. below details FYI. > > > > > > > > Hello, > > > > > > > > kernel test robot noticed a -17.3% regression of stress-ng.full.ops_per_sec on: > > > > > > > > > > > > commit: a5e57b4d370c6d320e5bfb0c919fe00aee29e039 ("fsnotify: optimize the case of no permission event watchers") > > > > > > Odd. This commit does add an extra fsnotify_sb_has_priority_watchers() > > > inline check for reads and writes, but the inline helper > > > fsnotify_sb_has_watchers() > > > already exists in fsnotify_parent() and it already accesses fsnotify_sb_info. > > > > > > It seems like stress-ng.full does read/write/mmap operations on /dev/full, > > > so the fsnotify_sb_info object would be that of devtmpfs. > > > > > > I think that the permission events on special files are not very relevant, > > > but I am not sure. > > > > > > Jan, any ideas? > > > > So I'm not 100% sure but this load simply seems to run 'stress-ng' with all > > the syscalls it is able to exercise (one per CPU if I'm right). Hum... > > looking at perf numbers I've noticed changes like: > > > > 0.43 ą 3% -0.2 0.21 ą 5% perf-profile.self.cycles-pp.__fsnotify_parent > > 0.00 +2.8 2.79 ą 5% perf-profile.self.cycles-pp.fsnotify_open_perm > > > > or > > > > 1.77 ą 12% +1.9 3.64 ą 8% perf-profile.calltrace.cycles-pp.rw_verify_area.vfs_read.__x64_sys_pread64.do_syscall_64.entry_SYSCALL_64_after_hwframe > > 1.71 ą 15% +1.9 3.64 ą 9% perf-profile.calltrace.cycles-pp.rw_verify_area.vfs_read.ksys_read.do_syscall_64.entry_SYSCALL_64_after_hwframe > > 0.00 +2.8 2.79 ą 5% perf-profile.calltrace.cycles-pp.fsnotify_open_perm.do_dentry_open.do_open.path_openat.do_filp_open > > > > So the savings in __fsnotify_parent() don't really outweight the costs in > > fsnotify_file()... I can see stress-ng exercises also inotify so maybe > > there's some contention on the counters which is causing the regression now > > that we have more of them? > > > > BTW, I'm not sure how you've arrived at the conclusing the test is using > > /dev/full. For all I can tell the e.g. the stress-mmap test is using a file > > in a subdir of CWD. > > > > Oh, I just saw the file stress-full.c in stress-ng and wrongly assumed that > test stress-ng.full refers to this code. > > Where do I find the code for this test? Ah, now that I've investigated the LKP details again, you're indeed right. repro-script shows how stress-ng is run and when I do that with cloned stress-ng repository, it is the test using /dev/full. So with that I'm not sure why patch adds so much cost to fsnotify_file()... Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
