| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20080920004608.a3123052.akpm@linux-foundation.org>
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