login
Search
Hi, OK, so here's version #2 of the patchset. Changes since the last posting: - Better Kconfig setup, thanks Sam. - Fix data leak, thanks Linus. - Misplaced sparc64 bit in the s390 patch, thanks Martin. - Arm should work, thanks Catalin. -- Jens Axboe --
This converts parisc to use the new helpers for smp_call_function() and
friends, and adds support for smp_call_function_single(). Not tested,
not even compiled.
Cc: Kyle McMartin <kyle@parisc-linux.org>
Cc: Matthew Wilcox <matthew@wil.cx>
Cc: Grant Grundler <grundler@parisc-linux.org>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
arch/parisc/Kconfig | 1 +
arch/parisc/kernel/smp.c | 134 +++++++--------------------------------------
2 files changed, 22 insertions(+), 113 deletions(-)
diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index bc7a19d..a7d4fd3 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -199,6 +199,7 @@ endchoice
config SMP
bool "Symmetric multi-processing support"
+ select USE_GENERIC_SMP_HELPERS
---help---
This enables support for systems with more than one CPU. If you have
a system with only one CPU, like most personal computers, say N. If
diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
index 85fc775..126105c 100644
--- a/arch/parisc/kernel/smp.c
+++ b/arch/parisc/kernel/smp.c
@@ -84,19 +84,11 @@ EXPORT_SYMBOL(cpu_possible_map);
DEFINE_PER_CPU(spinlock_t, ipi_lock) = SPIN_LOCK_UNLOCKED;
-struct smp_call_struct {
- void (*func) (void *info);
- void *info;
- long wait;
- atomic_t unstarted_count;
- atomic_t unfinished_count;
-};
-static volatile struct smp_call_struct *smp_call_function_data;
-
enum ipi_message_type {
IPI_NOP=0,
IPI_RESCHEDULE=1,
IPI_CALL_FUNC,
+ IPI_CALL_FUNC_SINGLE,
IPI_CPU_START,
IPI_CPU_STOP,
IPI_CPU_TEST
@@ -187,33 +179,12 @@ ipi_interrupt(int irq, void *dev_id)
case IPI_CALL_FUNC:
smp_debug(100, KERN_DEBUG "CPU%d IPI_CALL_FUNC\n", this_cpu);
- {
- volatile struct smp_call_struct *data;
- void (*func)(void *info);
- void *info;
- int wait;
-
- data = smp_call_function_data;
- func = data->func;
- info = data->info;
- wait = data->wait;
-
- mb();
- atomic_dec ...This converts sh to use the new helpers for smp_call_function() and
friends, and adds support for smp_call_function_single(). Not tested,
but it compiles.
Cc: Paul Mundt <lethal@linux-sh.org>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
arch/sh/Kconfig | 1 +
arch/sh/kernel/smp.c | 48 ++++++++----------------------------------------
include/asm-sh/smp.h | 12 ++----------
3 files changed, 11 insertions(+), 50 deletions(-)
diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index 6a679c3..60cfdf5 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -698,6 +698,7 @@ config CRASH_DUMP
config SMP
bool "Symmetric multi-processing support"
depends on SYS_SUPPORTS_SMP
+ select USE_GENERIC_SMP_HELPERS
---help---
This enables support for systems with more than one CPU. If you have
a system with only one CPU, like most personal computers, say N. If
diff --git a/arch/sh/kernel/smp.c b/arch/sh/kernel/smp.c
index 5d039d1..2ed8dce 100644
--- a/arch/sh/kernel/smp.c
+++ b/arch/sh/kernel/smp.c
@@ -36,13 +36,6 @@ EXPORT_SYMBOL(cpu_possible_map);
cpumask_t cpu_online_map;
EXPORT_SYMBOL(cpu_online_map);
-static atomic_t cpus_booted = ATOMIC_INIT(0);
-
-/*
- * Run specified function on a particular processor.
- */
-void __smp_call_function(unsigned int cpu);
-
static inline void __init smp_store_cpu_info(unsigned int cpu)
{
struct sh_cpuinfo *c = cpu_data + cpu;
@@ -178,42 +171,17 @@ void smp_send_stop(void)
smp_call_function(stop_this_cpu, 0, 1, 0);
}
-struct smp_fn_call_struct smp_fn_call = {
- .lock = __SPIN_LOCK_UNLOCKED(smp_fn_call.lock),
- .finished = ATOMIC_INIT(0),
-};
-
-/*
- * The caller of this wants the passed function to run on every cpu. If wait
- * is set, wait until all cpus have finished the function before returning.
- * The lock is here to protect the call structure.
- * You must not call this function with disabled interrupts or from a
- * hardware interrupt handler or from a bottom half handler.
- ...Acked-by: Paul Mundt <lethal@linux-sh.org> --
This converts s390 to use the new helpers for smp_call_function() and
friends, and adds support for smp_call_function_single(). Not tested,
but it compiles.
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
arch/s390/Kconfig | 1 +
arch/s390/kernel/smp.c | 160 +++--------------------------------------------
include/asm-s390/sigp.h | 1 +
include/asm-s390/smp.h | 2 -
4 files changed, 10 insertions(+), 154 deletions(-)
diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index f6a68e1..b8f5fda 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -90,6 +90,7 @@ config 32BIT
config SMP
bool "Symmetric multi-processing support"
+ select USE_GENERIC_SMP_HELPERS
---help---
This enables support for systems with more than one CPU. If you have
a system with only one CPU, like most personal computers, say N. If
diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
index 0dfa988..2ee3484 100644
--- a/arch/s390/kernel/smp.c
+++ b/arch/s390/kernel/smp.c
@@ -77,164 +77,18 @@ static DEFINE_PER_CPU(struct cpu, cpu_devices);
static void smp_ext_bitcall(int, ec_bit_sig);
-/*
- * Structure and data for __smp_call_function_map(). This is designed to
- * minimise static memory requirements. It also looks cleaner.
- */
-static DEFINE_SPINLOCK(call_lock);
-
-struct call_data_struct {
- void (*func) (void *info);
- void *info;
- cpumask_t started;
- cpumask_t finished;
- int wait;
-};
-
-static struct call_data_struct *call_data;
-
-/*
- * 'Call function' interrupt callback
- */
-static void do_call_function(void)
+void arch_send_call_function_ipi(cpumask_t mask)
{
- void (*func) (void *info) = call_data->func;
- void *info = call_data->info;
- int wait = call_data->wait;
-
- cpu_set(smp_processor_id(), call_data->started);
- (*func)(info);
- if (wait)
- cpu_set(smp_processor_id(), call_data->finished);;
-}
-
-static ...Your generic patch changes semantics of smp_call_function(). At least the s390 version of smp_call_function() mask used to not return unless all IPIs where received on the target cpus. Your patch changes that. It will already return as soon as all IPIs have been sent. Unfortunately Martin's etr code in arch/s390/kernel/time.c relies on exactly the old semantics. Since this seems to be an s390 only issue, could you just drop the s390 conversion patch? We'll have to think about how to change the err code and convert later I guess. --
OK, I could not see anything which relied on that and I did think that it was a somewhat odd interface. By the time that smp_call_function() stops waiting for startups, we could easily be in the condition that the N-1 CPUs executed the function part a long time ago. I just didn't see much usefulness in that, you may as well just set wait == 1 in that I'll drop the s390 bits. I can easily add something like smp_call_function_foo() that has this behaviour. -- Jens Axboe --
I added an smp_call_function_sync() for this purpose and kept the s390 conversion, I would very much appreciate if you could look it over. I wont post the full patchset again today, but you can inspect it here: http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=refs/heads/generic-ipi The interesting bits are patch #1 for the generic bits and patch #11 for the s390 part. Thanks! -- Jens Axboe --
I don't think that works: the old code also relied on the fact that there could be only one cpu sending an smp_call_* IPI (serialized by call_lock). The current etr code exploits that to serialize all cpus: Do smp_call_function() to force all other cpus into clock_sync_cpu_start() where they sort of busy wait(!). Then the master cpu is syncing the clock and when it finished the other cpus may continue again and finally leave the smp_call_function interrupt handler. Your patch series however doesn't make sure anymore that there is only one cpu doing an smp_call_function*, so we may deadlock as soon as two cpus are trying to synchronize all cpus this way. After all I think the etr code should be converted to use stop_machine_run instead. However that doesn't allow for master/slave cpus. --
Changing the etr code to stop_machine_run() will require a common code change. We can use the fn() parameter of stop_machine_run() for the cpu that does the clock synchronization but all the other cpus are not just disabled. They need to do something before going back to normal operation, namely waiting for the clock to get back into sync and then reprogram the clock comparator before we can allow interrupts again. Looking at the stop_machine() code it doesn't seem to be to hard to get this implemented. stop_machine_run() would get a second function argument that replaces the call to cpu_relax() in stopmachine(). The default if the additional function argument is NULL would be cpu_relax(). For the etr sync code it would be etr_sync_cpu_start(). -- blue skies, Martin. "Reality continues to ruin my life." - Calvin. --
Sure. I've got changes for stop_machine planned, so please send a patch and I'll merge it in. Cheers, Rusty. --
You are right. I'll drop the code again, it would be nice to get rid of this interface, I guess s390 can do that when they convert to the generic helpers. -- Jens Axboe --
This adds kernel/smp.c which contains helpers for IPI function calls. In
addition to supporting the existing smp_call_function() in a more efficient
manner, it also adds a more scalable variant called smp_call_function_single()
for calling a given function on a single CPU only.
The core of this is based on the x86-64 patch from Nick Piggin, lots of
changes since then. "Alan D. Brunelle" <Alan.Brunelle@hp.com> has
contributed lots of fixes and suggestions as well.
Acked-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
arch/Kconfig | 3 +
include/linux/smp.h | 27 ++++-
init/main.c | 3 +
kernel/Makefile | 1 +
kernel/smp.c | 366 +++++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 398 insertions(+), 2 deletions(-)
create mode 100644 kernel/smp.c
diff --git a/arch/Kconfig b/arch/Kconfig
index 694c9af..a5a0184 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -36,3 +36,6 @@ config HAVE_KPROBES
config HAVE_KRETPROBES
def_bool n
+
+config USE_GENERIC_SMP_HELPERS
+ def_bool n
diff --git a/include/linux/smp.h b/include/linux/smp.h
index 55232cc..4a5418b 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -7,9 +7,19 @@
*/
#include <linux/errno.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/cpumask.h>
extern void cpu_idle(void);
+struct call_single_data {
+ struct list_head list;
+ void (*func) (void *info);
+ void *info;
+ unsigned int flags;
+};
+
#ifdef CONFIG_SMP
#include <linux/preempt.h>
@@ -53,9 +63,23 @@ extern void smp_cpus_done(unsigned int max_cpus);
* Call a function on all other processors
*/
int smp_call_function(void(*func)(void *info), void *info, int retry, int wait);
-
+int smp_call_function_mask(cpumask_t mask, void(*func)(void *info), void *info,
+ int wait);
int smp_call_function_single(int cpuid, void (*func) (void *info), void *info,
int retry, int wait);
+void ...What deadlock are you concerned with here? Would making cfd_fallback per-cpu make you feel better? -- Jens Axboe --
CPU0 CPU1
local_irq_disable() local_irq_disable()
smp_call_function_single(0,..,0)
test_and_set_bit_lock()
send IPI
smp_call_function_single(1,..,0)
while(test_and_set_bit_lock())
cpu_relax();
This will spin forever, because it needs to handle the IPI in order to
free the cfd_fallback thingy, but can't for its waiting for it.
That particular deadlock can indeed be solved by making cfd_fallback
per-cpu.
But if you were to use multiple smp_call_function*() calls under a
single IRQ disabled, then that would not be sufficient. Now I can't
directly come up with a good reason to need to do that, but still.
You'd need somethine like:
local_irq_disable()
smp_call_function_single(n, func_a,..,0)
smp_call_function_single(m, func_b,..,0)
local_irq_enable()
And invite 3 cpus to the party while under memory pressure and you get
deadlock potential.
[ if it were both the same function, you'd want to use
smp_call_function() and provide a mask; if it were the same cpu you'd
want to call a function doing both ]
--
Right, that is the case I was thinking of. I added per-cpu fallbacks to I think that is plenty far off into theoretical country that we can get by with just documenting this limitation. Nobody is doing that currently in the kernel, and I see no practical use case for it. -- Jens Axboe --
Probably a silly question, but what does data->refs do that cpus_empty(data->cpumask) wouldn't do? (as indeed ARM presently does.) -- Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: --
I guess it can be marginally slower for NR_CPUS > BITS_PER_LONG, otherwise there's absolutely no reason to have a seperate ref counter. -- Jens Axboe --
Jes was concerned about scanning bitmasks on a 4096 CPU Altix. I'm not
sure its all that important, but a refcount check would definitely be
quicker.
J
--
I just felt it was silly to do a bigger test if it wasn't necessary. Even on a 4096 CPU box it's probably barely noticeable, but if it adds cost then I'd be in favor of keeping the slightly faster version. Maybe it would be worth doing a branched version, one for NR_CPUS <= BITS_PER_LONG and one for the other case? Cheers, Jes --
and we can lose this ifdef here. Also, init_call_single_data() is __cpuinit but isn't declared that way. There have been rare occasions (FRV was one) where this matters - iirc the compiler was emitting a short-relative-addressing-form instruction which which wasn't able to branch far enough once things were linked. We hae this problem in eight zillion other places, of course. And it's a pita to go adding __cpunit etc to the declaration because the compiler usually won't tell us when it gets out of sync with reality. So we could leave he for_each_possible_cpu()? Do we _have_ to consider all possible CPUs here? That can be much larger Is this a must-be-called-with-local-interrupts-enable function? (It doesn't say so in the covering comment) If so, a runtime check for that would be needed - we screw that up regularly. Ditto any other places where A runtime check would be nice. The get_cpu() will give us partial coverage Sigh. irqs are disabled, so this is a waste of CPU cycles. With some of cfd_fallback_used is rather mysterious - it could do with a comment at its definition site. I'm wondering if we should/could be doing bit_spin_lock --
Erm, that's a bug in the frv toolchain, isn't it? The linker should *never* rely on C code annotation for jump lengths ... mainly because you can screw it all up again in the linker script, so the sectional annotations should only be in the function body (think -ffunction-sections) We have exactly this problem on parisc: our standard relocation jumps are only 17 bits (on ILP32). The linker actually lays down special sandwich sections between individual functions (places where we jump to as a 17 bit relative and then do another absolute jump via a pointer). The sandwich sections should be added at final link time every place the linker sees the relative jump is out of range. James --
The problem with FRV has to do with global variables, not functions. With FRV, we attempt to pack as many small variables into the "small data section" (.sdata) as we can, and then retain a pointer to it in a register. Thus for small variables, we can use a single instruction, such as: LDI @(GR16,%gprel(my_variable)),GRx to access it. However, this limits the offset of my_variable to be +/-2048 from GR16 because all the instructions are the same size, and that's the size of the offset field therein. Alternatively, we can use: SETHI.P %hi(my_variable),GRy SETLO %lo(my_variable),GRy LD @(GRy,GR0),GRx which obviously takes two more instructions, although they can be executed simultaneously, and 8 more bytes of RAM/icache. However, the compiler will assume that it should access _all_ global variables using GPREL-addressing unless it is told otherwise. This means that if a global variable should not be GPREL-addressed, its *declaration* should be annotated, as this affects how it is addressed. Furthermore, if the variable is *defined* in C, that definition should be annotated the same as the declaration, otherwise it'll eat storage from .sdata rather than the appropriate section. Global functions are another matter. The FRV's PC+offset call and branch instructions have 24-bit displacements, which is fine for leaping about within the kernel core with a single instruction. In modules, we instead (a) suppress GPREL addressing entirely because we can't put module variables in .sdata, and (b) make all interfunction jumps that the assembler can't resolve into: SETHI.P %hi(that_function),GRy SETLO %lo(that_function),GRy JMPL @(GRy,GR0) because a 24-bit offset can only span 16MB, which isn't far enough. We could use a GOT, but I believe that would add an extra memory access to any memory access or jump - which I'd prefer to avoid. David --
Not necessarily; this is how we work on PARISC with a GOT as the ELF spec defines. The point is that the GOT is linker constructed, so we only lay down GOT entries when the linker sees that relative displacements fail, so they don't consume memory (or even get brought into existence) in the ordinary case. The flip side of this is that we do double jump (relative to jump through got) for the long displacements we can't reach. James --
Where is the new smp_call_function() defined? I see only the declaration.
Probably just blind...
But don't we need the smp_mb() -after- the CSD_FLAG_WAIT check?
We want the caller to see the effects of the on-other-CPU-called
Well, one question... Is it really OK to clear this bit immediately?
Why don't you need to wait for a grace period to elapse before freeing it?
(OK, I eventually saw why you can't wait for a grace period, but you
To safely omit the rcu_read_lock(), Jens would need to use the in-flight
call_rcu_sched() instead of call_rcu(). In addition, if we ever get
threaded interrupts, this interrupt handler would need IRQ_NODELAY.
How about using list_for_each_entry_rcu() instead?
list_for_each_entry_rcu(pos, &call_function_queue, csd.list) {
int refs;
And then move the definition of "data" up to replace that of "pos".
Need an smp_mb() here, at least in the case where you are using the
fallback queue element. See my comments in smp_call_function_mask()
for the justification -- the key point is that if the CPU sees itself
in the mask, it also needs to see the data element all filled out.
See below for a scenario requiring this.
Such a memory barrier is -not- needed for the normally allocated
queue elements.
But memory barriers are a bit ugly... How about instead acquiring the
And the above statements are one reason that I believe you need to wait
for a grace period even when using the static fallback element. Here
is the sequence of events that I am worried about:
o CPU 0 is the last CPU to invoke a given queue element, and is just
about to list_del_rcu() it.
o CPU 1 gets the interrupt (but already did this block last time).
It nevertheless finds the queue element, because CPU 0 has not
yet list_del_rcu() it.
CPU 1 is just about to execute the:
"if (!cpu_isset(cpu, data->cpumask))"
o CPU 0 now does the list_del_rcu() that CPU 1 already has a
reference to, and eventually invokes the ...Thanks a lot! I'll go over these tomorrow, the fallback graceperiod definitely looks valid. -- Jens Axboe --
We could do online CPUs and use a notifier for hotplug, but does it If you think that is too large to inline, then can we inline anything? It's a barrier and a andl basically, a handful of instructions perhaps? That said, I don't care much either way. It's a sync wait, so it need Yes I know, I can add it to the comment instead. And even if it wasn't OK, will do. This one is to serialize data/csd stores so they are seen Probably not for __smp_call_function_single(), but the others should be The bit lock is smaller, but apart from that there should be zero functional difference. -- Jens Axboe --
This converts x86 and x86-64 to use the new helpers for smp_call_function() and friends, and adds support for smp_call_function_single(). Acked-by: Ingo Molnar <mingo@elte.hu> Signed-off-by: Jens Axboe <jens.axboe@oracle.com> --- arch/x86/Kconfig | 1 + arch/x86/kernel/apic_32.c | 4 + arch/x86/kernel/entry_64.S | 3 + arch/x86/kernel/i8259_64.c | 4 + arch/x86/kernel/smp.c | 148 ++++------------------------ arch/x86/kernel/smpcommon.c | 56 ----------- arch/x86/mach-voyager/voyager_smp.c | 91 +++-------------- arch/x86/xen/enlighten.c | 1 - arch/x86/xen/mmu.c | 2 +- arch/x86/xen/smp.c | 108 ++++++--------------- include/asm-x86/hw_irq_32.h | 1 + include/asm-x86/hw_irq_64.h | 2 + include/asm-x86/mach-default/entry_arch.h | 1 + include/asm-x86/mach-default/irq_vectors.h | 1 + include/asm-x86/mach-voyager/entry_arch.h | 2 +- include/asm-x86/mach-voyager/irq_vectors.h | 4 +- include/asm-x86/smp.h | 10 -- 17 files changed, 84 insertions(+), 355 deletions(-) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 87a693c..033062e 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -159,6 +159,7 @@ config GENERIC_PENDING_IRQ config X86_SMP bool depends on SMP && ((X86_32 && !X86_VOYAGER) || X86_64) + select USE_GENERIC_SMP_HELPERS default y config X86_32_SMP diff --git a/arch/x86/kernel/apic_32.c b/arch/x86/kernel/apic_32.c index 6872081..750a555 100644 --- a/arch/x86/kernel/apic_32.c +++ b/arch/x86/kernel/apic_32.c @@ -1357,6 +1357,10 @@ void __init smp_intr_init(void) /* IPI for generic function call */ set_intr_gate(CALL_FUNCTION_VECTOR, call_function_interrupt); + + /* IPI for single call function ...
Xen vs native is a runtime choice, so you can't just statically define
arch_send_call_function(_single)_ipi for each. You need to define the
arch_* functions once, make them call via smp_ops, and call into the
respective native and Xen versions of the ipi functions.
Aside from that it looks good, with a very appealing code size reduction.
J
--
Noted, I'll add it to smp_ops (like smp_call_function_mask() was Thanks! -- Jens Axboe --
[ Ingo added to cc, since this is x86-specific ]
Ok, one more comment..
Why bother with separate vectors for this?
Why not just make the single vector do
void smp_call_function_interrupt(void)
{
ack_APIC_irq();
irq_enter();
generic_smp_call_function_single_interrupt();
generic_smp_call_function_interrupt();
#ifdef CONFIG_X86_32
__get_cpu_var(irq_stat).irq_call_count++;
#else
add_pda(irq_call_count, 1);
#endif
irq_exit();
}
since they are both doing the exact same thing anyway?
Do we really require us to be able to handle the "single" case _while_ a
"multiple" case is busy? Aren't we running all of these things with
interrupts disabled anyway, so that it cannot happen?
Or is it just a performance optimization? Do we expect to really have so
many of the multiple interrupts that it's expensive to walk the list just
because we also had a single interrupt to another CPU? That sounds a bit
unlikely, but if true, very interesting..
Inquiring minds want to know..
Linus
--
yes and i gave in - Nick and Jens wants to do some crazy stuff and they had the numbers. Here's the previous discussion: http://lkml.org/lkml/2008/2/27/125 Ingo --
No, the previous discussion was about single *queues* vs single *vectors*. I agree unconditionally with the decision to use a separate per-cpu queue from the shared queue (in fact, I would argue that the "mask" code might want to notice when the mask is just a single CPU, and turn a mask request into a targeted request). But I wonder why we want to then have two IPI target vectors, when it would appear to be perfectly fine and cheap to have just a single vector that can handle both the per-cpu case and the shared queue case (since the thing would tend to be one or the other, not both). A single vector is still pefectly fine, if 99% of all usage cases are the targeted-to-a-single-cpu thing, because the shared queue will basically be empty (and you can test that without even taking any locks). Linus --
ok. In which case the reschedule vector could be consolidated into that as well (it's just a special single-CPU call). Then there would be no new vector allocations needed at all, just the renaming of RESCHEDULE_VECTOR to something more generic. Ingo --
Yes. Btw, don't get me wrong - I'm not against multiple vectors per se. I just wonder if there is any real reason for the code duplication. And there certainly *can* be tons of valid reasons for it. For example, some of the LAPIC can only have something like two pending interrupts per vector, and after that IPI's would get lost. However, since the queuing is actually done with the data structures, I don't think it matters for the IPI's - they don't need any hardware queuing at all, afaik, since even if two IPI's would be merged into one (due to lack of hw queueing) the IPI handling code still has its list of events, so it doesn't matter. And performance can be a valid reason ("too expensive to check the shared queue if we only have per-cpu events"), although I$ issues can cause that argument to go both ways. I was also wondering whether there are deadlock issues (ie one type of IPI has to complete even if a lock is held for the other type). So I don't dislike the patch per se, I just wanted to understand _why_ the IPI's wanted separate vectors. Linus --
The "too expensive to check the shared queue" is one aspect of it. The shared queue need not have events *for us* (at least, unless Jens has changed the implementation a bit) but it can still have events that we would need to check through. I don't think deadlock is a problem (any more than with multiple vectors). --
That is still the case, the loop works the same way still. To answer Linus' question on why it was done the way it was - the thought of sharing the IPI just didn't occur to me. For performance reasons I'd like to keep the current setup, but it's certainly a viable alternative for archs with limited number of IPIs available (like the mips case that Ralf has disclosed). -- Jens Axboe --
I'm happy with it either way, but as pointed out by Ralf, for the MIPS version of the patch there aren't even other vectors available. So sharing the vector is going to be a requirement on other architectures, maybe we should look at whether we can make it generic and peform well regardless? Linus --
I don't think it will perform terribly _poorly_ by sharing the vector. If you have a look at how serialised the old code is... If there is a noticable slowdown anywhere, we could I suppose create a 2nd variant for small numbers of CPUs, or shared vector case, but I'd say the existing code should be fine I think. --
Regarding that last comment... The reason why I'm doing this work is because I want to use smp_call_function_single() to redirect IO completions. So there WILL be lots of smp_call_function_single_interrupt() interrupts, they will be a lot more prevalent than smp_call_function() interrupts. I don't have any numbers on this since I haven't tried collapsing them all, but I'd be surprised if it wasn't noticable. That said, some archs do use a single IPI for multiple actions and just keep a bitmask of what to do in that IPI. So it would still be possible to use a single hardware IPI to do various things, without resorting to calling into the interrupt handler for each of them. The _single() interrupt handler is a cheap check though, an smp memory barrier and a list_empty() check is enough (like it currently does). -- Jens Axboe --
This converts ppc to use the new helpers for smp_call_function() and
friends, and adds support for smp_call_function_single().
ppc loses the timeout functionality of smp_call_function_mask() with
this change, as the generic code does not provide that.
Acked-by: Paul Mackerras <paulus@samba.org>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/kernel/smp.c | 220 ++-----------------------------
arch/powerpc/platforms/cell/interrupt.c | 1 +
arch/powerpc/platforms/ps3/smp.c | 7 +-
arch/powerpc/platforms/pseries/xics.c | 6 +-
arch/powerpc/sysdev/mpic.c | 2 +-
include/asm-powerpc/smp.h | 5 +-
7 files changed, 23 insertions(+), 219 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 4bb2e93..f2a985b 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -110,6 +110,7 @@ config PPC
select HAVE_KPROBES
select HAVE_KRETPROBES
select HAVE_LMB
+ select USE_GENERIC_SMP_HELPERS if SMP
config EARLY_PRINTK
bool
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index be35ffa..facd49d 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -72,12 +72,8 @@ struct smp_ops_t *smp_ops;
static volatile unsigned int cpu_callin_map[NR_CPUS];
-void smp_call_function_interrupt(void);
-
int smt_enabled_at_boot = 1;
-static int ipi_fail_ok;
-
static void (*crash_ipi_function_ptr)(struct pt_regs *) = NULL;
#ifdef CONFIG_PPC64
@@ -99,12 +95,15 @@ void smp_message_recv(int msg)
{
switch(msg) {
case PPC_MSG_CALL_FUNCTION:
- smp_call_function_interrupt();
+ generic_smp_call_function_interrupt();
break;
case PPC_MSG_RESCHEDULE:
/* XXX Do we have to do this? */
set_need_resched();
break;
+ case PPC_MSG_CALL_FUNC_SINGLE:
+ generic_smp_call_function_single_interrupt();
+ break;
case PPC_MSG_DEBUGGER_BREAK:
if ...This converts ia64 to use the new helpers for smp_call_function() and
friends, and adds support for smp_call_function_single().
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
arch/ia64/Kconfig | 1 +
arch/ia64/kernel/smp.c | 239 +++---------------------------------------------
include/asm-ia64/smp.h | 3 -
3 files changed, 16 insertions(+), 227 deletions(-)
diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index ed21737..beae928 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -296,6 +296,7 @@ config VIRT_CPU_ACCOUNTING
config SMP
bool "Symmetric multi-processing support"
+ select USE_GENERIC_SMP_HELPERS
help
This enables support for systems with more than one CPU. If you have
a system with only one CPU, say N. If you have a system with more
diff --git a/arch/ia64/kernel/smp.c b/arch/ia64/kernel/smp.c
index 9a9d4c4..c5dcd03 100644
--- a/arch/ia64/kernel/smp.c
+++ b/arch/ia64/kernel/smp.c
@@ -60,25 +60,9 @@ static struct local_tlb_flush_counts {
static DEFINE_PER_CPU(unsigned int, shadow_flush_counts[NR_CPUS]) ____cacheline_aligned;
-
-/*
- * Structure and data for smp_call_function(). This is designed to minimise static memory
- * requirements. It also looks cleaner.
- */
-static __cacheline_aligned DEFINE_SPINLOCK(call_lock);
-
-struct call_data_struct {
- void (*func) (void *info);
- void *info;
- long wait;
- atomic_t started;
- atomic_t finished;
-};
-
-static volatile struct call_data_struct *call_data;
-
#define IPI_CALL_FUNC 0
#define IPI_CPU_STOP 1
+#define IPI_CALL_FUNC_SINGLE 2
#define IPI_KDUMP_CPU_STOP 3
/* This needs to be cacheline aligned because it is written to by *other* CPUs. */
@@ -89,13 +73,13 @@ extern void cpu_halt (void);
void
lock_ipi_calllock(void)
{
- spin_lock_irq(&call_lock);
+ spin_lock_irq(&call_function_lock);
}
void
unlock_ipi_calllock(void)
...This converts alpha to use the new helpers for smp_call_function() and
friends, and adds support for smp_call_function_single().
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
arch/alpha/Kconfig | 1 +
arch/alpha/kernel/core_marvel.c | 6 +-
arch/alpha/kernel/smp.c | 170 +++------------------------------------
include/asm-alpha/smp.h | 2 -
4 files changed, 14 insertions(+), 165 deletions(-)
diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
index 729cdbd..dbe8c28 100644
--- a/arch/alpha/Kconfig
+++ b/arch/alpha/Kconfig
@@ -528,6 +528,7 @@ config ARCH_MAY_HAVE_PC_FDC
config SMP
bool "Symmetric multi-processing support"
depends on ALPHA_SABLE || ALPHA_LYNX || ALPHA_RAWHIDE || ALPHA_DP264 || ALPHA_WILDFIRE || ALPHA_TITAN || ALPHA_GENERIC || ALPHA_SHARK || ALPHA_MARVEL
+ select USE_GENERIC_SMP_HELPERS
---help---
This enables support for systems with more than one CPU. If you have
a system with only one CPU, like most personal computers, say N. If
diff --git a/arch/alpha/kernel/core_marvel.c b/arch/alpha/kernel/core_marvel.c
index f10d2ed..45cc4f2 100644
--- a/arch/alpha/kernel/core_marvel.c
+++ b/arch/alpha/kernel/core_marvel.c
@@ -660,9 +660,9 @@ __marvel_rtc_io(u8 b, unsigned long addr, int write)
#ifdef CONFIG_SMP
if (smp_processor_id() != boot_cpuid)
- smp_call_function_on_cpu(__marvel_access_rtc,
- &rtc_access, 1, 1,
- cpumask_of_cpu(boot_cpuid));
+ smp_call_function_single(boot_cpuid,
+ __marvel_access_rtc,
+ &rtc_access, 1, 1);
else
__marvel_access_rtc(&rtc_access);
#else
diff --git a/arch/alpha/kernel/smp.c b/arch/alpha/kernel/smp.c
index 63c2073..95c905b 100644
--- a/arch/alpha/kernel/smp.c
+++ b/arch/alpha/kernel/smp.c
@@ -62,6 +62,7 @@ static struct {
enum ipi_message_type {
IPI_RESCHEDULE,
IPI_CALL_FUNC,
+ IPI_CALL_FUNC_SINGLE,
IPI_CPU_STOP,
};
@@ -558,51 +559,6 @@ send_ipi_message(cpumask_t to_whom, enum ipi_message_type ...This converts arm to use the new helpers for smp_call_function() and
friends, and adds support for smp_call_function_single().
Fixups and testing done by Catalin Marinas <catalin.marinas@arm.com>
Cc: Russell King <rmk@arm.linux.org.uk>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
arch/arm/Kconfig | 1 +
arch/arm/kernel/smp.c | 157 +++++--------------------------------------------
2 files changed, 16 insertions(+), 142 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d8d2532..fed48c4 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -663,6 +663,7 @@ source "kernel/time/Kconfig"
config SMP
bool "Symmetric Multi-Processing (EXPERIMENTAL)"
depends on EXPERIMENTAL && (REALVIEW_EB_ARM11MP || MACH_REALVIEW_PB11MP)
+ select USE_GENERIC_SMP_HELPERS
help
This enables support for systems with more than one CPU. If you have
a system with only one CPU, like most personal computers, say N. If
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index eefae1d..6344466 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -68,20 +68,10 @@ enum ipi_msg_type {
IPI_TIMER,
IPI_RESCHEDULE,
IPI_CALL_FUNC,
+ IPI_CALL_FUNC_SINGLE,
IPI_CPU_STOP,
};
-struct smp_call_struct {
- void (*func)(void *info);
- void *info;
- int wait;
- cpumask_t pending;
- cpumask_t unfinished;
-};
-
-static struct smp_call_struct * volatile smp_call_function_data;
-static DEFINE_SPINLOCK(smp_call_function_lock);
-
int __cpuinit __cpu_up(unsigned int cpu)
{
struct cpuinfo_arm *ci = &per_cpu(cpu_data, cpu);
@@ -366,114 +356,15 @@ static void send_ipi_message(cpumask_t callmap, enum ipi_msg_type msg)
local_irq_restore(flags);
}
-/*
- * You must not call this function with disabled interrupts, from a
- * hardware interrupt handler, nor from a bottom half handler.
- */
-static int smp_call_function_on_cpu(void (*func)(void *info), void *info,
- int retry, int wait, cpumask_t callmap)
-{
- struct ...This converts m32r to use the new helpers for smp_call_function() and
friends, and adds support for smp_call_function_single(). Not tested,
not even compiled.
Cc: Hirokazu Takata <takata@linux-m32r.org>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
arch/m32r/Kconfig | 1 +
arch/m32r/kernel/smp.c | 128 +++++-----------------------------------------
arch/m32r/kernel/traps.c | 3 +-
include/asm-m32r/smp.h | 1 +
4 files changed, 17 insertions(+), 116 deletions(-)
diff --git a/arch/m32r/Kconfig b/arch/m32r/Kconfig
index de153de..a5f864c 100644
--- a/arch/m32r/Kconfig
+++ b/arch/m32r/Kconfig
@@ -296,6 +296,7 @@ config PREEMPT
config SMP
bool "Symmetric multi-processing support"
+ select USE_GENERIC_SMP_HELPERS
---help---
This enables support for systems with more than one CPU. If you have
a system with only one CPU, like most personal computers, say N. If
diff --git a/arch/m32r/kernel/smp.c b/arch/m32r/kernel/smp.c
index c837bc1..74eb7bc 100644
--- a/arch/m32r/kernel/smp.c
+++ b/arch/m32r/kernel/smp.c
@@ -35,22 +35,6 @@
/*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*=*/
/*
- * Structure and data for smp_call_function(). This is designed to minimise
- * static memory requirements. It also looks cleaner.
- */
-static DEFINE_SPINLOCK(call_lock);
-
-struct call_data_struct {
- void (*func) (void *info);
- void *info;
- atomic_t started;
- atomic_t finished;
- int wait;
-} __attribute__ ((__aligned__(SMP_CACHE_BYTES)));
-
-static struct call_data_struct *call_data;
-
-/*
* For flush_cache_all()
*/
static DEFINE_SPINLOCK(flushcache_lock);
@@ -96,9 +80,6 @@ void smp_invalidate_interrupt(void);
void smp_send_stop(void);
static void stop_this_cpu(void *);
-int smp_call_function(void (*) (void *), void *, int, int);
-void smp_call_function_interrupt(void);
-
void smp_send_timer(void);
void smp_ipi_timer_interrupt(struct pt_regs *);
void smp_local_timer_interrupt(void);
@@ ...This converts mips to use the new helpers for smp_call_function() and
friends, and adds support for smp_call_function_single(). Not tested,
but it compiles.
Cc: Ralf Baechle <ralf@linux-mips.org>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
arch/mips/Kconfig | 1 +
arch/mips/kernel/smp-mt.c | 27 ++++++++-
arch/mips/kernel/smp.c | 133 +++-------------------------------------
arch/mips/kernel/smtc.c | 7 ++
arch/mips/sibyte/bcm1480/smp.c | 3 +
arch/mips/sibyte/sb1250/smp.c | 2 +
include/asm-mips/smp.h | 12 +---
7 files changed, 49 insertions(+), 136 deletions(-)
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 8724ed3..5092a9e 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -1742,6 +1742,7 @@ config SMP
bool "Multi-Processing support"
depends on SYS_SUPPORTS_SMP
select IRQ_PER_CPU
+ select USE_GENERIC_SMP_HELPERS
help
This enables support for systems with more than one CPU. If you have
a system with only one CPU, like most personal computers, say N. If
diff --git a/arch/mips/kernel/smp-mt.c b/arch/mips/kernel/smp-mt.c
index 89e6f6a..31049fc 100644
--- a/arch/mips/kernel/smp-mt.c
+++ b/arch/mips/kernel/smp-mt.c
@@ -38,8 +38,9 @@
#define MIPS_CPU_IPI_RESCHED_IRQ 0
#define MIPS_CPU_IPI_CALL_IRQ 1
+#define MIPS_CPU_IPI_CALL_SINGLE_IRQ 2
-static int cpu_ipi_resched_irq, cpu_ipi_call_irq;
+static int cpu_ipi_resched_irq, cpu_ipi_call_irq, cpu_ipi_call_single_irq;
#if 0
static void dump_mtregisters(int vpe, int tc)
@@ -115,6 +116,11 @@ static void ipi_call_dispatch(void)
do_IRQ(MIPS_CPU_IRQ_BASE + MIPS_CPU_IPI_CALL_IRQ);
}
+static void ipi_call_single_dispatch(void)
+{
+ do_IRQ(MIPS_CPU_IRQ_BASE + MIPS_CPU_IPI_CALL_SINGLE_IRQ);
+}
+
static irqreturn_t ipi_resched_interrupt(int irq, void *dev_id)
{
return IRQ_HANDLED;
@@ -127,6 +133,13 @@ static irqreturn_t ipi_call_interrupt(int irq, void *dev_id)
return IRQ_HANDLED;
}
+static ...The smp-mt.c has no chance of working. There are only two hardware interrupts available so just using one more won't quite work. But I like the basic idea of this patch series. Ralf --
I'll switch mips over to sharing the IPI for both ipi function calls then, thanks Ralf! -- Jens Axboe --
