Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1681942 > unrolled thread

[PATCH v2 0/9] Remove spin_unlock_wait()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 01:40 +0200
    [PATCH v2 9/9] arch: Remove spin_unlock_wait() arch-specific definitions "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 01:40 +0200
    RE: [PATCH v2 0/9] Remove spin_unlock_wait() David Laight <David.Laight@ACULAB.COM> - 2017-07-06 16:20 +0200
      Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 17:30 +0200
        Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-06 18:20 +0200
          Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 18:30 +0200
            Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-06 18:50 +0200
              Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 19:10 +0200
            Re: [PATCH v2 0/9] Remove spin_unlock_wait() Alan Stern <stern@rowland.harvard.edu> - 2017-07-06 18:50 +0200
              Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-06 19:00 +0200
                Re: [PATCH v2 0/9] Remove spin_unlock_wait() Alan Stern <stern@rowland.harvard.edu> - 2017-07-06 21:40 +0200
      Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-06 18:10 +0200
        Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 18:30 +0200
          Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-06 19:00 +0200
            Re: [PATCH v2 0/9] Remove spin_unlock_wait() Will Deacon <will.deacon@arm.com> - 2017-07-06 19:10 +0200
              Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 19:30 +0200
            Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-06 19:20 +0200
            Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-07 10:40 +0200
              Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-07 10:50 +0200
                Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-07 12:40 +0200
                  Re: [PATCH v2 0/9] Remove spin_unlock_wait() Peter Zijlstra <peterz@infradead.org> - 2017-07-07 13:30 +0200
              Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-07 16:50 +0200
                Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-08 10:50 +0200
                  Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-08 13:50 +0200
              Re: [PATCH v2 0/9] Remove spin_unlock_wait() Manfred Spraul <manfred@colorfullife.com> - 2017-07-07 19:50 +0200
                Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-08 10:40 +0200
                  Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-08 13:50 +0200
                    Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-08 14:40 +0200
                      Re: [PATCH v2 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-08 16:50 +0200
                      Re: [PATCH v2 0/9] Remove spin_unlock_wait() Alan Stern <stern@rowland.harvard.edu> - 2017-07-08 18:30 +0200
                        Re: [PATCH v2 0/9] Remove spin_unlock_wait() Manfred Spraul <manfred@colorfullife.com> - 2017-07-10 19:30 +0200
        Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-07 10:10 +0200
          Re: [PATCH v2 0/9] Remove spin_unlock_wait() Ingo Molnar <mingo@kernel.org> - 2017-07-07 11:40 +0200
    [PATCH v3 0/9] Remove spin_unlock_wait() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-07 21:30 +0200
      [PATCH v3 9/9] arch: Remove spin_unlock_wait() arch-specific definitions "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-07 21:40 +0200
      [PATCH v3 5/9] exit: Replace spin_unlock_wait() with lock/unlock pair "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-07 21:40 +0200
      [PATCH v3 8/9] locking: Remove spin_unlock_wait() generic definitions "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-07-07 21:40 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1683141

On Fri, Jul 07, 2017 at 12:33:49PM +0200, Ingo Molnar wrote:

>  [1997/04] v2.1.36:
> 
>       the spin_unlock_wait() primitive gets introduced as part of release()


Whee, that goes _way_ further back than I thought it did :-)

>  [2017/07] v4.12:
> 
>       wait_task_inactive() is still alive and kicking. Its poll loop has
>       increased in complexity, but it still does not use spin_unlock_wait()
> 

I've tried 'fixing' that wait_task_inactive() thing a number of times,
but always failed :/ That is very nasty code indeed.

[toc] | [prev] | [next] | [standalone]

#1683228

On Fri, Jul 07, 2017 at 10:31:28AM +0200, Ingo Molnar wrote:

[ . . . ]

> In fact I'd argue that any future high performance spin_unlock_wait() user is 
> probably better off open coding the unlock-wait poll loop (and possibly thinking 
> hard about eliminating it altogether). If such patterns pop up in the kernel we 
> can think about consolidating them into a single read-only primitive again.

I would like any reintroduction to include a header comment saying exactly
what the consolidated primitive actually does and does not do.  ;-)

> I.e. I think the proposed changes are doing no harm, and the unavailability of a 
> generic primitive does not hinder future optimizations either in any significant 
> fashion.

I will have a v3 with updated comments from Manfred.  Thoughts on when/where
to push this?

The reason I ask is if this does not go in during this merge window, I need
to fix the header comment on spin_unlock_wait().

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1683562

* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> On Fri, Jul 07, 2017 at 10:31:28AM +0200, Ingo Molnar wrote:
> 
> [ . . . ]
> 
> > In fact I'd argue that any future high performance spin_unlock_wait() user is 
> > probably better off open coding the unlock-wait poll loop (and possibly thinking 
> > hard about eliminating it altogether). If such patterns pop up in the kernel we 
> > can think about consolidating them into a single read-only primitive again.
> 
> I would like any reintroduction to include a header comment saying exactly
> what the consolidated primitive actually does and does not do.  ;-)
> 
> > I.e. I think the proposed changes are doing no harm, and the unavailability of a 
> > generic primitive does not hinder future optimizations either in any significant 
> > fashion.
> 
> I will have a v3 with updated comments from Manfred.  Thoughts on when/where
> to push this?

Once everyone agrees I can apply it to the locking tree. I think PeterZ's was the 
only objection?

> The reason I ask is if this does not go in during this merge window, I need
> to fix the header comment on spin_unlock_wait().

Can try it next week after some testing - let's see how busy things get for Linus 
in the merge window?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1683590

On Sat, Jul 08, 2017 at 10:43:24AM +0200, Ingo Molnar wrote:
> 
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > On Fri, Jul 07, 2017 at 10:31:28AM +0200, Ingo Molnar wrote:
> > 
> > [ . . . ]
> > 
> > > In fact I'd argue that any future high performance spin_unlock_wait() user is 
> > > probably better off open coding the unlock-wait poll loop (and possibly thinking 
> > > hard about eliminating it altogether). If such patterns pop up in the kernel we 
> > > can think about consolidating them into a single read-only primitive again.
> > 
> > I would like any reintroduction to include a header comment saying exactly
> > what the consolidated primitive actually does and does not do.  ;-)
> > 
> > > I.e. I think the proposed changes are doing no harm, and the unavailability of a 
> > > generic primitive does not hinder future optimizations either in any significant 
> > > fashion.
> > 
> > I will have a v3 with updated comments from Manfred.  Thoughts on when/where
> > to push this?
> 
> Once everyone agrees I can apply it to the locking tree. I think PeterZ's was the 
> only objection?

Oleg wasn't all that happy, either, but he did supply the relevant patch.

> > The reason I ask is if this does not go in during this merge window, I need
> > to fix the header comment on spin_unlock_wait().
> 
> Can try it next week after some testing - let's see how busy things get for Linus 
> in the merge window?

Sounds good!  Either way is fine with me.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1683346

Hi Ingo,

On 07/07/2017 10:31 AM, Ingo Molnar wrote:
>
> There's another, probably just as significant advantage: queued_spin_unlock_wait()
> is 'read-only', while spin_lock()+spin_unlock() dirties the lock cache line. On
> any bigger system this should make a very measurable difference - if
> spin_unlock_wait() is ever used in a performance critical code path.
At least for ipc/sem:
Dirtying the cacheline (in the slow path) allows to remove a smp_mb() in 
the hot path.
So for sem_lock(), I either need a primitive that dirties the cacheline 
or sem_lock() must continue to use spin_lock()/spin_unlock().

--
     Manfred

[toc] | [prev] | [next] | [standalone]

#1683560

* Manfred Spraul <manfred@colorfullife.com> wrote:

> Hi Ingo,
> 
> On 07/07/2017 10:31 AM, Ingo Molnar wrote:
> > 
> > There's another, probably just as significant advantage: queued_spin_unlock_wait()
> > is 'read-only', while spin_lock()+spin_unlock() dirties the lock cache line. On
> > any bigger system this should make a very measurable difference - if
> > spin_unlock_wait() is ever used in a performance critical code path.
> At least for ipc/sem:
> Dirtying the cacheline (in the slow path) allows to remove a smp_mb() in the
> hot path.
> So for sem_lock(), I either need a primitive that dirties the cacheline or
> sem_lock() must continue to use spin_lock()/spin_unlock().

Technically you could use spin_trylock()+spin_unlock() and avoid the lock acquire 
spinning on spin_unlock() and get very close to the slow path performance of a 
pure cacheline-dirtying behavior.

But adding something like spin_barrier(), which purely dirties the lock cacheline, 
would be even faster, right?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1683591

On Sat, Jul 08, 2017 at 10:35:43AM +0200, Ingo Molnar wrote:
> 
> * Manfred Spraul <manfred@colorfullife.com> wrote:
> 
> > Hi Ingo,
> > 
> > On 07/07/2017 10:31 AM, Ingo Molnar wrote:
> > > 
> > > There's another, probably just as significant advantage: queued_spin_unlock_wait()
> > > is 'read-only', while spin_lock()+spin_unlock() dirties the lock cache line. On
> > > any bigger system this should make a very measurable difference - if
> > > spin_unlock_wait() is ever used in a performance critical code path.
> > At least for ipc/sem:
> > Dirtying the cacheline (in the slow path) allows to remove a smp_mb() in the
> > hot path.
> > So for sem_lock(), I either need a primitive that dirties the cacheline or
> > sem_lock() must continue to use spin_lock()/spin_unlock().
> 
> Technically you could use spin_trylock()+spin_unlock() and avoid the lock acquire 
> spinning on spin_unlock() and get very close to the slow path performance of a 
> pure cacheline-dirtying behavior.
> 
> But adding something like spin_barrier(), which purely dirties the lock cacheline, 
> would be even faster, right?

Interestingly enough, the arm64 and powerpc implementations of
spin_unlock_wait() were very close to what it sounds like you are
describing.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1683599

* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:

> On Sat, Jul 08, 2017 at 10:35:43AM +0200, Ingo Molnar wrote:
> > 
> > * Manfred Spraul <manfred@colorfullife.com> wrote:
> > 
> > > Hi Ingo,
> > > 
> > > On 07/07/2017 10:31 AM, Ingo Molnar wrote:
> > > > 
> > > > There's another, probably just as significant advantage: queued_spin_unlock_wait()
> > > > is 'read-only', while spin_lock()+spin_unlock() dirties the lock cache line. On
> > > > any bigger system this should make a very measurable difference - if
> > > > spin_unlock_wait() is ever used in a performance critical code path.
> > > At least for ipc/sem:
> > > Dirtying the cacheline (in the slow path) allows to remove a smp_mb() in the
> > > hot path.
> > > So for sem_lock(), I either need a primitive that dirties the cacheline or
> > > sem_lock() must continue to use spin_lock()/spin_unlock().
> > 
> > Technically you could use spin_trylock()+spin_unlock() and avoid the lock acquire 
> > spinning on spin_unlock() and get very close to the slow path performance of a 
> > pure cacheline-dirtying behavior.
> > 
> > But adding something like spin_barrier(), which purely dirties the lock cacheline, 
> > would be even faster, right?
> 
> Interestingly enough, the arm64 and powerpc implementations of
> spin_unlock_wait() were very close to what it sounds like you are
> describing.

So could we perhaps solve all our problems by defining the generic version thusly:

void spin_unlock_wait(spinlock_t *lock)
{
	if (spin_trylock(lock))
		spin_unlock(lock);
}

... and perhaps rename it to spin_barrier() [or whatever proper name there would 
be]?

Architectures can still optimize it, to remove the small window where the lock is 
held locally - as long as the ordering is at least as strong as the generic 
version.

This would have various advantages:

 - semantics are well-defined

 - the generic implementation is already pretty well optimized (no spinning)

 - it would make it usable for the IPC performance optimization

 - architectures could still optimize it to eliminate the window where the lock is
   held locally - if there's such instructions available.

Was this proposed before, or am I missing something?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1683607

On Sat, Jul 08, 2017 at 02:30:19PM +0200, Ingo Molnar wrote:
> 
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > On Sat, Jul 08, 2017 at 10:35:43AM +0200, Ingo Molnar wrote:
> > > 
> > > * Manfred Spraul <manfred@colorfullife.com> wrote:
> > > 
> > > > Hi Ingo,
> > > > 
> > > > On 07/07/2017 10:31 AM, Ingo Molnar wrote:
> > > > > 
> > > > > There's another, probably just as significant advantage: queued_spin_unlock_wait()
> > > > > is 'read-only', while spin_lock()+spin_unlock() dirties the lock cache line. On
> > > > > any bigger system this should make a very measurable difference - if
> > > > > spin_unlock_wait() is ever used in a performance critical code path.
> > > > At least for ipc/sem:
> > > > Dirtying the cacheline (in the slow path) allows to remove a smp_mb() in the
> > > > hot path.
> > > > So for sem_lock(), I either need a primitive that dirties the cacheline or
> > > > sem_lock() must continue to use spin_lock()/spin_unlock().
> > > 
> > > Technically you could use spin_trylock()+spin_unlock() and avoid the lock acquire 
> > > spinning on spin_unlock() and get very close to the slow path performance of a 
> > > pure cacheline-dirtying behavior.
> > > 
> > > But adding something like spin_barrier(), which purely dirties the lock cacheline, 
> > > would be even faster, right?
> > 
> > Interestingly enough, the arm64 and powerpc implementations of
> > spin_unlock_wait() were very close to what it sounds like you are
> > describing.
> 
> So could we perhaps solve all our problems by defining the generic version thusly:
> 
> void spin_unlock_wait(spinlock_t *lock)
> {
> 	if (spin_trylock(lock))
> 		spin_unlock(lock);
> }
> 
> ... and perhaps rename it to spin_barrier() [or whatever proper name there would 
> be]?

As lockdep, 0day Test Robot, Linus Torvalds, and several others let me
know in response to my original (thankfully RFC!) patch series, this needs
to disable irqs to work in the general case.  For example, if the lock
in question is an irq-disabling lock, you take an interrupt just after
a successful spin_trylock(), and that interrupt acquires the same lock,
the actuarial statistics of your kernel degrade sharply and suddenly.

What I get for sending out untested patches!  :-/

> Architectures can still optimize it, to remove the small window where the lock is 
> held locally - as long as the ordering is at least as strong as the generic 
> version.
> 
> This would have various advantages:
> 
>  - semantics are well-defined
> 
>  - the generic implementation is already pretty well optimized (no spinning)
> 
>  - it would make it usable for the IPC performance optimization
> 
>  - architectures could still optimize it to eliminate the window where the lock is
>    held locally - if there's such instructions available.
> 
> Was this proposed before, or am I missing something?

It was sort of proposed...

https://marc.info/?l=linux-arch&m=149912878628355&w=2

But do we have a situation where normal usage of spin_lock() and
spin_unlock() is causing performance or scalability trouble?

(We do have at least one situation in fnic that appears to be buggy use of
spin_is_locked(), and proposing a patch for that case in on my todo list.)

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1683629

Pardon me for barging in, but I found this whole interchange extremely 
confusing...

On Sat, 8 Jul 2017, Ingo Molnar wrote:

> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > On Sat, Jul 08, 2017 at 10:35:43AM +0200, Ingo Molnar wrote:
> > > 
> > > * Manfred Spraul <manfred@colorfullife.com> wrote:
> > > 
> > > > Hi Ingo,
> > > > 
> > > > On 07/07/2017 10:31 AM, Ingo Molnar wrote:
> > > > > 
> > > > > There's another, probably just as significant advantage: queued_spin_unlock_wait()
> > > > > is 'read-only', while spin_lock()+spin_unlock() dirties the lock cache line. On
> > > > > any bigger system this should make a very measurable difference - if
> > > > > spin_unlock_wait() is ever used in a performance critical code path.
> > > > At least for ipc/sem:
> > > > Dirtying the cacheline (in the slow path) allows to remove a smp_mb() in the
> > > > hot path.
> > > > So for sem_lock(), I either need a primitive that dirties the cacheline or
> > > > sem_lock() must continue to use spin_lock()/spin_unlock().

This statement doesn't seem to make sense.  Did Manfred mean to write 
"smp_mb()" instead of "spin_lock()/spin_unlock()"?

> > > Technically you could use spin_trylock()+spin_unlock() and avoid the lock acquire 
> > > spinning on spin_unlock() and get very close to the slow path performance of a 
> > > pure cacheline-dirtying behavior.

This is even more confusing.  Did Ingo mean to suggest using 
"spin_trylock()+spin_unlock()" in place of "spin_lock()+spin_unlock()" 
could provide the desired ordering guarantee without delaying other 
CPUs that may try to acquire the lock?  That seems highly questionable.

> > > But adding something like spin_barrier(), which purely dirties the lock cacheline, 
> > > would be even faster, right?
> > 
> > Interestingly enough, the arm64 and powerpc implementations of
> > spin_unlock_wait() were very close to what it sounds like you are
> > describing.
> 
> So could we perhaps solve all our problems by defining the generic version thusly:
> 
> void spin_unlock_wait(spinlock_t *lock)
> {
> 	if (spin_trylock(lock))
> 		spin_unlock(lock);
> }

How could this possibly be a generic version of spin_unlock_wait()?  
It does nothing at all (with no ordering properties) if some other CPU
currently holds the lock, whereas the real spin_unlock_wait() would
wait until the other CPU released the lock (or possibly longer).

And if no other CPU currently holds the lock, this has exactly the same
performance properties as spin_lock()+spin_unlock(), so what's the
advantage?

Alan Stern

> ... and perhaps rename it to spin_barrier() [or whatever proper name there would 
> be]?
> 
> Architectures can still optimize it, to remove the small window where the lock is 
> held locally - as long as the ordering is at least as strong as the generic 
> version.
> 
> This would have various advantages:
> 
>  - semantics are well-defined
> 
>  - the generic implementation is already pretty well optimized (no spinning)
> 
>  - it would make it usable for the IPC performance optimization
> 
>  - architectures could still optimize it to eliminate the window where the lock is
>    held locally - if there's such instructions available.
> 
> Was this proposed before, or am I missing something?
> 
> Thanks,
> 
> 	Ingo

[toc] | [prev] | [next] | [standalone]

#1684517

Hi Alan,

On 07/08/2017 06:21 PM, Alan Stern wrote:
> Pardon me for barging in, but I found this whole interchange extremely
> confusing...
>
> On Sat, 8 Jul 2017, Ingo Molnar wrote:
>
>> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>>
>>> On Sat, Jul 08, 2017 at 10:35:43AM +0200, Ingo Molnar wrote:
>>>> * Manfred Spraul <manfred@colorfullife.com> wrote:
>>>>
>>>>> Hi Ingo,
>>>>>
>>>>> On 07/07/2017 10:31 AM, Ingo Molnar wrote:
>>>>>> There's another, probably just as significant advantage: queued_spin_unlock_wait()
>>>>>> is 'read-only', while spin_lock()+spin_unlock() dirties the lock cache line. On
>>>>>> any bigger system this should make a very measurable difference - if
>>>>>> spin_unlock_wait() is ever used in a performance critical code path.
>>>>> At least for ipc/sem:
>>>>> Dirtying the cacheline (in the slow path) allows to remove a smp_mb() in the
>>>>> hot path.
>>>>> So for sem_lock(), I either need a primitive that dirties the cacheline or
>>>>> sem_lock() must continue to use spin_lock()/spin_unlock().
> This statement doesn't seem to make sense.  Did Manfred mean to write
> "smp_mb()" instead of "spin_lock()/spin_unlock()"?
Option 1:
     fastpath:
         spin_lock(local_lock)
         smp_mb(); [[1]]
         smp_load_acquire(global_flag);
     slow path:
         global_flag = 1;
         smp_mb();
         <spin_unlock_wait_without_cacheline_dirtying>

Option 2:
     fastpath:
         spin_lock(local_lock);
         smp_load_acquire(global_flag)
     slow path:
         global_flag = 1;
         spin_lock(local_lock);spin_unlock(local_lock).

Rational:
The ACQUIRE from spin_lock is at the read of local_lock, not at the write.
i.e.: Without the smp_mb() at [[1]], the CPU can do:
         read local_lock;
         read global_flag;
         write local_lock;
For Option 2, the smp_mb() is not required, because fast path and slow 
path acquire the same lock.

>>>> Technically you could use spin_trylock()+spin_unlock() and avoid the lock acquire
>>>> spinning on spin_unlock() and get very close to the slow path performance of a
>>>> pure cacheline-dirtying behavior.
> This is even more confusing.  Did Ingo mean to suggest using
> "spin_trylock()+spin_unlock()" in place of "spin_lock()+spin_unlock()"
> could provide the desired ordering guarantee without delaying other
> CPUs that may try to acquire the lock?  That seems highly questionable.
I agree :-)

--
     Manfred

[toc] | [prev] | [next] | [standalone]

#1683004

* Peter Zijlstra <peterz@infradead.org> wrote:

> > It might even be that this is the defined semantics of spin_unlock_wait().
> 
> As is, spin_unlock_wait() is somewhat ill defined. IIRC it grew from an 
> optimization by Oleg and subsequently got used elsewhere. And it being the 
> subtle bugger it is, there were bugs.

I believe the historical, original spin_unlock_wait() came from early SMP 
optimizations of the networking code - and then spread elsewhere, step by step. 
All but one of the networking uses went away since then - so I don't think there's 
any original usecase left.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1683079

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > It might even be that this is the defined semantics of spin_unlock_wait().
> > 
> > As is, spin_unlock_wait() is somewhat ill defined. IIRC it grew from an 
> > optimization by Oleg and subsequently got used elsewhere. And it being the 
> > subtle bugger it is, there were bugs.
> 
> I believe the historical, original spin_unlock_wait() came from early SMP 
> optimizations of the networking code - and then spread elsewhere, step by step. 
> All but one of the networking uses went away since then - so I don't think there's 
> any original usecase left.

No - the original usecase was task teardown: I still remembered that but didn't 
find the commit - but it's there in very old Linux kernel patches, done by DaveM 
originally in v2.1.36 (!):

--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -136,6 +136,12 @@ void release(struct task_struct * p)
        }
        for (i=1 ; i<NR_TASKS ; i++)
                if (task[i] == p) {
+#ifdef __SMP__
+                       /* FIXME! Cheesy, but kills the window... -DaveM */
+                       while(p->processor != NO_PROC_ID)
+                               barrier();
+                       spin_unlock_wait(&scheduler_lock);
+#endif


Other code learned to use spin_unlock_wait(): the original version of 
[hard]irq_enter() was the second user, net_family_read_lock() was the third user, 
followed by more uses in networking. All but one of those are not present in the 
current upstream kernel anymore.

This task-teardown FIXME was fixed in v2.1.114 (was replaced by an open coded poll 
loop), but the spin_unlock_wait() primitive remained.

The rest is history! ;-)

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1683395 — [PATCH v3 0/9] Remove spin_unlock_wait()

Hello!

There is no agreed-upon definition of spin_unlock_wait()'s semantics,
and it appears that all callers could do just as well with a lock/unlock
pair.  This series therefore removes spin_unlock_wait() and changes
its users to instead use a lock/unlock pair.  The commits are as follows,
in three groups:

1-7.	Change uses of spin_unlock_wait() and raw_spin_unlock_wait()
	to instead use a spin_lock/spin_unlock pair.  These may be
	applied in any order, but must be applied before any later
	commits in this series.  The commit logs state why I believe
	that these commits won't noticeably degrade performance.

8.	Remove core-kernel definitions for spin_unlock_wait() and
	raw_spin_unlock_wait().

9.	Remove arch-specific definitions of arch_spin_unlock_wait().

Changes since v2:

o	Add comment-only update from Manfred.

Changes since v1:

o	Disable interrupts where needed, thus avoiding embarrassing
	interrupt-based self-deadlocks.

o	Substitute Manfred's patch for contrack_lock (#1 above).

o	Substitute Oleg's patch for task_work (#2 above).

o	Use more efficient barrier based on Arnd Bergmann feedback.

o	Merge the arch commits.

							Thanx, Paul

------------------------------------------------------------------------

 arch/alpha/include/asm/spinlock.h    |    5 -
 arch/arc/include/asm/spinlock.h      |    5 -
 arch/arm/include/asm/spinlock.h      |   16 ----
 arch/arm64/include/asm/spinlock.h    |   58 +----------------
 arch/blackfin/include/asm/spinlock.h |    5 -
 arch/hexagon/include/asm/spinlock.h  |    5 -
 arch/ia64/include/asm/spinlock.h     |   21 ------
 arch/m32r/include/asm/spinlock.h     |    5 -
 arch/metag/include/asm/spinlock.h    |    5 -
 arch/mips/include/asm/spinlock.h     |   16 ----
 arch/mn10300/include/asm/spinlock.h  |    5 -
 arch/parisc/include/asm/spinlock.h   |    7 --
 arch/powerpc/include/asm/spinlock.h  |   33 ---------
 arch/s390/include/asm/spinlock.h     |    7 --
 arch/sh/include/asm/spinlock-cas.h   |    5 -
 arch/sh/include/asm/spinlock-llsc.h  |    5 -
 arch/sparc/include/asm/spinlock_32.h |    5 -
 arch/sparc/include/asm/spinlock_64.h |    5 -
 arch/tile/include/asm/spinlock_32.h  |    2 
 arch/tile/include/asm/spinlock_64.h  |    2 
 arch/tile/lib/spinlock_32.c          |   23 ------
 arch/tile/lib/spinlock_64.c          |   22 ------
 arch/xtensa/include/asm/spinlock.h   |    5 -
 drivers/ata/libata-eh.c              |    8 --
 include/asm-generic/qspinlock.h      |   14 ----
 include/linux/spinlock.h             |   31 ---------
 include/linux/spinlock_up.h          |    6 -
 ipc/sem.c                            |    3 
 kernel/exit.c                        |    3 
 kernel/locking/qspinlock.c           |  117 -----------------------------------
 kernel/sched/completion.c            |    9 --
 kernel/sched/core.c                  |    5 -
 kernel/task_work.c                   |    8 --
 net/netfilter/nf_conntrack_core.c    |   52 ++++++++-------
 34 files changed, 48 insertions(+), 475 deletions(-)

[toc] | [prev] | [next] | [standalone]

#1683396 — [PATCH v3 9/9] arch: Remove spin_unlock_wait() arch-specific definitions

There is no agreed-upon definition of spin_unlock_wait()'s semantics,
and it appears that all callers could do just as well with a lock/unlock
pair.  This commit therefore removes the underlying arch-specific
arch_spin_unlock_wait() for all architectures providing them.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: <linux-arch@vger.kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Andrea Parri <parri.andrea@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Acked-by: Will Deacon <will.deacon@arm.com>
Acked-by: Boqun Feng <boqun.feng@gmail.com>
---
 arch/alpha/include/asm/spinlock.h    |  5 ----
 arch/arc/include/asm/spinlock.h      |  5 ----
 arch/arm/include/asm/spinlock.h      | 16 ----------
 arch/arm64/include/asm/spinlock.h    | 58 ++++--------------------------------
 arch/blackfin/include/asm/spinlock.h |  5 ----
 arch/hexagon/include/asm/spinlock.h  |  5 ----
 arch/ia64/include/asm/spinlock.h     | 21 -------------
 arch/m32r/include/asm/spinlock.h     |  5 ----
 arch/metag/include/asm/spinlock.h    |  5 ----
 arch/mips/include/asm/spinlock.h     | 16 ----------
 arch/mn10300/include/asm/spinlock.h  |  5 ----
 arch/parisc/include/asm/spinlock.h   |  7 -----
 arch/powerpc/include/asm/spinlock.h  | 33 --------------------
 arch/s390/include/asm/spinlock.h     |  7 -----
 arch/sh/include/asm/spinlock-cas.h   |  5 ----
 arch/sh/include/asm/spinlock-llsc.h  |  5 ----
 arch/sparc/include/asm/spinlock_32.h |  5 ----
 arch/sparc/include/asm/spinlock_64.h |  5 ----
 arch/tile/include/asm/spinlock_32.h  |  2 --
 arch/tile/include/asm/spinlock_64.h  |  2 --
 arch/tile/lib/spinlock_32.c          | 23 --------------
 arch/tile/lib/spinlock_64.c          | 22 --------------
 arch/xtensa/include/asm/spinlock.h   |  5 ----
 23 files changed, 5 insertions(+), 262 deletions(-)

diff --git a/arch/alpha/include/asm/spinlock.h b/arch/alpha/include/asm/spinlock.h
index a40b9fc0c6c3..718ac0b64adf 100644
--- a/arch/alpha/include/asm/spinlock.h
+++ b/arch/alpha/include/asm/spinlock.h
@@ -16,11 +16,6 @@
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 #define arch_spin_is_locked(x)	((x)->lock != 0)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, !VAL);
-}
-
 static inline int arch_spin_value_unlocked(arch_spinlock_t lock)
 {
         return lock.lock == 0;
diff --git a/arch/arc/include/asm/spinlock.h b/arch/arc/include/asm/spinlock.h
index 233d5ffe6ec7..a325e6a36523 100644
--- a/arch/arc/include/asm/spinlock.h
+++ b/arch/arc/include/asm/spinlock.h
@@ -16,11 +16,6 @@
 #define arch_spin_is_locked(x)	((x)->slock != __ARCH_SPIN_LOCK_UNLOCKED__)
 #define arch_spin_lock_flags(lock, flags)	arch_spin_lock(lock)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->slock, !VAL);
-}
-
 #ifdef CONFIG_ARC_HAS_LLSC
 
 static inline void arch_spin_lock(arch_spinlock_t *lock)
diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h
index 4bec45442072..c030143c18c6 100644
--- a/arch/arm/include/asm/spinlock.h
+++ b/arch/arm/include/asm/spinlock.h
@@ -52,22 +52,6 @@ static inline void dsb_sev(void)
  * memory.
  */
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	u16 owner = READ_ONCE(lock->tickets.owner);
-
-	for (;;) {
-		arch_spinlock_t tmp = READ_ONCE(*lock);
-
-		if (tmp.tickets.owner == tmp.tickets.next ||
-		    tmp.tickets.owner != owner)
-			break;
-
-		wfe();
-	}
-	smp_acquire__after_ctrl_dep();
-}
-
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
 static inline void arch_spin_lock(arch_spinlock_t *lock)
diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
index cae331d553f8..f445bd7f2b9f 100644
--- a/arch/arm64/include/asm/spinlock.h
+++ b/arch/arm64/include/asm/spinlock.h
@@ -26,58 +26,6 @@
  * The memory barriers are implicit with the load-acquire and store-release
  * instructions.
  */
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	unsigned int tmp;
-	arch_spinlock_t lockval;
-	u32 owner;
-
-	/*
-	 * Ensure prior spin_lock operations to other locks have completed
-	 * on this CPU before we test whether "lock" is locked.
-	 */
-	smp_mb();
-	owner = READ_ONCE(lock->owner) << 16;
-
-	asm volatile(
-"	sevl\n"
-"1:	wfe\n"
-"2:	ldaxr	%w0, %2\n"
-	/* Is the lock free? */
-"	eor	%w1, %w0, %w0, ror #16\n"
-"	cbz	%w1, 3f\n"
-	/* Lock taken -- has there been a subsequent unlock->lock transition? */
-"	eor	%w1, %w3, %w0, lsl #16\n"
-"	cbz	%w1, 1b\n"
-	/*
-	 * The owner has been updated, so there was an unlock->lock
-	 * transition that we missed. That means we can rely on the
-	 * store-release of the unlock operation paired with the
-	 * load-acquire of the lock operation to publish any of our
-	 * previous stores to the new lock owner and therefore don't
-	 * need to bother with the writeback below.
-	 */
-"	b	4f\n"
-"3:\n"
-	/*
-	 * Serialise against any concurrent lockers by writing back the
-	 * unlocked lock value
-	 */
-	ARM64_LSE_ATOMIC_INSN(
-	/* LL/SC */
-"	stxr	%w1, %w0, %2\n"
-	__nops(2),
-	/* LSE atomics */
-"	mov	%w1, %w0\n"
-"	cas	%w0, %w0, %2\n"
-"	eor	%w1, %w1, %w0\n")
-	/* Somebody else wrote to the lock, GOTO 10 and reload the value */
-"	cbnz	%w1, 2b\n"
-"4:"
-	: "=&r" (lockval), "=&r" (tmp), "+Q" (*lock)
-	: "r" (owner)
-	: "memory");
-}
 
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
@@ -176,7 +124,11 @@ static inline int arch_spin_value_unlocked(arch_spinlock_t lock)
 
 static inline int arch_spin_is_locked(arch_spinlock_t *lock)
 {
-	smp_mb(); /* See arch_spin_unlock_wait */
+	/*
+	 * Ensure prior spin_lock operations to other locks have completed
+	 * on this CPU before we test whether "lock" is locked.
+	 */
+	smp_mb(); /* ^^^ */
 	return !arch_spin_value_unlocked(READ_ONCE(*lock));
 }
 
diff --git a/arch/blackfin/include/asm/spinlock.h b/arch/blackfin/include/asm/spinlock.h
index c58f4a83ed6f..f6431439d15d 100644
--- a/arch/blackfin/include/asm/spinlock.h
+++ b/arch/blackfin/include/asm/spinlock.h
@@ -48,11 +48,6 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock)
 	__raw_spin_unlock_asm(&lock->lock);
 }
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, !VAL);
-}
-
 static inline int arch_read_can_lock(arch_rwlock_t *rw)
 {
 	return __raw_uncached_fetch_asm(&rw->lock) > 0;
diff --git a/arch/hexagon/include/asm/spinlock.h b/arch/hexagon/include/asm/spinlock.h
index a1c55788c5d6..53a8d5885887 100644
--- a/arch/hexagon/include/asm/spinlock.h
+++ b/arch/hexagon/include/asm/spinlock.h
@@ -179,11 +179,6 @@ static inline unsigned int arch_spin_trylock(arch_spinlock_t *lock)
  */
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, !VAL);
-}
-
 #define arch_spin_is_locked(x) ((x)->lock != 0)
 
 #define arch_read_lock_flags(lock, flags) arch_read_lock(lock)
diff --git a/arch/ia64/include/asm/spinlock.h b/arch/ia64/include/asm/spinlock.h
index ca9e76149a4a..df2c121164b8 100644
--- a/arch/ia64/include/asm/spinlock.h
+++ b/arch/ia64/include/asm/spinlock.h
@@ -76,22 +76,6 @@ static __always_inline void __ticket_spin_unlock(arch_spinlock_t *lock)
 	ACCESS_ONCE(*p) = (tmp + 2) & ~1;
 }
 
-static __always_inline void __ticket_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	int	*p = (int *)&lock->lock, ticket;
-
-	ia64_invala();
-
-	for (;;) {
-		asm volatile ("ld4.c.nc %0=[%1]" : "=r"(ticket) : "r"(p) : "memory");
-		if (!(((ticket >> TICKET_SHIFT) ^ ticket) & TICKET_MASK))
-			return;
-		cpu_relax();
-	}
-
-	smp_acquire__after_ctrl_dep();
-}
-
 static inline int __ticket_spin_is_locked(arch_spinlock_t *lock)
 {
 	long tmp = ACCESS_ONCE(lock->lock);
@@ -143,11 +127,6 @@ static __always_inline void arch_spin_lock_flags(arch_spinlock_t *lock,
 	arch_spin_lock(lock);
 }
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	__ticket_spin_unlock_wait(lock);
-}
-
 #define arch_read_can_lock(rw)		(*(volatile int *)(rw) >= 0)
 #define arch_write_can_lock(rw)	(*(volatile int *)(rw) == 0)
 
diff --git a/arch/m32r/include/asm/spinlock.h b/arch/m32r/include/asm/spinlock.h
index 323c7fc953cd..a56825592b90 100644
--- a/arch/m32r/include/asm/spinlock.h
+++ b/arch/m32r/include/asm/spinlock.h
@@ -30,11 +30,6 @@
 #define arch_spin_is_locked(x)		(*(volatile int *)(&(x)->slock) <= 0)
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->slock, VAL > 0);
-}
-
 /**
  * arch_spin_trylock - Try spin lock and return a result
  * @lock: Pointer to the lock variable
diff --git a/arch/metag/include/asm/spinlock.h b/arch/metag/include/asm/spinlock.h
index c0c7a22be1ae..ddf7fe5708a6 100644
--- a/arch/metag/include/asm/spinlock.h
+++ b/arch/metag/include/asm/spinlock.h
@@ -15,11 +15,6 @@
  * locked.
  */
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, !VAL);
-}
-
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
 #define	arch_read_lock_flags(lock, flags) arch_read_lock(lock)
diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h
index a8df44d60607..81b4945031ee 100644
--- a/arch/mips/include/asm/spinlock.h
+++ b/arch/mips/include/asm/spinlock.h
@@ -50,22 +50,6 @@ static inline int arch_spin_value_unlocked(arch_spinlock_t lock)
 
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	u16 owner = READ_ONCE(lock->h.serving_now);
-	smp_rmb();
-	for (;;) {
-		arch_spinlock_t tmp = READ_ONCE(*lock);
-
-		if (tmp.h.serving_now == tmp.h.ticket ||
-		    tmp.h.serving_now != owner)
-			break;
-
-		cpu_relax();
-	}
-	smp_acquire__after_ctrl_dep();
-}
-
 static inline int arch_spin_is_contended(arch_spinlock_t *lock)
 {
 	u32 counters = ACCESS_ONCE(lock->lock);
diff --git a/arch/mn10300/include/asm/spinlock.h b/arch/mn10300/include/asm/spinlock.h
index 9c7b8f7942d8..fe413b41df6c 100644
--- a/arch/mn10300/include/asm/spinlock.h
+++ b/arch/mn10300/include/asm/spinlock.h
@@ -26,11 +26,6 @@
 
 #define arch_spin_is_locked(x)	(*(volatile signed char *)(&(x)->slock) != 0)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->slock, !VAL);
-}
-
 static inline void arch_spin_unlock(arch_spinlock_t *lock)
 {
 	asm volatile(
diff --git a/arch/parisc/include/asm/spinlock.h b/arch/parisc/include/asm/spinlock.h
index e32936cd7f10..55bfe4affca3 100644
--- a/arch/parisc/include/asm/spinlock.h
+++ b/arch/parisc/include/asm/spinlock.h
@@ -14,13 +14,6 @@ static inline int arch_spin_is_locked(arch_spinlock_t *x)
 
 #define arch_spin_lock(lock) arch_spin_lock_flags(lock, 0)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *x)
-{
-	volatile unsigned int *a = __ldcw_align(x);
-
-	smp_cond_load_acquire(a, VAL);
-}
-
 static inline void arch_spin_lock_flags(arch_spinlock_t *x,
 					 unsigned long flags)
 {
diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
index 8c1b913de6d7..d256e448ea49 100644
--- a/arch/powerpc/include/asm/spinlock.h
+++ b/arch/powerpc/include/asm/spinlock.h
@@ -170,39 +170,6 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock)
 	lock->slock = 0;
 }
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	arch_spinlock_t lock_val;
-
-	smp_mb();
-
-	/*
-	 * Atomically load and store back the lock value (unchanged). This
-	 * ensures that our observation of the lock value is ordered with
-	 * respect to other lock operations.
-	 */
-	__asm__ __volatile__(
-"1:	" PPC_LWARX(%0, 0, %2, 0) "\n"
-"	stwcx. %0, 0, %2\n"
-"	bne- 1b\n"
-	: "=&r" (lock_val), "+m" (*lock)
-	: "r" (lock)
-	: "cr0", "xer");
-
-	if (arch_spin_value_unlocked(lock_val))
-		goto out;
-
-	while (lock->slock) {
-		HMT_low();
-		if (SHARED_PROCESSOR)
-			__spin_yield(lock);
-	}
-	HMT_medium();
-
-out:
-	smp_mb();
-}
-
 /*
  * Read-write spinlocks, allowing multiple readers
  * but only one writer.
diff --git a/arch/s390/include/asm/spinlock.h b/arch/s390/include/asm/spinlock.h
index f7838ecd83c6..217ee5210c32 100644
--- a/arch/s390/include/asm/spinlock.h
+++ b/arch/s390/include/asm/spinlock.h
@@ -98,13 +98,6 @@ static inline void arch_spin_unlock(arch_spinlock_t *lp)
 		: "cc", "memory");
 }
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	while (arch_spin_is_locked(lock))
-		arch_spin_relax(lock);
-	smp_acquire__after_ctrl_dep();
-}
-
 /*
  * Read-write spinlocks, allowing multiple readers
  * but only one writer.
diff --git a/arch/sh/include/asm/spinlock-cas.h b/arch/sh/include/asm/spinlock-cas.h
index c46e8cc7b515..5ed7dbbd94ff 100644
--- a/arch/sh/include/asm/spinlock-cas.h
+++ b/arch/sh/include/asm/spinlock-cas.h
@@ -29,11 +29,6 @@ static inline unsigned __sl_cas(volatile unsigned *p, unsigned old, unsigned new
 #define arch_spin_is_locked(x)		((x)->lock <= 0)
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, VAL > 0);
-}
-
 static inline void arch_spin_lock(arch_spinlock_t *lock)
 {
 	while (!__sl_cas(&lock->lock, 1, 0));
diff --git a/arch/sh/include/asm/spinlock-llsc.h b/arch/sh/include/asm/spinlock-llsc.h
index cec78143fa83..f77263aae760 100644
--- a/arch/sh/include/asm/spinlock-llsc.h
+++ b/arch/sh/include/asm/spinlock-llsc.h
@@ -21,11 +21,6 @@
 #define arch_spin_is_locked(x)		((x)->lock <= 0)
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, VAL > 0);
-}
-
 /*
  * Simple spin lock operations.  There are two variants, one clears IRQ's
  * on the local processor, one does not.
diff --git a/arch/sparc/include/asm/spinlock_32.h b/arch/sparc/include/asm/spinlock_32.h
index 8011e79f59c9..67345b2dc408 100644
--- a/arch/sparc/include/asm/spinlock_32.h
+++ b/arch/sparc/include/asm/spinlock_32.h
@@ -14,11 +14,6 @@
 
 #define arch_spin_is_locked(lock) (*((volatile unsigned char *)(lock)) != 0)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, !VAL);
-}
-
 static inline void arch_spin_lock(arch_spinlock_t *lock)
 {
 	__asm__ __volatile__(
diff --git a/arch/sparc/include/asm/spinlock_64.h b/arch/sparc/include/asm/spinlock_64.h
index 07c9f2e9bf57..923d57f9b79d 100644
--- a/arch/sparc/include/asm/spinlock_64.h
+++ b/arch/sparc/include/asm/spinlock_64.h
@@ -26,11 +26,6 @@
 
 #define arch_spin_is_locked(lp)	((lp)->lock != 0)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->lock, !VAL);
-}
-
 static inline void arch_spin_lock(arch_spinlock_t *lock)
 {
 	unsigned long tmp;
diff --git a/arch/tile/include/asm/spinlock_32.h b/arch/tile/include/asm/spinlock_32.h
index b14b1ba5bf9c..cba8ba9b8da6 100644
--- a/arch/tile/include/asm/spinlock_32.h
+++ b/arch/tile/include/asm/spinlock_32.h
@@ -64,8 +64,6 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock)
 	lock->current_ticket = old_ticket + TICKET_QUANTUM;
 }
 
-void arch_spin_unlock_wait(arch_spinlock_t *lock);
-
 /*
  * Read-write spinlocks, allowing multiple readers
  * but only one writer.
diff --git a/arch/tile/include/asm/spinlock_64.h b/arch/tile/include/asm/spinlock_64.h
index b9718fb4e74a..9a2c2d605752 100644
--- a/arch/tile/include/asm/spinlock_64.h
+++ b/arch/tile/include/asm/spinlock_64.h
@@ -58,8 +58,6 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock)
 	__insn_fetchadd4(&lock->lock, 1U << __ARCH_SPIN_CURRENT_SHIFT);
 }
 
-void arch_spin_unlock_wait(arch_spinlock_t *lock);
-
 void arch_spin_lock_slow(arch_spinlock_t *lock, u32 val);
 
 /* Grab the "next" ticket number and bump it atomically.
diff --git a/arch/tile/lib/spinlock_32.c b/arch/tile/lib/spinlock_32.c
index 076c6cc43113..db9333f2447c 100644
--- a/arch/tile/lib/spinlock_32.c
+++ b/arch/tile/lib/spinlock_32.c
@@ -62,29 +62,6 @@ int arch_spin_trylock(arch_spinlock_t *lock)
 }
 EXPORT_SYMBOL(arch_spin_trylock);
 
-void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	u32 iterations = 0;
-	int curr = READ_ONCE(lock->current_ticket);
-	int next = READ_ONCE(lock->next_ticket);
-
-	/* Return immediately if unlocked. */
-	if (next == curr)
-		return;
-
-	/* Wait until the current locker has released the lock. */
-	do {
-		delay_backoff(iterations++);
-	} while (READ_ONCE(lock->current_ticket) == curr);
-
-	/*
-	 * The TILE architecture doesn't do read speculation; therefore
-	 * a control dependency guarantees a LOAD->{LOAD,STORE} order.
-	 */
-	barrier();
-}
-EXPORT_SYMBOL(arch_spin_unlock_wait);
-
 /*
  * The low byte is always reserved to be the marker for a "tns" operation
  * since the low bit is set to "1" by a tns.  The next seven bits are
diff --git a/arch/tile/lib/spinlock_64.c b/arch/tile/lib/spinlock_64.c
index a4b5b2cbce93..de414c22892f 100644
--- a/arch/tile/lib/spinlock_64.c
+++ b/arch/tile/lib/spinlock_64.c
@@ -62,28 +62,6 @@ int arch_spin_trylock(arch_spinlock_t *lock)
 }
 EXPORT_SYMBOL(arch_spin_trylock);
 
-void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	u32 iterations = 0;
-	u32 val = READ_ONCE(lock->lock);
-	u32 curr = arch_spin_current(val);
-
-	/* Return immediately if unlocked. */
-	if (arch_spin_next(val) == curr)
-		return;
-
-	/* Wait until the current locker has released the lock. */
-	do {
-		delay_backoff(iterations++);
-	} while (arch_spin_current(READ_ONCE(lock->lock)) == curr);
-
-	/*
-	 * The TILE architecture doesn't do read speculation; therefore
-	 * a control dependency guarantees a LOAD->{LOAD,STORE} order.
-	 */
-	barrier();
-}
-EXPORT_SYMBOL(arch_spin_unlock_wait);
 
 /*
  * If the read lock fails due to a writer, we retry periodically
diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h
index a36221cf6363..3bb49681ee24 100644
--- a/arch/xtensa/include/asm/spinlock.h
+++ b/arch/xtensa/include/asm/spinlock.h
@@ -33,11 +33,6 @@
 
 #define arch_spin_is_locked(x) ((x)->slock != 0)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->slock, !VAL);
-}
-
 #define arch_spin_lock_flags(lock, flags) arch_spin_lock(lock)
 
 static inline void arch_spin_lock(arch_spinlock_t *lock)
-- 
2.5.2

[toc] | [prev] | [next] | [standalone]

#1683398 — [PATCH v3 5/9] exit: Replace spin_unlock_wait() with lock/unlock pair

There is no agreed-upon definition of spin_unlock_wait()'s semantics, and
it appears that all callers could do just as well with a lock/unlock pair.
This commit therefore replaces the spin_unlock_wait() call in do_exit()
with spin_lock() followed immediately by spin_unlock().  This should be
safe from a performance perspective because the lock is a per-task lock,
and this is happening only at task-exit time.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Andrea Parri <parri.andrea@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
 kernel/exit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index 516acdb0e0ec..6d19c9090d43 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -832,7 +832,8 @@ void __noreturn do_exit(long code)
 	 * Ensure that we must observe the pi_state in exit_mm() ->
 	 * mm_release() -> exit_pi_state_list().
 	 */
-	raw_spin_unlock_wait(&tsk->pi_lock);
+	raw_spin_lock_irq(&tsk->pi_lock);
+	raw_spin_unlock_irq(&tsk->pi_lock);
 
 	if (unlikely(in_atomic())) {
 		pr_info("note: %s[%d] exited with preempt_count %d\n",
-- 
2.5.2

[toc] | [prev] | [next] | [standalone]

#1683399 — [PATCH v3 8/9] locking: Remove spin_unlock_wait() generic definitions

There is no agreed-upon definition of spin_unlock_wait()'s semantics,
and it appears that all callers could do just as well with a lock/unlock
pair.  This commit therefore removes spin_unlock_wait() and related
definitions from core code.

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Andrea Parri <parri.andrea@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
---
 include/asm-generic/qspinlock.h |  14 -----
 include/linux/spinlock.h        |  31 -----------
 include/linux/spinlock_up.h     |   6 ---
 kernel/locking/qspinlock.c      | 117 ----------------------------------------
 4 files changed, 168 deletions(-)

diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
index 9f0681bf1e87..66260777d644 100644
--- a/include/asm-generic/qspinlock.h
+++ b/include/asm-generic/qspinlock.h
@@ -22,17 +22,6 @@
 #include <asm-generic/qspinlock_types.h>
 
 /**
- * queued_spin_unlock_wait - wait until the _current_ lock holder releases the lock
- * @lock : Pointer to queued spinlock structure
- *
- * There is a very slight possibility of live-lock if the lockers keep coming
- * and the waiter is just unfortunate enough to not see any unlock state.
- */
-#ifndef queued_spin_unlock_wait
-extern void queued_spin_unlock_wait(struct qspinlock *lock);
-#endif
-
-/**
  * queued_spin_is_locked - is the spinlock locked?
  * @lock: Pointer to queued spinlock structure
  * Return: 1 if it is locked, 0 otherwise
@@ -41,8 +30,6 @@ extern void queued_spin_unlock_wait(struct qspinlock *lock);
 static __always_inline int queued_spin_is_locked(struct qspinlock *lock)
 {
 	/*
-	 * See queued_spin_unlock_wait().
-	 *
 	 * Any !0 state indicates it is locked, even if _Q_LOCKED_VAL
 	 * isn't immediately observable.
 	 */
@@ -135,6 +122,5 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
 #define arch_spin_trylock(l)		queued_spin_trylock(l)
 #define arch_spin_unlock(l)		queued_spin_unlock(l)
 #define arch_spin_lock_flags(l, f)	queued_spin_lock(l)
-#define arch_spin_unlock_wait(l)	queued_spin_unlock_wait(l)
 
 #endif /* __ASM_GENERIC_QSPINLOCK_H */
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index d9510e8522d4..ef018a6e4985 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -130,12 +130,6 @@ do {								\
 #define smp_mb__before_spinlock()	smp_wmb()
 #endif
 
-/**
- * raw_spin_unlock_wait - wait until the spinlock gets unlocked
- * @lock: the spinlock in question.
- */
-#define raw_spin_unlock_wait(lock)	arch_spin_unlock_wait(&(lock)->raw_lock)
-
 #ifdef CONFIG_DEBUG_SPINLOCK
  extern void do_raw_spin_lock(raw_spinlock_t *lock) __acquires(lock);
 #define do_raw_spin_lock_flags(lock, flags) do_raw_spin_lock(lock)
@@ -369,31 +363,6 @@ static __always_inline int spin_trylock_irq(spinlock_t *lock)
 	raw_spin_trylock_irqsave(spinlock_check(lock), flags); \
 })
 
-/**
- * spin_unlock_wait - Interpose between successive critical sections
- * @lock: the spinlock whose critical sections are to be interposed.
- *
- * Semantically this is equivalent to a spin_lock() immediately
- * followed by a spin_unlock().  However, most architectures have
- * more efficient implementations in which the spin_unlock_wait()
- * cannot block concurrent lock acquisition, and in some cases
- * where spin_unlock_wait() does not write to the lock variable.
- * Nevertheless, spin_unlock_wait() can have high overhead, so if
- * you feel the need to use it, please check to see if there is
- * a better way to get your job done.
- *
- * The ordering guarantees provided by spin_unlock_wait() are:
- *
- * 1.  All accesses preceding the spin_unlock_wait() happen before
- *     any accesses in later critical sections for this same lock.
- * 2.  All accesses following the spin_unlock_wait() happen after
- *     any accesses in earlier critical sections for this same lock.
- */
-static __always_inline void spin_unlock_wait(spinlock_t *lock)
-{
-	raw_spin_unlock_wait(&lock->rlock);
-}
-
 static __always_inline int spin_is_locked(spinlock_t *lock)
 {
 	return raw_spin_is_locked(&lock->rlock);
diff --git a/include/linux/spinlock_up.h b/include/linux/spinlock_up.h
index 0d9848de677d..612fb530af41 100644
--- a/include/linux/spinlock_up.h
+++ b/include/linux/spinlock_up.h
@@ -26,11 +26,6 @@
 #ifdef CONFIG_DEBUG_SPINLOCK
 #define arch_spin_is_locked(x)		((x)->slock == 0)
 
-static inline void arch_spin_unlock_wait(arch_spinlock_t *lock)
-{
-	smp_cond_load_acquire(&lock->slock, VAL);
-}
-
 static inline void arch_spin_lock(arch_spinlock_t *lock)
 {
 	lock->slock = 0;
@@ -73,7 +68,6 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock)
 
 #else /* DEBUG_SPINLOCK */
 #define arch_spin_is_locked(lock)	((void)(lock), 0)
-#define arch_spin_unlock_wait(lock)	do { barrier(); (void)(lock); } while (0)
 /* for sched/core.c and kernel_lock.c: */
 # define arch_spin_lock(lock)		do { barrier(); (void)(lock); } while (0)
 # define arch_spin_lock_flags(lock, flags)	do { barrier(); (void)(lock); } while (0)
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index b2caec7315af..64a9051e4c2c 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -267,123 +267,6 @@ static __always_inline u32  __pv_wait_head_or_lock(struct qspinlock *lock,
 #define queued_spin_lock_slowpath	native_queued_spin_lock_slowpath
 #endif
 
-/*
- * Various notes on spin_is_locked() and spin_unlock_wait(), which are
- * 'interesting' functions:
- *
- * PROBLEM: some architectures have an interesting issue with atomic ACQUIRE
- * operations in that the ACQUIRE applies to the LOAD _not_ the STORE (ARM64,
- * PPC). Also qspinlock has a similar issue per construction, the setting of
- * the locked byte can be unordered acquiring the lock proper.
- *
- * This gets to be 'interesting' in the following cases, where the /should/s
- * end up false because of this issue.
- *
- *
- * CASE 1:
- *
- * So the spin_is_locked() correctness issue comes from something like:
- *
- *   CPU0				CPU1
- *
- *   global_lock();			local_lock(i)
- *     spin_lock(&G)			  spin_lock(&L[i])
- *     for (i)				  if (!spin_is_locked(&G)) {
- *       spin_unlock_wait(&L[i]);	    smp_acquire__after_ctrl_dep();
- *					    return;
- *					  }
- *					  // deal with fail
- *
- * Where it is important CPU1 sees G locked or CPU0 sees L[i] locked such
- * that there is exclusion between the two critical sections.
- *
- * The load from spin_is_locked(&G) /should/ be constrained by the ACQUIRE from
- * spin_lock(&L[i]), and similarly the load(s) from spin_unlock_wait(&L[i])
- * /should/ be constrained by the ACQUIRE from spin_lock(&G).
- *
- * Similarly, later stuff is constrained by the ACQUIRE from CTRL+RMB.
- *
- *
- * CASE 2:
- *
- * For spin_unlock_wait() there is a second correctness issue, namely:
- *
- *   CPU0				CPU1
- *
- *   flag = set;
- *   smp_mb();				spin_lock(&l)
- *   spin_unlock_wait(&l);		if (!flag)
- *					  // add to lockless list
- *					spin_unlock(&l);
- *   // iterate lockless list
- *
- * Which wants to ensure that CPU1 will stop adding bits to the list and CPU0
- * will observe the last entry on the list (if spin_unlock_wait() had ACQUIRE
- * semantics etc..)
- *
- * Where flag /should/ be ordered against the locked store of l.
- */
-
-/*
- * queued_spin_lock_slowpath() can (load-)ACQUIRE the lock before
- * issuing an _unordered_ store to set _Q_LOCKED_VAL.
- *
- * This means that the store can be delayed, but no later than the
- * store-release from the unlock. This means that simply observing
- * _Q_LOCKED_VAL is not sufficient to determine if the lock is acquired.
- *
- * There are two paths that can issue the unordered store:
- *
- *  (1) clear_pending_set_locked():	*,1,0 -> *,0,1
- *
- *  (2) set_locked():			t,0,0 -> t,0,1 ; t != 0
- *      atomic_cmpxchg_relaxed():	t,0,0 -> 0,0,1
- *
- * However, in both cases we have other !0 state we've set before to queue
- * ourseves:
- *
- * For (1) we have the atomic_cmpxchg_acquire() that set _Q_PENDING_VAL, our
- * load is constrained by that ACQUIRE to not pass before that, and thus must
- * observe the store.
- *
- * For (2) we have a more intersting scenario. We enqueue ourselves using
- * xchg_tail(), which ends up being a RELEASE. This in itself is not
- * sufficient, however that is followed by an smp_cond_acquire() on the same
- * word, giving a RELEASE->ACQUIRE ordering. This again constrains our load and
- * guarantees we must observe that store.
- *
- * Therefore both cases have other !0 state that is observable before the
- * unordered locked byte store comes through. This means we can use that to
- * wait for the lock store, and then wait for an unlock.
- */
-#ifndef queued_spin_unlock_wait
-void queued_spin_unlock_wait(struct qspinlock *lock)
-{
-	u32 val;
-
-	for (;;) {
-		val = atomic_read(&lock->val);
-
-		if (!val) /* not locked, we're done */
-			goto done;
-
-		if (val & _Q_LOCKED_MASK) /* locked, go wait for unlock */
-			break;
-
-		/* not locked, but pending, wait until we observe the lock */
-		cpu_relax();
-	}
-
-	/* any unlock is good */
-	while (atomic_read(&lock->val) & _Q_LOCKED_MASK)
-		cpu_relax();
-
-done:
-	smp_acquire__after_ctrl_dep();
-}
-EXPORT_SYMBOL(queued_spin_unlock_wait);
-#endif
-
 #endif /* _GEN_PV_LOCK_SLOWPATH */
 
 /**
-- 
2.5.2

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web