[PATCH] fs: group frequently accessed fields of struct super_block together

[PATCH] fs: group frequently accessed fields of struct super_block together

[Amir Goldstein]

Kernel test robot reported [1] a 6% performance regression in a
concurrent unlink(2) workload on commit 60f7ed8c7c4d ("fsnotify: send
path type events to group with super block marks").

The performance test was run with no fsnotify marks at all on the
data set, so the only extra instructions added by the offending
commit are tests of the super_block fields s_fsnotify_{marks,mask}
and these tests happen on almost every single inode access.

When adding those fields to the super_block struct, we did not give much
thought of placing them on a hot cache lines (we just placed them at the
end of the struct).

Re-organize struct super_block to try and keep some frequently accessed
fields on the same cache line.

Move the frequently accessed fields s_fsnotify_{marks,mask} near the
frequently accessed fields s_fs_info,s_time_gran, while filling a 64bit
alignment hole after s_time_gran.

Move the seldom accessed fields s_id,s_uuid,s_max_links,s_mode near the
seldom accessed fields s_vfs_rename_mutex,s_subtype.

Rong Chen confirmed that this patch solved the reported problem.

[1] https://lkml.org/lkml/2018/9/30/206

Reported-by: kernel test robot <rong.a.chen@intel.com>
Tested-by: kernel test robot <rong.a.chen@intel.com>
Fixes: 1e6cb72399 ("fsnotify: add super block object type")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/fs.h | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

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;
 
 /* Helper functions so that in most cases filesystems will
-- 
2.17.1

Re: [PATCH] fs: group frequently accessed fields of struct super_block together

[Jan Kara]

On Thu 18-10-18 14:22:55, Amir Goldstein wrote:
> Kernel test robot reported [1] a 6% performance regression in a
> concurrent unlink(2) workload on commit 60f7ed8c7c4d ("fsnotify: send
> path type events to group with super block marks").
> 
> The performance test was run with no fsnotify marks at all on the
> data set, so the only extra instructions added by the offending
> commit are tests of the super_block fields s_fsnotify_{marks,mask}
> and these tests happen on almost every single inode access.
> 
> When adding those fields to the super_block struct, we did not give much
> thought of placing them on a hot cache lines (we just placed them at the
> end of the struct).
> 
> Re-organize struct super_block to try and keep some frequently accessed
> fields on the same cache line.
> 
> Move the frequently accessed fields s_fsnotify_{marks,mask} near the
> frequently accessed fields s_fs_info,s_time_gran, while filling a 64bit
> alignment hole after s_time_gran.
> 
> Move the seldom accessed fields s_id,s_uuid,s_max_links,s_mode near the
> seldom accessed fields s_vfs_rename_mutex,s_subtype.
> 
> Rong Chen confirmed that this patch solved the reported problem.

...

> +	/* START frequently accessed fields block */

The movement of struct entries is fine. But I don't think the comments
about START / END of sections are really useful there are much more entries
in the struct which are frequently or seldomly accesses and furthemore it's
going to depend on the workload.

Also how are you going to make sure the entries are going to fall into the
same cache line? Different architectures are going to have different
cacheline size and also the offset is going to be different... OTOH
s_writers is usually relatively frequently accessed so probably it doesn't
matter too much.

So maybe just change the comment to something like:

	/*
	 * Keep s_fs_info, s_time_gran, s_fsnotify_mask, and
	 * s_fsnotify_marks together for cache efficiency. They are frequently
	 * accessed and rarely modified.
	 */

> +	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 */

							Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: group frequently accessed fields of struct super_block together

[Amir Goldstein]

On Thu, Oct 18, 2018 at 3:11 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 18-10-18 14:22:55, Amir Goldstein wrote:
> > Kernel test robot reported [1] a 6% performance regression in a
> > concurrent unlink(2) workload on commit 60f7ed8c7c4d ("fsnotify: send
> > path type events to group with super block marks").
> >
> > The performance test was run with no fsnotify marks at all on the
> > data set, so the only extra instructions added by the offending
> > commit are tests of the super_block fields s_fsnotify_{marks,mask}
> > and these tests happen on almost every single inode access.
> >
> > When adding those fields to the super_block struct, we did not give much
> > thought of placing them on a hot cache lines (we just placed them at the
> > end of the struct).
> >
> > Re-organize struct super_block to try and keep some frequently accessed
> > fields on the same cache line.
> >
> > Move the frequently accessed fields s_fsnotify_{marks,mask} near the
> > frequently accessed fields s_fs_info,s_time_gran, while filling a 64bit
> > alignment hole after s_time_gran.
> >
> > Move the seldom accessed fields s_id,s_uuid,s_max_links,s_mode near the
> > seldom accessed fields s_vfs_rename_mutex,s_subtype.
> >
> > Rong Chen confirmed that this patch solved the reported problem.
>
> ...
>
> > +     /* START frequently accessed fields block */
>
> The movement of struct entries is fine. But I don't think the comments
> about START / END of sections are really useful there are much more entries
> in the struct which are frequently or seldomly accesses and furthemore it's
> going to depend on the workload.
>
> Also how are you going to make sure the entries are going to fall into the
> same cache line? Different architectures are going to have different
> cacheline size and also the offset is going to be different... OTOH
> s_writers is usually relatively frequently accessed so probably it doesn't
> matter too much.
>
> So maybe just change the comment to something like:
>
>         /*
>          * Keep s_fs_info, s_time_gran, s_fsnotify_mask, and
>          * s_fsnotify_marks together for cache efficiency. They are frequently
>          * accessed and rarely modified.
>          */
>

Yes, fine by me. I am assuming you are making those changes on apply.

I was going to include s_writers and s_dquot in the "frequently accessed block",
they are also quite frequently accessed, but only on write access patterns, but
those fields are too big to try to optimize cache lines, so I kept
them out of the
comment.

Thanks,
Amir.

Re: [PATCH] fs: group frequently accessed fields of struct super_block together

[Jan Kara]

On Thu 18-10-18 15:48:04, Amir Goldstein wrote:
> On Thu, Oct 18, 2018 at 3:11 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 18-10-18 14:22:55, Amir Goldstein wrote:
> > > Kernel test robot reported [1] a 6% performance regression in a
> > > concurrent unlink(2) workload on commit 60f7ed8c7c4d ("fsnotify: send
> > > path type events to group with super block marks").
> > >
> > > The performance test was run with no fsnotify marks at all on the
> > > data set, so the only extra instructions added by the offending
> > > commit are tests of the super_block fields s_fsnotify_{marks,mask}
> > > and these tests happen on almost every single inode access.
> > >
> > > When adding those fields to the super_block struct, we did not give much
> > > thought of placing them on a hot cache lines (we just placed them at the
> > > end of the struct).
> > >
> > > Re-organize struct super_block to try and keep some frequently accessed
> > > fields on the same cache line.
> > >
> > > Move the frequently accessed fields s_fsnotify_{marks,mask} near the
> > > frequently accessed fields s_fs_info,s_time_gran, while filling a 64bit
> > > alignment hole after s_time_gran.
> > >
> > > Move the seldom accessed fields s_id,s_uuid,s_max_links,s_mode near the
> > > seldom accessed fields s_vfs_rename_mutex,s_subtype.
> > >
> > > Rong Chen confirmed that this patch solved the reported problem.
> >
> > ...
> >
> > > +     /* START frequently accessed fields block */
> >
> > The movement of struct entries is fine. But I don't think the comments
> > about START / END of sections are really useful there are much more entries
> > in the struct which are frequently or seldomly accesses and furthemore it's
> > going to depend on the workload.
> >
> > Also how are you going to make sure the entries are going to fall into the
> > same cache line? Different architectures are going to have different
> > cacheline size and also the offset is going to be different... OTOH
> > s_writers is usually relatively frequently accessed so probably it doesn't
> > matter too much.
> >
> > So maybe just change the comment to something like:
> >
> >         /*
> >          * Keep s_fs_info, s_time_gran, s_fsnotify_mask, and
> >          * s_fsnotify_marks together for cache efficiency. They are frequently
> >          * accessed and rarely modified.
> >          */
> >
> 
> Yes, fine by me. I am assuming you are making those changes on apply.

Yes, I can. I'll push the patch to my tree so that it can go with other
fsnotify changes during the merge window.

> I was going to include s_writers and s_dquot in the "frequently accessed block",
> they are also quite frequently accessed, but only on write access patterns, but
> those fields are too big to try to optimize cache lines, so I kept
> them out of the
> comment.

Yeah, we do have ____cacheline_aligned_in_smp directive when you want to
force cacheline alignment but I don't think it is warranted here.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR