Re: [PATCH 2/2] mm/ksm: add initialization helpers for scan_get_next_rmap_item()

[Xu Xin (vivo)]

> > diff --git a/mm/ksm.c b/mm/ksm.c
> > index 49977bcf04f7..e79e64372e49 100644
> > --- a/mm/ksm.c
> > +++ b/mm/ksm.c
> > @@ -2640,6 +2640,49 @@ static void ksm_reset_unstable_trees(void)
> >  		root_unstable_tree[nid] = RB_ROOT;
> >  }
> >  
> > +static bool scan_begin_new_cycle(void)
> > +{
> > +	/* Check if the cursor points to the head. */
> > +	if (ksm_scan.mm_slot != &ksm_mm_head)
> > +		return false;
> > +
> > +	/*
> > +	 * A number of pages can hang around indefinitely in per-cpu
> > +	 * LRU cache, raised page count preventing write_protect_page
> > +	 * from merging them.  Though it doesn't really matter much,
> > +	 * it is puzzling to see some stuck in pages_volatile until
> > +	 * other activity jostles them out, and they also prevented
> > +	 * LTP's KSM test from succeeding deterministically; so drain
> > +	 * them here (here rather than on entry to ksm_do_scan(),
> > +	 * so we don't IPI too often when pages_to_scan is set low).
> > +	 */
> > +	lru_add_drain_all();
> > +	return true;
> > +}
> > +
> 
> Can we just leave the lru_add_drain_all() where it is, or what's the reason for
> that change?

Sure, There was no special reason for moving lru_add_drain_all()—I just wanted to
keep low-level details out of the main scan function to make it look cleaner.
Since it is going to be removed soon anyway, leaving it in place makes total
sense.

> 
> Note that that might soon get removed either way, so I wouldn't worry about it.
> 
> Then, I would rename the function into something like
> 
> "scan_should_start_new_cycle"
> 
> And keep it simple.

I will rename the helper to scan_should_start_new_cycle(), keep it simple,
and send out v2 shortly.