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: <20150801222903.GC25159@twins.programming.kicks-ass.net>
Date:	Sun, 2 Aug 2015 00:29:03 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Waiman Long <Waiman.Long@...com>
Cc:	Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
	linux-kernel@...r.kernel.org, Scott J Norton <scott.norton@...com>,
	Douglas Hatch <doug.hatch@...com>,
	Davidlohr Bueso <dave@...olabs.net>
Subject: Re: [PATCH v4 1/7] locking/pvqspinlock: Unconditional PV kick with
 _Q_SLOW_VAL

On Fri, Jul 31, 2015 at 10:21:58PM -0400, Waiman Long wrote:
> The smp_store_release() is not a full barrier. In order to avoid missed
> wakeup, we may need to add memory barrier around locked and cpu state
> variables adding to complexity. As the chance of spurious wakeup is very
> low, it is easier and safer to just do an unconditional kick at unlock
> time.
> 
> Signed-off-by: Waiman Long <Waiman.Long@...com>
> ---
>  kernel/locking/qspinlock_paravirt.h |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlock_paravirt.h
> index 15d3733..2dd4b39 100644
> --- a/kernel/locking/qspinlock_paravirt.h
> +++ b/kernel/locking/qspinlock_paravirt.h
> @@ -240,7 +240,6 @@ static void pv_wait_head(struct qspinlock *lock, struct mcs_spinlock *node)
>  			cpu_relax();
>  		}
>  
> -		WRITE_ONCE(pn->state, vcpu_halted);
>  		if (!lp) { /* ONCE */
>  			lp = pv_hash(lock, pn);
>  			/*
> @@ -320,9 +319,15 @@ __visible void __pv_queued_spin_unlock(struct qspinlock *lock)
>  	/*
>  	 * At this point the memory pointed at by lock can be freed/reused,
>  	 * however we can still use the pv_node to kick the CPU.
> +	 *
> +	 * As smp_store_release() is not a full barrier, adding a check to
> +	 * the node->state doesn't guarantee the checking is really done
> +	 * after clearing the lock byte

This is true, but _WHY_ is that a problem ?

                                         since they are in 2 separate
> +	 * cachelines and so hardware can reorder them. 

That's just gibberish, even in the same cacheline stuff can get
reordered.

                                                        So either we insert
> +	 * memory barrier here and in the corresponding pv_wait_head()
> +	 * function or we do an unconditional kick which is what is done here.

why, why why ? You've added words, but you've not actually described
what the problem is you're trying to fix.

AFAICT the only thing we really care about here is that the load in
question happens _after_ we observe SLOW, and that is still true.

The order against the unlock is irrelevant.

So we set ->state before we hash and before we set SLOW. Given that
we've seen SLOW, we must therefore also see ->state.

If ->state == halted, this means the CPU in question is blocked and the
pv_node will not get re-used -- if it does get re-used, it wasn't
blocked and we don't care either.

Therefore, ->cpu is stable and we'll kick it into action.

How do you end up not waking a waiting cpu? Explain that.

>  	 */
> -	if (READ_ONCE(node->state) == vcpu_halted)
> -		pv_kick(node->cpu);
> +	pv_kick(node->cpu);
>  }

Also, this patch clearly isn't against my tree.

--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ