Re: [PATCH 13/28] mm, page_alloc: Remove redundant check for empty zonelist

From: Mel Gorman
Date: Tue Apr 26 2016 - 09:00:23 EST


On Tue, Apr 26, 2016 at 02:04:51PM +0200, Vlastimil Babka wrote:
> On 04/15/2016 11:07 AM, Mel Gorman wrote:
> >A check is made for an empty zonelist early in the page allocator fast path
> >but it's unnecessary. When get_page_from_freelist() is called, it'll return
> >NULL immediately. Removing the first check is slower for machines with
> >memoryless nodes but that is a corner case that can live with the overhead.
> >
> >Signed-off-by: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
> >---
> > mm/page_alloc.c | 11 -----------
> > 1 file changed, 11 deletions(-)
> >
> >diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> >index df03ccc7f07c..21aaef6ddd7a 100644
> >--- a/mm/page_alloc.c
> >+++ b/mm/page_alloc.c
> >@@ -3374,14 +3374,6 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
> > if (should_fail_alloc_page(gfp_mask, order))
> > return NULL;
> >
> >- /*
> >- * Check the zones suitable for the gfp_mask contain at least one
> >- * valid zone. It's possible to have an empty zonelist as a result
> >- * of __GFP_THISNODE and a memoryless node
> >- */
> >- if (unlikely(!zonelist->_zonerefs->zone))
> >- return NULL;
> >-
> > if (IS_ENABLED(CONFIG_CMA) && ac.migratetype == MIGRATE_MOVABLE)
> > alloc_flags |= ALLOC_CMA;
> >
> >@@ -3394,8 +3386,6 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
> > /* The preferred zone is used for statistics later */
> > preferred_zoneref = first_zones_zonelist(ac.zonelist, ac.high_zoneidx,
> > ac.nodemask, &ac.preferred_zone);
> >- if (!ac.preferred_zone)
> >- goto out;
>
> Is this part really safe? Besides changelog doesn't mention preferred_zone.
> What if somebody attempts e.g. a DMA allocation with ac.nodemask being set
> to cpuset_current_mems_allowed and initially only containing nodes without
> ZONE_DMA. Then ac.preferred_zone is NULL, yet we proceed to
> get_page_from_freelist(). Meanwhile cpuset_current_mems_allowed gets changed
> so in fact it does contains a suitable node, so we manage to get inside
> for_each_zone_zonelist_nodemask(). Then there's
> zone_local(ac->preferred_zone, zone), which will defererence the NULL
> ac->preferred_zone?
>

You're right, this is a potential problem. I thought of a few solutions
but they're not necessarily cheaper than the current code. If Andrew is
watching, please drop this patch if possible. Otherwise, I'll post a revert
within the next 2 days and find an alternative solution that still saves
cycles.

--
Mel Gorman
SUSE Labs