Re: [PATCH] fs: place f_ref to 3rd cache line in struct file to resolve false sharing

[Christian Brauner <brauner@kernel.org>]

On Fri, Feb 28, 2025 at 08:51:27PM +0100, Jan Kara wrote:
> On Fri 28-02-25 10:00:59, Pan Deng wrote:
> > When running syscall pread in a high core count system, f_ref contends
> > with the reading of f_mode, f_op, f_mapping, f_inode, f_flags in the
> > same cache line.
> 
> Well, but you have to have mulithreaded process using the same struct file
> for the IO, don't you? Otherwise f_ref is not touched...

Yes, it's specifically designed to scale better under high contention.

> 
> > This change places f_ref to the 3rd cache line where fields are not
> > updated as frequently as the 1st cache line, and the contention is
> > grealy reduced according to tests. In addition, the size of file
> > object is kept in 3 cache lines.
> > 
> > This change has been tested with rocksdb benchmark readwhilewriting case
> > in 1 socket 64 physical core 128 logical core baremetal machine, with
> > build config CONFIG_RANDSTRUCT_NONE=y
> > Command:
> > ./db_bench --benchmarks="readwhilewriting" --threads $cnt --duration 60
> > The throughput(ops/s) is improved up to ~21%.
> > =====
> > thread		baseline	compare
> > 16		 100%		 +1.3%
> > 32		 100%		 +2.2%
> > 64		 100%		 +7.2%
> > 128		 100%		 +20.9%
> > 
> > It was also tested with UnixBench: syscall, fsbuffer, fstime,
> > fsdisk cases that has been used for file struct layout tuning, no
> > regression was observed.
> 
> So overall keeping the first cacheline read mostly with important stuff
> makes sense to limit cache traffic. But:
> 
> >  struct file {
> > -	file_ref_t			f_ref;
> >  	spinlock_t			f_lock;
> >  	fmode_t				f_mode;
> >  	const struct file_operations	*f_op;
> > @@ -1102,6 +1101,7 @@ struct file {
> >  	unsigned int			f_flags;
> >  	unsigned int			f_iocb_flags;
> >  	const struct cred		*f_cred;
> > +	u8				padding[8];
> >  	/* --- cacheline 1 boundary (64 bytes) --- */
> >  	struct path			f_path;
> >  	union {
> > @@ -1127,6 +1127,7 @@ struct file {
> >  		struct file_ra_state	f_ra;
> >  		freeptr_t		f_freeptr;
> >  	};
> > +	file_ref_t			f_ref;
> >  	/* --- cacheline 3 boundary (192 bytes) --- */
> >  } __randomize_layout
> 
> This keeps struct file within 3 cachelines but it actually grows it from
> 184 to 192 bytes (and yes, that changes how many file structs we can fit in
> a slab). So instead of adding 8 bytes of padding, just pick some
> read-mostly element and move it into the hole - f_owner looks like one
> possible candidate.

This is what I did. See vfs-6.15.misc! Thanks!

> 
> Also did you test how moving f_ref to the second cache line instead of the
> third one behaves?
> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR