Re: [PATCH] mm: Simplify shrink_inactive_list()

[Baoquan He <bhe@redhat.com>]

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;