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@xxxxxxxxxx> 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;
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;
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's why I asked about max operation. What am I missing?
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
Kind regards,
Minchan Kim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>