linux-kernel - Re: [PATCH] MIPS: implement smp_cond_load

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]

Message-ID: <20180619071710.GB2494@hirez.programming.kicks-ass.net>
Date:   Tue, 19 Jun 2018 09:17:10 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Paul Burton <paul.burton@...s.com>
Cc:     Huacai Chen <chenhc@...ote.com>,
        Ralf Baechle <ralf@...ux-mips.org>,
        James Hogan <james.hogan@...s.com>, linux-mips@...ux-mips.org,
        Fuxin Zhang <zhangfx@...ote.com>,
        Zhangjin Wu <wuzhangjin@...il.com>,
        Huacai Chen <chenhuacai@...il.com>, stable@...r.kernel.org,
        Alan Stern <stern@...land.harvard.edu>,
        Andrea Parri <andrea.parri@...rulasolutions.com>,
        Will Deacon <will.deacon@....com>,
        Boqun Feng <boqun.feng@...il.com>,
        Nicholas Piggin <npiggin@...il.com>,
        David Howells <dhowells@...hat.com>,
        Jade Alglave <j.alglave@....ac.uk>,
        Luc Maranget <luc.maranget@...ia.fr>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Akira Yokosawa <akiyks@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] MIPS: implement smp_cond_load_acquire() for Loongson-3

On Mon, Jun 18, 2018 at 11:51:41AM -0700, Paul Burton wrote:
> Hi Huacai,
> 
> On Fri, Jun 15, 2018 at 02:07:38PM +0800, Huacai Chen wrote:
> > After commit 7f56b58a92aaf2c ("locking/mcs: Use smp_cond_load_acquire()
> > in MCS spin loop") Loongson-3 fails to boot. This is because Loongson-3
> > has SFB (Store Fill Buffer) and READ_ONCE() may get an old value in a
> > tight loop. So in smp_cond_load_acquire() we need a __smp_mb() after
> > every READ_ONCE().
> 
> Thanks - modifying smp_cond_load_acquire() is a step better than
> modifying arch_mcs_spin_lock_contended() to avoid it, but I'm still not
> sure we've reached the root of the problem. 

Agreed, this looks entirely dodgy.

> If tight loops using
> READ_ONCE() are at fault then what's special about
> smp_cond_load_acquire()? Could other such loops not hit the same
> problem?

Right again, Linux has a number of places where it relies on loops like
this.

	for (;;) {
		if (READ_ONCE(*ptr))
			break;

		cpu_relax();
	}

That is assumed to terminate -- provided the store to make *ptr != 0
happens of course.

And this has nothing to do with store buffers per se, sure store-buffers
might delay the store from being visible for a (little) while, but we
very much assume store buffers will not indefinitely hold on to data.

The proposed __smp_mb() doesn't make any kind of sense here. I presume
it's effect is to flush remote store buffers, but that is not something
LKMM allows for.

> Is the scenario you encounter the same as that outlined in the "DATA
> DEPENDENCY BARRIERS (HISTORICAL)" section of
> Documentation/memory-barriers.txt by any chance? If so then perhaps it
> would be better to implement smp_read_barrier_depends(), or just raw
> read_barrier_depends() depending upon how your SFB functions.

That doesn't make any sense, there is no actual data dependency here. We
load a single variable. Data dependencies are where you have (at least)
2 loads, where the second depends on the first, like:

	struct obj *obj = rcu_dereference(objp);
	int val = obj->val;

Here the load of val depends on the load of obj.

> Is there any public documentation describing the behaviour of the store
> fill buffer in Loongson-3?

I know of Store Buffers, but what exactly is a Store Fill Buffer? Better
would be to get a coherent memory model document on Loongson, because
this all smells horribly.

> Part of the problem is that I'm still not sure what's actually happening
> in your system - it would be helpful to have further information in the
> commit message about what actually happens. For example if you could
> walk us through an example of the problem step by step in the style of
> the diagrams you'll see in Documentation/memory-barriers.txt then I
> think that would help us to see what the best solution here is.
> 
> I've copied the LKMM maintainers in case they have further input.

Thanks, patches like proposed certainly require closer scrutiny and I
agree with you that this needs far better explanation.