Add assign_bits() to give an atomic-bitops safe assignment on all archs without
having to rely on the presence of xchg or cmpxchg instructions.
This is needed where archs emulate atomic bitops using spinlocks as there's a
gap between reading the counter and updating it that a direct assignment could
get lost in.
The bitops-proofed wrapper assign_bits() is, for example, to test_and_set_bit()
as atomic_set() is to atomic_inc().
On most arches the wrapper really is just direct assignment, but where
spinlocks are used to implement atomic and bitops, then direct assignment to a
word operated on by bitops must be wrapped in the same spinlocks.
This _cannot_ be handled by having, say, test_and_set_bit() just not write to
the target word if it's not going to actually change anything. The following
situation is still a problem:
*x y CPU 0 CPU 1
=== === =============================== ===============================
0
0 ==>test_and_set_bit(3, x)
0 spin_lock_irq(lock_for(x))
0 0 z = y = *x;
0 0 if (!(y & (1 << 3))) { *x = 32;
32 8 y |= (1 << 3);
8 8 *x = y;
8 }
8 spin_unlock_irq(lock_for(x))
8 z &= (1 << 3);
8 return z;
8 <==test_and_set_bit(3, x)
This does not produce the right result. Either *x should be 32 at the end, or
it should be 40. It should not be 8.
Locks are required. Then you end up with one of two situations:
*x y CPU 0 CPU 1
=== === =============================== ===============================
0
0 ==>test_and_set_bit(3, x)
0 spin_lock_irq(lock_for(x)) spin_lock_irq(lock_for(x))
32 <waiting> *x = 32;
32 <waiting> spin_unlock_irq(lock_for(x))
32 32 z = y = *x;
32 32 if (!(y & (1 << 3))) {
32 40 y |= (1 << 3);
40 40 *x = y;
40 }
40 spin_unlock_irq(lock_for(x))
40 z &= (1 << 3);
40 return z;
40 <==test_and_set_bit(3, x)
Or:
*x y CPU 0 CPU 1
=== === =============================== ===============================
0
0 ==>test_and_set_bit(3, x)
0 spin_lock_irq(lock_for(x)) spin_lock_irq(lock_for(x))
0 0 z = y = *x; <waiting>
0 0 if (!(y & (1 << 3))) { <waiting>
0 8 y |= (1 << 3); <waiting>
8 8 *x = y; <waiting>
8 } <waiting>
8 spin_unlock_irq(lock_for(x)) <waiting>
8 z &= (1 << 3); *x = 32;
32 return z; spin_unlock_irq(lock_for(x))
32 <==test_and_set_bit(3, x)
Note that if the above is wrong, and locks aren't required here, then they
ought not to be required in atomic_set() either.
Signed-Off-By: David Howells <[email protected]>
---
arch/sparc/lib/bitops.S | 24 +++++++++++++++++++++++
include/asm-alpha/bitops.h | 1 +
include/asm-arm/bitops.h | 26 ++++++++++++++++++++++++
include/asm-arm26/bitops.h | 9 ++++++++
include/asm-avr32/bitops.h | 1 +
include/asm-cris/bitops.h | 1 +
include/asm-frv/bitops.h | 1 +
include/asm-generic/bitops/assign-bits.h | 32 ++++++++++++++++++++++++++++++
include/asm-generic/bitops/atomic.h | 21 ++++++++++++++++++++
include/asm-h8300/bitops.h | 1 +
include/asm-i386/bitops.h | 1 +
include/asm-ia64/bitops.h | 1 +
include/asm-m32r/bitops.h | 1 +
include/asm-m68k/bitops.h | 1 +
include/asm-m68knommu/bitops.h | 1 +
include/asm-mips/bitops.h | 1 +
include/asm-parisc/bitops.h | 9 ++++++++
include/asm-powerpc/bitops.h | 1 +
include/asm-s390/bitops.h | 2 ++
include/asm-sh/bitops.h | 1 +
include/asm-sh64/bitops.h | 1 +
include/asm-sparc/bitops.h | 21 ++++++++++++++++++++
include/asm-sparc64/bitops.h | 1 +
include/asm-v850/bitops.h | 1 +
include/asm-x86_64/bitops.h | 1 +
25 files changed, 161 insertions(+), 0 deletions(-)
diff --git a/arch/sparc/lib/bitops.S b/arch/sparc/lib/bitops.S
index cb7fb66..d6b9491 100644
--- a/arch/sparc/lib/bitops.S
+++ b/arch/sparc/lib/bitops.S
@@ -105,5 +105,29 @@ #endif
jmpl %o7, %g0
mov %g4, %o7
+ /* And then just replace all the bits with those from %g2. */
+ .globl ___assign_bits
+___assign_bits:
+ rd %psr, %g3
+ nop; nop; nop
+ or %g3, PSR_PIL, %g5
+ wr %g5, 0x0, %psr
+ nop; nop; nop
+#ifdef CONFIG_SMP
+ set bitops_spinlock, %g5
+2: ldstub [%g5], %g7 ! Spin on the byte lock for SMP.
+ orcc %g7, 0x0, %g0 ! Did we get it?
+ bne 2b ! Nope...
+ st %g2, [%g1]
+ set bitops_spinlock, %g5
+ stb %g0, [%g5]
+#else
+ st %g2, [%g1]
+#endif
+ wr %g3, 0x0, %psr
+ nop; nop; nop
+ jmpl %o7, %g0
+ mov %g4, %o7
+
.globl __bitops_end
__bitops_end:
diff --git a/include/asm-alpha/bitops.h b/include/asm-alpha/bitops.h
index 4b6ef7f..1263b26 100644
--- a/include/asm-alpha/bitops.h
+++ b/include/asm-alpha/bitops.h
@@ -2,6 +2,7 @@ #ifndef _ALPHA_BITOPS_H
#define _ALPHA_BITOPS_H
#include <asm/compiler.h>
+#include <asm-generic/bitops/assign-bits.h>
/*
* Copyright 1994, Linus Torvalds.
diff --git a/include/asm-arm/bitops.h b/include/asm-arm/bitops.h
index b41831b..5932134 100644
--- a/include/asm-arm/bitops.h
+++ b/include/asm-arm/bitops.h
@@ -117,6 +117,32 @@ ____atomic_test_and_change_bit(unsigned
return res & mask;
}
+#if __LINUX_ARM_ARCH__ >= 6 && defined(CONFIG_CPU_32v6K)
+static inline void assign_bits(unsigned long v, unsigned long *addr)
+{
+ unsigned long tmp;
+
+ __asm__ __volatile__("@ atomic_set\n"
+"1: ldrex %0, [%1]\n"
+" strex %0, %2, [%1]\n"
+" teq %0, #0\n"
+" bne 1b"
+ : "=&r" (tmp)
+ : "r" (addr), "r" (v)
+ : "cc");
+}
+
+#else
+static inline void assign_bits(unsigned long v, unsigned long *addr)
+{
+ unsigned long flags;
+
+ raw_local_irq_save(flags);
+ *addr = v;
+ raw_local_irq_restore(flags);
+}
+#endif
+
#include <asm-generic/bitops/non-atomic.h>
/*
diff --git a/include/asm-arm26/bitops.h b/include/asm-arm26/bitops.h
index 19a6957..9b489c0 100644
--- a/include/asm-arm26/bitops.h
+++ b/include/asm-arm26/bitops.h
@@ -117,6 +117,15 @@ ____atomic_test_and_change_bit(unsigned
return res & mask;
}
+static inline void assign_bits(unsigned long v, unsigned long *addr)
+{
+ unsigned long flags;
+
+ local_irq_save(flags);
+ *addr = v;
+ local_irq_restore(flags);
+}
+
#include <asm-generic/bitops/non-atomic.h>
/*
diff --git a/include/asm-avr32/bitops.h b/include/asm-avr32/bitops.h
index 5299f8c..11d8fb0 100644
--- a/include/asm-avr32/bitops.h
+++ b/include/asm-avr32/bitops.h
@@ -230,6 +230,7 @@ static inline int test_and_change_bit(in
return (old & mask) != 0;
}
+#include <asm-generic/bitops/assign-bits.h>
#include <asm-generic/bitops/non-atomic.h>
/* Find First bit Set */
diff --git a/include/asm-cris/bitops.h b/include/asm-cris/bitops.h
index a569065..8ea3dc9 100644
--- a/include/asm-cris/bitops.h
+++ b/include/asm-cris/bitops.h
@@ -141,6 +141,7 @@ static inline int test_and_change_bit(in
return retval;
}
+#include <asm-generic/bitops/assign-bits.h>
#include <asm-generic/bitops/non-atomic.h>
/*
diff --git a/include/asm-frv/bitops.h b/include/asm-frv/bitops.h
index f8560ed..93fab5c 100644
--- a/include/asm-frv/bitops.h
+++ b/include/asm-frv/bitops.h
@@ -157,6 +157,7 @@ (__builtin_constant_p(nr) ? \
__constant_test_bit((nr),(addr)) : \
__test_bit((nr),(addr)))
+#include <asm-generic/bitops/assign-bits.h>
#include <asm-generic/bitops/find.h>
/**
diff --git a/include/asm-generic/bitops/assign-bits.h b/include/asm-generic/bitops/assign-bits.h
new file mode 100644
index 0000000..db6d5b2
--- /dev/null
+++ b/include/asm-generic/bitops/assign-bits.h
@@ -0,0 +1,32 @@
+/* Safely assign a value to a word modified by atomic bitops
+ *
+ * Copyright (C) 2006 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells ([email protected])
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#ifndef _ASM_GENERIC_BITOPS_ASSIGN_BITS_H
+#define _ASM_GENERIC_BITOPS_ASSIGN_BITS_H
+
+/**
+ * assign_bits - Safely assign to a word containing atomically modified bits
+ * @v: Value to assign to the word
+ * @addr: The word to modify
+ *
+ * Safely assign a value to a word that contains bits that are atomically
+ * modified by such as clear_bit() or test_and_set_bit().
+ *
+ * On some arches direct assignment isn't safe because the bitops have to be
+ * implemented using spinlocks to gain atomicity - which means they're only
+ * safe with respect to related operations.
+ */
+static inline void assign_bits(unsigned long v, unsigned long *addr)
+{
+ *addr = v;
+}
+
+#endif /* _ASM_GENERIC_BITOPS_ASSIGN_BITS_H */
diff --git a/include/asm-generic/bitops/atomic.h b/include/asm-generic/bitops/atomic.h
index 7833931..32d2ca7 100644
--- a/include/asm-generic/bitops/atomic.h
+++ b/include/asm-generic/bitops/atomic.h
@@ -188,4 +188,25 @@ static inline int test_and_change_bit(in
return (old & mask) != 0;
}
+/**
+ * assign_bits - Safely assign to a word containing atomically modified bits
+ * @v: Value to assign to the word
+ * @addr: The word to modify
+ *
+ * Safely assign a value to a word that contains bits that are atomically
+ * modified by such as clear_bit() or test_and_set_bit().
+ *
+ * Direct assignment isn't safe if bitops are implemented using spinlocks to
+ * gain atomicity - which means they're only safe with respect to related
+ * operations.
+ */
+static inline void assign_bits(unsigned long v, volatile unsigned long *addr)
+{
+ unsigned long flags;
+
+ _atomic_spin_lock_irqsave(addr, flags);
+ *addr = v;
+ _atomic_spin_unlock_irqrestore(addr, flags);
+}
+
#endif /* _ASM_GENERIC_BITOPS_ATOMIC_H */
diff --git a/include/asm-h8300/bitops.h b/include/asm-h8300/bitops.h
index d76299c..d5dea31 100644
--- a/include/asm-h8300/bitops.h
+++ b/include/asm-h8300/bitops.h
@@ -175,6 +175,7 @@ #undef H8300_GEN_TEST_BITOP_CONST
#undef H8300_GEN_TEST_BITOP_CONST_INT
#undef H8300_GEN_TEST_BITOP
+#include <asm-generic/bitops/assign-bits.h>
#include <asm-generic/bitops/ffs.h>
static __inline__ unsigned long __ffs(unsigned long word)
diff --git a/include/asm-i386/bitops.h b/include/asm-i386/bitops.h
index 1c780fa..51ea0bb 100644
--- a/include/asm-i386/bitops.h
+++ b/include/asm-i386/bitops.h
@@ -7,6 +7,7 @@ #define _I386_BITOPS_H
#include <linux/compiler.h>
#include <asm/alternative.h>
+#include <asm-generic/bitops/assign-bits.h>
/*
* These have to be done with inline assembly: that way the bit-setting
diff --git a/include/asm-ia64/bitops.h b/include/asm-ia64/bitops.h
index 6cc517e..452e565 100644
--- a/include/asm-ia64/bitops.h
+++ b/include/asm-ia64/bitops.h
@@ -12,6 +12,7 @@ #define _ASM_IA64_BITOPS_H
#include <linux/compiler.h>
#include <linux/types.h>
#include <asm/intrinsics.h>
+#include <asm-generic/bitops/assign-bits.h>
/**
* set_bit - Atomically set a bit in memory
diff --git a/include/asm-m32r/bitops.h b/include/asm-m32r/bitops.h
index 66ab672..bb67351 100644
--- a/include/asm-m32r/bitops.h
+++ b/include/asm-m32r/bitops.h
@@ -243,6 +243,7 @@ static __inline__ int test_and_change_bi
return (oldbit != 0);
}
+#include <asm-generic/bitops/assign-bits.h>
#include <asm-generic/bitops/non-atomic.h>
#include <asm-generic/bitops/ffz.h>
#include <asm-generic/bitops/__ffs.h>
diff --git a/include/asm-m68k/bitops.h b/include/asm-m68k/bitops.h
index 1a61fdb..8a302ce 100644
--- a/include/asm-m68k/bitops.h
+++ b/include/asm-m68k/bitops.h
@@ -9,6 +9,7 @@ #define _M68K_BITOPS_H
*/
#include <linux/compiler.h>
+#include <asm-generic/bitops/assign-bits.h>
/*
* Require 68020 or better.
diff --git a/include/asm-m68knommu/bitops.h b/include/asm-m68knommu/bitops.h
index d7fa7d9..75792ba 100644
--- a/include/asm-m68knommu/bitops.h
+++ b/include/asm-m68knommu/bitops.h
@@ -15,6 +15,7 @@ #include <asm-generic/bitops/ffs.h>
#include <asm-generic/bitops/__ffs.h>
#include <asm-generic/bitops/sched.h>
#include <asm-generic/bitops/ffz.h>
+#include <asm-generic/bitops/assign-bits.h>
static __inline__ void set_bit(int nr, volatile unsigned long * addr)
{
diff --git a/include/asm-mips/bitops.h b/include/asm-mips/bitops.h
index 06445de..cd18bf9 100644
--- a/include/asm-mips/bitops.h
+++ b/include/asm-mips/bitops.h
@@ -382,6 +382,7 @@ static inline int test_and_change_bit(un
smp_mb();
}
+#include <asm-generic/bitops/assign-bits.h>
#include <asm-generic/bitops/non-atomic.h>
/*
diff --git a/include/asm-parisc/bitops.h b/include/asm-parisc/bitops.h
index 9005619..416bd99 100644
--- a/include/asm-parisc/bitops.h
+++ b/include/asm-parisc/bitops.h
@@ -102,6 +102,15 @@ static __inline__ int test_and_change_bi
return (oldbit & mask) ? 1 : 0;
}
+static inline void assign_bits(unsigned long v, volatile unsigned long *addr)
+{
+ unsigned long flags;
+
+ _atomic_spin_lock_irqsave(addr, flags);
+ *addr = v;
+ _atomic_spin_unlock_irqrestore(addr, flags);
+}
+
#include <asm-generic/bitops/non-atomic.h>
#ifdef __KERNEL__
diff --git a/include/asm-powerpc/bitops.h b/include/asm-powerpc/bitops.h
index 8f757f6..28167b4 100644
--- a/include/asm-powerpc/bitops.h
+++ b/include/asm-powerpc/bitops.h
@@ -184,6 +184,7 @@ static __inline__ void set_bits(unsigned
: "cc");
}
+#include <asm-generic/bitops/assign-bits.h>
#include <asm-generic/bitops/non-atomic.h>
/*
diff --git a/include/asm-s390/bitops.h b/include/asm-s390/bitops.h
index f79c9b7..653b965 100644
--- a/include/asm-s390/bitops.h
+++ b/include/asm-s390/bitops.h
@@ -409,6 +409,8 @@ #define test_and_clear_bit test_and_cle
#define test_and_change_bit test_and_change_bit_simple
#endif
+#include <asm-generic/bitops/assign-bits.h>
+
/*
* This routine doesn't need to be atomic.
diff --git a/include/asm-sh/bitops.h b/include/asm-sh/bitops.h
index 1c16792..342d05a 100644
--- a/include/asm-sh/bitops.h
+++ b/include/asm-sh/bitops.h
@@ -98,6 +98,7 @@ static inline int test_and_change_bit(in
return retval;
}
+#include <asm-generic/bitops/assign-bits.h>
#include <asm-generic/bitops/non-atomic.h>
static inline unsigned long ffz(unsigned long word)
diff --git a/include/asm-sh64/bitops.h b/include/asm-sh64/bitops.h
index f3bdcdb..b299e6c 100644
--- a/include/asm-sh64/bitops.h
+++ b/include/asm-sh64/bitops.h
@@ -109,6 +109,7 @@ static __inline__ int test_and_change_bi
return retval;
}
+#include <asm-generic/bitops/assign-bits.h>
#include <asm-generic/bitops/non-atomic.h>
static __inline__ unsigned long ffz(unsigned long word)
diff --git a/include/asm-sparc/bitops.h b/include/asm-sparc/bitops.h
index 04aa331..fa729e4 100644
--- a/include/asm-sparc/bitops.h
+++ b/include/asm-sparc/bitops.h
@@ -152,6 +152,27 @@ static inline void change_bit(unsigned l
: "memory", "cc");
}
+static inline void assign_bits(unsigned long v, volatile unsigned long *addr)
+{
+ register unsigned long value asm("g2");
+ register unsigned long *ADDR asm("g1");
+ register int tmp1 asm("g3");
+ register int tmp2 asm("g4");
+ register int tmp3 asm("g5");
+ register int tmp4 asm("g7");
+
+ ADDR = (unsigned long *) addr;
+ value = v;
+
+ __asm__ __volatile__(
+ "mov %%o7, %%g4\n\t"
+ "call ___assign_bits\n\t"
+ " add %%o7, 8, %%o7\n"
+ : "=&r" (value), "=r" (tmp1), "=r" (tmp2), "=r" (tmp3), "=r" (tmp4)
+ : "0" (value), "r" (ADDR)
+ : "memory", "cc");
+}
+
#include <asm-generic/bitops/non-atomic.h>
#define smp_mb__before_clear_bit() do { } while(0)
diff --git a/include/asm-sparc64/bitops.h b/include/asm-sparc64/bitops.h
index 3d5e1af..a8f7ef1 100644
--- a/include/asm-sparc64/bitops.h
+++ b/include/asm-sparc64/bitops.h
@@ -17,6 +17,7 @@ extern void set_bit(unsigned long nr, vo
extern void clear_bit(unsigned long nr, volatile unsigned long *addr);
extern void change_bit(unsigned long nr, volatile unsigned long *addr);
+#include <asm-generic/bitops/assign-bits.h>
#include <asm-generic/bitops/non-atomic.h>
#ifdef CONFIG_SMP
diff --git a/include/asm-v850/bitops.h b/include/asm-v850/bitops.h
index 1fa99ba..3bd8fbb 100644
--- a/include/asm-v850/bitops.h
+++ b/include/asm-v850/bitops.h
@@ -22,6 +22,7 @@ #include <asm/system.h> /* interrupt en
#ifdef __KERNEL__
#include <asm-generic/bitops/ffz.h>
+#include <asm-generic/bitops/assign-bits.h>
/*
* The __ functions are not atomic
diff --git a/include/asm-x86_64/bitops.h b/include/asm-x86_64/bitops.h
index 5b535ea..3eaa848 100644
--- a/include/asm-x86_64/bitops.h
+++ b/include/asm-x86_64/bitops.h
@@ -6,6 +6,7 @@ #define _X86_64_BITOPS_H
*/
#include <asm/alternative.h>
+#include <asm-generic/bitops/assign-bits.h>
#define ADDR (*(volatile long *) addr)
Replace the direct assignment in set_wq_data() with a bitops-proofed wrapper
(assign_bits()). This defends against the test_and_set_bit() used to mark a
work item active.
Signed-Off-By: David Howells <[email protected]>
---
kernel/workqueue.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index db49886..f5e9540 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -97,7 +97,7 @@ static inline void set_wq_data(struct wo
new = (unsigned long) wq | (1UL << WORK_STRUCT_PENDING);
new |= work->management & WORK_STRUCT_FLAG_MASK;
- work->management = new;
+ assign_bits(new, &work->management);
}
static inline void *get_wq_data(struct work_struct *work)
On Tue, Dec 12, 2006 at 08:11:12PM +0000, David Howells wrote:
> diff --git a/include/asm-arm/bitops.h b/include/asm-arm/bitops.h
> index b41831b..5932134 100644
> --- a/include/asm-arm/bitops.h
> +++ b/include/asm-arm/bitops.h
> @@ -117,6 +117,32 @@ ____atomic_test_and_change_bit(unsigned
> return res & mask;
> }
>
> +#if __LINUX_ARM_ARCH__ >= 6 && defined(CONFIG_CPU_32v6K)
> +static inline void assign_bits(unsigned long v, unsigned long *addr)
> +{
> + unsigned long tmp;
> +
> + __asm__ __volatile__("@ atomic_set\n"
> +"1: ldrex %0, [%1]\n"
> +" strex %0, %2, [%1]\n"
> +" teq %0, #0\n"
> +" bne 1b"
> + : "=&r" (tmp)
> + : "r" (addr), "r" (v)
> + : "cc");
> +}
This seems to be a very silly question (and I'm bound to be utterly
wrong as proven in my last round) but why are we implementing a new
set of atomic primitives which effectively do the same thing as our
existing set?
Why can't we just use atomic_t for this?
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
Russell King writes:
> Why can't we just use atomic_t for this?
On 64-bit platforms, atomic_t tends to be 4 bytes, whereas bitops work
on arrays of unsigned long, i.e. multiples of 8 bytes. We could
use atomic_long_t for this, however.
Paul.
Russell King <[email protected]> wrote:
> This seems to be a very silly question (and I'm bound to be utterly
> wrong as proven in my last round) but why are we implementing a new
> set of atomic primitives which effectively do the same thing as our
> existing set?
>
> Why can't we just use atomic_t for this?
atomic_t is the wrong thing as it's basically an int, not an unsigned long.
atomic64_t/atomic_long_t is also probably the wrong thing to use as it's a
signed long (and the long is also volatile on some platforms - x86_64 for
example). Bitops operate on unsigned long.
But the most important point is that assign_bits() has to take the same pointer
type as test_bit(), set_bit(), test_and_set_bit(), etc., and none of those
operate on an atomic*_t.
We could change that, of course, but I don't fancy tackling the task just at
the moment. It oughtn't to be a difficult change, but there are a lot of flags
words in the kernel.
David
On Tue, 12 Dec 2006, Russell King wrote:
>
> This seems to be a very silly question (and I'm bound to be utterly
> wrong as proven in my last round) but why are we implementing a new
> set of atomic primitives which effectively do the same thing as our
> existing set?
>
> Why can't we just use atomic_t for this?
Well, others have answered that ("wrong sizes"), but I'm wavering on using
atomic_long_t. I have to admit that I'd rather not add a new accessor
function, when it _should_ be easier to use the current ones.
That does depend on every arch maintainer saying they're ok with mixing
bitops and "atomic*_t"s. It would also require us to at least add some
_minimal_ function to get at the actual value, and turn the pointer into a
"unsigned long *" for the bitop functions.
I would _hope_ that people hopefully already use the same locking for
atomic_t and for bitops, and that arch maintainers could just say "sure,
that works for me". Obvously, anybody with LL/SC or otherwise just
basically atomic bitops (which covers a fair part of the spectrum) should
be ok, but sparc and the usual cast of suspects would have to say that
it's ok.
Should we also just make the rule be that the architecture _must_ allow
the silly
(atomic_long_t *) -> (unsigned long *)
casting (so that we can make _one_ generic inline function that takes an
atomic_long_t and returns the same pointer as an "unsigned long *" to make
bitop functions happy), or would this have to be another arch-specific
function?
Comments?
Linus
Linus Torvalds wrote:
>
> On Tue, 12 Dec 2006, Russell King wrote:
>
>>This seems to be a very silly question (and I'm bound to be utterly
>>wrong as proven in my last round) but why are we implementing a new
>>set of atomic primitives which effectively do the same thing as our
>>existing set?
>>
>>Why can't we just use atomic_t for this?
>
>
> Well, others have answered that ("wrong sizes"), but I'm wavering on using
> atomic_long_t. I have to admit that I'd rather not add a new accessor
> function, when it _should_ be easier to use the current ones.
I agree.
> That does depend on every arch maintainer saying they're ok with mixing
> bitops and "atomic*_t"s. It would also require us to at least add some
> _minimal_ function to get at the actual value, and turn the pointer into a
> "unsigned long *" for the bitop functions.
>
> I would _hope_ that people hopefully already use the same locking for
> atomic_t and for bitops, and that arch maintainers could just say "sure,
> that works for me". Obvously, anybody with LL/SC or otherwise just
> basically atomic bitops (which covers a fair part of the spectrum) should
> be ok, but sparc and the usual cast of suspects would have to say that
> it's ok.
parisc seems to, but sparc uses its own open coded spinlock for bitops, and
the array of regular spinlocks for atomic ops. OTOH, consolidating them
might give more scalable code *and* a smaller cacheline footprint?
> Should we also just make the rule be that the architecture _must_ allow
> the silly
>
> (atomic_long_t *) -> (unsigned long *)
>
> casting (so that we can make _one_ generic inline function that takes an
> atomic_long_t and returns the same pointer as an "unsigned long *" to make
> bitop functions happy), or would this have to be another arch-specific
> function?
>
> Comments?
AFAIK no architecture does anything special, so maybe a generic converter
would be best, until one comes along that does. (the only thing of note
really is that half of the atomics use volatile types and half don't, is
that a problem?).
--
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com
On Wed, 13 Dec 2006, Nick Piggin wrote:
>
> parisc seems to, but sparc uses its own open coded spinlock for bitops, and
> the array of regular spinlocks for atomic ops. OTOH, consolidating them
> might give more scalable code *and* a smaller cacheline footprint?
Yeah, I think you'd actually end up with better behaviour by just sharing
the lock logic, so I don't think there are any downsides there.
> > Should we also just make the rule be that the architecture _must_ allow the
> > silly
> >
> > (atomic_long_t *) -> (unsigned long *)
> >
> > casting (so that we can make _one_ generic inline function that takes an
> > atomic_long_t and returns the same pointer as an "unsigned long *" to make
> > bitop functions happy), or would this have to be another arch-specific
> > function?
> >
> > Comments?
>
> AFAIK no architecture does anything special, so maybe a generic converter
> would be best, until one comes along that does. (the only thing of note
> really is that half of the atomics use volatile types and half don't, is
> that a problem?).
No, the cast would cast away any such differences, and since anybody would
have to use asm for the actual implementation, the code can't care about
the absense or presense of "volatile" anyway.
Linus
On Wed, 13 Dec 2006, Nick Piggin wrote:
> Linus Torvalds wrote:
> >
> > On Tue, 12 Dec 2006, Russell King wrote:
> > >
> > > Why can't we just use atomic_t for this?
> >
> >
> > Well, others have answered that ("wrong sizes"), but I'm wavering on using
> > atomic_long_t. I have to admit that I'd rather not add a new accessor
> > function, when it _should_ be easier to use the current ones.
>
> I agree.
Ok, nobody ever did anything about this, so here's my try.
This uses "atomic_long_t" for the workstruct "data" field, which shares
the per-cpu pointer and the workstruct flag bits in one field.
This ONLY works if "atomic_set()" is actually atomic wrt the atomic bit
operations too, which is generally true on any architecture that does
atomics natively (or on UP when done by disabling interrupts), but may not
be true on architectures where atomic operations are faked with spinlocks,
and the two different kinds of atomic ops use different spinlocks.
Right now sparc32 is one such architecture, possibly the only one. It
would need to be fixed. Davem?
NOTE! This patch also depends on an unrealted fix that I already pushed
out, which fixes "delayed_work_pending()" which was totally broken before
(macro expansion would replace "work" whether it was used as a variable
_or_ as a struct member name). If that hasn't mirrored out yet, you should
just fix the "delayed_work_pending()" macro to look like
#define delayed_work_pending(w) \
work_pending(&(w)->work)
(ie use the "work_pending()" macro to do the heavy lifting, and do NOT use
the name "work" for the macro argument).
This is untested, other than to see that it compiles. It all looks very
obvious, but then, all my code always does, yet somehow bugs still creep
in occasionally. I personally suspect it's some really subtle SMTP bug
that corrupts my patches, but I've never caught it outright.
Anyway. It's bug-free-by-definition, since it's written by yours truly,
but people should probably test it and comment on it, in the unlikely
event that the evil gnomes lurking in the intarnet tubes corrupt it.
Comments?
Linus
---
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 5b13dcf..2a7b38d 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -8,16 +8,21 @@
#include <linux/timer.h>
#include <linux/linkage.h>
#include <linux/bitops.h>
+#include <asm/atomic.h>
struct workqueue_struct;
struct work_struct;
typedef void (*work_func_t)(struct work_struct *work);
+/*
+ * The first word is the work queue pointer and the flags rolled into
+ * one
+ */
+#define work_data_bits(work) ((unsigned long *)(&(work)->data))
+
struct work_struct {
- /* the first word is the work queue pointer and the flags rolled into
- * one */
- unsigned long management;
+ atomic_long_t data;
#define WORK_STRUCT_PENDING 0 /* T if work item pending execution */
#define WORK_STRUCT_NOAUTOREL 1 /* F if work item automatically released on exec */
#define WORK_STRUCT_FLAG_MASK (3UL)
@@ -26,6 +31,9 @@ struct work_struct {
work_func_t func;
};
+#define WORK_DATA_INIT(autorelease) \
+ ATOMIC_LONG_INIT((autorelease) << WORK_STRUCT_NOAUTOREL)
+
struct delayed_work {
struct work_struct work;
struct timer_list timer;
@@ -36,13 +44,13 @@ struct execute_work {
};
#define __WORK_INITIALIZER(n, f) { \
- .management = 0, \
+ .data = WORK_DATA_INIT(0), \
.entry = { &(n).entry, &(n).entry }, \
.func = (f), \
}
#define __WORK_INITIALIZER_NAR(n, f) { \
- .management = (1 << WORK_STRUCT_NOAUTOREL), \
+ .data = WORK_DATA_INIT(1), \
.entry = { &(n).entry, &(n).entry }, \
.func = (f), \
}
@@ -82,17 +90,21 @@ struct execute_work {
/*
* initialize all of a work item in one go
+ *
+ * NOTE! No point in using "atomic_long_set()": useing a direct
+ * assignment of the work data initializer allows the compiler
+ * to generate better code.
*/
#define INIT_WORK(_work, _func) \
do { \
- (_work)->management = 0; \
+ (_work)->data = (atomic_long_t) WORK_DATA_INIT(0); \
INIT_LIST_HEAD(&(_work)->entry); \
PREPARE_WORK((_work), (_func)); \
} while (0)
#define INIT_WORK_NAR(_work, _func) \
do { \
- (_work)->management = (1 << WORK_STRUCT_NOAUTOREL); \
+ (_work)->data = (atomic_long_t) WORK_DATA_INIT(1); \
INIT_LIST_HEAD(&(_work)->entry); \
PREPARE_WORK((_work), (_func)); \
} while (0)
@@ -114,7 +126,7 @@ struct execute_work {
* @work: The work item in question
*/
#define work_pending(work) \
- test_bit(WORK_STRUCT_PENDING, &(work)->management)
+ test_bit(WORK_STRUCT_PENDING, work_data_bits(work))
/**
* delayed_work_pending - Find out whether a delayable work item is currently
@@ -143,7 +155,7 @@ struct execute_work {
* This should also be used to release a delayed work item.
*/
#define work_release(work) \
- clear_bit(WORK_STRUCT_PENDING, &(work)->management)
+ clear_bit(WORK_STRUCT_PENDING, work_data_bits(work))
extern struct workqueue_struct *__create_workqueue(const char *name,
@@ -188,7 +200,7 @@ static inline int cancel_delayed_work(struct delayed_work *work)
ret = del_timer_sync(&work->timer);
if (ret)
- clear_bit(WORK_STRUCT_PENDING, &work->work.management);
+ work_release(&work->work);
return ret;
}
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index db49886..742cbbe 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -96,13 +96,13 @@ static inline void set_wq_data(struct work_struct *work, void *wq)
BUG_ON(!work_pending(work));
new = (unsigned long) wq | (1UL << WORK_STRUCT_PENDING);
- new |= work->management & WORK_STRUCT_FLAG_MASK;
- work->management = new;
+ new |= WORK_STRUCT_FLAG_MASK & *work_data_bits(work);
+ atomic_long_set(&work->data, new);
}
static inline void *get_wq_data(struct work_struct *work)
{
- return (void *) (work->management & WORK_STRUCT_WQ_DATA_MASK);
+ return (void *) (atomic_long_read(&work->data) & WORK_STRUCT_WQ_DATA_MASK);
}
static int __run_work(struct cpu_workqueue_struct *cwq, struct work_struct *work)
@@ -133,7 +133,7 @@ static int __run_work(struct cpu_workqueue_struct *cwq, struct work_struct *work
list_del_init(&work->entry);
spin_unlock_irqrestore(&cwq->lock, flags);
- if (!test_bit(WORK_STRUCT_NOAUTOREL, &work->management))
+ if (!test_bit(WORK_STRUCT_NOAUTOREL, work_data_bits(work)))
work_release(work);
f(work);
@@ -206,7 +206,7 @@ int fastcall queue_work(struct workqueue_struct *wq, struct work_struct *work)
{
int ret = 0, cpu = get_cpu();
- if (!test_and_set_bit(WORK_STRUCT_PENDING, &work->management)) {
+ if (!test_and_set_bit(WORK_STRUCT_PENDING, work_data_bits(work))) {
if (unlikely(is_single_threaded(wq)))
cpu = singlethread_cpu;
BUG_ON(!list_empty(&work->entry));
@@ -248,7 +248,7 @@ int fastcall queue_delayed_work(struct workqueue_struct *wq,
if (delay == 0)
return queue_work(wq, work);
- if (!test_and_set_bit(WORK_STRUCT_PENDING, &work->management)) {
+ if (!test_and_set_bit(WORK_STRUCT_PENDING, work_data_bits(work))) {
BUG_ON(timer_pending(timer));
BUG_ON(!list_empty(&work->entry));
@@ -280,7 +280,7 @@ int queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
struct timer_list *timer = &dwork->timer;
struct work_struct *work = &dwork->work;
- if (!test_and_set_bit(WORK_STRUCT_PENDING, &work->management)) {
+ if (!test_and_set_bit(WORK_STRUCT_PENDING, work_data_bits(work))) {
BUG_ON(timer_pending(timer));
BUG_ON(!list_empty(&work->entry));
@@ -321,7 +321,7 @@ static void run_workqueue(struct cpu_workqueue_struct *cwq)
spin_unlock_irqrestore(&cwq->lock, flags);
BUG_ON(get_wq_data(work) != cwq);
- if (!test_bit(WORK_STRUCT_NOAUTOREL, &work->management))
+ if (!test_bit(WORK_STRUCT_NOAUTOREL, work_data_bits(work)))
work_release(work);
f(work);
On Fri, 2006-12-15 at 14:45 -0800, Linus Torvalds wrote:
> This uses "atomic_long_t" for the workstruct "data" field, which shares
> the per-cpu pointer and the workstruct flag bits in one field.
This fixes drivers/connector/connector.c to cope...
Signed-off-by: David Woodhouse <[email protected]>
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index 5e7cd45..27f377b 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -135,9 +135,8 @@ static int cn_call_callback(struct cn_msg *msg, void (*destruct_data)(void *), v
spin_lock_bh(&dev->cbdev->queue_lock);
list_for_each_entry(__cbq, &dev->cbdev->queue_list, callback_entry) {
if (cn_cb_equal(&__cbq->id.id, &msg->id)) {
- if (likely(!test_bit(WORK_STRUCT_PENDING,
- &__cbq->work.work.management) &&
- __cbq->data.ddata == NULL)) {
+ if (likely(!work_pending(&__cbq->work.work) &&
+ __cbq->data.ddata == NULL)) {
__cbq->data.callback_priv = msg;
__cbq->data.ddata = data;
--
dwmw2
On Mon, Dec 18, 2006 at 08:56:24AM +0000, David Woodhouse ([email protected]) wrote:
> On Fri, 2006-12-15 at 14:45 -0800, Linus Torvalds wrote:
> > This uses "atomic_long_t" for the workstruct "data" field, which shares
> > the per-cpu pointer and the workstruct flag bits in one field.
>
> This fixes drivers/connector/connector.c to cope...
>
> Signed-off-by: David Woodhouse <[email protected]>
Yep, there are already four different patches to fix it.
Proper one will be pushed through David Miller's netdev tree splitted into
fix and delayed work removal.
Thanks David.
> --
> dwmw2
--
Evgeniy Polyakov
From: David Woodhouse <[email protected]>
Date: Mon, 18 Dec 2006 08:56:24 +0000
> On Fri, 2006-12-15 at 14:45 -0800, Linus Torvalds wrote:
> > This uses "atomic_long_t" for the workstruct "data" field, which shares
> > the per-cpu pointer and the workstruct flag bits in one field.
>
> This fixes drivers/connector/connector.c to cope...
>
> Signed-off-by: David Woodhouse <[email protected]>
I have a fix for this already in my net-2.6 tree and I'll push it
later tonight.