linux-kernel - Re: [PATCH] mm: Optimize put_mems

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 17 May 2012 13:16:10 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc:	Mel Gorman <mgorman@...e.de>, Miao Xie <miaox@...fujitsu.com>,
	David Rientjes <rientjes@...gle.com>,
	Christoph Lameter <cl@...ux.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, Pekka Enberg <penberg@...helsinki.fi>
Subject: Re: [PATCH] mm: Optimize put_mems_allowed() usage

On Tue, 27 Mar 2012 15:14:30 +0200
Peter Zijlstra <a.p.zijlstra@...llo.nl> wrote:

> Subject: mm: Optimize put_mems_allowed() usage
> From: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Date: Mon Mar 26 14:13:05 CEST 2012
> 
> Since put_mems_allowed() is strictly optional, its a seqcount retry,
> we don't need to evaluate the function if the allocation was in fact
> successful, saving a smp_rmb some loads and comparisons on some
> relative fast-paths.
> 
> Since the naming, get/put_mems_allowed() does suggest a mandatory
> pairing, rename the interface, as suggested by Mel, to resemble the
> seqcount interface.
> 
> This gives us: read_mems_allowed_begin() and
> read_mems_allowed_retry(), where it is important to note that the
> return value of the latter call is inverted from its previous
> incarnation.
> 
> ...
>
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1606,7 +1606,7 @@ static struct page *get_any_partial(stru
>  		return NULL;
>  
>  	do {
> -		cpuset_mems_cookie = get_mems_allowed();
> +		cpuset_mems_cookie = read_mems_allowed_begin();
>  		zonelist = node_zonelist(slab_node(current->mempolicy), flags);
>  		for_each_zone_zonelist(zone, z, zonelist, high_zoneidx) {
>  			struct kmem_cache_node *n;
> @@ -1616,21 +1616,11 @@ static struct page *get_any_partial(stru
>  			if (n && cpuset_zone_allowed_hardwall(zone, flags) &&
>  					n->nr_partial > s->min_partial) {
>  				object = get_partial_node(s, n, c);
> -				if (object) {
> -					/*
> -					 * Return the object even if
> -					 * put_mems_allowed indicated that
> -					 * the cpuset mems_allowed was
> -					 * updated in parallel. It's a
> -					 * harmless race between the alloc
> -					 * and the cpuset update.
> -					 */
> -					put_mems_allowed(cpuset_mems_cookie);
> +				if (object)
>  					return object;
> -				}
>  			}
>  		}
> -	} while (!put_mems_allowed(cpuset_mems_cookie));
> +	} while (read_mems_allowed_retry(cpuset_mems_cookie));

I do think it was a bad idea to remove that comment.  As it stands, the
reader will be wondering why we did the read_mems_allowed_begin() at
all, and whether failing to check for a change is a bug.

--- a/mm/slub.c~mm-optimize-put_mems_allowed-usage-fix
+++ a/mm/slub.c
@@ -1624,8 +1624,16 @@ static struct page *get_any_partial(stru
 			if (n && cpuset_zone_allowed_hardwall(zone, flags) &&
 					n->nr_partial > s->min_partial) {
 				object = get_partial_node(s, n, c);
-				if (object)
+				if (object) {
+					/*
+					 * Don't check read_mems_allowed_retry()
+					 * here - if mems_allowed was updated in
+					 * parallel, that was a harmless race
+					 * between allocation and the cpuset
+					 * update
+					 */
 					return object;
+				}
 			}
 		}
 	} while (read_mems_allowed_retry(cpuset_mems_cookie));
_

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/