Date:	Sat, 20 Sep 2008 00:46:08 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	David Miller <davem@...emloft.net>
Cc:	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	jens.axboe@...cle.com, steffen.klassert@...unet.com
Subject: Re: [PATCH 2/2]: softirq: Add support for triggering softirq work
 on softirqs.

On Fri, 19 Sep 2008 23:48:32 -0700 (PDT) David Miller <davem@...emloft.net> wrote:

> 
> 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@...emloft.net>
> Signed-off-by: Jens Axboe <jens.axboe@...cle.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 = &__get_cpu_var(softirq_work_list[softirq]);
> +
> +	list_add_tail(&cp->list, head);
> +	if (head->next == &cp->list)

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

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

would be nice.

> +		raise_softirq_irqoff(softirq);
> +}
> +
> +#if defined(CONFIG_SMP) && defined(CONFIG_USE_GENERIC_SMP_HELPERS)

CONFIG_USE_GENERIC_SMP_HELPERS=y, CONFIG_SMP=n shouldn't be possible,
if we care..

> +static void remote_softirq_receive(void *data)
> +{
> +	struct call_single_data *cp = data;
> +	unsigned long flags;
> +	int softirq;
> +
> +	softirq = cp->flags >> 16;

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
`unsigned int', to match call_single_data.flags.

> +	local_irq_save(flags);
> +	__local_trigger(cp, softirq);
> +	local_irq_restore(flags);
> +}
> +
> +static int __try_remote_softirq(struct call_single_data *cp, int cpu, int softirq)
> +{
> +	if (cpu_online(cpu)) {

Is locking needed to keep that CPU online after this test?

> +		cp->func = remote_softirq_receive;
> +		cp->info = cp;
> +		cp->flags = softirq << 16;
> +		__smp_call_function_single(cpu, cp);
> +		return 0;
> +	}
> +	return 1;
> +}
> +#else /* CONFIG_SMP && CONFIG_USE_GENERIC_SMP_HELPERS */
> +static int __try_remote_softirq(struct call_single_data *cp, int cpu, int softirq)
> +{
> +	return 1;
> +}
> +#endif
> +
> +void __send_remote_softirq(struct call_single_data *cp, int cpu, int this_cpu, int softirq)
> +{
> +	if (cpu == this_cpu || __try_remote_softirq(cp, cpu, softirq))
> +		__local_trigger(cp, softirq);
> +}
> +EXPORT_SYMBOL(__send_remote_softirq);
> +
> +void send_remote_softirq(struct call_single_data *cp, int cpu, int softirq)
> +{
> +	unsigned long flags;
> +	int this_cpu;
> +
> +	local_irq_save(flags);
> +	this_cpu = smp_processor_id();
> +	__send_remote_softirq(cp, cpu, this_cpu, softirq);

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
interface description, if not a WARN_ON().

> +	local_irq_restore(flags);
> +}
> +EXPORT_SYMBOL(send_remote_softirq);

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

> +static int __cpuinit remote_softirq_cpu_notify(struct notifier_block *self,
> +					       unsigned long action, void *hcpu)
> +{
> +	/*
> +	 * If a CPU goes away, splice its entries to the current CPU
> +	 * and trigger a run of the softirq
> +	 */
> +	if (action == CPU_DEAD || action == CPU_DEAD_FROZEN) {
> +		int cpu = (unsigned long) hcpu;
> +		int i;
> +
> +		local_irq_disable();
> +		for (i = 0; i < NR_SOFTIRQ; i++) {
> +			struct list_head *head = &per_cpu(softirq_work_list[i], cpu);
> +			struct list_head *local_head;
> +
> +			if (list_empty(head))
> +				continue;
> +
> +			local_head = &__get_cpu_var(softirq_work_list[i]);
> +			list_splice_init(head, local_head);
> +			raise_softirq_irqoff(i);

Could use __local_trigger() here I think, although that wouldn't really
improve anything much.


> +		}
> +		local_irq_enable();
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block __cpuinitdata remote_softirq_cpu_notifier = {
> +	.notifier_call	= remote_softirq_cpu_notify,
> +};
> +
>  void __init softirq_init(void)
>  {
>  	int cpu;
>  
>  	for_each_possible_cpu(cpu) {
> +		int i;
> +
>  		per_cpu(tasklet_vec, cpu).tail =
>  			&per_cpu(tasklet_vec, cpu).head;
>  		per_cpu(tasklet_hi_vec, cpu).tail =
>  			&per_cpu(tasklet_hi_vec, cpu).head;
> +		for (i = 0; i < NR_SOFTIRQ; i++)
> +			INIT_LIST_HEAD(&per_cpu(softirq_work_list[i], cpu));
>  	}
>  
> +	register_hotcpu_notifier(&remote_softirq_cpu_notifier);
> +
>  	open_softirq(TASKLET_SOFTIRQ, tasklet_action);
>  	open_softirq(HI_SOFTIRQ, tasklet_hi_action);
>  }

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists