Re: [LKP] [fsnotify] 60f7ed8c7c: will-it-scale.per_thread_ops -5.9% regression

[Amir Goldstein]

On Mon, Oct 15, 2018 at 12:27 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Mon, Oct 15, 2018 at 10:50 AM Rong Chen <rong.a.chen@xxxxxxxxx> wrote:
> [...]
> > the patch seems not work.
> >
> > tests: 1
> > testcase/path_params/tbox_group/run: will-it-scale/16-thread-unlink2-performance/lkp-bdw-ep3d
> >
> >  commit:
> >    1e6cb72399 ("fsnotify: add super block object type")
> >    298cd0b2f4 (the below patch)
> >
> > 1e6cb72399fd58b3  298cd0b2f481d9cc2e2cd5bfd3
> > ----------------  --------------------------
> >          %stddev      change         %stddev
> >              \          |                \
> >     103.21              -5%      98.54        will-it-scale.time.user_time
> >      46266              -6%      43516        will-it-scale.time.involuntary_context_switches
> >      54483              -7%      50610        will-it-scale.per_thread_ops
> >     871749              -7%     809765        will-it-scale.workload
>
> Thanks for testing my patch. As Jan commented, it is not surprising
> that the patch
> makes no difference.
>
> I would like to clarify a few things about how you ran the test before
> I continue to
> investigate:
>
> 1. When I ran the workload I saw that it writes files to whatever filesystem is
>     mounted on /tmp. Can I assume you have tmpfs mounted at /tmp?
>
> 2. Can you confirm that there is no fanotify mount mark on the /tmp mount?
>     for example:
> # ls -l /proc/*/fd/*|grep fanotify
> lrwx------ 1 root root 64 Oct 15 08:36 /proc/3927/fd/3 -> anon_inode:[fanotify]
> # grep fanotify.mnt_id /proc/3927/fdinfo/3
> fanotify mnt_id:33 mflags:0 mask:3b ignored_mask:0
> # grep ^$(( 0x33 )) /proc/3927/mountinfo
> 51 16 0:27 / /tmp rw,relatime shared:18 - tmpfs tmpfs rw
>
> 3. I saw that LKP caches the results for a specific commit
>     (i.e. 1e6cb72399 ("fsnotify: add super block object type")).
> Did you use cached results when comparing to patch or did you re-run the
> test with the "good" commit? The reason I am asking is because
> sometimes performance result may differ between boots even with no
> kernel code change.
> Where all the "good" bisect samples taken from the same boot/machine?
> or different boots/machines?
>
> 4. If this regression is reliably reproduced, then our best bet is on the
> cost of access to s_fsnotify_{marks,mask} fields.
> The patch below moves those frequently accessed fields near the
> frequently accessed fields s_time_gran,s_writers and moves
> the seldom accessed fields s_id,s_uuid further away.
> Could you please try this patch?
>

Better test this patch instead. It does a bit more re-organizing.
If this works well for 16-thread-unlink2 workload, could you please
also run it through other workloads to see if it improves them as well?
and does not degrade them...

Thanks,
Amir.

---
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 25a449f37bb1..baec0b3ff53f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1393,17 +1393,24 @@ struct super_block {

        struct sb_writers       s_writers;

+       /* START frequently accessed fields block */
+       void                    *s_fs_info;     /* Filesystem private info */
+
+       /* Granularity of c/m/atime in ns (cannot be worse than a second) */
+       u32                     s_time_gran;
+#ifdef CONFIG_FSNOTIFY
+       __u32                   s_fsnotify_mask;
+       struct fsnotify_mark_connector __rcu    *s_fsnotify_marks;
+#endif
+       /* END frequently accessed fields block */
+
+       /* START seldom accessed fields block */
        char                    s_id[32];       /* Informational name */
        uuid_t                  s_uuid;         /* UUID */

-       void                    *s_fs_info;     /* Filesystem private info */
        unsigned int            s_max_links;
        fmode_t                 s_mode;

-       /* Granularity of c/m/atime in ns.
-          Cannot be worse than a second */
-       u32                s_time_gran;
-
        /*
         * The next field is for VFS *only*. No filesystems have any business
         * even looking at it. You had been warned.
@@ -1415,6 +1422,7 @@ struct super_block {
         * in /proc/mounts will be "type.subtype"
         */
        char *s_subtype;
+       /* END seldom accessed fields block */

        const struct dentry_operations *s_d_op; /* default d_op for dentries */

@@ -1464,11 +1472,6 @@ struct super_block {

        spinlock_t              s_inode_wblist_lock;
        struct list_head        s_inodes_wb;    /* writeback inodes */
-
-#ifdef CONFIG_FSNOTIFY
-       __u32                   s_fsnotify_mask;
-       struct fsnotify_mark_connector __rcu    *s_fsnotify_marks;
-#endif
 } __randomize_layout;