[Devel] [PATCH vz9 v3] mm: per memory cgroup page cache limit

[Alexander Atanasov]

On 24.01.23 10:21, nb wrote:
> 
>> @@ -2260,9 +2263,16 @@ static bool consume_stock(struct mem_cgroup 
>> *memcg, unsigned int nr_pages)
>>       local_lock_irqsave(&memcg_stock.stock_lock, flags);
>>       stock = this_cpu_ptr(&memcg_stock);
>> -    if (memcg == stock->cached && stock->nr_pages >= nr_pages) {
>> -        stock->nr_pages -= nr_pages;
>> -        ret = true;
>> +    if (memcg == stock->cached) {
>> +        if (cache && stock->cache_nr_pages >= nr_pages) {
>> +            stock->cache_nr_pages -= nr_pages;
>> +            ret = true;
>> +        }
>> +
>> +        if (!cache && stock->nr_pages >= nr_pages) {
>> +            stock->nr_pages -= nr_pages;
>> +            ret = true;
>> +        }
> 
> nit: I find the original condition somewhat more readaable i.e having 
> the memch stock->cached and cache_nr_pages check in the same if.
> 
>   if (memcg == stock->cached && stock->cache_nr_pages >= nr_pages) {
>          if (cached)
>              stock->cache_nr_pages -= nr_pages;
>          else
>              stock->nr_pages -= nr_pages;
> 
>          ret = true;
> }


I've made it exactly like this initially but on a second look it doesn't 
check one of the conditions so i left it like this.
see cache_nr_pages vs nr_pages checks.

>> -    stock->nr_pages += nr_pages;
>> +    if (!cache)
>> +        stock->nr_pages += nr_pages;
>> +    else
>> +        stock->cache_nr_pages += nr_pages;
> nit:
> 
> It reads more natural if we do if (cache) else  rathen with the negative 
> condition.

afaik the first condition is the likely condition so i put it like this.
as for readability i do not find a major difference.


-- 
Regards,
Alexander Atanasov