Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()

[Minchan Kim <minchan@kernel.org>]

On Tue, Apr 22, 2014 at 08:33:35AM +0200, Vlastimil Babka wrote:
> On 22.4.2014 1:53, Minchan Kim wrote:
> >On Mon, Apr 21, 2014 at 11:43:24PM +0200, Vlastimil Babka wrote:
> >>On 21.4.2014 21:41, Andrew Morton wrote:
> >>>On Thu, 17 Apr 2014 09:07:45 +0900 Minchan Kim <minchan@kernel.org> wrote:
> >>>
> >>>>Hi Vlastimil,
> >>>>
> >>>>Below just nitpicks.
> >>>It seems you were ignored ;)
> >>Oops, I managed to miss your e-mail, sorry.
> >>
> >>>>>  {
> >>>>>  	struct page *page;
> >>>>>-	unsigned long high_pfn, low_pfn, pfn, z_end_pfn;
> >>>>>+	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >>>>Could you add comment for each variable?
> >>>>
> >>>>unsigned long pfn; /* scanning cursor */
> >>>>unsigned long low_pfn; /* lowest pfn free scanner is able to scan */
> >>>>unsigned long next_free_pfn; /* start pfn for scaning at next truen */
> >>>>unsigned long z_end_pfn; /* zone's end pfn */
> >>>>
> >>>>
> >>>>>@@ -688,11 +688,10 @@ static void isolate_freepages(struct zone *zone,
> >>>>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>>>  	/*
> >>>>>-	 * Take care that if the migration scanner is at the end of the zone
> >>>>>-	 * that the free scanner does not accidentally move to the next zone
> >>>>>-	 * in the next isolation cycle.
> >>>>>+	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >>>>>+	 * none, the pfn < low_pfn check will kick in.
> >>>>        "none" what? I'd like to clear more.
> >>If there are no updates to next_free_pfn within the for cycle. Which
> >>matches Andrew's formulation below.
> >>
> >>>I did this:
> >>Thanks!
> >>
> >>>--- a/mm/compaction.c~mm-compaction-cleanup-isolate_freepages-fix
> >>>+++ a/mm/compaction.c
> >>>@@ -662,7 +662,10 @@ static void isolate_freepages(struct zon
> >>>  				struct compact_control *cc)
> >>>  {
> >>>  	struct page *page;
> >>>-	unsigned long pfn, low_pfn, next_free_pfn, z_end_pfn;
> >>>+	unsigned long pfn;	     /* scanning cursor */
> >>>+	unsigned long low_pfn;	     /* lowest pfn scanner is able to scan */
> >>>+	unsigned long next_free_pfn; /* start pfn for scaning at next round */
> >>>+	unsigned long z_end_pfn;     /* zone's end pfn */
> >>Yes that works.
> >>
> >>>  	int nr_freepages = cc->nr_freepages;
> >>>  	struct list_head *freelist = &cc->freepages;
> >>>@@ -679,8 +682,8 @@ static void isolate_freepages(struct zon
> >>>  	low_pfn = ALIGN(cc->migrate_pfn + 1, pageblock_nr_pages);
> >>>  	/*
> >>>-	 * Seed the value for max(next_free_pfn, pfn) updates. If there are
> >>>-	 * none, the pfn < low_pfn check will kick in.
> >>>+	 * Seed the value for max(next_free_pfn, pfn) updates. If no pages are
> >>>+	 * isolated, the pfn < low_pfn check will kick in.
> >>OK.
> >>
> >>>  	 */
> >>>  	next_free_pfn = 0;
> >>>>>@@ -766,9 +765,9 @@ static void isolate_freepages(struct zone *zone,
> >>>>>  	 * so that compact_finished() may detect this
> >>>>>  	 */
> >>>>>  	if (pfn < low_pfn)
> >>>>>-		cc->free_pfn = max(pfn, zone->zone_start_pfn);
> >>>>>-	else
> >>>>>-		cc->free_pfn = high_pfn;
> >>>>>+		next_free_pfn = max(pfn, zone->zone_start_pfn);
> >>>>Why we need max operation?
> >>>>IOW, what's the problem if we do (next_free_pfn = pfn)?
> >>>An answer to this would be useful, thanks.
> >>The idea (originally, not new here) is that the free scanner wants
> >>to remember the highest-pfn
> >>block where it managed to isolate some pages. If the following page
> >>migration fails, these isolated
> >>pages might be put back and would be skipped in further compaction
> >>attempt if we used just
> >>"next_free_pfn = pfn", until the scanners get reset.
> >>
> >>The question of course is if such situations are frequent and makes
> >>any difference to compaction
> >>outcome. And the downsides are potentially useless rescans and code
> >>complexity. Maybe Mel
> >>remembers how important this is? It should probably be profiled
> >>before changes are made.
> >I didn't mean it. What I mean is code snippet you introduced in 7ed695e069c3c.
> >At that time, I didn't Cced so I missed that code so let's ask this time.
> >In that patch, you added this.
> >
> >if (pfn < low_pfn)
> >   cc->free_pfn = max(pfn, zone->zone_start_pfn);
> >else
> >   cc->free_pfn = high_pfn;
> 
> Oh, right, this max(), not the one in the for loop. Sorry, I should
> have read more closely.
> But still maybe it's a good opportunity to kill the other max() as
> well. I'll try some testing.
> 
> Anyway, this is what I answered to Mel when he asked the same thing
> when I sent
> that 7ed695069c3c patch:
> 
> If a zone starts in a middle of a pageblock and migrate scanner isolates
> enough pages early to stay within that pageblock, low_pfn will be at the
> end of that pageblock and after the for cycle in this function ends, pfn
> might be at the beginning of that pageblock. It might not be an actual
> problem (this compaction will finish at this point, and if someone else
> is racing, he will probably check the boundaries himself), but I played
> it safe.
> 
> 
> >So the purpose of max(pfn, zone->zone_start_pfn) is to be detected by
> >compact_finished to stop compaction. And your [1/2] patch in this patchset
> >always makes free page scanner start on pageblock boundary so when the
> >loop in isolate_freepages is finished and pfn is lower low_pfn, the pfn
> >would be lower than migration scanner so compact_finished will always detect
> >it so I think you could just do
> >
> >if (pfn < low_pfn)
> >   next_free_pfn = pfn;
> >
> >cc->free_pfn = next_free_pfn;
> 
> That could work. I was probably wrong about danger of racing in the
> reply to Mel,
> because free_pfn is stored in cc (private), not zone (shared).
> 
> >
> >Or, if you want to clear *reset*,
> >if (pfn < lown_pfn)
> >   next_free_pfn = zone->zone_start_pfn;
> >
> >cc->free_pfn = next_free_pfn;
> 
> That would work as well but is less straightforward I think. Might
> be misleading if
> someone added tracepoints to track the free scanner progress with
> pfn's (which
> might happen soon...)

My preference is to add following with pair of compact_finished

static inline void finish_compact(struct compact_control *cc)
{
  cc->free_pfn = cc->migrate_pfn;
}

But I don't care.
If you didn't send this patch as clean up, I would never interrupt
on the way but you said it's cleanup patch and the one made me spend a
few minutes to understand the code so it's not a clean up patch. ;-).
So, IMO, it's worth to tidy it up.


-- 
Kind regards,
Minchan Kim