[PATCH] x86: remove byte locks

[PATCH] x86: remove byte locks

[Jiri Kosina]

Remove byte locks implementation, which was introduced by Jeremy in 
8efcbab6 ("paravirt: introduce a "lock-byte" spinlock implementation"), 
but turned out to be dead code that is not used by any in-kernel 
virtualization guest (Xen uses its own variant of spinlocks implementation 
and KVM is not planning to move to byte locks).

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 arch/x86/include/asm/paravirt.h      |    2 -
 arch/x86/include/asm/spinlock.h      |   66 +--------------------------------
 arch/x86/kernel/paravirt-spinlocks.c |   10 -----
 3 files changed, 2 insertions(+), 76 deletions(-)

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index ba3e2ff..32bc6c2 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -1389,8 +1389,6 @@ static inline void __set_fixmap(unsigned /* enum fixed_addresses */ idx,
 void _paravirt_nop(void);
 #define paravirt_nop	((void *)_paravirt_nop)
 
-void paravirt_use_bytelocks(void);
-
 #ifdef CONFIG_SMP
 
 static inline int __raw_spin_is_locked(struct raw_spinlock *lock)
diff --git a/arch/x86/include/asm/spinlock.h b/arch/x86/include/asm/spinlock.h
index d17c919..2bd6b11 100644
--- a/arch/x86/include/asm/spinlock.h
+++ b/arch/x86/include/asm/spinlock.h
@@ -172,70 +172,8 @@ static inline int __ticket_spin_is_contended(raw_spinlock_t *lock)
 	return (((tmp >> TICKET_SHIFT) - tmp) & ((1 << TICKET_SHIFT) - 1)) > 1;
 }
 
-#ifdef CONFIG_PARAVIRT
-/*
- * Define virtualization-friendly old-style lock byte lock, for use in
- * pv_lock_ops if desired.
- *
- * This differs from the pre-2.6.24 spinlock by always using xchgb
- * rather than decb to take the lock; this allows it to use a
- * zero-initialized lock structure.  It also maintains a 1-byte
- * contention counter, so that we can implement
- * __byte_spin_is_contended.
- */
-struct __byte_spinlock {
-	s8 lock;
-	s8 spinners;
-};
-
-static inline int __byte_spin_is_locked(raw_spinlock_t *lock)
-{
-	struct __byte_spinlock *bl = (struct __byte_spinlock *)lock;
-	return bl->lock != 0;
-}
-
-static inline int __byte_spin_is_contended(raw_spinlock_t *lock)
-{
-	struct __byte_spinlock *bl = (struct __byte_spinlock *)lock;
-	return bl->spinners != 0;
-}
-
-static inline void __byte_spin_lock(raw_spinlock_t *lock)
-{
-	struct __byte_spinlock *bl = (struct __byte_spinlock *)lock;
-	s8 val = 1;
-
-	asm("1: xchgb %1, %0\n"
-	    "   test %1,%1\n"
-	    "   jz 3f\n"
-	    "   " LOCK_PREFIX "incb %2\n"
-	    "2: rep;nop\n"
-	    "   cmpb $1, %0\n"
-	    "   je 2b\n"
-	    "   " LOCK_PREFIX "decb %2\n"
-	    "   jmp 1b\n"
-	    "3:"
-	    : "+m" (bl->lock), "+q" (val), "+m" (bl->spinners): : "memory");
-}
-
-static inline int __byte_spin_trylock(raw_spinlock_t *lock)
-{
-	struct __byte_spinlock *bl = (struct __byte_spinlock *)lock;
-	u8 old = 1;
-
-	asm("xchgb %1,%0"
-	    : "+m" (bl->lock), "+q" (old) : : "memory");
+#ifndef CONFIG_PARAVIRT
 
-	return old == 0;
-}
-
-static inline void __byte_spin_unlock(raw_spinlock_t *lock)
-{
-	struct __byte_spinlock *bl = (struct __byte_spinlock *)lock;
-	smp_wmb();
-	bl->lock = 0;
-}
-#else  /* !CONFIG_PARAVIRT */
 static inline int __raw_spin_is_locked(raw_spinlock_t *lock)
 {
 	return __ticket_spin_is_locked(lock);
@@ -267,7 +205,7 @@ static __always_inline void __raw_spin_lock_flags(raw_spinlock_t *lock,
 	__raw_spin_lock(lock);
 }
 
-#endif	/* CONFIG_PARAVIRT */
+#endif
 
 static inline void __raw_spin_unlock_wait(raw_spinlock_t *lock)
 {
diff --git a/arch/x86/kernel/paravirt-spinlocks.c b/arch/x86/kernel/paravirt-spinlocks.c
index 95777b0..3a7c5a4 100644
--- a/arch/x86/kernel/paravirt-spinlocks.c
+++ b/arch/x86/kernel/paravirt-spinlocks.c
@@ -26,13 +26,3 @@ struct pv_lock_ops pv_lock_ops = {
 };
 EXPORT_SYMBOL(pv_lock_ops);
 
-void __init paravirt_use_bytelocks(void)
-{
-#ifdef CONFIG_SMP
-	pv_lock_ops.spin_is_locked = __byte_spin_is_locked;
-	pv_lock_ops.spin_is_contended = __byte_spin_is_contended;
-	pv_lock_ops.spin_lock = __byte_spin_lock;
-	pv_lock_ops.spin_trylock = __byte_spin_trylock;
-	pv_lock_ops.spin_unlock = __byte_spin_unlock;
-#endif
-}
-- 
1.6.0.2

Re: [PATCH] x86: remove byte locks

[Ingo Molnar]

* Jiri Kosina <jkosina@suse.cz> wrote:

> Remove byte locks implementation, which was introduced by Jeremy in 
> 8efcbab6 ("paravirt: introduce a "lock-byte" spinlock implementation"), 
> but turned out to be dead code that is not used by any in-kernel 
> virtualization guest (Xen uses its own variant of spinlocks implementation 
> and KVM is not planning to move to byte locks).
> 
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
>  arch/x86/include/asm/paravirt.h      |    2 -
>  arch/x86/include/asm/spinlock.h      |   66 +--------------------------------
>  arch/x86/kernel/paravirt-spinlocks.c |   10 -----
>  3 files changed, 2 insertions(+), 76 deletions(-)

didnt you send a patch in this lkml thread:

  Subject: Re: Is 386 processor still supported?

that makes use of byte-locks on i386 ?

But i guess we should solve M386 and M486 by only allowing it on !SMP, 
hence spinlock support is moot there, right?

	Ingo

Re: [PATCH] x86: remove byte locks

[Jiri Kosina]

On Mon, 12 Jan 2009, Ingo Molnar wrote:

> > Remove byte locks implementation, which was introduced by Jeremy in 
> > 8efcbab6 ("paravirt: introduce a "lock-byte" spinlock implementation"), 
> > but turned out to be dead code that is not used by any in-kernel 
> > virtualization guest (Xen uses its own variant of spinlocks implementation 
> > and KVM is not planning to move to byte locks).
> > 
> > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> > ---
> >  arch/x86/include/asm/paravirt.h      |    2 -
> >  arch/x86/include/asm/spinlock.h      |   66 +--------------------------------
> >  arch/x86/kernel/paravirt-spinlocks.c |   10 -----
> >  3 files changed, 2 insertions(+), 76 deletions(-)
> didnt you send a patch in this lkml thread:
> 
>   Subject: Re: Is 386 processor still supported?

> that makes use of byte-locks on i386 ?

I did, but that patch was bogus, as we indeed don't support smp on M386.

This is totally independent -- it just removes dead code (byte locks) that 
has no in-tree user at all.

> But i guess we should solve M386 and M486 by only allowing it on !SMP, 
> hence spinlock support is moot there, right?

Agreed. But that's a different issue.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] x86: remove byte locks

[Ingo Molnar]

* Jiri Kosina <jkosina@suse.cz> wrote:

> On Mon, 12 Jan 2009, Ingo Molnar wrote:
> 
> > > Remove byte locks implementation, which was introduced by Jeremy in 
> > > 8efcbab6 ("paravirt: introduce a "lock-byte" spinlock implementation"), 
> > > but turned out to be dead code that is not used by any in-kernel 
> > > virtualization guest (Xen uses its own variant of spinlocks implementation 
> > > and KVM is not planning to move to byte locks).
> > > 
> > > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> > > ---
> > >  arch/x86/include/asm/paravirt.h      |    2 -
> > >  arch/x86/include/asm/spinlock.h      |   66 +--------------------------------
> > >  arch/x86/kernel/paravirt-spinlocks.c |   10 -----
> > >  3 files changed, 2 insertions(+), 76 deletions(-)
> > didnt you send a patch in this lkml thread:
> > 
> >   Subject: Re: Is 386 processor still supported?
> 
> > that makes use of byte-locks on i386 ?
> 
> I did, but that patch was bogus, as we indeed don't support smp on M386.
> 
> This is totally independent -- it just removes dead code (byte locks) that 
> has no in-tree user at all.
> 
> > But i guess we should solve M386 and M486 by only allowing it on !SMP, 
> > hence spinlock support is moot there, right?
> 
> Agreed. But that's a different issue.

ok. Jeremy, can we apply Jiri's patch or do you still have plans with that 
code?

	Ingo

Re: [PATCH] x86: remove byte locks

[Jeremy Fitzhardinge]

Ingo Molnar wrote:
> * Jiri Kosina <jkosina@suse.cz> wrote:
>
>   
>> On Mon, 12 Jan 2009, Ingo Molnar wrote:
>>
>>     
>>>> Remove byte locks implementation, which was introduced by Jeremy in 
>>>> 8efcbab6 ("paravirt: introduce a "lock-byte" spinlock implementation"), 
>>>> but turned out to be dead code that is not used by any in-kernel 
>>>> virtualization guest (Xen uses its own variant of spinlocks implementation 
>>>> and KVM is not planning to move to byte locks).
>>>>
>>>> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
>>>> ---
>>>>  arch/x86/include/asm/paravirt.h      |    2 -
>>>>  arch/x86/include/asm/spinlock.h      |   66 +--------------------------------
>>>>  arch/x86/kernel/paravirt-spinlocks.c |   10 -----
>>>>  3 files changed, 2 insertions(+), 76 deletions(-)
>>>>         
>>> didnt you send a patch in this lkml thread:
>>>
>>>   Subject: Re: Is 386 processor still supported?
>>>       
>>> that makes use of byte-locks on i386 ?
>>>       
>> I did, but that patch was bogus, as we indeed don't support smp on M386.
>>
>> This is totally independent -- it just removes dead code (byte locks) that 
>> has no in-tree user at all.
>>
>>     
>>> But i guess we should solve M386 and M486 by only allowing it on !SMP, 
>>> hence spinlock support is moot there, right?
>>>       
>> Agreed. But that's a different issue.
>>     
>
> ok. Jeremy, can we apply Jiri's patch or do you still have plans with that 
> code?

My intention for that code was always that it be a simplest-possible 
reference implementation for the spinlock pvops, and perhaps a basis for 
a more specialized version (the Xen version is based on byte locks, for 
example). 

The code is "dead" in the sense that it has no users, but it also 
results in no generated code, and should be easy to maintain if the 
spinlock API is changed (as it is a canary to show that the other 
implementations will need changing too).  In particular, the "paravirt 
spinlock" mechanism relies on all implementations using the same static 
initializer, and I wanted there to be an obvious second implementation 
so that if someone decided to change the ticketlock initializer, they'd 
be forced to consider what happens with the bytelock initializer (and by 
extension, any other implementation).

It's also useful as a baseline for benchmarking.

    J

Re: [PATCH] x86: remove byte locks

[Jiri Kosina]

On Tue, 13 Jan 2009, Jeremy Fitzhardinge wrote:

> My intention for that code was always that it be a simplest-possible 
> reference implementation for the spinlock pvops, and perhaps a basis for 
> a more specialized version (the Xen version is based on byte locks, for 
> example). The code is "dead" in the sense that it has no users, but it 
> also results in no generated code, and should be easy to maintain if the 
> spinlock API is changed (as it is a canary to show that the other 
> implementations will need changing too).  In particular, the "paravirt 
> spinlock" mechanism relies on all implementations using the same static 
> initializer, and I wanted there to be an obvious second implementation 
> so that if someone decided to change the ticketlock initializer, they'd 
> be forced to consider what happens with the bytelock initializer (and by 
> extension, any other implementation).

Why can't this just be somewhere in documentation? (possibly even with the 
byte locks code as a reference).

It is IMHO just totally confusing to have a spinlock implementation that 
is not used at all in the tree. It took me quite some time to go through 
this until I finally figured out that this code is actually never used. 
Currently, on first sight it might seem that byte locks are used whenever 
CONFIG_PARAVIRT is set, which is not true.

And apparently even Linus got confused by this, which also tells us 
something by itself, see [1].

[1] http://marc.info/?l=linux-kernel&m=123144211719754&w=2

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] x86: remove byte locks

[Jeremy Fitzhardinge]

Jiri Kosina wrote:
> Why can't this just be somewhere in documentation? (possibly even with the 
> byte locks code as a reference).
>   

Because Ingo's compil-o-matic will never fail on a documentation error.

> It is IMHO just totally confusing to have a spinlock implementation that 
> is not used at all in the tree. It took me quite some time to go through 
> this until I finally figured out that this code is actually never used. 
> Currently, on first sight it might seem that byte locks are used whenever 
> CONFIG_PARAVIRT is set, which is not true.
>   

Well, a comment next to the code explaining the rationale probably 
wouldn't go astray.

> And apparently even Linus got confused by this, which also tells us 
> something by itself, see [1].
>
> [1] http://marc.info/?l=linux-kernel&m=123144211719754&w=2
>   

It tells us that Linus couldn't give a rat's arse about virtualization, 
which is just something we have to cope with ;)

    J

Re: [PATCH] x86: remove byte locks

[Jiri Kosina]

On Tue, 13 Jan 2009, Jeremy Fitzhardinge wrote:

> > Why can't this just be somewhere in documentation? (possibly even with 
> > the byte locks code as a reference).
> Because Ingo's compil-o-matic will never fail on a documentation error.

Hmm, I have always considered the "we don't accept any code that would 
have zero in-kernel users" rule as a quite reasonable one, at least in 
order to prevent from bloat and code getting confusing.
But apparently it's not the intention here.

> > It is IMHO just totally confusing to have a spinlock implementation that is
> > not used at all in the tree. It took me quite some time to go through this
> > until I finally figured out that this code is actually never used.
> > Currently, on first sight it might seem that byte locks are used whenever
> > CONFIG_PARAVIRT is set, which is not true.
> Well, a comment next to the code explaining the rationale probably 
> wouldn't go astray.

I still strongly feel that if the only purpose of the code in kernel is 
"to provide example", then it belongs to documentation.

> > And apparently even Linus got confused by this, which also tells us
> > something by itself, see [1].
> > [1] http://marc.info/?l=linux-kernel&m=123144211719754&w=2
> It tells us that Linus couldn't give a rat's arse about virtualization, 
> which is just something we have to cope with ;)

I am afraid this has nothing to do with virtualization. It's simply 
confusing when looking at the code.

Thanks,

-- 
Jiri Kosina

Re: [PATCH] x86: remove byte locks

[Ingo Molnar]

* Jiri Kosina <jkosina@suse.cz> wrote:

> On Tue, 13 Jan 2009, Jeremy Fitzhardinge wrote:
> 
> > > Why can't this just be somewhere in documentation? (possibly even with 
> > > the byte locks code as a reference).
> > Because Ingo's compil-o-matic will never fail on a documentation error.
> 
> Hmm, I have always considered the "we don't accept any code that would 
> have zero in-kernel users" rule as a quite reasonable one, at least in 
> order to prevent from bloat and code getting confusing.
> But apparently it's not the intention here.
> 
> > > It is IMHO just totally confusing to have a spinlock implementation that is
> > > not used at all in the tree. It took me quite some time to go through this
> > > until I finally figured out that this code is actually never used.
> > > Currently, on first sight it might seem that byte locks are used whenever
> > > CONFIG_PARAVIRT is set, which is not true.
> > Well, a comment next to the code explaining the rationale probably 
> > wouldn't go astray.
> 
> I still strongly feel that if the only purpose of the code in kernel is 
> "to provide example", then it belongs to documentation.
> 
> > > And apparently even Linus got confused by this, which also tells us
> > > something by itself, see [1].
> > > [1] http://marc.info/?l=linux-kernel&m=123144211719754&w=2
> > It tells us that Linus couldn't give a rat's arse about virtualization, 
> > which is just something we have to cope with ;)
> 
> I am afraid this has nothing to do with virtualization. It's simply 
> confusing when looking at the code.

i'd tend to agree, that area of code is quite complex already.

	Ingo

Re: [PATCH] x86: remove byte locks

[Jiri Kosina]

On Thu, 15 Jan 2009, Ingo Molnar wrote:

> > > > Why can't this just be somewhere in documentation? (possibly even with 
> > > > the byte locks code as a reference).
> > > Because Ingo's compil-o-matic will never fail on a documentation error.
> > 
> > Hmm, I have always considered the "we don't accept any code that would 
> > have zero in-kernel users" rule as a quite reasonable one, at least in 
> > order to prevent from bloat and code getting confusing.
> > But apparently it's not the intention here.
> > 
> > > > It is IMHO just totally confusing to have a spinlock implementation that is
> > > > not used at all in the tree. It took me quite some time to go through this
> > > > until I finally figured out that this code is actually never used.
> > > > Currently, on first sight it might seem that byte locks are used whenever
> > > > CONFIG_PARAVIRT is set, which is not true.
> > > Well, a comment next to the code explaining the rationale probably 
> > > wouldn't go astray.
> > 
> > I still strongly feel that if the only purpose of the code in kernel is 
> > "to provide example", then it belongs to documentation.
> > 
> > > > And apparently even Linus got confused by this, which also tells us
> > > > something by itself, see [1].
> > > > [1] http://marc.info/?l=linux-kernel&m=123144211719754&w=2
> > > It tells us that Linus couldn't give a rat's arse about virtualization, 
> > > which is just something we have to cope with ;)
> > 
> > I am afraid this has nothing to do with virtualization. It's simply 
> > confusing when looking at the code.
> 
> i'd tend to agree, that area of code is quite complex already.

ping, any final word here? Should I consider the silence a implicit nak, 
or should I resend the patch?

-- 
Jiri Kosina

Re: [PATCH] x86: remove byte locks

[Ingo Molnar]

* Jiri Kosina <jkosina@suse.cz> wrote:

> On Thu, 15 Jan 2009, Ingo Molnar wrote:
> 
> > > > > Why can't this just be somewhere in documentation? (possibly even with 
> > > > > the byte locks code as a reference).
> > > > Because Ingo's compil-o-matic will never fail on a documentation error.
> > > 
> > > Hmm, I have always considered the "we don't accept any code that would 
> > > have zero in-kernel users" rule as a quite reasonable one, at least in 
> > > order to prevent from bloat and code getting confusing.
> > > But apparently it's not the intention here.
> > > 
> > > > > It is IMHO just totally confusing to have a spinlock implementation that is
> > > > > not used at all in the tree. It took me quite some time to go through this
> > > > > until I finally figured out that this code is actually never used.
> > > > > Currently, on first sight it might seem that byte locks are used whenever
> > > > > CONFIG_PARAVIRT is set, which is not true.
> > > > Well, a comment next to the code explaining the rationale probably 
> > > > wouldn't go astray.
> > > 
> > > I still strongly feel that if the only purpose of the code in kernel is 
> > > "to provide example", then it belongs to documentation.
> > > 
> > > > > And apparently even Linus got confused by this, which also tells us
> > > > > something by itself, see [1].
> > > > > [1] http://marc.info/?l=linux-kernel&m=123144211719754&w=2
> > > > It tells us that Linus couldn't give a rat's arse about virtualization, 
> > > > which is just something we have to cope with ;)
> > > 
> > > I am afraid this has nothing to do with virtualization. It's simply 
> > > confusing when looking at the code.
> > 
> > i'd tend to agree, that area of code is quite complex already.
> 
> ping, any final word here? Should I consider the silence a implicit nak, 
> or should I resend the patch?

ok - i've applied it to tip/x86/cleanups.

	Ingo