| 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 |
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>
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
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.
On 2024/10/11 5:43, 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 > > . Thank you, I will receive your advice.
On 2024/10/11 20:38, Jeff Layton wrote: > 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. Thanks, I'll take NeilBrown's advice.
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