Re: [PATCH 2/2]: softirq: Add support for triggering softirq work on softirqs.

Previous thread: [PATCH 1/2]: softirq: Define and use NR_SOFTIRQ by David Miller on Friday, September 19, 2008 - 11:48 pm. (1 message)

Next thread: [PATCH] max3100 driver by Christian Pellegrin on Saturday, September 20, 2008 - 12:20 am. (12 messages)
From: David Miller
Date: Friday, September 19, 2008 - 11:48 pm

softirq: Add support for triggering softirq work on softirqs.

This is basically a genericization of Jens Axboe's block layer
remote softirq changes.

Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
 include/linux/interrupt.h |    6 ++-
 kernel/softirq.c          |  103 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+), 1 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index fdd7b90..91ca3ac 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -11,6 +11,8 @@
 #include <linux/hardirq.h>
 #include <linux/sched.h>
 #include <linux/irqflags.h>
+#include <linux/smp.h>
+#include <linux/percpu.h>
 #include <asm/atomic.h>
 #include <asm/ptrace.h>
 #include <asm/system.h>
@@ -271,7 +273,9 @@ extern void softirq_init(void);
 #define __raise_softirq_irqoff(nr) do { or_softirq_pending(1UL << (nr)); } while (0)
 extern void raise_softirq_irqoff(unsigned int nr);
 extern void raise_softirq(unsigned int nr);
-
+DECLARE_PER_CPU(struct list_head, softirq_work_list[NR_SOFTIRQ]);
+extern void __send_remote_softirq(struct call_single_data *cp, int cpu, int this_cpu, int softirq);
+extern void send_remote_softirq(struct call_single_data *cp, int cpu, int softirq);
 
 /* Tasklets --- multithreaded analogue of BHs.
 
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 27642a2..9c0723d 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -6,6 +6,8 @@
  *	Distribute under GPLv2.
  *
  *	Rewritten. Old one was good in 2.2, but in 2.3 it was immoral. --ANK (990903)
+ *
+ *	Remote softirq infrastructure is by Jens Axboe.
  */
 
 #include <linux/module.h>
@@ -463,17 +465,118 @@ void tasklet_kill(struct tasklet_struct *t)
 
 EXPORT_SYMBOL(tasklet_kill);
 
+DEFINE_PER_CPU(struct list_head, softirq_work_list[NR_SOFTIRQ]);
+
+static void __local_trigger(struct call_single_data *cp, int softirq)
+{
+	struct list_head *head = ...
From: Andrew Morton
Date: Saturday, September 20, 2008 - 12:46 am

Took a little staring to work out what that test is doing.  Adding

	/* If the list was previouly empty, trigger a softirq run */


CONFIG_USE_GENERIC_SMP_HELPERS=y, CONFIG_SMP=n shouldn't be possible,

OK, now what's going on with call_single_data.flags?

After a bit of grepping around it seems that it's a bitfield consisting
of the undocumented CSD_FLAG_WAIT and/or CSD_FLAG_ALLOC, which are
(somewhat strangely) private to kernel/smp.c.

IOW: some comments describing call_single_data.flags are somewhat
needed.  That documentation should include the locking rules, which
afaict are "local to this CPU, with local interrupts disabled".

Seems from the above `>> 16' you're stuffing the softirq ID into the
upper 16 bits of call_single_data.flags.  Is it needed?  Another u32
can be added to call_single_data for free on 64-bit, or it could be
split into two u16's.

Otherwise I'd suggest that local variable `softirq' here be changed to


Ah.  So it is required that the __send_remote_softirq() caller disable
local interrupts.  There's my locking.  It's worth a mention in the

It's best to provide some documentation for global, exported-to-modules

Could use __local_trigger() here I think, although that wouldn't really

--

From: David Miller
Date: Saturday, September 20, 2008 - 3:35 am

From: Andrew Morton <akpm@linux-foundation.org>

call_single_data is already too large, simply adding it to
struct sk_buff had negative performance impacts that are
forcing me to trim the size of the existing structure a bit.

I think just documenting the usage of the bits is better, and
I'll do that in my next rev.
--

From: David Miller
Date: Saturday, September 20, 2008 - 3:56 am

From: Andrew Morton <akpm@linux-foundation.org>







This is a splice, so we'd need to iterate which would be cumbersome
and error prone.

Here is the updated patch.

softirq: Add support for triggering softirq work on softirqs.

This is basically a genericization of Jens Axboe's block layer
remote softirq changes.

Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
 include/linux/interrupt.h |   13 +++++
 include/linux/smp.h       |    4 +-
 kernel/softirq.c          |  128 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 144 insertions(+), 1 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index fdd7b90..d8713c7 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -11,6 +11,8 @@
 #include <linux/hardirq.h>
 #include <linux/sched.h>
 #include <linux/irqflags.h>
+#include <linux/smp.h>
+#include <linux/percpu.h>
 #include <asm/atomic.h>
 #include <asm/ptrace.h>
 #include <asm/system.h>
@@ -271,7 +273,18 @@ extern void softirq_init(void);
 #define __raise_softirq_irqoff(nr) do { or_softirq_pending(1UL << (nr)); } while (0)
 extern void raise_softirq_irqoff(unsigned int nr);
 extern void raise_softirq(unsigned int nr);
+DECLARE_PER_CPU(struct list_head, softirq_work_list[NR_SOFTIRQ]);
 
+/* Try to send a softirq to a remote cpu.  If this cannot be done, the
+ * work will be queued to the local cpu.
+ */
+extern void send_remote_softirq(struct call_single_data *cp, int cpu, int softirq);
+
+/* Like send_remote_softirq(), but the caller must disable local cpu interrupts
+ * and compute the current cpu, passed in as 'this_cpu'.
+ */
+extern void __send_remote_softirq(struct call_single_data *cp, int cpu,
+				  int this_cpu, int softirq);
 
 /* Tasklets --- multithreaded analogue of BHs.
 
diff --git a/include/linux/smp.h b/include/linux/smp.h
index 66484d4..2e4d58b 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ ...
From: Andrew Morton
Date: Saturday, September 20, 2008 - 10:42 am

/** is sufficient & conventional for kerneldoc.

--

From: Daniel Walker
Date: Saturday, September 20, 2008 - 8:43 am

This goes in the GIT log so I hear, so you shouldn't need to add it to
the top.. It sounds like your saying Jens is the author, but I'm sure

This list your adding is rather confusing .. You add to it, but never
remove anything.. You've got it in the header file, so you must use it
someplace else .. Then I don't see what else it could be used for other

This whole patch really needs ifdefs. There's no value here on UP, since
what other cpu are you going to send softirqs to?

Daniel

--

From: David Miller
Date: Saturday, September 20, 2008 - 1:03 pm

From: Daniel Walker <dwalker@mvista.com>

No, Jens wrote this code, I just merely made it generic.
He also deserves a mention at the top of the file.  If
we had left it in the block layer, he would have received
such a mention.


The layers that use this stuff dequeue from their softirq

On UP we get a single queue, which the layer needs anyways.
It just always queues to the one queue.

Unlike Andrew's, your review comments have been completely and utterly
useless, as well as a total waste of my time.
--

From: Daniel Walker
Date: Saturday, September 20, 2008 - 1:16 pm

If you don't want your code reviewed, _don't send it_ .. Don't start an
argument with me cause you don't want deal with reviewers..

Daniel

--

From: David Miller
Date: Saturday, September 20, 2008 - 1:19 pm

From: Daniel Walker <dwalker@mvista.com>

Sounds like a plan.

--

Previous thread: [PATCH 1/2]: softirq: Define and use NR_SOFTIRQ by David Miller on Friday, September 19, 2008 - 11:48 pm. (1 message)

Next thread: [PATCH] max3100 driver by Christian Pellegrin on Saturday, September 20, 2008 - 12:20 am. (12 messages)