public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Nick Piggin <npiggin@kernel.dk>
To: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Chris Mason <chris.mason@oracle.com>,
	Frank Rowand <frank.rowand@am.sony.com>,
	Ingo Molnar <mingo@elte.hu>, Thomas Gleixner <tglx@linutronix.de>,
	Mike Galbraith <efault@gmx.de>, Oleg Nesterov <oleg@redhat.com>,
	Paul Turner <pjt@google.com>, Jens Axboe <axboe@kernel.dk>,
	Yong Zhang <yong.zhang0@gmail.com>,
	linux-kernel@vger.kernel.org, Nick Piggin <npiggin@kernel.dk>,
	Jeremy Fitzhardinge <jeremy@goop.org>
Subject: Re: [RFC][PATCH 05/17] x86: Optimize arch_spin_unlock_wait()
Date: Tue, 4 Jan 2011 17:45:42 +1100	[thread overview]
Message-ID: <20110104064542.GF3402@amd> (raw)
In-Reply-To: <1294054362.2016.74.camel@laptop>

On Mon, Jan 03, 2011 at 12:32:42PM +0100, Peter Zijlstra wrote:
> On Fri, 2010-12-24 at 10:26 -0800, Linus Torvalds wrote:
> > On Fri, Dec 24, 2010 at 4:23 AM, Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > > Only wait for the current holder to release the lock.
> > >
> > > spin_unlock_wait() can only be about the current holder, since
> > > completion of this function is inherently racy with new contenders.
> > > Therefore, there is no reason to wait until the lock is completely
> > > unlocked.
> > 
> > Is there really any reason for this patch? I'd rather keep the simpler
> > and more straightforward code unless you have actual numbers.
> 
> No numbers, the testcase I use for this series is too unstable to really
> give that fine results. Its more a result of seeing the code an going:
> "oohh that can wait a long time when the lock is severely contended".

It would be kind of nice to fix, with ticket locks, dumb spin_unlock_wait
can infinitely starve if the lock queue is never empty, wheras at least
the simple spinlocks it would have a statistical chance of being given
the cacheline in unlocked state.

 
> But I think I can get rid of the need for calling this primitive
> alltogether, which is even better.

I always hated it because it seems hard to use right and verify result
is correct, particularly because it has no memory ordering guarantees.

assert(active == 1);
spin_lock(&blah);
if (should_die)
  active = 0;
counter++;
spin_unlock(&blah);

if (active) {
  spin_lock(&blah);
  /* do something */
  spin_unlock(&blah);
} else {
  /* wait for last to go away */
  spin_unlock_wait(&blah);
  counter++;
}

I don't know, stupid example but I can't really think of good ways to
use it off the top of my head.

Anyway this has a lost update problem even on x86 because counter can
be speculatively loaded out of order from the load of the lock word.
So the nice simple lock APIs which supposedly don't require any thought
of barriers have tricked us!

So I agree, taking it out the back and shooting it in the head would make
the world a better place.


  reply	other threads:[~2011-01-04  6:45 UTC|newest]

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-12-24 12:23 [RFC][PATCH 00/17] sched: Reduce runqueue lock contention -v3 Peter Zijlstra
2010-12-24 12:23 ` [RFC][PATCH 01/17] sched: Always provide p->on_cpu Peter Zijlstra
2010-12-24 12:23 ` [RFC][PATCH 02/17] mutex: Use p->on_cpu for the adaptive spin Peter Zijlstra
2010-12-24 12:23 ` [RFC][PATCH 03/17] sched: Change the ttwu success details Peter Zijlstra
2010-12-24 12:23 ` [RFC][PATCH 04/17] sched: Clean up ttwu stats Peter Zijlstra
2010-12-24 12:23 ` [RFC][PATCH 05/17] x86: Optimize arch_spin_unlock_wait() Peter Zijlstra
2010-12-24 18:26   ` Linus Torvalds
2011-01-03 11:32     ` Peter Zijlstra
2011-01-04  6:45       ` Nick Piggin [this message]
2011-01-05 19:14         ` [RFC][PATCH] spinlock: Kill spin_unlock_wait() Peter Zijlstra
2011-01-05 19:26           ` Oleg Nesterov
2011-01-05 19:43           ` Linus Torvalds
2011-01-06  9:32             ` Peter Zijlstra
2011-01-06 10:38               ` Nick Piggin
2011-01-06 18:26                 ` Peter Zijlstra
2011-01-07 21:01                   ` Tejun Heo
2011-01-07 21:13                     ` Jeff Garzik
2011-01-07 21:33                       ` Tejun Heo
2010-12-24 12:23 ` [RFC][PATCH 06/17] sched: Provide p->on_rq Peter Zijlstra
2010-12-29 14:14   ` Yong Zhang
2010-12-24 12:23 ` [RFC][PATCH 07/17] sched: Serialize p->cpus_allowed and ttwu() using p->pi_lock Peter Zijlstra
2010-12-29 14:20   ` Yong Zhang
2011-01-03 11:12     ` Peter Zijlstra
2010-12-24 12:23 ` [RFC][PATCH 08/17] sched: Drop the rq argument to sched_class::select_task_rq() Peter Zijlstra
2010-12-29 14:31   ` Yong Zhang
2011-01-03 11:16     ` Peter Zijlstra
2011-01-03 14:59       ` Oleg Nesterov
2011-01-03 15:21         ` Peter Zijlstra
2011-01-03 15:49           ` Oleg Nesterov
2011-01-03 16:35             ` Peter Zijlstra
2011-01-03 16:41               ` Peter Zijlstra
2011-01-04  7:27             ` Yong Zhang
2011-01-04 12:34               ` Peter Zijlstra
2011-01-04  5:59       ` Yong Zhang
2011-01-04 13:00         ` Peter Zijlstra
2011-01-03 18:05   ` Oleg Nesterov
2011-01-04 13:01     ` Peter Zijlstra
2010-12-24 12:23 ` [RFC][PATCH 09/17] sched: Remove rq argument to sched_class::task_waking() Peter Zijlstra
2010-12-24 12:23 ` [RFC][PATCH 10/17] sched: Add TASK_WAKING to task_rq_lock Peter Zijlstra
2010-12-24 12:23 ` [RFC][PATCH 11/17] sched: Delay task_contributes_to_load() Peter Zijlstra
2010-12-24 12:23 ` [RFC][PATCH 12/17] sched: Also serialize ttwu_local() with p->pi_lock Peter Zijlstra
2011-01-03 17:32   ` Oleg Nesterov
2011-01-09 23:11     ` Tejun Heo
2010-12-24 12:23 ` [RFC][PATCH 13/17] sched: Remove rq->lock from the first half of ttwu() Peter Zijlstra
2010-12-24 12:23 ` [RFC][PATCH 14/17] sched: Remove rq argument to ttwu_stat() Peter Zijlstra
2010-12-29 14:40   ` Yong Zhang
2011-01-03 11:20     ` Peter Zijlstra
2010-12-24 12:23 ` [RFC][PATCH 15/17] sched: Rename ttwu_post_activation Peter Zijlstra
2010-12-24 12:23 ` [RFC][PATCH 16/17] sched: Move the second half of ttwu() to the remote cpu Peter Zijlstra
2011-01-03 14:36   ` [RFC][PATCH] sembench: add stddev to the burn stats Peter Zijlstra
2011-01-04 14:28   ` [RFC][PATCH 16/17] sched: Move the second half of ttwu() to the remote cpu Oleg Nesterov
2011-01-04 14:47     ` Peter Zijlstra
2011-01-04 15:18       ` Oleg Nesterov
2011-01-04 15:43         ` Peter Zijlstra
2011-01-04 16:06           ` Oleg Nesterov
2010-12-24 12:23 ` [RFC][PATCH 17/17] sched: Sort hotplug vs ttwu queueing Peter Zijlstra
2010-12-29 14:51   ` Yong Zhang
2011-01-03 11:21     ` Peter Zijlstra
2010-12-24 13:15 ` [RFC][PATCH 00/17] sched: Reduce runqueue lock contention -v3 Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110104064542.GF3402@amd \
    --to=npiggin@kernel.dk \
    --cc=a.p.zijlstra@chello.nl \
    --cc=axboe@kernel.dk \
    --cc=chris.mason@oracle.com \
    --cc=efault@gmx.de \
    --cc=frank.rowand@am.sony.com \
    --cc=jeremy@goop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=oleg@redhat.com \
    --cc=pjt@google.com \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=yong.zhang0@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox