| Message ID | 20250304183506.498724-2-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | avoid the extra atomic on a ref when closing a fd | expand |
On Tue, Mar 4, 2025 at 7:35 PM Mateusz Guzik <mjguzik@gmail.com> wrote: > +void fput_close_sync(struct file *file) > +{ > + if (unlikely(file_ref_put_close(&file->f_ref))) > + __fput(file); > +} this of course should be: *likely*
On Tue, Mar 04, 2025 at 07:35:03PM +0100, Mateusz Guzik wrote: > Vast majority of the time closing a file descriptor also operates on the > last reference, where a regular fput usage will result in 2 atomics. > This can be changed to only suffer 1. > > See commentary above file_ref_put_close() for more information. > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > --- I'm not enthused about the patches tbh because we end up with two new primivites and I really dislike so many new primitives with slightly different semantics. But it should at least all be kept private to fs/. > fs/file.c | 75 ++++++++++++++++++++++++++++++---------- > fs/file_table.c | 72 +++++++++++++++++++++++++++----------- > include/linux/file.h | 2 ++ > include/linux/file_ref.h | 1 + > 4 files changed, 111 insertions(+), 39 deletions(-) > > diff --git a/fs/file.c b/fs/file.c > index 44efdc8c1e27..ea753f9c8e08 100644 > --- a/fs/file.c > +++ b/fs/file.c > @@ -26,6 +26,28 @@ > > #include "internal.h" > > +static bool __file_ref_put_badval(file_ref_t *ref, unsigned long cnt) > +{ > + /* > + * If the reference count was already in the dead zone, then this > + * put() operation is imbalanced. Warn, put the reference count back to > + * DEAD and tell the caller to not deconstruct the object. > + */ > + if (WARN_ONCE(cnt >= FILE_REF_RELEASED, "imbalanced put on file reference count")) { > + atomic_long_set(&ref->refcnt, FILE_REF_DEAD); > + return false; > + } > + > + /* > + * This is a put() operation on a saturated refcount. Restore the > + * mean saturation value and tell the caller to not deconstruct the > + * object. > + */ > + if (cnt > FILE_REF_MAXREF) > + atomic_long_set(&ref->refcnt, FILE_REF_SATURATED); > + return false; > +} > + > /** > * __file_ref_put - Slowpath of file_ref_put() > * @ref: Pointer to the reference count > @@ -67,27 +89,44 @@ bool __file_ref_put(file_ref_t *ref, unsigned long cnt) > return true; > } > > - /* > - * If the reference count was already in the dead zone, then this > - * put() operation is imbalanced. Warn, put the reference count back to > - * DEAD and tell the caller to not deconstruct the object. > - */ > - if (WARN_ONCE(cnt >= FILE_REF_RELEASED, "imbalanced put on file reference count")) { > - atomic_long_set(&ref->refcnt, FILE_REF_DEAD); > - return false; > - } > - > - /* > - * This is a put() operation on a saturated refcount. Restore the > - * mean saturation value and tell the caller to not deconstruct the > - * object. > - */ > - if (cnt > FILE_REF_MAXREF) > - atomic_long_set(&ref->refcnt, FILE_REF_SATURATED); > - return false; > + return __file_ref_put_badval(ref, cnt); > } > EXPORT_SYMBOL_GPL(__file_ref_put); > > +/** > + * file_ref_put_close - drop a reference expecting it would transition to FILE_REF_NOREF > + * @ref: Pointer to the reference count > + * > + * Semantically it is equivalent to calling file_ref_put(), but it trades lower > + * performance in face of other CPUs also modifying the refcount for higher > + * performance when this happens to be the last reference. > + * > + * For the last reference file_ref_put() issues 2 atomics. One to drop the > + * reference and another to transition it to FILE_REF_DEAD. This routine does > + * the work in one step, but in order to do it has to pre-read the variable which > + * decreases scalability. > + * > + * Use with close() et al, stick to file_ref_put() by default. > + */ > +bool file_ref_put_close(file_ref_t *ref) > +{ > + long old, new; > + > + old = atomic_long_read(&ref->refcnt); > + do { > + if (unlikely(old < 0)) > + return __file_ref_put_badval(ref, old); > + > + if (old == FILE_REF_ONEREF) > + new = FILE_REF_DEAD; > + else > + new = old - 1; > + } while (!atomic_long_try_cmpxchg(&ref->refcnt, &old, new)); > + > + return new == FILE_REF_DEAD; > +} > +EXPORT_SYMBOL_GPL(file_ref_put_close); > + > unsigned int sysctl_nr_open __read_mostly = 1024*1024; > unsigned int sysctl_nr_open_min = BITS_PER_LONG; > /* our min() is unusable in constant expressions ;-/ */ > diff --git a/fs/file_table.c b/fs/file_table.c > index 5c00dc38558d..4189c682eb06 100644 > --- a/fs/file_table.c > +++ b/fs/file_table.c > @@ -511,31 +511,37 @@ void flush_delayed_fput(void) > } > EXPORT_SYMBOL_GPL(flush_delayed_fput); > > -void fput(struct file *file) > +static void __fput_defer_free(struct file *file) Imho just call it __fput_deferred(). > { > - if (file_ref_put(&file->f_ref)) { > - struct task_struct *task = current; > + struct task_struct *task = current; > > - if (unlikely(!(file->f_mode & (FMODE_BACKING | FMODE_OPENED)))) { > - file_free(file); > + if (unlikely(!(file->f_mode & (FMODE_BACKING | FMODE_OPENED)))) { > + file_free(file); > + return; > + } > + if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) { > + init_task_work(&file->f_task_work, ____fput); > + if (!task_work_add(task, &file->f_task_work, TWA_RESUME)) > return; > - } > - if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) { > - init_task_work(&file->f_task_work, ____fput); > - if (!task_work_add(task, &file->f_task_work, TWA_RESUME)) > - return; > - /* > - * After this task has run exit_task_work(), > - * task_work_add() will fail. Fall through to delayed > - * fput to avoid leaking *file. > - */ > - } > - > - if (llist_add(&file->f_llist, &delayed_fput_list)) > - schedule_delayed_work(&delayed_fput_work, 1); > + /* > + * After this task has run exit_task_work(), > + * task_work_add() will fail. Fall through to delayed > + * fput to avoid leaking *file. > + */ > } > + > + if (llist_add(&file->f_llist, &delayed_fput_list)) > + schedule_delayed_work(&delayed_fput_work, 1); > } > > +void fput(struct file *file) > +{ > + if (unlikely(file_ref_put(&file->f_ref))) { > + __fput_defer_free(file); > + } > +} > +EXPORT_SYMBOL(fput); > + > /* > * synchronous analog of fput(); for kernel threads that might be needed > * in some umount() (and thus can't use flush_delayed_fput() without > @@ -549,10 +555,34 @@ void __fput_sync(struct file *file) > if (file_ref_put(&file->f_ref)) > __fput(file); > } > - > -EXPORT_SYMBOL(fput); > EXPORT_SYMBOL(__fput_sync); > > +/* > + * Equivalent to __fput_sync(), but optimized for being called with the last > + * reference. > + * > + * See file_ref_put_close() for details. > + */ > +void fput_close_sync(struct file *file) > +{ > + if (unlikely(file_ref_put_close(&file->f_ref))) > + __fput(file); > +} > +EXPORT_SYMBOL(fput_close_sync); Shouldn't be exported to modules, please. > + > +/* > + * Equivalent to fput(), but optimized for being called with the last > + * reference. > + * > + * See file_ref_put_close() for details. > + */ > +void fput_close(struct file *file) > +{ > + if (file_ref_put_close(&file->f_ref)) > + __fput_defer_free(file); > +} > +EXPORT_SYMBOL(fput_close); Shouldn't be exported to modules, please. > + > void __init files_init(void) > { > struct kmem_cache_args args = { > diff --git a/include/linux/file.h b/include/linux/file.h > index 302f11355b10..7b04e87cbde6 100644 > --- a/include/linux/file.h > +++ b/include/linux/file.h > @@ -124,6 +124,8 @@ int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags); > > extern void flush_delayed_fput(void); > extern void __fput_sync(struct file *); > +void fput_close_sync(struct file *); > +void fput_close(struct file *); Should go into internal.h. > > extern unsigned int sysctl_nr_open_min, sysctl_nr_open_max; > > diff --git a/include/linux/file_ref.h b/include/linux/file_ref.h > index 9b3a8d9b17ab..f269299941aa 100644 > --- a/include/linux/file_ref.h > +++ b/include/linux/file_ref.h > @@ -62,6 +62,7 @@ static inline void file_ref_init(file_ref_t *ref, unsigned long cnt) > } > > bool __file_ref_put(file_ref_t *ref, unsigned long cnt); > +bool file_ref_put_close(file_ref_t *ref); > > /** > * file_ref_get - Acquire one reference on a file > -- > 2.43.0 >
On Wed, Mar 5, 2025 at 11:47 AM Christian Brauner <brauner@kernel.org> wrote: > > On Tue, Mar 04, 2025 at 07:35:03PM +0100, Mateusz Guzik wrote: > > Vast majority of the time closing a file descriptor also operates on the > > last reference, where a regular fput usage will result in 2 atomics. > > This can be changed to only suffer 1. > > > > See commentary above file_ref_put_close() for more information. > > > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > > --- > > I'm not enthused about the patches tbh because we end up with two new > primivites and I really dislike so many new primitives with slightly > different semantics. But it should at least all be kept private to fs/. > Caller-observable behavior is the same, so I don't think this is a big deal. The intent was to keep this very much for vfs-internal usage. The EXPORT thing was copy-pasto. Anyhow, everyone interested can still just fput so this does not put any burden on other people afaics. The few patched spots are very much vfs-internal. I sent v3 with the feedback addressed.
diff --git a/fs/file.c b/fs/file.c index 44efdc8c1e27..ea753f9c8e08 100644 --- a/fs/file.c +++ b/fs/file.c @@ -26,6 +26,28 @@ #include "internal.h" +static bool __file_ref_put_badval(file_ref_t *ref, unsigned long cnt) +{ + /* + * If the reference count was already in the dead zone, then this + * put() operation is imbalanced. Warn, put the reference count back to + * DEAD and tell the caller to not deconstruct the object. + */ + if (WARN_ONCE(cnt >= FILE_REF_RELEASED, "imbalanced put on file reference count")) { + atomic_long_set(&ref->refcnt, FILE_REF_DEAD); + return false; + } + + /* + * This is a put() operation on a saturated refcount. Restore the + * mean saturation value and tell the caller to not deconstruct the + * object. + */ + if (cnt > FILE_REF_MAXREF) + atomic_long_set(&ref->refcnt, FILE_REF_SATURATED); + return false; +} + /** * __file_ref_put - Slowpath of file_ref_put() * @ref: Pointer to the reference count @@ -67,27 +89,44 @@ bool __file_ref_put(file_ref_t *ref, unsigned long cnt) return true; } - /* - * If the reference count was already in the dead zone, then this - * put() operation is imbalanced. Warn, put the reference count back to - * DEAD and tell the caller to not deconstruct the object. - */ - if (WARN_ONCE(cnt >= FILE_REF_RELEASED, "imbalanced put on file reference count")) { - atomic_long_set(&ref->refcnt, FILE_REF_DEAD); - return false; - } - - /* - * This is a put() operation on a saturated refcount. Restore the - * mean saturation value and tell the caller to not deconstruct the - * object. - */ - if (cnt > FILE_REF_MAXREF) - atomic_long_set(&ref->refcnt, FILE_REF_SATURATED); - return false; + return __file_ref_put_badval(ref, cnt); } EXPORT_SYMBOL_GPL(__file_ref_put); +/** + * file_ref_put_close - drop a reference expecting it would transition to FILE_REF_NOREF + * @ref: Pointer to the reference count + * + * Semantically it is equivalent to calling file_ref_put(), but it trades lower + * performance in face of other CPUs also modifying the refcount for higher + * performance when this happens to be the last reference. + * + * For the last reference file_ref_put() issues 2 atomics. One to drop the + * reference and another to transition it to FILE_REF_DEAD. This routine does + * the work in one step, but in order to do it has to pre-read the variable which + * decreases scalability. + * + * Use with close() et al, stick to file_ref_put() by default. + */ +bool file_ref_put_close(file_ref_t *ref) +{ + long old, new; + + old = atomic_long_read(&ref->refcnt); + do { + if (unlikely(old < 0)) + return __file_ref_put_badval(ref, old); + + if (old == FILE_REF_ONEREF) + new = FILE_REF_DEAD; + else + new = old - 1; + } while (!atomic_long_try_cmpxchg(&ref->refcnt, &old, new)); + + return new == FILE_REF_DEAD; +} +EXPORT_SYMBOL_GPL(file_ref_put_close); + unsigned int sysctl_nr_open __read_mostly = 1024*1024; unsigned int sysctl_nr_open_min = BITS_PER_LONG; /* our min() is unusable in constant expressions ;-/ */ diff --git a/fs/file_table.c b/fs/file_table.c index 5c00dc38558d..4189c682eb06 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -511,31 +511,37 @@ void flush_delayed_fput(void) } EXPORT_SYMBOL_GPL(flush_delayed_fput); -void fput(struct file *file) +static void __fput_defer_free(struct file *file) { - if (file_ref_put(&file->f_ref)) { - struct task_struct *task = current; + struct task_struct *task = current; - if (unlikely(!(file->f_mode & (FMODE_BACKING | FMODE_OPENED)))) { - file_free(file); + if (unlikely(!(file->f_mode & (FMODE_BACKING | FMODE_OPENED)))) { + file_free(file); + return; + } + if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) { + init_task_work(&file->f_task_work, ____fput); + if (!task_work_add(task, &file->f_task_work, TWA_RESUME)) return; - } - if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) { - init_task_work(&file->f_task_work, ____fput); - if (!task_work_add(task, &file->f_task_work, TWA_RESUME)) - return; - /* - * After this task has run exit_task_work(), - * task_work_add() will fail. Fall through to delayed - * fput to avoid leaking *file. - */ - } - - if (llist_add(&file->f_llist, &delayed_fput_list)) - schedule_delayed_work(&delayed_fput_work, 1); + /* + * After this task has run exit_task_work(), + * task_work_add() will fail. Fall through to delayed + * fput to avoid leaking *file. + */ } + + if (llist_add(&file->f_llist, &delayed_fput_list)) + schedule_delayed_work(&delayed_fput_work, 1); } +void fput(struct file *file) +{ + if (unlikely(file_ref_put(&file->f_ref))) { + __fput_defer_free(file); + } +} +EXPORT_SYMBOL(fput); + /* * synchronous analog of fput(); for kernel threads that might be needed * in some umount() (and thus can't use flush_delayed_fput() without @@ -549,10 +555,34 @@ void __fput_sync(struct file *file) if (file_ref_put(&file->f_ref)) __fput(file); } - -EXPORT_SYMBOL(fput); EXPORT_SYMBOL(__fput_sync); +/* + * Equivalent to __fput_sync(), but optimized for being called with the last + * reference. + * + * See file_ref_put_close() for details. + */ +void fput_close_sync(struct file *file) +{ + if (unlikely(file_ref_put_close(&file->f_ref))) + __fput(file); +} +EXPORT_SYMBOL(fput_close_sync); + +/* + * Equivalent to fput(), but optimized for being called with the last + * reference. + * + * See file_ref_put_close() for details. + */ +void fput_close(struct file *file) +{ + if (file_ref_put_close(&file->f_ref)) + __fput_defer_free(file); +} +EXPORT_SYMBOL(fput_close); + void __init files_init(void) { struct kmem_cache_args args = { diff --git a/include/linux/file.h b/include/linux/file.h index 302f11355b10..7b04e87cbde6 100644 --- a/include/linux/file.h +++ b/include/linux/file.h @@ -124,6 +124,8 @@ int receive_fd_replace(int new_fd, struct file *file, unsigned int o_flags); extern void flush_delayed_fput(void); extern void __fput_sync(struct file *); +void fput_close_sync(struct file *); +void fput_close(struct file *); extern unsigned int sysctl_nr_open_min, sysctl_nr_open_max; diff --git a/include/linux/file_ref.h b/include/linux/file_ref.h index 9b3a8d9b17ab..f269299941aa 100644 --- a/include/linux/file_ref.h +++ b/include/linux/file_ref.h @@ -62,6 +62,7 @@ static inline void file_ref_init(file_ref_t *ref, unsigned long cnt) } bool __file_ref_put(file_ref_t *ref, unsigned long cnt); +bool file_ref_put_close(file_ref_t *ref); /** * file_ref_get - Acquire one reference on a file
Vast majority of the time closing a file descriptor also operates on the last reference, where a regular fput usage will result in 2 atomics. This can be changed to only suffer 1. See commentary above file_ref_put_close() for more information. Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> --- fs/file.c | 75 ++++++++++++++++++++++++++++++---------- fs/file_table.c | 72 +++++++++++++++++++++++++++----------- include/linux/file.h | 2 ++ include/linux/file_ref.h | 1 + 4 files changed, 111 insertions(+), 39 deletions(-)