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.
