Re: + mm-vmscan-add-mm_vmscan_inactive_list_is_low-tracepoint.patch added to -mm tree

[Minchan Kim]

Hello,

On Wed, Jan 11, 2017 at 04:52:39PM +0100, Michal Hocko wrote:
> On Wed 11-01-17 08:52:50, Minchan Kim wrote:
> [...]
> > > @@ -2055,8 +2055,8 @@ static bool inactive_list_is_low(struct
> > >  	if (!file && !total_swap_pages)
> > >  		return false;
> > >  
> > > -	inactive = lruvec_lru_size(lruvec, file * LRU_FILE);
> > > -	active = lruvec_lru_size(lruvec, file * LRU_FILE + LRU_ACTIVE);
> > > +	total_inactive = inactive = lruvec_lru_size(lruvec, file * LRU_FILE);
> > > +	total_active = active = lruvec_lru_size(lruvec, file * LRU_FILE + LRU_ACTIVE);
> > >  
> > 
> > the decision of deactivating is based on eligible zone's LRU size,
> > not whole zone so why should we need to get a trace of all zones's LRU?
> 
> Strictly speaking, the total_ counters are not necessary for making the
> decision. I found reporting those numbers useful regardless because this
> will give us also an information how large is the eligible portion of
> the LRU list. We do not have any other tracepoint which would report
> that.

The patch doesn't say anything why it's useful. Could you tell why it's
useful and inactive_list_is_low should be right place?

Don't get me wrong, please. I don't want to bother you.
I really don't want to add random stuff although it's tracepoint for
debugging.

>  
> [...]
> > > @@ -2223,7 +2228,7 @@ static void get_scan_count(struct lruvec
> > >  	 * lruvec even if it has plenty of old anonymous pages unless the
> > >  	 * system is under heavy pressure.
> > >  	 */
> > > -	if (!inactive_list_is_low(lruvec, true, sc) &&
> > > +	if (!inactive_list_is_low(lruvec, true, sc, false) &&
> > 
> > Hmm, I was curious why you added trace boolean arguement and found it here.
> > Yes, here is not related to deactivation directly but couldn't we help to
> > trace it unconditionally?
> 
> I've had it like that when I was debugging the mentioned bug and found
> it a bit disturbing. It generated more output than I would like and it
> wasn't really clear from which code path  this has been called from.

Indeed.

Personally, I want to move inactive_list_is_low in shrink_active_list
and shrink_active_list calls inactive_list_is_low(...., true),
unconditionally so that it can make code simple/clear but cannot remove
trace boolean variable , which what I want. So, it's okay if you love
your version.

> 
> > With that, we can know why VM reclaim only
> > file-backed page on slow device although enough anonymous pages on fast
> > swap like zram are enough.
> 
> That would be something for a separate tracepoint in g_s_c

Agree.