Re: [RFC PATCH v v2 0/4] avoid the extra atomic on a ref when closing a fd

[Christian Brauner <brauner@kernel.org>]

On Tue, Mar 04, 2025 at 07:35:02PM +0100, Mateusz Guzik wrote:
> The stock kernel transitioning the file to no refs held penalizes the
> caller with an extra atomic to block any increments.
> 
> For cases where the file is highly likely to be going away this is
> easily avoidable.
> 
> In the open+close case the win is very modest because of the following
> problems:
> - kmem and memcg having terrible performance

I thought that was going to be addressed by Vlastimil, i.e., the mm guys
to provide a new memcg api.

> - putname using an atomic (I have a wip to whack that)
> - open performing an extra ref/unref on the dentry (there are patches to
>   do it, including by Al. I mailed about them in [1])
> - creds using atomics (I have a wip to whack that)
> - apparmor using atomics (ditto, same mechanism)
> 
> On top of that I have a WIP patch to dodge some of the work at lookup
> itself.
> 
> All in all there is several % avoidably lost here.
> 
> stats colected during a kernel build with:
> bpftrace -e 'kprobe:filp_close,kprobe:fput,kprobe:fput_close* { @[probe] = hist(((struct file *)arg0)->f_ref.refcnt.counter > 0); }'
> 
> @[kprobe:filp_close]:
> [0]                32195 |@@@@@@@@@@                                          |
> [1]               164567 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> 
> @[kprobe:fput]:
> [0]               339240 |@@@@@@                                              |
> [1]              2888064 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> 
> @[kprobe:fput_close]:
> [0]              5116767 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> [1]               164544 |@                                                   |
> 
> @[kprobe:fput_close_sync]:
> [0]              5340660 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> [1]               358943 |@@@                                                 |
> 
> 
> 0 indicates the last reference, 1 that there is more.
> 
> filp_close is largely skewed because of close_on_exec.
> 
> vast majority of last fputs are from remove_vma. I think that code wants
> to be patched to batch them (as in something like fput_many should be
> added -- something for later).

We used to have that for io_uring and got rid of it. The less fput()
primitives the better tbh. But let's see.

> 
> [1] https://lore.kernel.org/linux-fsdevel/20250304165728.491785-1-mjguzik@gmail.com/T/#u
> 
> v2:
> - patch filp_close
> - patch failing open
> 
> Mateusz Guzik (4):
>   file: add fput and file_ref_put routines optimized for use when
>     closing a fd
>   fs: use fput_close_sync() in close()
>   fs: use fput_close() in filp_close()
>   fs: use fput_close() in path_openat()
> 
>  fs/file.c                | 75 ++++++++++++++++++++++++++++++----------
>  fs/file_table.c          | 72 +++++++++++++++++++++++++++-----------
>  fs/namei.c               |  2 +-
>  fs/open.c                |  4 +--
>  include/linux/file.h     |  2 ++
>  include/linux/file_ref.h |  1 +
>  6 files changed, 114 insertions(+), 42 deletions(-)
> 
> -- 
> 2.43.0
>