Add "always lock'd" implementations of set_bit, clear_bit and
change_bit and the corresponding test_and_ functions. Also add
"always lock'd" implementation of cmpxchg. These give guaranteed
strong synchronisation and are required for non-SMP kernels running on
an SMP hypervisor.
Signed-off-by: Ian Pratt <[email protected]>
Signed-off-by: Christian Limpach <[email protected]>
Signed-off-by: Chris Wright <[email protected]>
Signed-off-by: Jeremy Fitzhardinge <[email protected]>
Cc: Christoph Lameter <[email protected]>
---
include/asm-i386/sync_bitops.h | 156 ++++++++++++++++++++++++++++++++++++++++
include/asm-i386/system.h | 36 +++++++++
2 files changed, 192 insertions(+)
===================================================================
--- a/include/asm-i386/system.h
+++ b/include/asm-i386/system.h
@@ -261,6 +261,9 @@ static inline unsigned long __xchg(unsig
#define cmpxchg(ptr,o,n)\
((__typeof__(*(ptr)))__cmpxchg((ptr),(unsigned long)(o),\
(unsigned long)(n),sizeof(*(ptr))))
+#define sync_cmpxchg(ptr,o,n)\
+ ((__typeof__(*(ptr)))__sync_cmpxchg((ptr),(unsigned long)(o),\
+ (unsigned long)(n),sizeof(*(ptr))))
#endif
static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
@@ -282,6 +285,39 @@ static inline unsigned long __cmpxchg(vo
return prev;
case 4:
__asm__ __volatile__(LOCK_PREFIX "cmpxchgl %1,%2"
+ : "=a"(prev)
+ : "r"(new), "m"(*__xg(ptr)), "0"(old)
+ : "memory");
+ return prev;
+ }
+ return old;
+}
+
+/*
+ * Always use locked operations when touching memory shared with a
+ * hypervisor, since the system may be SMP even if the guest kernel
+ * isn't.
+ */
+static inline unsigned long __sync_cmpxchg(volatile void *ptr,
+ unsigned long old,
+ unsigned long new, int size)
+{
+ unsigned long prev;
+ switch (size) {
+ case 1:
+ __asm__ __volatile__("lock; cmpxchgb %b1,%2"
+ : "=a"(prev)
+ : "q"(new), "m"(*__xg(ptr)), "0"(old)
+ : "memory");
+ return prev;
+ case 2:
+ __asm__ __volatile__("lock; cmpxchgw %w1,%2"
+ : "=a"(prev)
+ : "r"(new), "m"(*__xg(ptr)), "0"(old)
+ : "memory");
+ return prev;
+ case 4:
+ __asm__ __volatile__("lock; cmpxchgl %1,%2"
: "=a"(prev)
: "r"(new), "m"(*__xg(ptr)), "0"(old)
: "memory");
===================================================================
--- /dev/null
+++ b/include/asm-i386/sync_bitops.h
@@ -0,0 +1,156 @@
+#ifndef _I386_SYNC_BITOPS_H
+#define _I386_SYNC_BITOPS_H
+
+/*
+ * Copyright 1992, Linus Torvalds.
+ */
+
+/*
+ * These have to be done with inline assembly: that way the bit-setting
+ * is guaranteed to be atomic. All bit operations return 0 if the bit
+ * was cleared before the operation and != 0 if it was not.
+ *
+ * bit 0 is the LSB of addr; bit 32 is the LSB of (addr+1).
+ */
+
+#define ADDR (*(volatile long *) addr)
+
+/**
+ * sync_set_bit - Atomically set a bit in memory
+ * @nr: the bit to set
+ * @addr: the address to start counting from
+ *
+ * This function is atomic and may not be reordered. See __set_bit()
+ * if you do not require the atomic guarantees.
+ *
+ * Note: there are no guarantees that this function will not be reordered
+ * on non x86 architectures, so if you are writting portable code,
+ * make sure not to rely on its reordering guarantees.
+ *
+ * Note that @nr may be almost arbitrarily large; this function is not
+ * restricted to acting on a single-word quantity.
+ */
+static inline void sync_set_bit(int nr, volatile unsigned long * addr)
+{
+ __asm__ __volatile__("lock; btsl %1,%0"
+ :"+m" (ADDR)
+ :"Ir" (nr)
+ : "memory");
+}
+
+/**
+ * sync_clear_bit - Clears a bit in memory
+ * @nr: Bit to clear
+ * @addr: Address to start counting from
+ *
+ * sync_clear_bit() is atomic and may not be reordered. However, it does
+ * not contain a memory barrier, so if it is used for locking purposes,
+ * you should call smp_mb__before_clear_bit() and/or smp_mb__after_clear_bit()
+ * in order to ensure changes are visible on other processors.
+ */
+static inline void sync_clear_bit(int nr, volatile unsigned long * addr)
+{
+ __asm__ __volatile__("lock; btrl %1,%0"
+ :"+m" (ADDR)
+ :"Ir" (nr)
+ : "memory");
+}
+
+/**
+ * sync_change_bit - Toggle a bit in memory
+ * @nr: Bit to change
+ * @addr: Address to start counting from
+ *
+ * change_bit() is atomic and may not be reordered. It may be
+ * reordered on other architectures than x86.
+ * Note that @nr may be almost arbitrarily large; this function is not
+ * restricted to acting on a single-word quantity.
+ */
+static inline void sync_change_bit(int nr, volatile unsigned long * addr)
+{
+ __asm__ __volatile__("lock; btcl %1,%0"
+ :"+m" (ADDR)
+ :"Ir" (nr)
+ : "memory");
+}
+
+/**
+ * sync_test_and_set_bit - Set a bit and return its old value
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This operation is atomic and cannot be reordered.
+ * It may be reordered on other architectures than x86.
+ * It also implies a memory barrier.
+ */
+static inline int sync_test_and_set_bit(int nr, volatile unsigned long * addr)
+{
+ int oldbit;
+
+ __asm__ __volatile__("lock; btsl %2,%1\n\tsbbl %0,%0"
+ :"=r" (oldbit),"+m" (ADDR)
+ :"Ir" (nr) : "memory");
+ return oldbit;
+}
+
+/**
+ * sync_test_and_clear_bit - Clear a bit and return its old value
+ * @nr: Bit to clear
+ * @addr: Address to count from
+ *
+ * This operation is atomic and cannot be reordered.
+ * It can be reorderdered on other architectures other than x86.
+ * It also implies a memory barrier.
+ */
+static inline int sync_test_and_clear_bit(int nr, volatile unsigned long * addr)
+{
+ int oldbit;
+
+ __asm__ __volatile__("lock; btrl %2,%1\n\tsbbl %0,%0"
+ :"=r" (oldbit),"+m" (ADDR)
+ :"Ir" (nr) : "memory");
+ return oldbit;
+}
+
+/**
+ * sync_test_and_change_bit - Change a bit and return its old value
+ * @nr: Bit to change
+ * @addr: Address to count from
+ *
+ * This operation is atomic and cannot be reordered.
+ * It also implies a memory barrier.
+ */
+static inline int sync_test_and_change_bit(int nr, volatile unsigned long* addr)
+{
+ int oldbit;
+
+ __asm__ __volatile__("lock; btcl %2,%1\n\tsbbl %0,%0"
+ :"=r" (oldbit),"+m" (ADDR)
+ :"Ir" (nr) : "memory");
+ return oldbit;
+}
+
+static __always_inline int sync_const_test_bit(int nr, const volatile unsigned long *addr)
+{
+ return ((1UL << (nr & 31)) &
+ (((const volatile unsigned int *)addr)[nr >> 5])) != 0;
+}
+
+static inline int sync_var_test_bit(int nr, const volatile unsigned long * addr)
+{
+ int oldbit;
+
+ __asm__ __volatile__("btl %2,%1\n\tsbbl %0,%0"
+ :"=r" (oldbit)
+ :"m" (ADDR),"Ir" (nr));
+ return oldbit;
+}
+
+#define sync_test_bit(nr,addr) \
+ (__builtin_constant_p(nr) ? \
+ sync_constant_test_bit((nr),(addr)) : \
+ sync_var_test_bit((nr),(addr)))
+
+#undef ADDR
+
+#endif /* _I386_SYNC_BITOPS_H */
--
On Wed, 2 Aug 2006, Jeremy Fitzhardinge wrote:
> Add "always lock'd" implementations of set_bit, clear_bit and
> change_bit and the corresponding test_and_ functions. Also add
> "always lock'd" implementation of cmpxchg. These give guaranteed
> strong synchronisation and are required for non-SMP kernels running on
> an SMP hypervisor.
I think I asked this before....
Would it not be simpler to always use the locked implementation on UP? At
least when the kernel is compiled with hypervisor support? This is going
to add yet another series of bit operations.
Christoph Lameter wrote:
> I think I asked this before....
>
> Would it not be simpler to always use the locked implementation on UP? At
> least when the kernel is compiled with hypervisor support? This is going
> to add yet another series of bit operations
You mean make the standard bit-ops atomic on UP when compiling for
CONFIG_PARAVIRT? We think its too much of a burden; there are only a
few operations which must be locked in the hypervisor case, and bit ops
are used everywhere. I'd like to get it to the point where there's no
significant overhead in configuring for PARAVIRT, even if you run on
native hardware.
J
On Wed, 2 Aug 2006, Jeremy Fitzhardinge wrote:
> > Would it not be simpler to always use the locked implementation on UP? At
> > least when the kernel is compiled with hypervisor support? This is going to
> > add yet another series of bit operations
>
> You mean make the standard bit-ops atomic on UP when compiling for
> CONFIG_PARAVIRT? We think its too much of a burden; there are only a few
> operations which must be locked in the hypervisor case, and bit ops are used
> everywhere. I'd like to get it to the point where there's no significant
> overhead in configuring for PARAVIRT, even if you run on native hardware.
Thats a good goal but what about the rest of us who have to maintain
additional forms of bit operations for all architectures. How much is this
burden? Are locked atomic bitops really that more expensive?
Christoph Lameter wrote:
> Thats a good goal but what about the rest of us who have to maintain
> additional forms of bit operations for all architectures. How much is this
> burden? Are locked atomic bitops really that more expensive?
>
It needn't be all architectures yet - only architectures that want to
compile Xen drivers are really affected. Perhaps a better place for
these locking primitives is in a Xen-specific driver header which
defines appropriate primitives for the architectures required? Last I
remember, there were still some issues here where atomic partial word
operations couldn't be supported on some architectures.
To answer your question, yes. On most i386 cores, locks destroy
performance, and even unintentional use of a single locked operation in
a critical path, on uncontended local memory, can have several hundred
cycles downstream penalty. I accidentally used one once during context
switch, and saw a 30% reduction in switch performance - on a modern
processor.
Zach
On Wed, 2 Aug 2006, Zachary Amsden wrote:
> It needn't be all architectures yet - only architectures that want to compile
> Xen drivers are really affected. Perhaps a better place for these locking
> primitives is in a Xen-specific driver header which defines appropriate
> primitives for the architectures required? Last I remember, there were still
> some issues here where atomic partial word operations couldn't be supported on
> some architectures.
Good idea. This will also make it easier to support this special
functionality. And it will avoid use in contexts where these are
not necessary.
> Thats a good goal but what about the rest of us who have to maintain
> additional forms of bit operations for all architectures. How much is this
> burden?
I don't think it's that big an issue because most architectures either
use always locked bitops already or don't need them because they don't do
SMP.
So it will be fine with just a asm-generic header that defines them
to the normal bitops. Not much burden.
-Andi
On Thursday 03 August 2006 03:25, Christoph Lameter wrote:
> Good idea. This will also make it easier to support this special
> functionality. And it will avoid use in contexts where these are
> not necessary.
I think it's a bad idea. We don't want lots of architecture ifdefs
in some Xen specific file
-Andi
On Thu, 3 Aug 2006, Andi Kleen wrote:
> On Thursday 03 August 2006 03:25, Christoph Lameter wrote:
>
> > Good idea. This will also make it easier to support this special
> > functionality. And it will avoid use in contexts where these are
> > not necessary.
>
> I think it's a bad idea. We don't want lots of architecture ifdefs
> in some Xen specific file
Thats not how it would be done. We would do this with
architecture specific xen files and a default in asm-generic.
On Thu, 3 Aug 2006, Andi Kleen wrote:
>
> > Thats a good goal but what about the rest of us who have to maintain
> > additional forms of bit operations for all architectures. How much is this
> > burden?
>
> I don't think it's that big an issue because most architectures either
> use always locked bitops already or don't need them because they don't do
> SMP.
Those architectures that always use locked bitops or dont need them would
not need to be modified if we put this in a special fail. I think this is
a i386 speciality here?
Those operations are only needed for special xen driver and not for
regular kernel code!
> So it will be fine with just a asm-generic header that defines them
> to the normal bitops. Not much burden.
asm-generic/xen-bitops.h asm-i386/xen-bitops.h is even less of a burden
and would only require a
#include <asm/xen-bitops.h>
for those special xen drivers.
On Thursday 03 August 2006 06:25, Christoph Lameter wrote:
> On Thu, 3 Aug 2006, Andi Kleen wrote:
>
> > On Thursday 03 August 2006 03:25, Christoph Lameter wrote:
> >
> > > Good idea. This will also make it easier to support this special
> > > functionality. And it will avoid use in contexts where these are
> > > not necessary.
> >
> > I think it's a bad idea. We don't want lots of architecture ifdefs
> > in some Xen specific file
>
> Thats not how it would be done. We would do this with
> architecture specific xen files and a default in asm-generic.
It's cleaner to just do it in the generic code.
I think for most architectures it is only a one liner anyways if
done properly.
-Andi
On Thursday 03 August 2006 06:27, Christoph Lameter wrote:
> On Thu, 3 Aug 2006, Andi Kleen wrote:
>
> >
> > > Thats a good goal but what about the rest of us who have to maintain
> > > additional forms of bit operations for all architectures. How much is this
> > > burden?
> >
> > I don't think it's that big an issue because most architectures either
> > use always locked bitops already or don't need them because they don't do
> > SMP.
>
> Those architectures that always use locked bitops or dont need them would
> not need to be modified if we put this in a special fail. I think this is
> a i386 speciality here?
i386/x86-64
They could do a single line #include for asm-generic that defines them
to the normal bitops.
>
> Those operations are only needed for special xen driver and not for
> regular kernel code!
The Xen driver will be "regular" kernel code.
> > So it will be fine with just a asm-generic header that defines them
> > to the normal bitops. Not much burden.
>
> asm-generic/xen-bitops.h asm-i386/xen-bitops.h is even less of a burden
> and would only require a
>
> #include <asm/xen-bitops.h>
>
> for those special xen drivers.
Well there might be reasons someone else uses this in the future too.
It's also not exactly Linux style - normally we try to add generic
facilities.
-Andi
On Thu, 3 Aug 2006, Andi Kleen wrote:
> > Those operations are only needed for special xen driver and not for
> > regular kernel code!
>
> The Xen driver will be "regular" kernel code.
As far as I can tell from this conversation there are special "Xen"
drivers that need this not the rest of the system.
> > for those special xen drivers.
>
> Well there might be reasons someone else uses this in the future too.
> It's also not exactly Linux style - normally we try to add generic
> facilities.
What possible use could there be to someone else?
The "atomic" ops lock/unlock crap exists only for i386 as far as I can
tell. As you said most architectures either always use atomic ops or
never. The lock/unlock atomic ops are i386 specific material that
better stay contained. Its arch specific and not generic.
> As far as I can tell from this conversation there are special "Xen"
> drivers that need this not the rest of the system.
Yes, but in general when a driver that runs on multiple architectures
(including IA64 btw) needs something architecture specific we usually
add it to asm, not add ifdefs.
> > > for those special xen drivers.
> >
> > Well there might be reasons someone else uses this in the future too.
> > It's also not exactly Linux style - normally we try to add generic
> > facilities.
>
> What possible use could there be to someone else?
e.g. for other hypervisors or possibly for special hardware access
(e.g. I could imagine it being used for some kind of cluster interconnect)
I remember Alan was using a similar hack in his EDAC drivers because
it was the only way to clear ECC errors.
> The "atomic" ops lock/unlock crap exists only for i386 as far as I can
> tell. As you said most architectures either always use atomic ops or
> never. The lock/unlock atomic ops are i386 specific material that
> better stay contained. Its arch specific and not generic.
Well we have our share of weird hacks for IA64 too in generic code.
Just adding a single line #include for a wrapper asm-generic surely isn't
a undue burden for the other architectures, and it will save some
mess in the Xen drivers.
-Andi
On Thu, 3 Aug 2006, Andi Kleen wrote:
> > As far as I can tell from this conversation there are special "Xen"
> > drivers that need this not the rest of the system.
>
> Yes, but in general when a driver that runs on multiple architectures
> (including IA64 btw) needs something architecture specific we usually
> add it to asm, not add ifdefs.
I still wonder why you are so focused on ifdefs. Why would we need those?
> > What possible use could there be to someone else?
>
> e.g. for other hypervisors or possibly for special hardware access
> (e.g. I could imagine it being used for some kind of cluster interconnect)
> I remember Alan was using a similar hack in his EDAC drivers because
> it was the only way to clear ECC errors.
Maybe the best thing would be to have proper atomic ops in UP mode on
i386? The current way of just dropping the lock bit is the source of the
troubles.
> > The "atomic" ops lock/unlock crap exists only for i386 as far as I can
> > tell. As you said most architectures either always use atomic ops or
> > never. The lock/unlock atomic ops are i386 specific material that
> > better stay contained. Its arch specific and not generic.
>
> Well we have our share of weird hacks for IA64 too in generic code.
But we all try to contain them. Here we have i386 hacks multiplying
all over the system.
> Just adding a single line #include for a wrapper asm-generic surely isn't
> a undue burden for the other architectures, and it will save some
> mess in the Xen drivers.
Just adding a single line #include <asm/xen-bitops.h> to drivers that need
this functionality is not an undue burden for the drivers that support
Xen. They have to use special _xxx bitops anyways.
On Thursday 03 August 2006 07:32, Christoph Lameter wrote:
> On Thu, 3 Aug 2006, Andi Kleen wrote:
>
> > > As far as I can tell from this conversation there are special "Xen"
> > > drivers that need this not the rest of the system.
> >
> > Yes, but in general when a driver that runs on multiple architectures
> > (including IA64 btw) needs something architecture specific we usually
> > add it to asm, not add ifdefs.
>
> I still wonder why you are so focused on ifdefs. Why would we need those?
Because the Xen drivers will run on a couple of architectures, including
IA64 and PPC.
If IA64 or PPC didn't implement at least wrappers for the sync ops
then they would all need special ifdefs to handle this.
>
> > > What possible use could there be to someone else?
> >
> > e.g. for other hypervisors or possibly for special hardware access
> > (e.g. I could imagine it being used for some kind of cluster interconnect)
> > I remember Alan was using a similar hack in his EDAC drivers because
> > it was the only way to clear ECC errors.
>
> Maybe the best thing would be to have proper atomic ops in UP mode on
> i386? The current way of just dropping the lock bit is the source of the
> troubles.
It's a huge performance difference.
> > Just adding a single line #include for a wrapper asm-generic surely isn't
> > a undue burden for the other architectures, and it will save some
> > mess in the Xen drivers.
>
> Just adding a single line #include <asm/xen-bitops.h> to drivers that need
> this functionality is not an undue burden for the drivers that support
> Xen. They have to use special _xxx bitops anyways.
Ok it could be put into a separate file (although with a neutral name)
But you would still need to add that to IA64, PPC etc. too, so it
would only avoid adding a single to the other architectures.
-Andi
On Thu, 3 Aug 2006, Andi Kleen wrote:
> > I still wonder why you are so focused on ifdefs. Why would we need those?
>
> Because the Xen drivers will run on a couple of architectures, including
> IA64 and PPC.
>
> If IA64 or PPC didn't implement at least wrappers for the sync ops
> then they would all need special ifdefs to handle this.
No they would just need to do an #include <xen-bitops.h>
> > Maybe the best thing would be to have proper atomic ops in UP mode on
> > i386? The current way of just dropping the lock bit is the source of the
> > troubles.
>
> It's a huge performance difference.
I understand but why dont we use regular ops explicitly
instead of hacking the atomic ops. Then we would not have unhack them now.
> > Just adding a single line #include <asm/xen-bitops.h> to drivers that need
> > this functionality is not an undue burden for the drivers that support
> > Xen. They have to use special _xxx bitops anyways.
>
> Ok it could be put into a separate file (although with a neutral name)
>
> But you would still need to add that to IA64, PPC etc. too, so it
> would only avoid adding a single to the other architectures.
Could we not just add one fallback definition to asm-generic?
On Thursday 03 August 2006 07:54, Christoph Lameter wrote:
> On Thu, 3 Aug 2006, Andi Kleen wrote:
>
> > > I still wonder why you are so focused on ifdefs. Why would we need those?
> >
> > Because the Xen drivers will run on a couple of architectures, including
> > IA64 and PPC.
> >
> > If IA64 or PPC didn't implement at least wrappers for the sync ops
> > then they would all need special ifdefs to handle this.
>
> No they would just need to do an #include <xen-bitops.h>
If IA64 and PPC64 wouldn't have xen-bitops.h (which you seem to argue
for) then they would need ifdefs.
> > But you would still need to add that to IA64, PPC etc. too, so it
> > would only avoid adding a single to the other architectures.
>
> Could we not just add one fallback definition to asm-generic?
You mean into asm-generic/bitops.h? Then it would need ifdefs
to handle the i386/x86-64 case.
-Andi
On Thu, 3 Aug 2006, Andi Kleen wrote:
> On Thursday 03 August 2006 07:54, Christoph Lameter wrote:
> > On Thu, 3 Aug 2006, Andi Kleen wrote:
> >
> > > > I still wonder why you are so focused on ifdefs. Why would we need those?
> > >
> > > Because the Xen drivers will run on a couple of architectures, including
> > > IA64 and PPC.
> > >
> > > If IA64 or PPC didn't implement at least wrappers for the sync ops
> > > then they would all need special ifdefs to handle this.
> >
> > No they would just need to do an #include <xen-bitops.h>
>
> If IA64 and PPC64 wouldn't have xen-bitops.h (which you seem to argue
> for) then they would need ifdefs.
An include <asm/xen-bitops.h> would need to fall back to asm-generic if
there is no file in asm-arch/xen-bitops.h. I thought we had such a
mechanism?
> You mean into asm-generic/bitops.h? Then it would need ifdefs
> to handle the i386/x86-64 case.
No. Into asm-generic/xen-bitops.h.
* Christoph Lameter ([email protected]) wrote:
> An include <asm/xen-bitops.h> would need to fall back to asm-generic if
> there is no file in asm-arch/xen-bitops.h. I thought we had such a
> mechanism?
While Xen is the primary user, it is a misnomer to call it xen-bitops.
These are simply always locked, hence the sync-bitops name. Also,
there's a use of sync_cmpxchg, and cmpxchg is not in bitops.h.
As for the mechanism, it is manual. Arch specific header includes
asm-generic one.
> An include <asm/xen-bitops.h> would need to fall back to asm-generic if
> there is no file in asm-arch/xen-bitops.h. I thought we had such a
> mechanism?
AFAIK not. If you want generic you need a proxy include file.
-Andi
On Fri, 4 Aug 2006, Andi Kleen wrote:
> > An include <asm/xen-bitops.h> would need to fall back to asm-generic if
> > there is no file in asm-arch/xen-bitops.h. I thought we had such a
> > mechanism?
> AFAIK not. If you want generic you need a proxy include file.
Hmm... Well then we have no way of adding a new asm-arch file
without adding them to each arch. Makes it difficult to add
new functionality.