[PATCH] vfs: inline getname()

[PATCH] vfs: inline getname()

[Mateusz Guzik]

It is merely a trivial wrapper around getname_flags which adds a zeroed
argument, no point paying for an extra call.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 fs/namei.c         | 5 -----
 include/linux/fs.h | 5 ++++-
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 3ab9440c5b93..3a4039acdb3f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -218,11 +218,6 @@ struct filename *getname_uflags(const char __user *filename, int uflags)
 	return getname_flags(filename, flags);
 }
 
-struct filename *getname(const char __user * filename)
-{
-	return getname_flags(filename, 0);
-}
-
 struct filename *__getname_maybe_null(const char __user *pathname)
 {
 	struct filename *name;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e73d9b998780..85d88dd5ab6c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2840,7 +2840,10 @@ extern int filp_close(struct file *, fl_owner_t id);
 
 extern struct filename *getname_flags(const char __user *, int);
 extern struct filename *getname_uflags(const char __user *, int);
-extern struct filename *getname(const char __user *);
+static inline struct filename *getname(const char __user *name)
+{
+	return getname_flags(name, 0);
+}
 extern struct filename *getname_kernel(const char *);
 extern struct filename *__getname_maybe_null(const char __user *);
 static inline struct filename *getname_maybe_null(const char __user *name, int flags)
-- 
2.43.0

Re: [PATCH] vfs: inline getname()

[Jan Kara]

On Thu 06-02-25 01:01:05, Mateusz Guzik wrote:
> It is merely a trivial wrapper around getname_flags which adds a zeroed
> argument, no point paying for an extra call.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>

Well, the "extra call" in your changelog is a bit overrated. Such wrappers
get compiled into a constant jump anyway - e.g. in my kernel:

Dump of assembler code for function getname:
   0xffffffff815edb80 <+0>:	endbr64 
   0xffffffff815edb84 <+4>:	call   0xffffffff8131cad0 <__fentry__>
   0xffffffff815edb89 <+9>:	xor    %esi,%esi
   0xffffffff815edb8b <+11>:	jmp    0xffffffff815ed750 <getname_flags>

And the jmp to constant is practically free on current CPUs.

Overall inline function for this is I guess a more common way how we do
things like this so feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/namei.c         | 5 -----
>  include/linux/fs.h | 5 ++++-
>  2 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 3ab9440c5b93..3a4039acdb3f 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -218,11 +218,6 @@ struct filename *getname_uflags(const char __user *filename, int uflags)
>  	return getname_flags(filename, flags);
>  }
>  
> -struct filename *getname(const char __user * filename)
> -{
> -	return getname_flags(filename, 0);
> -}
> -
>  struct filename *__getname_maybe_null(const char __user *pathname)
>  {
>  	struct filename *name;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e73d9b998780..85d88dd5ab6c 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2840,7 +2840,10 @@ extern int filp_close(struct file *, fl_owner_t id);
>  
>  extern struct filename *getname_flags(const char __user *, int);
>  extern struct filename *getname_uflags(const char __user *, int);
> -extern struct filename *getname(const char __user *);
> +static inline struct filename *getname(const char __user *name)
> +{
> +	return getname_flags(name, 0);
> +}
>  extern struct filename *getname_kernel(const char *);
>  extern struct filename *__getname_maybe_null(const char __user *);
>  static inline struct filename *getname_maybe_null(const char __user *name, int flags)
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] vfs: inline getname()

[Christian Brauner]

On Thu, 06 Feb 2025 01:01:05 +0100, Mateusz Guzik wrote:
> It is merely a trivial wrapper around getname_flags which adds a zeroed
> argument, no point paying for an extra call.
> 
> 

Applied to the vfs-6.15.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.15.misc 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.misc

[1/1] vfs: inline getname()
      https://git.kernel.org/vfs/vfs/c/521fbc6e8653

Re: [PATCH] vfs: inline getname()

[Alexey Dobriyan]

[cc lists and people]

> +static inline struct filename *getname(const char __user *name)
> +{
> + return getname_flags(name, 0);
> +}

This may be misguided. The reason is that if function is used often enough
then all those clears of the second argument bloat icache at the call sites.
Uninlining moves all clears in one place, shrinking callers at the cost of
additional function which (in this case) tail calls into another function.
And tailcalling is quite efficient (essentially free):

	getname:
		xor esi, esi
		jmp getname_flags

Re: [PATCH] vfs: inline getname()

[Mateusz Guzik]

On Thu, Feb 6, 2025 at 6:11 PM Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> [cc lists and people]
>
> > +static inline struct filename *getname(const char __user *name)
> > +{
> > + return getname_flags(name, 0);
> > +}
>
> This may be misguided. The reason is that if function is used often enough
> then all those clears of the second argument bloat icache at the call sites.
> Uninlining moves all clears in one place, shrinking callers at the cost of
> additional function which (in this case) tail calls into another function.
> And tailcalling is quite efficient (essentially free):
>
>         getname:
>                 xor esi, esi
>                 jmp getname_flags

Side note is that so happens in this case the compiler had funnier
ideas of pulling out parts of getname_flags into getname itself.

As for the general notion, it is cheaper to xor at the callsite + call
than to call + xor + jmp. Also note the total i-cache footprint absent
sufficiently fewer consumer will also be *lower* without the func.

If the routine was doing anything fancy I would not be proposing the
patch. For something which merely adds a zeroed-out argument I don't
see a legitimate reason to keep a func just to xor. It is merely 2
bytes.

Ultimately this being a minor change which I don't believe is worth
arguing about and I'll have no opinion should the patch get dropped.
This only showed up because I"m looking at whacking atomics in
filename ref handling and *that* is definitely something I'm going to
argue about.
-- 
Mateusz Guzik <mjguzik gmail.com>