netdev - Re: [PATCH v3 -next 11/15] sunrpc: use vfs_pressure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b0cf00eec77014ad473d4510904eb2d4fd084e5a.camel@kernel.org>
Date: Fri, 11 Oct 2024 08:38:32 -0400
From: Jeff Layton <jlayton@...nel.org>
To: NeilBrown <neilb@...e.de>
Cc: Kaixiong Yu <yukaixiong@...wei.com>, akpm@...ux-foundation.org, 
 mcgrof@...nel.org, ysato@...rs.sourceforge.jp, dalias@...c.org, 
 glaubitz@...sik.fu-berlin.de, luto@...nel.org, tglx@...utronix.de, 
 mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com, hpa@...or.com,
  viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz,
 kees@...nel.org,  j.granados@...sung.com, willy@...radead.org,
 Liam.Howlett@...cle.com,  vbabka@...e.cz, lorenzo.stoakes@...cle.com,
 trondmy@...nel.org, anna@...nel.org,  chuck.lever@...cle.com,
 okorniev@...hat.com, Dai.Ngo@...cle.com, tom@...pey.com, 
 davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com,  paul@...l-moore.com, jmorris@...ei.org,
 linux-sh@...r.kernel.org,  linux-kernel@...r.kernel.org,
 linux-fsdevel@...r.kernel.org, linux-mm@...ck.org, 
 linux-nfs@...r.kernel.org, netdev@...r.kernel.org, 
 linux-security-module@...r.kernel.org, dhowells@...hat.com, 
 haifeng.xu@...pee.com, baolin.wang@...ux.alibaba.com,
 shikemeng@...weicloud.com,  dchinner@...hat.com, bfoster@...hat.com,
 souravpanda@...gle.com,  hannes@...xchg.org, rientjes@...gle.com,
 pasha.tatashin@...een.com,  david@...hat.com, ryan.roberts@....com,
 ying.huang@...el.com,  yang@...amperecomputing.com, zev@...ilderbeest.net,
 serge@...lyn.com,  vegard.nossum@...cle.com, wangkefeng.wang@...wei.com,
 sunnanyong@...wei.com
Subject: Re: [PATCH v3 -next 11/15] sunrpc: use vfs_pressure_ratio() helper

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@...wei.com>
> > > Reviewed-by: Kees Cook <kees@...nel.org>
> > > Acked-by: Anna Schumaker <anna.schumaker@...cle.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@...nel.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.

-- 
Jeff Layton <jlayton@...nel.org>