| Message ID | 20250319215801.1870660-1-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] fs: reduce work in fdget_pos() | expand |
On Wed, 19 Mar 2025 22:58:01 +0100, Mateusz Guzik wrote: > 1. predict the file was found > 2. explicitly compare the ref to "one", ignoring the dead zone > > The latter arguably improves the behavior to begin with. Suppose the > count turned bad -- the previously used ref routine is going to check > for it and return 0, indicating the count does not necessitate taking > ->f_pos_lock. But there very well may be several users. > > [...] Applied to the vfs-6.15.file branch of the vfs/vfs.git tree. Patches in the vfs-6.15.file branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs-6.15.file [1/1] fs: reduce work in fdget_pos() https://git.kernel.org/vfs/vfs/c/5370b43e4bcf
On Wed 19-03-25 22:58:01, Mateusz Guzik wrote: > 1. predict the file was found > 2. explicitly compare the ref to "one", ignoring the dead zone > > The latter arguably improves the behavior to begin with. Suppose the > count turned bad -- the previously used ref routine is going to check > for it and return 0, indicating the count does not necessitate taking > ->f_pos_lock. But there very well may be several users. > > i.e. not paying for special-casing the dead zone improves semantics. > > While here spell out each condition in a dedicated if statement. This > has no effect on generated code. > > Sizes are as follows (in bytes; gcc 13, x86-64): > stock: 321 > likely(): 298 > likely()+ref: 280 > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > diff --git a/fs/file.c b/fs/file.c > index ddefb5c80398..0e919bed6058 100644 > --- a/fs/file.c > +++ b/fs/file.c > @@ -1192,8 +1192,13 @@ struct fd fdget_raw(unsigned int fd) > */ > static inline bool file_needs_f_pos_lock(struct file *file) > { > - return (file->f_mode & FMODE_ATOMIC_POS) && > - (file_count(file) > 1 || file->f_op->iterate_shared); > + if (!(file->f_mode & FMODE_ATOMIC_POS)) > + return false; > + if (__file_ref_read_raw(&file->f_ref) != FILE_REF_ONEREF) > + return true; > + if (file->f_op->iterate_shared) > + return true; > + return false; > } > > bool file_seek_cur_needs_f_lock(struct file *file) > @@ -1211,7 +1216,7 @@ struct fd fdget_pos(unsigned int fd) > struct fd f = fdget(fd); > struct file *file = fd_file(f); > > - if (file && file_needs_f_pos_lock(file)) { > + if (likely(file) && file_needs_f_pos_lock(file)) { > f.word |= FDPUT_POS_UNLOCK; > mutex_lock(&file->f_pos_lock); > } > diff --git a/include/linux/file_ref.h b/include/linux/file_ref.h > index 6ef92d765a66..7db62fbc0500 100644 > --- a/include/linux/file_ref.h > +++ b/include/linux/file_ref.h > @@ -208,4 +208,18 @@ static inline unsigned long file_ref_read(file_ref_t *ref) > return c >= FILE_REF_RELEASED ? 0 : c + 1; > } > > +/* > + * __file_ref_read_raw - Return the value stored in ref->refcnt > + * @ref: Pointer to the reference count > + * > + * Return: The raw value found in the counter > + * > + * A hack for file_needs_f_pos_lock(), you probably want to use > + * file_ref_read() instead. > + */ > +static inline unsigned long __file_ref_read_raw(file_ref_t *ref) > +{ > + return atomic_long_read(&ref->refcnt); > +} > + > #endif > -- > 2.43.0 >
diff --git a/fs/file.c b/fs/file.c index ddefb5c80398..0e919bed6058 100644 --- a/fs/file.c +++ b/fs/file.c @@ -1192,8 +1192,13 @@ struct fd fdget_raw(unsigned int fd) */ static inline bool file_needs_f_pos_lock(struct file *file) { - return (file->f_mode & FMODE_ATOMIC_POS) && - (file_count(file) > 1 || file->f_op->iterate_shared); + if (!(file->f_mode & FMODE_ATOMIC_POS)) + return false; + if (__file_ref_read_raw(&file->f_ref) != FILE_REF_ONEREF) + return true; + if (file->f_op->iterate_shared) + return true; + return false; } bool file_seek_cur_needs_f_lock(struct file *file) @@ -1211,7 +1216,7 @@ struct fd fdget_pos(unsigned int fd) struct fd f = fdget(fd); struct file *file = fd_file(f); - if (file && file_needs_f_pos_lock(file)) { + if (likely(file) && file_needs_f_pos_lock(file)) { f.word |= FDPUT_POS_UNLOCK; mutex_lock(&file->f_pos_lock); } diff --git a/include/linux/file_ref.h b/include/linux/file_ref.h index 6ef92d765a66..7db62fbc0500 100644 --- a/include/linux/file_ref.h +++ b/include/linux/file_ref.h @@ -208,4 +208,18 @@ static inline unsigned long file_ref_read(file_ref_t *ref) return c >= FILE_REF_RELEASED ? 0 : c + 1; } +/* + * __file_ref_read_raw - Return the value stored in ref->refcnt + * @ref: Pointer to the reference count + * + * Return: The raw value found in the counter + * + * A hack for file_needs_f_pos_lock(), you probably want to use + * file_ref_read() instead. + */ +static inline unsigned long __file_ref_read_raw(file_ref_t *ref) +{ + return atomic_long_read(&ref->refcnt); +} + #endif
1. predict the file was found 2. explicitly compare the ref to "one", ignoring the dead zone The latter arguably improves the behavior to begin with. Suppose the count turned bad -- the previously used ref routine is going to check for it and return 0, indicating the count does not necessitate taking ->f_pos_lock. But there very well may be several users. i.e. not paying for special-casing the dead zone improves semantics. While here spell out each condition in a dedicated if statement. This has no effect on generated code. Sizes are as follows (in bytes; gcc 13, x86-64): stock: 321 likely(): 298 likely()+ref: 280 Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> --- v2: - split up conditions in file_needs_f_pos_lock - fix sizes in the commit message, i read the offset of the last instruction fs/file.c | 11 ++++++++--- include/linux/file_ref.h | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-)