Fix up some PA-RISC work items broken by the workstruct reduction.
Signed-Off-By: David Howells <[email protected]>
---
drivers/parisc/led.c | 12 ++++++------
drivers/parisc/power.c | 4 ++--
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/parisc/led.c b/drivers/parisc/led.c
index 8dac2ba..6818c10 100644
--- a/drivers/parisc/led.c
+++ b/drivers/parisc/led.c
@@ -66,8 +66,8 @@ static char lcd_text_default[32] __read
static struct workqueue_struct *led_wq;
-static void led_work_func(void *);
-static DECLARE_WORK(led_task, led_work_func, NULL);
+static void led_work_func(struct work_struct *);
+static DECLARE_DELAYED_WORK(led_task, led_work_func);
#if 0
#define DPRINTK(x) printk x
@@ -136,7 +136,7 @@ static int start_task(void)
/* Create the work queue and queue the LED task */
led_wq = create_singlethread_workqueue("led_wq");
- queue_work(led_wq, &led_task);
+ queue_delayed_work(led_wq, &led_task, 0);
return 0;
}
@@ -443,7 +443,7 @@ #define HEARTBEAT_2ND_RANGE_END (HEART
#define LED_UPDATE_INTERVAL (1 + (HZ*19/1000))
-static void led_work_func (void *unused)
+static void led_work_func (struct work_struct *unused)
{
static unsigned long last_jiffies;
static unsigned long count_HZ; /* counter in range 0..HZ */
@@ -590,7 +590,7 @@ int __init register_led_driver(int model
/* Ensure the work is queued */
if (led_wq) {
- queue_work(led_wq, &led_task);
+ queue_delayed_work(led_wq, &led_task, 0);
}
return 0;
@@ -660,7 +660,7 @@ int lcd_print( char *str )
/* re-queue the work */
if (led_wq) {
- queue_work(led_wq, &led_task);
+ queue_delayed_work(led_wq, &led_task, 0);
}
return lcd_info.lcd_width;
diff --git a/drivers/parisc/power.c b/drivers/parisc/power.c
index 97e9dc0..9228e21 100644
--- a/drivers/parisc/power.c
+++ b/drivers/parisc/power.c
@@ -82,7 +82,7 @@ #define __getDIAG(dr) ( { \
} )
-static void deferred_poweroff(void *dummy)
+static void deferred_poweroff(struct work_struct *unused)
{
if (kill_cad_pid(SIGINT, 1)) {
/* just in case killing init process failed */
@@ -96,7 +96,7 @@ static void deferred_poweroff(void *dumm
* use schedule_work().
*/
-static DECLARE_WORK(poweroff_work, deferred_poweroff, NULL);
+static DECLARE_WORK(poweroff_work, deferred_poweroff);
static void poweroff(void)
{
Add assign_bits() to give an atomic-bitops safe assignment on all archs without
the need for cmpxchg().
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.
Emulation by simply disabling interrupts on a UP-only arch should be sufficient
to permit direct assignment to work, provided NMI doesn't try to do bitops.
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 1f70d47..3af3a03 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 c341063..04fe968 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)
Use direct assignment rather than cmpxchg() as the latter is unavailable and
unimplementable on some platforms and is actually unnecessary.
The use of cmpxchg() was to guard against two possibilities, neither of which
can actually occur:
(1) The pending flag may have been unset or may be cleared. However, given
where it's called, the pending flag is _always_ set. I don't think it
can be unset whilst we're in set_wq_data().
Once the work is enqueued to be actually run, the only way off the queue
is for it to be actually run.
If it's a delayed work item, then the bit can't be cleared by the timer
because we haven't started the timer yet. Also, the pending bit can't be
cleared by cancelling the delayed work _until_ the work item has had its
timer started.
(2) The workqueue pointer might change. This can only happen in two cases:
(a) The work item has just been queued to actually run, and so we're
protected by the appropriate workqueue spinlock.
(b) A delayed work item is being queued, and so the timer hasn't been
started yet, and so no one else knows about the work item or can
access it (the pending bit protects us).
Besides, set_wq_data() _sets_ the workqueue pointer unconditionally, so
it can be assigned instead.
So, replacing the set_wq_data() with a straight assignment would be okay in
most cases. The problem is where we end up tangling with test_and_set_bit()
emulated using spinlocks, and even then it's not a problem _provided_
test_and_set_bit() doesn't attempt to modify the word if the bit was set.
If that's a problem, then a bitops-proofed assignment will be required -
equivalent to atomic_set() vs other atomic_xxx() ops.
Signed-Off-By: David Howells <[email protected]>
---
kernel/workqueue.c | 21 +++++++++------------
1 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 8d1e7cb..f5f3819 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -80,22 +80,19 @@ static inline int is_single_threaded(str
return list_empty(&wq->list);
}
+/*
+ * Set the workqueue on which a work item is to be run
+ * - Must *only* be called if the pending flag is set
+ */
static inline void set_wq_data(struct work_struct *work, void *wq)
{
- unsigned long new, old, res;
+ unsigned long new;
+
+ BUG_ON(!work_pending(work));
- /* assume the pending flag is already set and that the task has already
- * been queued on this workqueue */
new = (unsigned long) wq | (1UL << WORK_STRUCT_PENDING);
- res = work->management;
- if (res != new) {
- do {
- old = res;
- new = (unsigned long) wq;
- new |= (old & WORK_STRUCT_FLAG_MASK);
- res = cmpxchg(&work->management, old, new);
- } while (res != old);
- }
+ new |= work->management & WORK_STRUCT_FLAG_MASK;
+ assign_bits(new, &work->management);
}
static inline void *get_wq_data(struct work_struct *work)
Russell King <[email protected]> wrote:
> These are the constant versions, where the compiler can optimise the
> mask and word offset itself.
So my inclusion of ARM is correct... Under some circumstances it will write
to the target word when it wouldn't actually make a change:
static inline int
____atomic_test_and_set_bit(unsigned int bit, volatile unsigned long *p)
{
unsigned long flags;
unsigned int res;
unsigned long mask = 1UL << (bit & 31);
p += bit >> 5;
raw_local_irq_save(flags);
res = *p;
*p = res | mask;
raw_local_irq_restore(flags);
return res & mask;
}
Remember: *p is volatile.
David
On Thu, 07 Dec 2006 15:31:43 +0000
David Howells <[email protected]> wrote:
> Use direct assignment rather than cmpxchg() as the latter is unavailable and
> unimplementable on some platforms and is actually unnecessary.
>
> The use of cmpxchg() was to guard against two possibilities, neither of which
> can actually occur:
>
> (1) The pending flag may have been unset or may be cleared. However, given
> where it's called, the pending flag is _always_ set. I don't think it
> can be unset whilst we're in set_wq_data().
>
> Once the work is enqueued to be actually run, the only way off the queue
> is for it to be actually run.
>
> If it's a delayed work item, then the bit can't be cleared by the timer
> because we haven't started the timer yet. Also, the pending bit can't be
> cleared by cancelling the delayed work _until_ the work item has had its
> timer started.
>
> (2) The workqueue pointer might change. This can only happen in two cases:
>
> (a) The work item has just been queued to actually run, and so we're
> protected by the appropriate workqueue spinlock.
>
> (b) A delayed work item is being queued, and so the timer hasn't been
> started yet, and so no one else knows about the work item or can
> access it (the pending bit protects us).
>
> Besides, set_wq_data() _sets_ the workqueue pointer unconditionally, so
> it can be assigned instead.
>
> So, replacing the set_wq_data() with a straight assignment would be okay in
> most cases. The problem is where we end up tangling with test_and_set_bit()
> emulated using spinlocks, and even then it's not a problem _provided_
> test_and_set_bit() doesn't attempt to modify the word if the bit was set.
>
> If that's a problem, then a bitops-proofed assignment will be required -
> equivalent to atomic_set() vs other atomic_xxx() ops.
>
I don't understand, as usual.
afacit in all (but one) cases we do
if (!test_and_set_bit(WORK_STRUCT_PENDING, &work->management)) {
...
set_wq_data(work, wq);
...
<now do stuff which makes it possible for run_workqueue()
to get a look at the new work>
}
cancel_delayed_work() looks OK too.
The possible exception is schedule_on_each_cpu() which is being lazy, but
looks fixable.
So... afaict in all the places where there can be a concurrent
set_wq_data() and test_and_set_bit(), WORK_STRUCT_PENDING is reliably set,
and we can assume (and ensure) that a failing test_and_set_bit() will not
write to the affected word at all.
What am I missing?
On Thu, 07 Dec 2006 20:06:39 +0000
David Howells <[email protected]> wrote:
> Andrew Morton <[email protected]> wrote:
>
> > and we can assume (and ensure) that a failing test_and_set_bit() will not
> > write to the affected word at all.
>
> You may not assume that; and indeed that is not so in the generic
> spinlock-based bitops or ARM pre-v6 or PA-RISC or sparc32 or ...
Ah. How obnoxious of them.
> Remember: if you have to put a conditional jump in there, it's going to fail
> one way or the other a certain percentage of the time, and that's going to
> cause a pipeline stall, and these ops are used quite a lot.
>
> OTOH, I don't know that the stall would be that bad since the spin_lock and
> spin_unlock may cause a stall anyway.
>
Yes, the branch would cost. But in not uncommon cases that branch will save
the machine from dirtying a cacheline.
And if we add those branches, we bring those architectures' semantics in
line with all the other architectures. And we get better semantics
overall.
So I don't think we should rule this out.
James Bottomley <[email protected]> wrote:
> That we'd have to put a conditional jump in there is an incorrect
> assumption on risc machines.
I didn't say we would *have* to put a conditional jump in there. But I don't
know that I it can be avoided on all archs.
I don't know that sparc32 can do conditional instructions for example. If we
force this assumption it becomes a potential limitation on the archs we can
support. OTOH, it may be that every arch that supports SMP and has to emulate
bitops with spinlocks also supports conditional stores; but I don't know that.
David
Andrew Morton <[email protected]> wrote:
> and we can assume (and ensure) that a failing test_and_set_bit() will not
> write to the affected word at all.
You may not assume that; and indeed that is not so in the generic
spinlock-based bitops or ARM pre-v6 or PA-RISC or sparc32 or ...
Remember: if you have to put a conditional jump in there, it's going to fail
one way or the other a certain percentage of the time, and that's going to
cause a pipeline stall, and these ops are used quite a lot.
OTOH, I don't know that the stall would be that bad since the spin_lock and
spin_unlock may cause a stall anyway.
David
From: David Howells <[email protected]>
Date: Thu, 07 Dec 2006 21:16:03 +0000
> I don't know that sparc32 can do conditional instructions for
> example. If we force this assumption it becomes a potential
> limitation on the archs we can support. OTOH, it may be that every
> arch that supports SMP and has to emulate bitops with spinlocks also
> supports conditional stores; but I don't know that.
Sparc32 has normal branches but no conditional instruction execution.
On Thu, Dec 07, 2006 at 08:06:39PM +0000, David Howells wrote:
> Andrew Morton <[email protected]> wrote:
>
> > and we can assume (and ensure) that a failing test_and_set_bit() will not
> > write to the affected word at all.
>
> You may not assume that; and indeed that is not so in the generic
> spinlock-based bitops or ARM pre-v6 or PA-RISC or sparc32 or ...
Incorrect. pre-v6 ARM bitops for test_and_xxx_bit() all do:
save and disable irqs
load value
test bit
if not in desired state, alter bit and write it back
restore irqs
but I don't guarantee that we'll always do that - indeed, post-armv6
bitops always write back even if the bit was in the desired state.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Thu, Dec 07, 2006 at 11:58:23PM +0000, David Howells wrote:
> Russell King <[email protected]> wrote:
>
> > Incorrect. pre-v6 ARM bitops for test_and_xxx_bit() all do:
> >
> > save and disable irqs
> > load value
> > test bit
> > if not in desired state, alter bit and write it back
> > restore irqs
>
> Hmmm... ARM has two implementations. One in the header files which is what I
> consulted when writing that email:
>
> static inline void ____atomic_set_bit(unsigned int bit, volatile unsigned long *p)
> {
> unsigned long flags;
> unsigned long mask = 1UL << (bit & 31);
>
> p += bit >> 5;
>
> raw_local_irq_save(flags);
> *p |= mask;
> raw_local_irq_restore(flags);
> }
>
> And the other in the libs which does as you say. Why the one in the header
> file at all?
These are the constant versions, where the compiler can optimise the
mask and word offset itself.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Thu, 2006-12-07 at 20:06 +0000, David Howells wrote:
> Remember: if you have to put a conditional jump in there, it's going to fail
> one way or the other a certain percentage of the time, and that's going to
> cause a pipeline stall, and these ops are used quite a lot.
That we'd have to put a conditional jump in there is an incorrect
assumption on risc machines. At least on parisc we can do conditional
nullifies in the executing instruction pipeline, so we'd read, nullify
the following write if the bit were set. We do this a lot in our page
interruption handlers.
James
On Thu, Dec 07, 2006 at 03:31:38PM +0000, David Howells wrote:
> Fix up some PA-RISC work items broken by the workstruct reduction.
>
> Signed-Off-By: David Howells <[email protected]>
applied, thanks.
Russell King <[email protected]> wrote:
> Incorrect. pre-v6 ARM bitops for test_and_xxx_bit() all do:
>
> save and disable irqs
> load value
> test bit
> if not in desired state, alter bit and write it back
> restore irqs
Hmmm... ARM has two implementations. One in the header files which is what I
consulted when writing that email:
static inline void ____atomic_set_bit(unsigned int bit, volatile unsigned long *p)
{
unsigned long flags;
unsigned long mask = 1UL << (bit & 31);
p += bit >> 5;
raw_local_irq_save(flags);
*p |= mask;
raw_local_irq_restore(flags);
}
And the other in the libs which does as you say. Why the one in the header
file at all?
David