This patch creates an alternative for drivers from using tasklets.
It creates a "work_tasklet". When configured to use work_tasklets
instead of tasklets, instead of creating tasklets, a work queue
is made in its place. The API is still the same, and the drivers
don't know that a work queue is being used.
This is (for now) a proof of concept approach to using work queues
instead of tasklets. More can be done. For one thing, we could make
individual work queues for each tasklet instead of using the global
ktasklet_wq. But for now it's just easier to do this.
Signed-off-by: Steven Rostedt <[email protected]>
Index: linux-2.6-test/kernel/Kconfig.preempt
===================================================================
--- linux-2.6-test.orig/kernel/Kconfig.preempt
+++ linux-2.6-test/kernel/Kconfig.preempt
@@ -63,3 +63,18 @@ config PREEMPT_BKL
Say Y here if you are building a kernel for a desktop system.
Say N if you are unsure.
+config TASKLETS_AS_WORKQUEUES
+ bool "Treat tasklets as workqueues (EXPERIMENTAL)"
+ depends on EXPERIMENTAL
+ help
+ Tasklets are an old solution to an old problem with respect
+ to SMP. But today they are not necessary anymore.
+ There are better solutions to the problem that tasklets
+ are trying to solve.
+
+ This option converts tasklets into workqueues and
+ removes the tasklet softirq. This is a clean up until
+ we get rid of all tasklets that are currently in
+ the kernel.
+
+ Say Y if you are unsure and brave (not very well tested code!).
Index: linux-2.6-test/kernel/Makefile
===================================================================
--- linux-2.6-test.orig/kernel/Makefile
+++ linux-2.6-test/kernel/Makefile
@@ -21,7 +21,11 @@ obj-$(CONFIG_FUTEX) += futex.o
ifeq ($(CONFIG_COMPAT),y)
obj-$(CONFIG_FUTEX) += futex_compat.o
endif
+ifeq ($(CONFIG_TASKLETS_AS_WORKQUEUES), y)
+obj-y += tasklet_work.o
+else
obj-y += tasklet.o
+endif
obj-$(CONFIG_RT_MUTEXES) += rtmutex.o
obj-$(CONFIG_DEBUG_RT_MUTEXES) += rtmutex-debug.o
obj-$(CONFIG_RT_MUTEX_TESTER) += rtmutex-tester.o
Index: linux-2.6-test/init/main.c
===================================================================
--- linux-2.6-test.orig/init/main.c
+++ linux-2.6-test/init/main.c
@@ -121,6 +121,11 @@ extern void time_init(void);
/* Default late time init is NULL. archs can override this later. */
void (*late_time_init)(void);
extern void softirq_init(void);
+#ifdef CONFIG_TASKLETS_AS_WORKQUEUES
+ extern void init_tasklets(void);
+#else
+# define init_tasklets() do { } while(0)
+#endif
/* Untouched command line saved by arch-specific code. */
char __initdata boot_command_line[COMMAND_LINE_SIZE];
@@ -706,6 +711,7 @@ static void __init do_basic_setup(void)
{
/* drivers will send hotplug events */
init_workqueues();
+ init_tasklets();
usermodehelper_init();
driver_init();
init_irq_proc();
Index: linux-2.6-test/include/linux/tasklet.h
===================================================================
--- linux-2.6-test.orig/include/linux/tasklet.h
+++ linux-2.6-test/include/linux/tasklet.h
@@ -1,6 +1,10 @@
#ifndef _LINUX_TASKLET_H
#define _LINUX_TASKLET_H
-#include <linux/tasklet_softirq.h>
+#ifdef CONFIG_TASKLETS_AS_WORKQUEUES
+# include <linux/tasklet_work.h>
+#else
+# include <linux/tasklet_softirq.h>
+#endif
#endif
Index: linux-2.6-test/include/linux/tasklet_work.h
===================================================================
--- /dev/null
+++ linux-2.6-test/include/linux/tasklet_work.h
@@ -0,0 +1,62 @@
+#ifndef _LINUX_WORK_TASKLET_H
+#define _LINUX_WORK_TASKLET_H
+
+#ifndef _LINUX_INTERRUPT_H
+# error "Do not include this header directly! use linux/interrupt.h"
+#endif
+
+#include <linux/workqueue.h>
+
+extern void work_tasklet_exec(struct work_struct *work);
+
+struct tasklet_struct
+{
+ struct work_struct work;
+ struct list_head list;
+ unsigned long state;
+ atomic_t count;
+ void (*func)(unsigned long);
+ unsigned long data;
+ char *n;
+};
+
+#define DECLARE_TASKLET(name, func, data) \
+ struct tasklet_struct name = { \
+ __WORK_INITIALIZER((name).work, work_tasklet_exec), \
+ LIST_HEAD_INIT((name).list), \
+ 0, \
+ ATOMIC_INIT(0), \
+ func, \
+ data, \
+ #name \
+ }
+
+#define DECLARE_TASKLET_DISABLED(name, func, data) \
+ struct tasklet_struct name = { \
+ __WORK_INITIALIZER((name).work, work_tasklet_exec), \
+ LIST_HEAD_INIT((name).list), \
+ 0, \
+ ATOMIC_INIT(1), \
+ func, \
+ data, \
+ #name \
+ }
+
+void tasklet_schedule(struct tasklet_struct *t);
+#define tasklet_hi_schedule tasklet_schedule
+extern fastcall void tasklet_enable(struct tasklet_struct *t);
+#define tasklet_hi_enable tasklet_enable
+
+void tasklet_disable_nosync(struct tasklet_struct *t);
+void tasklet_disable(struct tasklet_struct *t);
+
+extern int tasklet_is_scheduled(struct tasklet_struct *t);
+
+extern void tasklet_kill(struct tasklet_struct *t);
+extern void tasklet_kill_immediate(struct tasklet_struct *t, unsigned int cpu);
+extern void tasklet_init(struct tasklet_struct *t,
+ void (*func)(unsigned long), unsigned long data);
+void takeover_tasklets(unsigned int cpu);
+
+
+#endif /* _LINUX_WORK_TASKLET_H */
Index: linux-2.6-test/kernel/tasklet_work.c
===================================================================
--- /dev/null
+++ linux-2.6-test/kernel/tasklet_work.c
@@ -0,0 +1,138 @@
+/*
+ * linux/kernel/work_tasklet.c
+ *
+ * Copyright (C) 2007 Steven Rostedt, Red Hat
+ *
+ */
+
+#include <linux/interrupt.h>
+
+static struct workqueue_struct *ktaskletd_wq;
+
+enum
+{
+ TASKLET_STATE_SCHED, /* Tasklet is scheduled for execution */
+ TASKLET_STATE_RUN, /* Tasklet is running (SMP only) */
+ TASKLET_STATE_PENDING /* Tasklet is pending */
+};
+
+#define TASKLET_STATEF_SCHED (1 << TASKLET_STATE_SCHED)
+#define TASKLET_STATEF_RUN (1 << TASKLET_STATE_RUN)
+#define TASKLET_STATEF_PENDING (1 << TASKLET_STATE_PENDING)
+
+void tasklet_schedule(struct tasklet_struct *t)
+{
+ BUG_ON(!ktaskletd_wq);
+ pr_debug("scheduling tasklet %s %p\n", t->n, t);
+ queue_work(ktaskletd_wq, &t->work);
+}
+
+EXPORT_SYMBOL(tasklet_schedule);
+
+int tasklet_is_scheduled(struct tasklet_struct *t)
+{
+ int ret;
+ ret = work_pending(&t->work);
+ pr_debug("sched %s pending=%d\n", t->n, ret);
+ return ret;
+}
+
+EXPORT_SYMBOL(tasklet_is_scheduled);
+
+void tasklet_disable_nosync(struct tasklet_struct *t)
+{
+ pr_debug("disable tasklet %s %p\n", t->n, t);
+ atomic_inc(&t->count);
+ smp_mb__after_atomic_inc();
+}
+
+EXPORT_SYMBOL(tasklet_disable_nosync);
+
+void tasklet_disable(struct tasklet_struct *t)
+{
+ tasklet_disable_nosync(t);
+ pr_debug("flush tasklet %s %p\n", t->n, t);
+ flush_workqueue(ktaskletd_wq);
+ smp_mb();
+}
+
+EXPORT_SYMBOL(tasklet_disable);
+
+void work_tasklet_exec(struct work_struct *work)
+{
+ struct tasklet_struct *t =
+ container_of(work, struct tasklet_struct, work);
+
+ if (unlikely(atomic_read(&t->count))) {
+ pr_debug("tasklet disabled %s %p\n", t->n, t);
+ set_bit(TASKLET_STATE_PENDING, &t->state);
+ smp_mb();
+ /* make sure we were not just enabled */
+ if (likely(atomic_read(&t->count)))
+ goto out;
+ clear_bit(TASKLET_STATE_PENDING, &t->state);
+ }
+
+ local_bh_disable();
+ pr_debug("run tasklet %s %p\n", t->n, t);
+ t->func(t->data);
+ local_bh_enable();
+
+out:
+ return;
+}
+
+EXPORT_SYMBOL(work_tasklet_exec);
+
+void __init softirq_init(void)
+{
+}
+
+void init_tasklets(void)
+{
+ ktaskletd_wq = create_workqueue("tasklets");
+ BUG_ON(!ktaskletd_wq);
+}
+
+void takeover_tasklets(unsigned int cpu)
+{
+ pr_debug("Implement takeover tasklets??\n");
+}
+
+void tasklet_init(struct tasklet_struct *t,
+ void (*func)(unsigned long), unsigned long data)
+{
+ INIT_WORK(&t->work, work_tasklet_exec);
+ INIT_LIST_HEAD(&t->list);
+ t->state = 0;
+ atomic_set(&t->count, 0);
+ t->func = func;
+ t->data = data;
+ t->n = "anonymous";
+ pr_debug("anonymous tasklet %p set at %p\n",
+ t, __builtin_return_address(0));
+}
+
+EXPORT_SYMBOL(tasklet_init);
+
+void fastcall tasklet_enable(struct tasklet_struct *t)
+{
+ pr_debug("enable tasklet %s (count was %d)\n",
+ t->n, atomic_read(&t->count));
+ if (!atomic_dec_and_test(&t->count))
+ return;
+ if (test_and_clear_bit(TASKLET_STATE_PENDING, &t->state)) {
+ pr_debug("tasklet %s was pending\n", t->n);
+ tasklet_schedule(t);
+ }
+}
+
+EXPORT_SYMBOL(tasklet_enable);
+
+void tasklet_kill(struct tasklet_struct *t)
+{
+ pr_debug("kill tasklet %s\n", t->n);
+ flush_workqueue(ktaskletd_wq);
+}
+
+EXPORT_SYMBOL(tasklet_kill);
--
On Fri, 2007-06-22 at 00:00 -0400, Steven Rostedt wrote:
> plain text document attachment (tasklets-to-workqueues.patch)
> This patch creates an alternative for drivers from using tasklets.
> It creates a "work_tasklet". When configured to use work_tasklets
> instead of tasklets, instead of creating tasklets, a work queue
> is made in its place. The API is still the same, and the drivers
> don't know that a work queue is being used.
>
> This is (for now) a proof of concept approach to using work queues
> instead of tasklets. More can be done. For one thing, we could make
> individual work queues for each tasklet instead of using the global
> ktasklet_wq. But for now it's just easier to do this.
>
> Signed-off-by: Steven Rostedt <[email protected]>
>
>
> Index: linux-2.6-test/kernel/Kconfig.preempt
> ===================================================================
> --- linux-2.6-test.orig/kernel/Kconfig.preempt
> +++ linux-2.6-test/kernel/Kconfig.preempt
> @@ -63,3 +63,18 @@ config PREEMPT_BKL
> Say Y here if you are building a kernel for a desktop system.
> Say N if you are unsure.
>
> +config TASKLETS_AS_WORKQUEUES
> + bool "Treat tasklets as workqueues (EXPERIMENTAL)"
> + depends on EXPERIMENTAL
> + help
> + Tasklets are an old solution to an old problem with respect
> + to SMP. But today they are not necessary anymore.
> + There are better solutions to the problem that tasklets
> + are trying to solve.
> +
> + This option converts tasklets into workqueues and
> + removes the tasklet softirq. This is a clean up until
> + we get rid of all tasklets that are currently in
> + the kernel.
> +
> + Say Y if you are unsure and brave (not very well tested code!).
> Index: linux-2.6-test/kernel/Makefile
> ===================================================================
> --- linux-2.6-test.orig/kernel/Makefile
> +++ linux-2.6-test/kernel/Makefile
> @@ -21,7 +21,11 @@ obj-$(CONFIG_FUTEX) += futex.o
> ifeq ($(CONFIG_COMPAT),y)
> obj-$(CONFIG_FUTEX) += futex_compat.o
> endif
> +ifeq ($(CONFIG_TASKLETS_AS_WORKQUEUES), y)
> +obj-y += tasklet_work.o
> +else
> obj-y += tasklet.o
> +endif
> obj-$(CONFIG_RT_MUTEXES) += rtmutex.o
> obj-$(CONFIG_DEBUG_RT_MUTEXES) += rtmutex-debug.o
> obj-$(CONFIG_RT_MUTEX_TESTER) += rtmutex-tester.o
> Index: linux-2.6-test/init/main.c
> ===================================================================
> --- linux-2.6-test.orig/init/main.c
> +++ linux-2.6-test/init/main.c
> @@ -121,6 +121,11 @@ extern void time_init(void);
> /* Default late time init is NULL. archs can override this later. */
> void (*late_time_init)(void);
> extern void softirq_init(void);
> +#ifdef CONFIG_TASKLETS_AS_WORKQUEUES
> + extern void init_tasklets(void);
> +#else
> +# define init_tasklets() do { } while(0)
> +#endif
>
> /* Untouched command line saved by arch-specific code. */
> char __initdata boot_command_line[COMMAND_LINE_SIZE];
> @@ -706,6 +711,7 @@ static void __init do_basic_setup(void)
> {
> /* drivers will send hotplug events */
> init_workqueues();
> + init_tasklets();
> usermodehelper_init();
> driver_init();
> init_irq_proc();
> Index: linux-2.6-test/include/linux/tasklet.h
> ===================================================================
> --- linux-2.6-test.orig/include/linux/tasklet.h
> +++ linux-2.6-test/include/linux/tasklet.h
> @@ -1,6 +1,10 @@
> #ifndef _LINUX_TASKLET_H
> #define _LINUX_TASKLET_H
>
> -#include <linux/tasklet_softirq.h>
> +#ifdef CONFIG_TASKLETS_AS_WORKQUEUES
> +# include <linux/tasklet_work.h>
> +#else
> +# include <linux/tasklet_softirq.h>
> +#endif
>
> #endif
> Index: linux-2.6-test/include/linux/tasklet_work.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6-test/include/linux/tasklet_work.h
> @@ -0,0 +1,62 @@
> +#ifndef _LINUX_WORK_TASKLET_H
> +#define _LINUX_WORK_TASKLET_H
> +
> +#ifndef _LINUX_INTERRUPT_H
> +# error "Do not include this header directly! use linux/interrupt.h"
> +#endif
> +
> +#include <linux/workqueue.h>
> +
> +extern void work_tasklet_exec(struct work_struct *work);
> +
> +struct tasklet_struct
> +{
> + struct work_struct work;
> + struct list_head list;
> + unsigned long state;
> + atomic_t count;
> + void (*func)(unsigned long);
> + unsigned long data;
> + char *n;
> +};
> +
> +#define DECLARE_TASKLET(name, func, data) \
> + struct tasklet_struct name = { \
> + __WORK_INITIALIZER((name).work, work_tasklet_exec), \
> + LIST_HEAD_INIT((name).list), \
> + 0, \
> + ATOMIC_INIT(0), \
> + func, \
> + data, \
> + #name \
> + }
> +
> +#define DECLARE_TASKLET_DISABLED(name, func, data) \
> + struct tasklet_struct name = { \
> + __WORK_INITIALIZER((name).work, work_tasklet_exec), \
> + LIST_HEAD_INIT((name).list), \
> + 0, \
> + ATOMIC_INIT(1), \
> + func, \
> + data, \
> + #name \
> + }
> +
> +void tasklet_schedule(struct tasklet_struct *t);
> +#define tasklet_hi_schedule tasklet_schedule
> +extern fastcall void tasklet_enable(struct tasklet_struct *t);
> +#define tasklet_hi_enable tasklet_enable
> +
> +void tasklet_disable_nosync(struct tasklet_struct *t);
> +void tasklet_disable(struct tasklet_struct *t);
> +
> +extern int tasklet_is_scheduled(struct tasklet_struct *t);
> +
> +extern void tasklet_kill(struct tasklet_struct *t);
> +extern void tasklet_kill_immediate(struct tasklet_struct *t, unsigned int cpu);
> +extern void tasklet_init(struct tasklet_struct *t,
> + void (*func)(unsigned long), unsigned long data);
> +void takeover_tasklets(unsigned int cpu);
> +
> +
> +#endif /* _LINUX_WORK_TASKLET_H */
> Index: linux-2.6-test/kernel/tasklet_work.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6-test/kernel/tasklet_work.c
> @@ -0,0 +1,138 @@
> +/*
> + * linux/kernel/work_tasklet.c
> + *
> + * Copyright (C) 2007 Steven Rostedt, Red Hat
> + *
> + */
> +
> +#include <linux/interrupt.h>
> +
> +static struct workqueue_struct *ktaskletd_wq;
> +
> +enum
> +{
> + TASKLET_STATE_SCHED, /* Tasklet is scheduled for execution */
> + TASKLET_STATE_RUN, /* Tasklet is running (SMP only) */
> + TASKLET_STATE_PENDING /* Tasklet is pending */
> +};
> +
> +#define TASKLET_STATEF_SCHED (1 << TASKLET_STATE_SCHED)
> +#define TASKLET_STATEF_RUN (1 << TASKLET_STATE_RUN)
> +#define TASKLET_STATEF_PENDING (1 << TASKLET_STATE_PENDING)
> +
> +void tasklet_schedule(struct tasklet_struct *t)
> +{
> + BUG_ON(!ktaskletd_wq);
> + pr_debug("scheduling tasklet %s %p\n", t->n, t);
I'd change these pr_debug lines to "tasklet : scheduling %s %p\n" for
readability ..
> + queue_work(ktaskletd_wq, &t->work);
> +}
> +
> +EXPORT_SYMBOL(tasklet_schedule);
> +
> +int tasklet_is_scheduled(struct tasklet_struct *t)
> +{
> + int ret;
> + ret = work_pending(&t->work);
> + pr_debug("sched %s pending=%d\n", t->n, ret);
> + return ret;
> +}
> +
> +EXPORT_SYMBOL(tasklet_is_scheduled);
> +
> +void tasklet_disable_nosync(struct tasklet_struct *t)
> +{
> + pr_debug("disable tasklet %s %p\n", t->n, t);
> + atomic_inc(&t->count);
> + smp_mb__after_atomic_inc();
> +}
> +
> +EXPORT_SYMBOL(tasklet_disable_nosync);
> +
> +void tasklet_disable(struct tasklet_struct *t)
> +{
> + tasklet_disable_nosync(t);
> + pr_debug("flush tasklet %s %p\n", t->n, t);
> + flush_workqueue(ktaskletd_wq);
> + smp_mb();
> +}
> +
> +EXPORT_SYMBOL(tasklet_disable);
> +
> +void work_tasklet_exec(struct work_struct *work)
> +{
> + struct tasklet_struct *t =
> + container_of(work, struct tasklet_struct, work);
> +
> + if (unlikely(atomic_read(&t->count))) {
> + pr_debug("tasklet disabled %s %p\n", t->n, t);
> + set_bit(TASKLET_STATE_PENDING, &t->state);
> + smp_mb();
> + /* make sure we were not just enabled */
> + if (likely(atomic_read(&t->count)))
> + goto out;
> + clear_bit(TASKLET_STATE_PENDING, &t->state);
smp_mb__before_clear_bit ?
> + }
> +
> + local_bh_disable();
> + pr_debug("run tasklet %s %p\n", t->n, t);
> + t->func(t->data);
> + local_bh_enable();
> +
> +out:
> + return;
> +}
> +
> +EXPORT_SYMBOL(work_tasklet_exec);
> +
> +void __init softirq_init(void)
> +{
> +}
ifdef's ?
> +void init_tasklets(void)
> +{
> + ktaskletd_wq = create_workqueue("tasklets");
> + BUG_ON(!ktaskletd_wq);
> +}
> +
> +void takeover_tasklets(unsigned int cpu)
> +{
> + pr_debug("Implement takeover tasklets??\n");
> +}
hmm .. Looks like it's for migration of tasklets .. I take it your not
sure that's handled ? Try cpu hotplug ..
> +void tasklet_init(struct tasklet_struct *t,
> + void (*func)(unsigned long), unsigned long data)
> +{
> + INIT_WORK(&t->work, work_tasklet_exec);
> + INIT_LIST_HEAD(&t->list);
> + t->state = 0;
> + atomic_set(&t->count, 0);
> + t->func = func;
> + t->data = data;
> + t->n = "anonymous";
Is this "anonymous" just used for debugging ? Is so you could fill it
with a kallsyms lookup with the __builtin_return_address() ..
> + pr_debug("anonymous tasklet %p set at %p\n",
> + t, __builtin_return_address(0));
> +}
> +
> +EXPORT_SYMBOL(tasklet_init);
> +
> +void fastcall tasklet_enable(struct tasklet_struct *t)
> +{
> + pr_debug("enable tasklet %s (count was %d)\n",
> + t->n, atomic_read(&t->count));
> + if (!atomic_dec_and_test(&t->count))
> + return;
> + if (test_and_clear_bit(TASKLET_STATE_PENDING, &t->state)) {
> + pr_debug("tasklet %s was pending\n", t->n);
> + tasklet_schedule(t);
> + }
> +}
> +
> +EXPORT_SYMBOL(tasklet_enable);
> +
> +void tasklet_kill(struct tasklet_struct *t)
> +{
> + pr_debug("kill tasklet %s\n", t->n);
> + flush_workqueue(ktaskletd_wq);
> +}
> +
> +EXPORT_SYMBOL(tasklet_kill);
>
On Fri, 2007-06-22 at 00:06 -0700, Daniel Walker wrote:
> > +void tasklet_schedule(struct tasklet_struct *t)
> > +{
> > + BUG_ON(!ktaskletd_wq);
> > + pr_debug("scheduling tasklet %s %p\n", t->n, t);
>
> I'd change these pr_debug lines to "tasklet : scheduling %s %p\n" for
> readability ..
As Ingo suggested, the next round won't even have them.
> truct tasklet_struct, work);
> > +
> > + if (unlikely(atomic_read(&t->count))) {
> > + pr_debug("tasklet disabled %s %p\n", t->n, t);
> > + set_bit(TASKLET_STATE_PENDING, &t->state);
> > + smp_mb();
> > + /* make sure we were not just enabled */
> > + if (likely(atomic_read(&t->count)))
> > + goto out;
> > + clear_bit(TASKLET_STATE_PENDING, &t->state);
>
> smp_mb__before_clear_bit ?
The smp_mb() is needed before the atomic_read. But since that atomic
read is in a conditional, no more barriers are needed if we continue.
Thanks go out to Oleg for pointing out the smp_mb was needed!
> > +
> > +void __init softirq_init(void)
> > +{
> > +}
>
> ifdef's ?
Nah, ifdefs are ugly. Even uglier than stubbed functions. I guess I
could simply put it in the header as a define do {} while(0).
>
> > +void init_tasklets(void)
> > +{
> > + ktaskletd_wq = create_workqueue("tasklets");
> > + BUG_ON(!ktaskletd_wq);
> > +}
> > +
> > +void takeover_tasklets(unsigned int cpu)
> > +{
> > + pr_debug("Implement takeover tasklets??\n");
> > +}
>
> hmm .. Looks like it's for migration of tasklets .. I take it your not
> sure that's handled ? Try cpu hotplug ..
Actually, I believe that the workqueues will handle it themselves, so I
don't need to do any special handling. Just another advantage of
converting tasklets into work queues.
>
> > +void tasklet_init(struct tasklet_struct *t,
> > + void (*func)(unsigned long), unsigned long data)
> > +{
> > + INIT_WORK(&t->work, work_tasklet_exec);
> > + INIT_LIST_HEAD(&t->list);
> > + t->state = 0;
> > + atomic_set(&t->count, 0);
> > + t->func = func;
> > + t->data = data;
> > + t->n = "anonymous";
>
> Is this "anonymous" just used for debugging ? Is so you could fill it
> with a kallsyms lookup with the __builtin_return_address() ..
Yeah, they were started for debugging, but I also thought about making
some sort of interface for user land to see what was defined. So using
kallsyms might not look well. It's easy enough to find if an anonymous
tasklet gives me trouble.
-- Steve
On 06/22, Steven Rostedt wrote:
>
> > truct tasklet_struct, work);
> > > +
> > > + if (unlikely(atomic_read(&t->count))) {
> > > + pr_debug("tasklet disabled %s %p\n", t->n, t);
> > > + set_bit(TASKLET_STATE_PENDING, &t->state);
> > > + smp_mb();
> > > + /* make sure we were not just enabled */
> > > + if (likely(atomic_read(&t->count)))
> > > + goto out;
> > > + clear_bit(TASKLET_STATE_PENDING, &t->state);
Looking closer, I think this is not right, and the smp_mb__before_clear_bit()
can't help.
/* t->count == 1 */
work_tasklet_exec() tasklet_enable()
...
set_bit(TASKLET_STATE_PENDING); atomic_dec_and_test(&t->count);
/* t->count == 0 */
// False
if (atomic_read(&t->count))
goto out;
// True
if (test_and_clear_bit(_PENDING))
tasklet_schedule();
clear_bit(TASKLET_STATE_PENDING);
execute t->func();
So, t->func() will be executed twice because tasklet_enable() does
tasklet_schedule().
So I think we need a fix for work_tasklet_exec,
- clear_bit(TASKLET_STATE_PENDING);
+ if (!test_and_clear_bit(TASKLET_STATE_PENDING))
goto out;
Steven, a very stupid suggestion, could you move the code for tasklet_enable()
up, closer to tasklet_disable() ?
Oleg.
On Fri, 2007-06-22 at 19:52 +0400, Oleg Nesterov wrote:
> On 06/22, Steven Rostedt wrote:
> >
> > > truct tasklet_struct, work);
> > > > +
> > > > + if (unlikely(atomic_read(&t->count))) {
> > > > + pr_debug("tasklet disabled %s %p\n", t->n, t);
> > > > + set_bit(TASKLET_STATE_PENDING, &t->state);
> > > > + smp_mb();
> > > > + /* make sure we were not just enabled */
> > > > + if (likely(atomic_read(&t->count)))
> > > > + goto out;
> > > > + clear_bit(TASKLET_STATE_PENDING, &t->state);
Yeah, I knew of the race but didn't think that running a tasklet
function twice would cause much harm here. But not running it when it
needs to run, can have quite a negative impact.
>
> So, t->func() will be executed twice because tasklet_enable() does
> tasklet_schedule().
>
>
> So I think we need a fix for work_tasklet_exec,
>
> - clear_bit(TASKLET_STATE_PENDING);
> + if (!test_and_clear_bit(TASKLET_STATE_PENDING))
> goto out;
>
OK, I like this. I'll add it in the next round.
>
>
> Steven, a very stupid suggestion, could you move the code for tasklet_enable()
> up, closer to tasklet_disable() ?
Not a stupid suggestion. I'll accommodate it.
Thanks,
-- Steve
On Friday 22 June 2007, Steven Rostedt wrote:
> This patch creates an alternative for drivers from using tasklets.
> It creates a "work_tasklet". When configured to use work_tasklets
> instead of tasklets, instead of creating tasklets, a work queue
> is made in its place. The API is still the same, and the drivers
> don't know that a work queue is being used.
>
Perhaps the API can be slimmed down in the process, because half
of the tasklet interface functions are hardly used at all.
> +#define DECLARE_TASKLET(name, func, data) \
> + struct tasklet_struct name = { \
> + __WORK_INITIALIZER((name).work, work_tasklet_exec), \
> + LIST_HEAD_INIT((name).list), \
> + 0, \
> + ATOMIC_INIT(0), \
> + func, \
> + data, \
> + #name \
> + }
18 users of this macro. Maybe too much for the start, but if we convert
all of them to use either tasklet_init or use work queues directly,
the macro can go away.
> +#define DECLARE_TASKLET_DISABLED(name, func, data) \
> + struct tasklet_struct name = { \
> + __WORK_INITIALIZER((name).work, work_tasklet_exec), \
> + LIST_HEAD_INIT((name).list), \
> + 0, \
> + ATOMIC_INIT(1), \
> + func, \
> + data, \
> + #name \
> + }
this one is easier, there are only four users in total: three input
drivers, and tipc.
> +void tasklet_schedule(struct tasklet_struct *t);
> +#define tasklet_hi_schedule tasklet_schedule
> +extern fastcall void tasklet_enable(struct tasklet_struct *t);
> +#define tasklet_hi_enable tasklet_enable
there are 34 files using tasklet_hi_* functions. In theory, these
could be converted to the non-hi version with a simple search and
replace, if it's clear that there is not much point in keeping them.
The most common use of tasklet_hi is in the alsa drivers. If it
actually makes a difference for them already, maybe there should
be an alsa softirq instead of moving them all over to work queues.
> +void tasklet_disable_nosync(struct tasklet_struct *t);
only has two users, bcm43xx and sc92031. If both are
converted to workqueue, the interface removed can be
removed.
> +extern void tasklet_kill_immediate(struct tasklet_struct *t, unsigned int cpu);
only one user in total, rcupdate.c. You already take care of that,
it seems the declaration is just a leftover.
Arnd <><