Re: [PATCH] mm: Simplify shrink_inactive_list()

[Kirill Tkhai <ktkhai@virtuozzo.com>]

On 12.04.2019 03:05, Baoquan He wrote:
> On 04/11/19 at 06:13pm, Daniel Jordan wrote:
>> On Wed, Apr 10, 2019 at 06:07:04PM +0300, Kirill Tkhai wrote:
>>> @@ -1934,17 +1935,10 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec,
>>>  	__mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, nr_taken);
>>>  	reclaim_stat->recent_scanned[file] += nr_taken;
>>>  
>>> -	if (current_is_kswapd()) {
>>> -		if (global_reclaim(sc))
>>> -			__count_vm_events(PGSCAN_KSWAPD, nr_scanned);
>>> -		count_memcg_events(lruvec_memcg(lruvec), PGSCAN_KSWAPD,
>>> -				   nr_scanned);
>>> -	} else {
>>> -		if (global_reclaim(sc))
>>> -			__count_vm_events(PGSCAN_DIRECT, nr_scanned);
>>> -		count_memcg_events(lruvec_memcg(lruvec), PGSCAN_DIRECT,
>>> -				   nr_scanned);
>>> -	}
>>> +	if (global_reclaim(sc))
>>> +		__count_vm_events(PGSCAN_KSWAPD + is_direct, nr_scanned);
>>> +	__count_memcg_events(lruvec_memcg(lruvec), PGSCAN_KSWAPD + is_direct,
>>> +			     nr_scanned);
>>
>> Nice to avoid duplication like this, but now it takes looking at
>> vm_event_item.h to understand that (PGSCAN_KSWAPD + is_direct) might mean
>> PGSCAN_DIRECT.
>>
>> What about this pattern for each block instead, which makes the stat used
>> explicit and avoids the header change?
>>
>>        stat = current_is_kswapd() ? PG*_KSWAPD : PG*_DIRECT;
> 
> Yeah, looks nice. Maybe name it as item or event since we have had stat
> locally defined as "struct reclaim_stat stat".
> 
> 	enum vm_event_item item;
> 	...
>         item = current_is_kswapd() ? PG*_KSWAPD : PG*_DIRECT;

Sounds good.