Re: [PATCH 2/2] mm/compaction: cleanup isolate_freepages()
From: Vlastimil Babka
Date: Tue Apr 22 2014 - 02:33:58 EST
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@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;
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...)
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@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/