Re: [PATCH -next v3 2/2] memcg: remove mem_cgroup_size()

[Chen Ridong]

On 2025/12/16 0:28, Michal Koutný wrote:
> Hi Ridong.
> 
> On Thu, Dec 11, 2025 at 01:30:19AM +0000, Chen Ridong <chenridong@xxxxxxxxxxxxxxx> wrote:
>> From: Chen Ridong <chenridong@xxxxxxxxxx>
>>
>> The mem_cgroup_size helper is used only in apply_proportional_protection
>> to read the current memory usage. Its semantics are unclear and
>> inconsistent with other sites, which directly call page_counter_read for
>> the same purpose.
>>
>> Remove this helper and get its usage via mem_cgroup_protection for
>> clarity. Additionally, rename the local variable 'cgroup_size' to 'usage'
>> to better reflect its meaning.
>>
>> No functional changes intended.
>>
>> Signed-off-by: Chen Ridong <chenridong@xxxxxxxxxx>
> 
> Why does mem_cgroup_calculate_protection "calculate" usage for its
> callers? Couldn't you just the change source in
> apply_proportional_protection()?
> 
> Thanks,
> Michal
> 

I apologize for missing this message earlier.

In my v2 patch, I was reading the usage directly:

+		unsigned long usage = page_counter_read(&memcg->memory);

This works fine when CONFIG_MEMCG=y, but fails to compile when memory cgroups are disabled. To
handle this, I initially added #ifdef CONFIG_MEMCG guards.

Following Johannes's suggestion, I have now moved this logic into mem_cgroup_protection() to
eliminate the #ifdef and keep the code cleaner.

Discussion:
https://lore.kernel.org/all/20251210163634.GB643576@xxxxxxxxxxx/

>> @@ -2485,7 +2485,6 @@ static unsigned long apply_proportional_protection(struct mem_cgroup *memcg,
>>  		 * again by how much of the total memory used is under
>>  		 * hard protection.
>>  		 */
>> -		unsigned long cgroup_size = mem_cgroup_size(memcg);
> +		unsigned long cgroup_size = page_counter_read(memcg);
> 
>>  		unsigned long protection;
>>  
>>  		/* memory.low scaling, make sure we retry before OOM */
>> @@ -2497,9 +2496,9 @@ static unsigned long apply_proportional_protection(struct mem_cgroup *memcg,
>>  		}
>>  
>>  		/* Avoid TOCTOU with earlier protection check */
>> -		cgroup_size = max(cgroup_size, protection);
>> +		usage = max(usage, protection);
>>  
>> -		scan -= scan * protection / (cgroup_size + 1);
>> +		scan -= scan * protection / (usage + 1);
>>  
>>  		/*
>>  		 * Minimally target SWAP_CLUSTER_MAX pages to keep

-- 
Best regards,
Ridong