linux-kernel - Re: [PATCH 13/16] ksm: Replace smp_read_barrier_depends() with lockless

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] [day] [month] [year] [list]

Date:	Sun, 30 Nov 2014 14:34:51 -0800 (PST)
From:	Hugh Dickins <hughd@...gle.com>
To:	Pranith Kumar <bobby.prani@...il.com>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	Peter Zijlstra <peterz@...radead.org>,
	David Rientjes <rientjes@...gle.com>,
	Joerg Roedel <jroedel@...e.de>, NeilBrown <neilb@...e.de>,
	Sasha Levin <sasha.levin@...cle.com>,
	Paul McQuade <paulmcquad@...il.com>,
	"open list:MEMORY MANAGEMENT" <linux-mm@...ck.org>,
	open list <linux-kernel@...r.kernel.org>,
	paulmck@...ux.vnet.ibm.com
Subject: Re: [PATCH 13/16] ksm: Replace smp_read_barrier_depends() with
 lockless_dereference()

On Thu, 13 Nov 2014, Pranith Kumar wrote:

> Recently lockless_dereference() was added which can be used in place of
> hard-coding smp_read_barrier_depends(). The following PATCH makes the change.
> 
> Signed-off-by: Pranith Kumar <bobby.prani@...il.com>

Sorry, I don't think your patch is buggy, but I do think it
makes this tricky piece of code harder to follow, not easier.

It is certainly not a standard use of lockless_dereference() (kpfn
is not a pointer), and it both hides and moves where the barrier is.
And then at the end of the function, there's still explicit barriers
and ACCESS_ONCE comparison with kpfn, which this makes more obscure.

Unless you are actually fixing a bug (I don't pretend to have
tested this on Alpha, and I can get barriers wrong as we all do),
or smp_read_barrier_depends() is about to be withdrawn from use,
I'd rather say NAK to this patch.

Hugh

> ---
>  mm/ksm.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index d247efa..a67de79 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -542,15 +542,14 @@ static struct page *get_ksm_page(struct stable_node *stable_node, bool lock_it)
>  	expected_mapping = (void *)stable_node +
>  				(PAGE_MAPPING_ANON | PAGE_MAPPING_KSM);
>  again:
> -	kpfn = ACCESS_ONCE(stable_node->kpfn);
> -	page = pfn_to_page(kpfn);
> -
>  	/*
>  	 * page is computed from kpfn, so on most architectures reading
>  	 * page->mapping is naturally ordered after reading node->kpfn,
>  	 * but on Alpha we need to be more careful.
>  	 */
> -	smp_read_barrier_depends();
> +	kpfn = lockless_dereference(stable_node->kpfn);
> +	page = pfn_to_page(kpfn);
> +
>  	if (ACCESS_ONCE(page->mapping) != expected_mapping)
>  		goto stale;
>  
> -- 
> 1.9.1
--
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/