linux-kernel - Re: [PATCH v3 2/2] locking: Implement an algorithm choice for Wound-Wait mutexes

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:   Mon, 18 Jun 2018 13:35:06 +0200
From:   Thomas Hellstrom <thellstrom@...are.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Ingo Molnar <mingo@...hat.com>,
        Jonathan Corbet <corbet@....net>,
        Gustavo Padovan <gustavo@...ovan.org>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Sean Paul <seanpaul@...omium.org>,
        David Airlie <airlied@...ux.ie>,
        Davidlohr Bueso <dave@...olabs.net>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Josh Triplett <josh@...htriplett.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Kate Stewart <kstewart@...uxfoundation.org>,
        Philippe Ombredanne <pombredanne@...b.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-doc@...r.kernel.org, linux-media@...r.kernel.org,
        linaro-mm-sig@...ts.linaro.org, stern@...land.harvard.edu
Subject: Re: [PATCH v3 2/2] locking: Implement an algorithm choice for
 Wound-Wait mutexes

On 06/15/2018 06:46 PM, Peter Zijlstra wrote:
> On Fri, Jun 15, 2018 at 02:08:27PM +0200, Thomas Hellstrom wrote:
>
>> @@ -772,6 +856,25 @@ __ww_mutex_add_waiter(struct mutex_waiter *waiter,
>>   	}
>>   
>>   	list_add_tail(&waiter->list, pos);
>> +	if (__mutex_waiter_is_first(lock, waiter))
>> +		__mutex_set_flag(lock, MUTEX_FLAG_WAITERS);
>> +
>> +	/*
>> +	 * Wound-Wait: if we're blocking on a mutex owned by a younger context,
>> +	 * wound that such that we might proceed.
>> +	 */
>> +	if (!is_wait_die) {
>> +		struct ww_mutex *ww = container_of(lock, struct ww_mutex, base);
>> +
>> +		/*
>> +		 * See ww_mutex_set_context_fastpath(). Orders setting
>> +		 * MUTEX_FLAG_WAITERS (atomic operation) vs the ww->ctx load,
>> +		 * such that either we or the fastpath will wound @ww->ctx.
>> +		 */
>> +		smp_mb__after_atomic();
>> +
>> +		__ww_mutex_wound(lock, ww_ctx, ww->ctx);
>> +	}
> I think we want the smp_mb__after_atomic() in the same branch as
> __mutex_set_flag(). So something like:
>
> 	if (__mutex_waiter_is_first()) {
> 		__mutex_set_flag();
> 		if (!is_wait_die)
> 			smp_mb__after_atomic();
> 	}
>
> Or possibly even without the !is_wait_die. The rules for
> smp_mb__*_atomic() are such that we want it unconditional after an
> atomic, otherwise the semantics get too fuzzy.
>
> Alan (rightfully) complained about that a while ago when he was auditing
> users.
>
>
Hmm, yes that's understandable, although I must admit that when one of 
the accesses we want to order is actually an atomic this shouldn't 
really be causing much confusion.

But I'll think I'll change it back to an smp_mb() then. It's in a 
slowpath, and awkward constructs around smp_mb__after_atomic() might be 
causing grief in the future.

/Thomas