Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1386554 > unrolled thread

Re: [PATCH 05/28] mm, page_alloc: Inline the fast path of the zonelist iterator

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 05/28] mm, page_alloc: Inline the fast path of the  zonelist iterator Vlastimil Babka <vbabka@suse.cz> - 2016-04-25 17:00 +0200
    Re: [PATCH 05/28] mm, page_alloc: Inline the fast path of the  zonelist iterator Mel Gorman <mgorman@techsingularity.net> - 2016-04-26 12:40 +0200
      Re: [PATCH 05/28] mm, page_alloc: Inline the fast path of the  zonelist iterator Vlastimil Babka <vbabka@suse.cz> - 2016-04-26 13:10 +0200

#1386554 — Re: [PATCH 05/28] mm, page_alloc: Inline the fast path of the zonelist iterator

On 04/15/2016 10:58 AM, Mel Gorman wrote:
> The page allocator iterates through a zonelist for zones that match
> the addressing limitations and nodemask of the caller but many allocations
> will not be restricted. Despite this, there is always functional call
> overhead which builds up.
> 
> This patch inlines the optimistic basic case and only calls the
> iterator function for the complex case. A hindrance was the fact that
> cpuset_current_mems_allowed is used in the fastpath as the allowed nodemask
> even though all nodes are allowed on most systems. The patch handles this
> by only considering cpuset_current_mems_allowed if a cpuset exists. As well
> as being faster in the fast-path, this removes some junk in the slowpath.

I don't think this part is entirely correct (or at least argued as being
correct above), see below.
 
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3193,17 +3193,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>   	 */
>   	alloc_flags = gfp_to_alloc_flags(gfp_mask);
>   
> -	/*
> -	 * Find the true preferred zone if the allocation is unconstrained by
> -	 * cpusets.
> -	 */
> -	if (!(alloc_flags & ALLOC_CPUSET) && !ac->nodemask) {
> -		struct zoneref *preferred_zoneref;
> -		preferred_zoneref = first_zones_zonelist(ac->zonelist,
> -				ac->high_zoneidx, NULL, &ac->preferred_zone);
> -		ac->classzone_idx = zonelist_zone_idx(preferred_zoneref);
> -	}
> -
>   	/* This is the last chance, in general, before the goto nopage. */
>   	page = get_page_from_freelist(gfp_mask, order,
>   				alloc_flags & ~ALLOC_NO_WATERMARKS, ac);
> @@ -3359,14 +3348,21 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
>   	struct zoneref *preferred_zoneref;
>   	struct page *page = NULL;
>   	unsigned int cpuset_mems_cookie;
> -	int alloc_flags = ALLOC_WMARK_LOW|ALLOC_CPUSET|ALLOC_FAIR;
> +	int alloc_flags = ALLOC_WMARK_LOW|ALLOC_FAIR;
>   	gfp_t alloc_mask; /* The gfp_t that was actually used for allocation */
>   	struct alloc_context ac = {
>   		.high_zoneidx = gfp_zone(gfp_mask),
> +		.zonelist = zonelist,
>   		.nodemask = nodemask,
>   		.migratetype = gfpflags_to_migratetype(gfp_mask),
>   	};
>   
> +	if (cpusets_enabled()) {
> +		alloc_flags |= ALLOC_CPUSET;
> +		if (!ac.nodemask)
> +			ac.nodemask = &cpuset_current_mems_allowed;
> +	}

My initial reaction is that this is setting ac.nodemask in stone outside
of cpuset_mems_cookie, but I guess it's ok since we're taking a pointer
into current's task_struct, not the contents of the current's nodemask.
It's however setting a non-NULL nodemask into stone, which means no
zonelist iterator fasthpaths... but only in the slowpath. I guess it's
not an issue then.

> +
>   	gfp_mask &= gfp_allowed_mask;
>   
>   	lockdep_trace_alloc(gfp_mask);
> @@ -3390,16 +3386,12 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
>   retry_cpuset:
>   	cpuset_mems_cookie = read_mems_allowed_begin();
>   
> -	/* We set it here, as __alloc_pages_slowpath might have changed it */
> -	ac.zonelist = zonelist;

This doesn't seem relevant to the preferred_zoneref changes in
__alloc_pages_slowpath, so why it became ok? Maybe it is, but it's not
clear from the changelog.

Anyway, thinking about it made me realize that maybe we could move the
whole mems_cookie thing into slowpath? As soon as the optimistic
fastpath succeeds, we don't check the cookie anyway, so what about
something like this on top?

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d18061535c8b..07bf1065e7c9 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3183,6 +3183,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
 	struct page *page = NULL;
 	int alloc_flags;
+	unsigned int cpuset_mems_cookie;
 	unsigned long pages_reclaimed = 0;
 	unsigned long did_some_progress;
 	enum migrate_mode migration_mode = MIGRATE_ASYNC;
@@ -3209,6 +3210,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		gfp_mask &= ~__GFP_ATOMIC;
 
 retry:
+	cpuset_mems_cookie = read_mems_allowed_begin();
+
 	if (gfp_mask & __GFP_KSWAPD_RECLAIM)
 		wake_all_kswapds(order, ac);
 
@@ -3219,17 +3222,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	 */
 	alloc_flags = gfp_to_alloc_flags(gfp_mask);
 
-	/*
-	 * Find the true preferred zone if the allocation is unconstrained by
-	 * cpusets.
-	 */
-	if (!(alloc_flags & ALLOC_CPUSET) && !ac->nodemask) {
-		struct zoneref *preferred_zoneref;
-		preferred_zoneref = first_zones_zonelist(ac->zonelist,
-				ac->high_zoneidx, NULL, &ac->preferred_zone);
-		ac->classzone_idx = zonelist_zone_idx(preferred_zoneref);
-	}
-
 	/* This is the last chance, in general, before the goto nopage. */
 	page = get_page_from_freelist(gfp_mask, order,
 				alloc_flags & ~ALLOC_NO_WATERMARKS, ac);
@@ -3370,7 +3362,17 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (page)
 		goto got_pg;
 nopage:
+	/*
+	 * When updating a task's mems_allowed, it is possible to race with
+	 * parallel threads in such a way that an allocation can fail while
+	 * the mask is being updated. If a page allocation is about to fail,
+	 * check if the cpuset changed during allocation and if so, retry.
+	 */
+	if (read_mems_allowed_retry(cpuset_mems_cookie))
+		goto retry;
+
 	warn_alloc_failed(gfp_mask, order, NULL);
+
 got_pg:
 	return page;
 }
@@ -3384,7 +3386,6 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
 {
 	struct zoneref *preferred_zoneref;
 	struct page *page = NULL;
-	unsigned int cpuset_mems_cookie;
 	int alloc_flags = ALLOC_WMARK_LOW|ALLOC_FAIR;
 	gfp_t alloc_mask; /* The gfp_t that was actually used for allocation */
 	struct alloc_context ac = {
@@ -3420,9 +3421,6 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
 	if (IS_ENABLED(CONFIG_CMA) && ac.migratetype == MIGRATE_MOVABLE)
 		alloc_flags |= ALLOC_CMA;
 
-retry_cpuset:
-	cpuset_mems_cookie = read_mems_allowed_begin();
-
 	/* Dirty zone balancing only done in the fast path */
 	ac.spread_dirty_pages = (gfp_mask & __GFP_WRITE);
 
@@ -3430,13 +3428,15 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
 	preferred_zoneref = first_zones_zonelist(ac.zonelist, ac.high_zoneidx,
 				ac.nodemask, &ac.preferred_zone);
 	if (!ac.preferred_zone)
-		goto out;
+		goto slowpath;
 	ac.classzone_idx = zonelist_zone_idx(preferred_zoneref);
 
 	/* First allocation attempt */
 	alloc_mask = gfp_mask|__GFP_HARDWALL;
 	page = get_page_from_freelist(alloc_mask, order, alloc_flags, &ac);
+
 	if (unlikely(!page)) {
+slowpath:
 		/*
 		 * Runtime PM, block IO and its error handling path
 		 * can deadlock because I/O on the device might not
@@ -3453,16 +3453,6 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
 
 	trace_mm_page_alloc(page, order, alloc_mask, ac.migratetype);
 
-out:
-	/*
-	 * When updating a task's mems_allowed, it is possible to race with
-	 * parallel threads in such a way that an allocation can fail while
-	 * the mask is being updated. If a page allocation is about to fail,
-	 * check if the cpuset changed during allocation and if so, retry.
-	 */
-	if (unlikely(!page && read_mems_allowed_retry(cpuset_mems_cookie)))
-		goto retry_cpuset;
-
 	return page;
 }
 EXPORT_SYMBOL(__alloc_pages_nodemask);

[toc] | [next] | [standalone]

#1387346

On Mon, Apr 25, 2016 at 04:50:18PM +0200, Vlastimil Babka wrote:
> > @@ -3193,17 +3193,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >   	 */
> >   	alloc_flags = gfp_to_alloc_flags(gfp_mask);
> >   
> > -	/*
> > -	 * Find the true preferred zone if the allocation is unconstrained by
> > -	 * cpusets.
> > -	 */
> > -	if (!(alloc_flags & ALLOC_CPUSET) && !ac->nodemask) {
> > -		struct zoneref *preferred_zoneref;
> > -		preferred_zoneref = first_zones_zonelist(ac->zonelist,
> > -				ac->high_zoneidx, NULL, &ac->preferred_zone);
> > -		ac->classzone_idx = zonelist_zone_idx(preferred_zoneref);
> > -	}
> > -
> >   	/* This is the last chance, in general, before the goto nopage. */
> >   	page = get_page_from_freelist(gfp_mask, order,
> >   				alloc_flags & ~ALLOC_NO_WATERMARKS, ac);
> > @@ -3359,14 +3348,21 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
> >   	struct zoneref *preferred_zoneref;
> >   	struct page *page = NULL;
> >   	unsigned int cpuset_mems_cookie;
> > -	int alloc_flags = ALLOC_WMARK_LOW|ALLOC_CPUSET|ALLOC_FAIR;
> > +	int alloc_flags = ALLOC_WMARK_LOW|ALLOC_FAIR;
> >   	gfp_t alloc_mask; /* The gfp_t that was actually used for allocation */
> >   	struct alloc_context ac = {
> >   		.high_zoneidx = gfp_zone(gfp_mask),
> > +		.zonelist = zonelist,
> >   		.nodemask = nodemask,
> >   		.migratetype = gfpflags_to_migratetype(gfp_mask),
> >   	};
> >   
> > +	if (cpusets_enabled()) {
> > +		alloc_flags |= ALLOC_CPUSET;
> > +		if (!ac.nodemask)
> > +			ac.nodemask = &cpuset_current_mems_allowed;
> > +	}
> 
> My initial reaction is that this is setting ac.nodemask in stone outside
> of cpuset_mems_cookie, but I guess it's ok since we're taking a pointer
> into current's task_struct, not the contents of the current's nodemask.
> It's however setting a non-NULL nodemask into stone, which means no
> zonelist iterator fasthpaths... but only in the slowpath. I guess it's
> not an issue then.
> 

You're right in that setting it in stone is problematic if the cpuset
nodemask changes duration allocation. The retry loop knows there is a
change but does not look it up which would loop once then potentially fail
unnecessarily. I should have moved the retry_cpuset label above the point
where cpuset_current_mems_allowed gets set. That's option 1 as a fixlet
to this patch.

> > +
> >   	gfp_mask &= gfp_allowed_mask;
> >   
> >   	lockdep_trace_alloc(gfp_mask);
> > @@ -3390,16 +3386,12 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
> >   retry_cpuset:
> >   	cpuset_mems_cookie = read_mems_allowed_begin();
> >   
> > -	/* We set it here, as __alloc_pages_slowpath might have changed it */
> > -	ac.zonelist = zonelist;
> 
> This doesn't seem relevant to the preferred_zoneref changes in
> __alloc_pages_slowpath, so why it became ok? Maybe it is, but it's not
> clear from the changelog.
> 

The slowpath is no longer altering the preferred_zoneref.

> Anyway, thinking about it made me realize that maybe we could move the
> whole mems_cookie thing into slowpath? As soon as the optimistic
> fastpath succeeds, we don't check the cookie anyway, so what about
> something like this on top?
> 

That in general would seem reasonable although I don't think it applies
to the series properly. Do you want to do this as a patch on top of the
series or will I use the fixlet for now and probably follow up with the
cookie move in a week or so when I've caught up after LSF/MM?

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1387374

On 04/26/2016 12:30 PM, Mel Gorman wrote:
> On Mon, Apr 25, 2016 at 04:50:18PM +0200, Vlastimil Babka wrote:
>> > @@ -3193,17 +3193,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>> >   	 */
>> >   	alloc_flags = gfp_to_alloc_flags(gfp_mask);
>> >
>> > -	/*
>> > -	 * Find the true preferred zone if the allocation is unconstrained by
>> > -	 * cpusets.
>> > -	 */
>> > -	if (!(alloc_flags & ALLOC_CPUSET) && !ac->nodemask) {
>> > -		struct zoneref *preferred_zoneref;
>> > -		preferred_zoneref = first_zones_zonelist(ac->zonelist,
>> > -				ac->high_zoneidx, NULL, &ac->preferred_zone);
>> > -		ac->classzone_idx = zonelist_zone_idx(preferred_zoneref);
>> > -	}
>> > -
>> >   	/* This is the last chance, in general, before the goto nopage. */
>> >   	page = get_page_from_freelist(gfp_mask, order,
>> >   				alloc_flags & ~ALLOC_NO_WATERMARKS, ac);
>> > @@ -3359,14 +3348,21 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
>> >   	struct zoneref *preferred_zoneref;
>> >   	struct page *page = NULL;
>> >   	unsigned int cpuset_mems_cookie;
>> > -	int alloc_flags = ALLOC_WMARK_LOW|ALLOC_CPUSET|ALLOC_FAIR;
>> > +	int alloc_flags = ALLOC_WMARK_LOW|ALLOC_FAIR;
>> >   	gfp_t alloc_mask; /* The gfp_t that was actually used for allocation */
>> >   	struct alloc_context ac = {
>> >   		.high_zoneidx = gfp_zone(gfp_mask),
>> > +		.zonelist = zonelist,
>> >   		.nodemask = nodemask,
>> >   		.migratetype = gfpflags_to_migratetype(gfp_mask),
>> >   	};
>> >
>> > +	if (cpusets_enabled()) {
>> > +		alloc_flags |= ALLOC_CPUSET;
>> > +		if (!ac.nodemask)
>> > +			ac.nodemask = &cpuset_current_mems_allowed;
>> > +	}
>>
>> My initial reaction is that this is setting ac.nodemask in stone outside
>> of cpuset_mems_cookie, but I guess it's ok since we're taking a pointer
>> into current's task_struct, not the contents of the current's nodemask.
>> It's however setting a non-NULL nodemask into stone, which means no
>> zonelist iterator fasthpaths... but only in the slowpath. I guess it's
>> not an issue then.
>>
>
> You're right in that setting it in stone is problematic if the cpuset
> nodemask changes duration allocation. The retry loop knows there is a
> change but does not look it up which would loop once then potentially fail
> unnecessarily.

That's what I thought first, but I think the *pointer* 
cpuset_current_mems_allowed itself doesn't change when cookie changes, only the 
bitmask it points to, so changes in that bitmask should be seen. But it deserves 
a comment maybe so people reading the code in future won't get the same suspicion.

> I should have moved the retry_cpuset label above the point
> where cpuset_current_mems_allowed gets set. That's option 1 as a fixlet
> to this patch.
>
>> > +
>> >   	gfp_mask &= gfp_allowed_mask;
>> >
>> >   	lockdep_trace_alloc(gfp_mask);
>> > @@ -3390,16 +3386,12 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
>> >   retry_cpuset:
>> >   	cpuset_mems_cookie = read_mems_allowed_begin();
>> >
>> > -	/* We set it here, as __alloc_pages_slowpath might have changed it */
>> > -	ac.zonelist = zonelist;
>>
>> This doesn't seem relevant to the preferred_zoneref changes in
>> __alloc_pages_slowpath, so why it became ok? Maybe it is, but it's not
>> clear from the changelog.
>>
>
> The slowpath is no longer altering the preferred_zoneref.

But the hunk above is about ac.zonelist, not preferred_zoneref?

>
>> Anyway, thinking about it made me realize that maybe we could move the
>> whole mems_cookie thing into slowpath? As soon as the optimistic
>> fastpath succeeds, we don't check the cookie anyway, so what about
>> something like this on top?
>>
>
> That in general would seem reasonable although I don't think it applies
> to the series properly. Do you want to do this as a patch on top of the
> series or will I use the fixlet for now and probably follow up with the
> cookie move in a week or so when I've caught up after LSF/MM?

I guess fixlet is fine for now and you have better setup to test the effect (if 
any) of the cookie move.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web