Messages in this thread | | | Date | Wed, 24 Sep 2014 15:30:26 +0200 | From | Vlastimil Babka <> | Subject | Re: [RFC PATCH v3 1/4] mm/page_alloc: fix incorrect isolation behavior by rechecking migratetype |
| |
On 09/15/2014 04:31 AM, Joonsoo Kim wrote: > On Mon, Sep 08, 2014 at 10:31:29AM +0200, Vlastimil Babka wrote: >> On 08/26/2014 10:08 AM, Joonsoo Kim wrote: >> >>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >>> index f86023b..51e0d13 100644 >>> --- a/mm/page_alloc.c >>> +++ b/mm/page_alloc.c >>> @@ -740,9 +740,15 @@ static void free_one_page(struct zone *zone, >>> if (nr_scanned) >>> __mod_zone_page_state(zone, NR_PAGES_SCANNED, -nr_scanned); >>> >>> + if (unlikely(has_isolate_pageblock(zone))) { >>> + migratetype = get_pfnblock_migratetype(page, pfn); >>> + if (is_migrate_isolate(migratetype)) >>> + goto skip_counting; >>> + } >>> + __mod_zone_freepage_state(zone, 1 << order, migratetype); >>> + >>> +skip_counting: >> >> Here, wouldn't a simple 'else __mod_zone_freepage_state...' look >> better than goto + label? (same for the following 2 patches). Or >> does that generate worse code? > > To remove goto label, we need two __mod_zone_freepage_state() like > as below. On my system, it doesn't generate worse code, but, I am not > sure that this is true if more logic would be added. I think that > goto + label is better.
Oh right, I missed that. It's a bit subtle, but I don't see a nicer solution right now.
> + if (unlikely(has_isolate_pageblock(zone))) { > + migratetype = get_pfnblock_migratetype(page, pfn); > + if (!is_migrate_isolate(migratetype)) > + __mod_zone_freepage_state(zone, 1 << order, migratetype); > + } else { > + __mod_zone_freepage_state(zone, 1 << order, migratetype); > } >
Yeah that would be uglier I guess.
> Anyway, What do you think which one is better, either v2 or v3? Still, v3? :)
Yeah v3 is much better than v1 was, and better for backporting than v2. The changelogs also look quite clear. The overhead shouldn't be bad with the per-zone flag guarding get_pfnblock_migratetype.
I'm just not sure about patch 4 and potentially leaving unmerged budies behind. How would it look if instead we made sure isolation works on whole MAX_ORDER blocks instead?
Vlastimil
> Thanks. >
| |