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

[yukaixiong <yukaixiong@huawei.com>]

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.