diff mbox series

[v3,-next,11/15] sunrpc: use vfs_pressure_ratio() helper

Message ID 20241010152215.3025842-12-yukaixiong@huawei.com (mailing list archive)
State New
Headers show
Series sysctl: move sysctls from vm_table into its own files | expand

Commit Message

yukaixiong Oct. 10, 2024, 3:22 p.m. UTC
Use vfs_pressure_ratio() to simplify code.

Signed-off-by: Kaixiong Yu <yukaixiong@huawei.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Acked-by: Anna Schumaker <anna.schumaker@oracle.com>
---
 net/sunrpc/auth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jeff Layton Oct. 10, 2024, 2:44 p.m. UTC | #1
On Thu, 2024-10-10 at 23:22 +0800, Kaixiong Yu wrote:
> Use vfs_pressure_ratio() to simplify code.
> 
> Signed-off-by: Kaixiong Yu <yukaixiong@huawei.com>
> Reviewed-by: Kees Cook <kees@kernel.org>
> Acked-by: Anna Schumaker <anna.schumaker@oracle.com>
> ---
>  net/sunrpc/auth.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> index 04534ea537c8..3d2b51d7e934 100644
> --- a/net/sunrpc/auth.c
> +++ b/net/sunrpc/auth.c
> @@ -489,7 +489,7 @@ static unsigned long
>  rpcauth_cache_shrink_count(struct shrinker *shrink, struct shrink_control *sc)
>  
>  {
> -	return number_cred_unused * sysctl_vfs_cache_pressure / 100;
> +	return vfs_pressure_ratio(number_cred_unused);
>  }
>  
>  static void

Acked-by: Jeff Layton <jlayton@kernel.org>
NeilBrown Oct. 10, 2024, 9:43 p.m. UTC | #2
On Fri, 11 Oct 2024, Jeff Layton wrote:
> On Thu, 2024-10-10 at 23:22 +0800, Kaixiong Yu wrote:
> > Use vfs_pressure_ratio() to simplify code.
> > 
> > Signed-off-by: Kaixiong Yu <yukaixiong@huawei.com>
> > Reviewed-by: Kees Cook <kees@kernel.org>
> > Acked-by: Anna Schumaker <anna.schumaker@oracle.com>
> > ---
> >  net/sunrpc/auth.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> > index 04534ea537c8..3d2b51d7e934 100644
> > --- a/net/sunrpc/auth.c
> > +++ b/net/sunrpc/auth.c
> > @@ -489,7 +489,7 @@ static unsigned long
> >  rpcauth_cache_shrink_count(struct shrinker *shrink, struct shrink_control *sc)
> >  
> >  {
> > -	return number_cred_unused * sysctl_vfs_cache_pressure / 100;
> > +	return vfs_pressure_ratio(number_cred_unused);
> >  }
> >  
> >  static void
> 
> Acked-by: Jeff Layton <jlayton@kernel.org>
> 

I realise this is a bit of a tangent, and I'm not objecting to this
patch, but I wonder what the justification is for using
vfs_cache_pressure here.  The sysctl is documented as

   This percentage value controls the tendency of the kernel to reclaim
   the memory which is used for caching of directory and inode objects.

So it can sensibly be used for dentries and inode, and for anything
directly related like the nfs access cache (which is attached to inodes)
and the nfs xattr cache.

But the sunrpc cred cache scales with the number of active users, not
the number of inodes/dentries.

So I think this should simply "return number_cred_unused;".

What do others think?

NeilBrown
Jeff Layton Oct. 11, 2024, 12:38 p.m. UTC | #3
On Fri, 2024-10-11 at 08:43 +1100, NeilBrown wrote:
> On Fri, 11 Oct 2024, Jeff Layton wrote:
> > On Thu, 2024-10-10 at 23:22 +0800, Kaixiong Yu wrote:
> > > Use vfs_pressure_ratio() to simplify code.
> > > 
> > > Signed-off-by: Kaixiong Yu <yukaixiong@huawei.com>
> > > Reviewed-by: Kees Cook <kees@kernel.org>
> > > Acked-by: Anna Schumaker <anna.schumaker@oracle.com>
> > > ---
> > >  net/sunrpc/auth.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
> > > index 04534ea537c8..3d2b51d7e934 100644
> > > --- a/net/sunrpc/auth.c
> > > +++ b/net/sunrpc/auth.c
> > > @@ -489,7 +489,7 @@ static unsigned long
> > >  rpcauth_cache_shrink_count(struct shrinker *shrink, struct shrink_control *sc)
> > >  
> > >  {
> > > -	return number_cred_unused * sysctl_vfs_cache_pressure / 100;
> > > +	return vfs_pressure_ratio(number_cred_unused);
> > >  }
> > >  
> > >  static void
> > 
> > Acked-by: Jeff Layton <jlayton@kernel.org>
> > 
> 
> I realise this is a bit of a tangent, and I'm not objecting to this
> patch, but I wonder what the justification is for using
> vfs_cache_pressure here.  The sysctl is documented as
> 
>    This percentage value controls the tendency of the kernel to reclaim
>    the memory which is used for caching of directory and inode objects.
> 
> So it can sensibly be used for dentries and inode, and for anything
> directly related like the nfs access cache (which is attached to inodes)
> and the nfs xattr cache.
> 
> But the sunrpc cred cache scales with the number of active users, not
> the number of inodes/dentries.
> 
> So I think this should simply "return number_cred_unused;".
> 
> What do others think?
> 
> NeilBrown
> 

-----------------8<------------------
 * @count_objects should return the number of freeable items in the cache. If
 * there are no objects to free, it should return SHRINK_EMPTY, while 0 is
 * returned in cases of the number of freeable items cannot be determined
 * or shrinker should skip this cache for this time (e.g., their number
 * is below shrinkable limit)...
-----------------8<------------------

number_cred_unused does sound like a better way to report this.
diff mbox series

Patch

diff --git a/net/sunrpc/auth.c b/net/sunrpc/auth.c
index 04534ea537c8..3d2b51d7e934 100644
--- a/net/sunrpc/auth.c
+++ b/net/sunrpc/auth.c
@@ -489,7 +489,7 @@  static unsigned long
 rpcauth_cache_shrink_count(struct shrinker *shrink, struct shrink_control *sc)
 
 {
-	return number_cred_unused * sysctl_vfs_cache_pressure / 100;
+	return vfs_pressure_ratio(number_cred_unused);
 }
 
 static void