[RFC PATCH 0/5 v2] Convert all tasklets to workqueues V2

[RFC PATCH 0/5 v2] Convert all tasklets to workqueues V2

[Steven Rostedt]

-- 

This is version 2 of the tasklet to workqueue conversion.

Changes from version 1.

- removed config option and simply replace the old implementation
  with the work queue one (recommended by Ingo Molnar).

- replaced clear_bit with test_and_clear_bit to avoid the race of
  executing the tasklet function twice. (thanks to Oleg Nesterov
  for pointing that out).

- Removed most of the pr_debug prints. (Kept one)
  (recommended by Ingo Molnar)

- Removed call to softirq_init.

- Added Author credit to Dipankar Sarma for the RCU tasklet to
  softirq conversion.

- Tested on my Powerbook to add another arch to the mix :-)
  Funny that booting with this change was the first time that
  the bcm43xx didn't get stuck for several seconds on bootup.
  It's also one of the few drivers that use tasklet_disable_nosync.
  So either this shows that the change fixed something, or that
  it broke something (or was just a fluke).


-- Steve

[RFC PATCH 1/5 v2] Convert the RCU tasklet into a softirq

[Steven Rostedt]

[-- Attachment #1: convert-rcu-tasklet-to-softirq.patch --]
[-- Type: text/plain, Size: 3016 bytes --]

I believe this was originally done by Dipankar Sarma. I pulled these
changes from the -rt kernel.

For better preformance, RCU should use a softirq instead of a
tasklet.

From: Dipankar Sarma <dipankar@in.ibm.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Index: linux-2.6-test/include/linux/interrupt.h
===================================================================
--- linux-2.6-test.orig/include/linux/interrupt.h
+++ linux-2.6-test/include/linux/interrupt.h
@@ -245,6 +245,9 @@ enum
 #ifdef CONFIG_HIGH_RES_TIMERS
 	HRTIMER_SOFTIRQ,
 #endif
+	RCU_SOFTIRQ,	/* Preferable RCU should always be the last softirq */
+	/* Entries after this are ignored in split softirq mode */
+	MAX_SOFTIRQ,
 };
 
 /* softirq mask and active fields moved to irq_cpustat_t in
Index: linux-2.6-test/kernel/rcupdate.c
===================================================================
--- linux-2.6-test.orig/kernel/rcupdate.c
+++ linux-2.6-test/kernel/rcupdate.c
@@ -67,7 +67,6 @@ DEFINE_PER_CPU(struct rcu_data, rcu_data
 DEFINE_PER_CPU(struct rcu_data, rcu_bh_data) = { 0L };
 
 /* Fake initialization required by compiler */
-static DEFINE_PER_CPU(struct tasklet_struct, rcu_tasklet) = {NULL};
 static int blimit = 10;
 static int qhimark = 10000;
 static int qlowmark = 100;
@@ -253,7 +252,7 @@ static void rcu_do_batch(struct rcu_data
 	if (!rdp->donelist)
 		rdp->donetail = &rdp->donelist;
 	else
-		tasklet_schedule(&per_cpu(rcu_tasklet, rdp->cpu));
+		raise_softirq(RCU_SOFTIRQ);
 }
 
 /*
@@ -405,7 +404,6 @@ static void rcu_offline_cpu(int cpu)
 					&per_cpu(rcu_bh_data, cpu));
 	put_cpu_var(rcu_data);
 	put_cpu_var(rcu_bh_data);
-	tasklet_kill_immediate(&per_cpu(rcu_tasklet, cpu), cpu);
 }
 
 #else
@@ -417,7 +415,7 @@ static void rcu_offline_cpu(int cpu)
 #endif
 
 /*
- * This does the RCU processing work from tasklet context. 
+ * This does the RCU processing work from softirq context.
  */
 static void __rcu_process_callbacks(struct rcu_ctrlblk *rcp,
 					struct rcu_data *rdp)
@@ -462,7 +460,7 @@ static void __rcu_process_callbacks(stru
 		rcu_do_batch(rdp);
 }
 
-static void rcu_process_callbacks(unsigned long unused)
+static void rcu_process_callbacks(struct softirq_action *unused)
 {
 	__rcu_process_callbacks(&rcu_ctrlblk, &__get_cpu_var(rcu_data));
 	__rcu_process_callbacks(&rcu_bh_ctrlblk, &__get_cpu_var(rcu_bh_data));
@@ -526,7 +524,7 @@ void rcu_check_callbacks(int cpu, int us
 		rcu_bh_qsctr_inc(cpu);
 	} else if (!in_softirq())
 		rcu_bh_qsctr_inc(cpu);
-	tasklet_schedule(&per_cpu(rcu_tasklet, cpu));
+	raise_softirq(RCU_SOFTIRQ);
 }
 
 static void rcu_init_percpu_data(int cpu, struct rcu_ctrlblk *rcp,
@@ -549,7 +547,7 @@ static void __devinit rcu_online_cpu(int
 
 	rcu_init_percpu_data(cpu, &rcu_ctrlblk, rdp);
 	rcu_init_percpu_data(cpu, &rcu_bh_ctrlblk, bh_rdp);
-	tasklet_init(&per_cpu(rcu_tasklet, cpu), rcu_process_callbacks, 0UL);
+	open_softirq(RCU_SOFTIRQ, rcu_process_callbacks, NULL);
 }
 
 static int __cpuinit rcu_cpu_notify(struct notifier_block *self,

-- 

[RFC PATCH 2/5 v2] Split out tasklets from softirq.c

[Steven Rostedt]

[-- Attachment #1: split-out-tasklet.patch --]
[-- Type: text/plain, Size: 18098 bytes --]

Tasklets are really a separate entity from softirqs, so they
deserve their own file. Also this allows us to easily replace
tasklets for something else ;-)

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Index: linux-2.6-test/include/linux/interrupt.h
===================================================================
--- linux-2.6-test.orig/include/linux/interrupt.h
+++ linux-2.6-test/include/linux/interrupt.h
@@ -13,6 +13,7 @@
 #include <linux/irqflags.h>
 #include <linux/bottom_half.h>
 #include <linux/device.h>
+#include <linux/tasklet.h>
 #include <asm/atomic.h>
 #include <asm/ptrace.h>
 #include <asm/system.h>
@@ -268,117 +269,6 @@ extern void FASTCALL(raise_softirq_irqof
 extern void FASTCALL(raise_softirq(unsigned int nr));
 
 
-/* Tasklets --- multithreaded analogue of BHs.
-
-   Main feature differing them of generic softirqs: tasklet
-   is running only on one CPU simultaneously.
-
-   Main feature differing them of BHs: different tasklets
-   may be run simultaneously on different CPUs.
-
-   Properties:
-   * If tasklet_schedule() is called, then tasklet is guaranteed
-     to be executed on some cpu at least once after this.
-   * If the tasklet is already scheduled, but its excecution is still not
-     started, it will be executed only once.
-   * If this tasklet is already running on another CPU (or schedule is called
-     from tasklet itself), it is rescheduled for later.
-   * Tasklet is strictly serialized wrt itself, but not
-     wrt another tasklets. If client needs some intertask synchronization,
-     he makes it with spinlocks.
- */
-
-struct tasklet_struct
-{
-	struct tasklet_struct *next;
-	unsigned long state;
-	atomic_t count;
-	void (*func)(unsigned long);
-	unsigned long data;
-};
-
-#define DECLARE_TASKLET(name, func, data) \
-struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(0), func, data }
-
-#define DECLARE_TASKLET_DISABLED(name, func, data) \
-struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(1), func, data }
-
-
-enum
-{
-	TASKLET_STATE_SCHED,	/* Tasklet is scheduled for execution */
-	TASKLET_STATE_RUN	/* Tasklet is running (SMP only) */
-};
-
-#ifdef CONFIG_SMP
-static inline int tasklet_trylock(struct tasklet_struct *t)
-{
-	return !test_and_set_bit(TASKLET_STATE_RUN, &(t)->state);
-}
-
-static inline void tasklet_unlock(struct tasklet_struct *t)
-{
-	smp_mb__before_clear_bit(); 
-	clear_bit(TASKLET_STATE_RUN, &(t)->state);
-}
-
-static inline void tasklet_unlock_wait(struct tasklet_struct *t)
-{
-	while (test_bit(TASKLET_STATE_RUN, &(t)->state)) { barrier(); }
-}
-#else
-#define tasklet_trylock(t) 1
-#define tasklet_unlock_wait(t) do { } while (0)
-#define tasklet_unlock(t) do { } while (0)
-#endif
-
-extern void FASTCALL(__tasklet_schedule(struct tasklet_struct *t));
-
-static inline void tasklet_schedule(struct tasklet_struct *t)
-{
-	if (!test_and_set_bit(TASKLET_STATE_SCHED, &t->state))
-		__tasklet_schedule(t);
-}
-
-extern void FASTCALL(__tasklet_hi_schedule(struct tasklet_struct *t));
-
-static inline void tasklet_hi_schedule(struct tasklet_struct *t)
-{
-	if (!test_and_set_bit(TASKLET_STATE_SCHED, &t->state))
-		__tasklet_hi_schedule(t);
-}
-
-
-static inline void tasklet_disable_nosync(struct tasklet_struct *t)
-{
-	atomic_inc(&t->count);
-	smp_mb__after_atomic_inc();
-}
-
-static inline void tasklet_disable(struct tasklet_struct *t)
-{
-	tasklet_disable_nosync(t);
-	tasklet_unlock_wait(t);
-	smp_mb();
-}
-
-static inline void tasklet_enable(struct tasklet_struct *t)
-{
-	smp_mb__before_atomic_dec();
-	atomic_dec(&t->count);
-}
-
-static inline void tasklet_hi_enable(struct tasklet_struct *t)
-{
-	smp_mb__before_atomic_dec();
-	atomic_dec(&t->count);
-}
-
-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);
-
 /*
  * Autoprobing for irqs:
  *
Index: linux-2.6-test/include/linux/tasklet.h
===================================================================
--- /dev/null
+++ linux-2.6-test/include/linux/tasklet.h
@@ -0,0 +1,120 @@
+#ifndef _LINUX_TASKLET_H
+#define _LINUX_TASKLET_H
+
+/* Tasklets --- multithreaded analogue of BHs.
+
+   Main feature differing them of generic softirqs: tasklet
+   is running only on one CPU simultaneously.
+
+   Main feature differing them of BHs: different tasklets
+   may be run simultaneously on different CPUs.
+
+   Properties:
+   * If tasklet_schedule() is called, then tasklet is guaranteed
+     to be executed on some cpu at least once after this.
+   * If the tasklet is already scheduled, but its excecution is still not
+     started, it will be executed only once.
+   * If this tasklet is already running on another CPU (or schedule is called
+     from tasklet itself), it is rescheduled for later.
+   * Tasklet is strictly serialized wrt itself, but not
+     wrt another tasklets. If client needs some intertask synchronization,
+     he makes it with spinlocks.
+ */
+
+struct tasklet_struct
+{
+	struct tasklet_struct *next;
+	unsigned long state;
+	atomic_t count;
+	void (*func)(unsigned long);
+	unsigned long data;
+};
+
+#define DECLARE_TASKLET(name, func, data) \
+struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(0), func, data }
+
+#define DECLARE_TASKLET_DISABLED(name, func, data) \
+struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(1), func, data }
+
+
+enum
+{
+	TASKLET_STATE_SCHED,	/* Tasklet is scheduled for execution */
+	TASKLET_STATE_RUN	/* Tasklet is running (SMP only) */
+};
+
+#ifdef CONFIG_SMP
+static inline int tasklet_trylock(struct tasklet_struct *t)
+{
+	return !test_and_set_bit(TASKLET_STATE_RUN, &(t)->state);
+}
+
+static inline void tasklet_unlock(struct tasklet_struct *t)
+{
+	smp_mb__before_clear_bit(); 
+	clear_bit(TASKLET_STATE_RUN, &(t)->state);
+}
+
+static inline void tasklet_unlock_wait(struct tasklet_struct *t)
+{
+	while (test_bit(TASKLET_STATE_RUN, &(t)->state)) { barrier(); }
+}
+#else
+#define tasklet_trylock(t) 1
+#define tasklet_unlock_wait(t) do { } while (0)
+#define tasklet_unlock(t) do { } while (0)
+#endif
+
+extern void FASTCALL(__tasklet_schedule(struct tasklet_struct *t));
+
+static inline void tasklet_schedule(struct tasklet_struct *t)
+{
+	if (!test_and_set_bit(TASKLET_STATE_SCHED, &t->state))
+		__tasklet_schedule(t);
+}
+
+extern void FASTCALL(__tasklet_hi_schedule(struct tasklet_struct *t));
+
+static inline void tasklet_hi_schedule(struct tasklet_struct *t)
+{
+	if (!test_and_set_bit(TASKLET_STATE_SCHED, &t->state))
+		__tasklet_hi_schedule(t);
+}
+
+
+static inline void tasklet_disable_nosync(struct tasklet_struct *t)
+{
+	atomic_inc(&t->count);
+	smp_mb__after_atomic_inc();
+}
+
+static inline void tasklet_disable(struct tasklet_struct *t)
+{
+	tasklet_disable_nosync(t);
+	tasklet_unlock_wait(t);
+	smp_mb();
+}
+
+static inline void tasklet_enable(struct tasklet_struct *t)
+{
+	smp_mb__before_atomic_dec();
+	atomic_dec(&t->count);
+}
+
+static inline void tasklet_hi_enable(struct tasklet_struct *t)
+{
+	smp_mb__before_atomic_dec();
+	atomic_dec(&t->count);
+}
+
+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);
+
+#ifdef CONFIG_HOTPLUG_CPU
+void takeover_tasklets(unsigned int cpu);
+#endif /* CONFIG_HOTPLUG_CPU */
+
+#endif /* _LINUX_TASKLET_H */
+
Index: linux-2.6-test/kernel/Makefile
===================================================================
--- linux-2.6-test.orig/kernel/Makefile
+++ linux-2.6-test/kernel/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_FUTEX) += futex.o
 ifeq ($(CONFIG_COMPAT),y)
 obj-$(CONFIG_FUTEX) += futex_compat.o
 endif
+obj-y += tasklet.o
 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/kernel/softirq.c
===================================================================
--- linux-2.6-test.orig/kernel/softirq.c
+++ linux-2.6-test/kernel/softirq.c
@@ -348,144 +348,6 @@ void open_softirq(int nr, void (*action)
 	softirq_vec[nr].action = action;
 }
 
-/* Tasklets */
-struct tasklet_head
-{
-	struct tasklet_struct *list;
-};
-
-/* Some compilers disobey section attribute on statics when not
-   initialized -- RR */
-static DEFINE_PER_CPU(struct tasklet_head, tasklet_vec) = { NULL };
-static DEFINE_PER_CPU(struct tasklet_head, tasklet_hi_vec) = { NULL };
-
-void fastcall __tasklet_schedule(struct tasklet_struct *t)
-{
-	unsigned long flags;
-
-	local_irq_save(flags);
-	t->next = __get_cpu_var(tasklet_vec).list;
-	__get_cpu_var(tasklet_vec).list = t;
-	raise_softirq_irqoff(TASKLET_SOFTIRQ);
-	local_irq_restore(flags);
-}
-
-EXPORT_SYMBOL(__tasklet_schedule);
-
-void fastcall __tasklet_hi_schedule(struct tasklet_struct *t)
-{
-	unsigned long flags;
-
-	local_irq_save(flags);
-	t->next = __get_cpu_var(tasklet_hi_vec).list;
-	__get_cpu_var(tasklet_hi_vec).list = t;
-	raise_softirq_irqoff(HI_SOFTIRQ);
-	local_irq_restore(flags);
-}
-
-EXPORT_SYMBOL(__tasklet_hi_schedule);
-
-static void tasklet_action(struct softirq_action *a)
-{
-	struct tasklet_struct *list;
-
-	local_irq_disable();
-	list = __get_cpu_var(tasklet_vec).list;
-	__get_cpu_var(tasklet_vec).list = NULL;
-	local_irq_enable();
-
-	while (list) {
-		struct tasklet_struct *t = list;
-
-		list = list->next;
-
-		if (tasklet_trylock(t)) {
-			if (!atomic_read(&t->count)) {
-				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
-					BUG();
-				t->func(t->data);
-				tasklet_unlock(t);
-				continue;
-			}
-			tasklet_unlock(t);
-		}
-
-		local_irq_disable();
-		t->next = __get_cpu_var(tasklet_vec).list;
-		__get_cpu_var(tasklet_vec).list = t;
-		__raise_softirq_irqoff(TASKLET_SOFTIRQ);
-		local_irq_enable();
-	}
-}
-
-static void tasklet_hi_action(struct softirq_action *a)
-{
-	struct tasklet_struct *list;
-
-	local_irq_disable();
-	list = __get_cpu_var(tasklet_hi_vec).list;
-	__get_cpu_var(tasklet_hi_vec).list = NULL;
-	local_irq_enable();
-
-	while (list) {
-		struct tasklet_struct *t = list;
-
-		list = list->next;
-
-		if (tasklet_trylock(t)) {
-			if (!atomic_read(&t->count)) {
-				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
-					BUG();
-				t->func(t->data);
-				tasklet_unlock(t);
-				continue;
-			}
-			tasklet_unlock(t);
-		}
-
-		local_irq_disable();
-		t->next = __get_cpu_var(tasklet_hi_vec).list;
-		__get_cpu_var(tasklet_hi_vec).list = t;
-		__raise_softirq_irqoff(HI_SOFTIRQ);
-		local_irq_enable();
-	}
-}
-
-
-void tasklet_init(struct tasklet_struct *t,
-		  void (*func)(unsigned long), unsigned long data)
-{
-	t->next = NULL;
-	t->state = 0;
-	atomic_set(&t->count, 0);
-	t->func = func;
-	t->data = data;
-}
-
-EXPORT_SYMBOL(tasklet_init);
-
-void tasklet_kill(struct tasklet_struct *t)
-{
-	if (in_interrupt())
-		printk("Attempt to kill tasklet from interrupt\n");
-
-	while (test_and_set_bit(TASKLET_STATE_SCHED, &t->state)) {
-		do
-			yield();
-		while (test_bit(TASKLET_STATE_SCHED, &t->state));
-	}
-	tasklet_unlock_wait(t);
-	clear_bit(TASKLET_STATE_SCHED, &t->state);
-}
-
-EXPORT_SYMBOL(tasklet_kill);
-
-void __init softirq_init(void)
-{
-	open_softirq(TASKLET_SOFTIRQ, tasklet_action, NULL);
-	open_softirq(HI_SOFTIRQ, tasklet_hi_action, NULL);
-}
-
 static int ksoftirqd(void * __bind_cpu)
 {
 	set_user_nice(current, 19);
@@ -532,58 +394,6 @@ wait_to_die:
 	return 0;
 }
 
-#ifdef CONFIG_HOTPLUG_CPU
-/*
- * tasklet_kill_immediate is called to remove a tasklet which can already be
- * scheduled for execution on @cpu.
- *
- * Unlike tasklet_kill, this function removes the tasklet
- * _immediately_, even if the tasklet is in TASKLET_STATE_SCHED state.
- *
- * When this function is called, @cpu must be in the CPU_DEAD state.
- */
-void tasklet_kill_immediate(struct tasklet_struct *t, unsigned int cpu)
-{
-	struct tasklet_struct **i;
-
-	BUG_ON(cpu_online(cpu));
-	BUG_ON(test_bit(TASKLET_STATE_RUN, &t->state));
-
-	if (!test_bit(TASKLET_STATE_SCHED, &t->state))
-		return;
-
-	/* CPU is dead, so no lock needed. */
-	for (i = &per_cpu(tasklet_vec, cpu).list; *i; i = &(*i)->next) {
-		if (*i == t) {
-			*i = t->next;
-			return;
-		}
-	}
-	BUG();
-}
-
-static void takeover_tasklets(unsigned int cpu)
-{
-	struct tasklet_struct **i;
-
-	/* CPU is dead, so no lock needed. */
-	local_irq_disable();
-
-	/* Find end, append list for that CPU. */
-	for (i = &__get_cpu_var(tasklet_vec).list; *i; i = &(*i)->next);
-	*i = per_cpu(tasklet_vec, cpu).list;
-	per_cpu(tasklet_vec, cpu).list = NULL;
-	raise_softirq_irqoff(TASKLET_SOFTIRQ);
-
-	for (i = &__get_cpu_var(tasklet_hi_vec).list; *i; i = &(*i)->next);
-	*i = per_cpu(tasklet_hi_vec, cpu).list;
-	per_cpu(tasklet_hi_vec, cpu).list = NULL;
-	raise_softirq_irqoff(HI_SOFTIRQ);
-
-	local_irq_enable();
-}
-#endif /* CONFIG_HOTPLUG_CPU */
-
 static int __cpuinit cpu_callback(struct notifier_block *nfb,
 				  unsigned long action,
 				  void *hcpu)
Index: linux-2.6-test/kernel/tasklet.c
===================================================================
--- /dev/null
+++ linux-2.6-test/kernel/tasklet.c
@@ -0,0 +1,203 @@
+/*
+ *	linux/kernel/softirq.c
+ *
+ *	Copyright (C) 1992 Linus Torvalds
+ *
+ * Rewritten. Old one was good in 2.2, but in 2.3 it was immoral. --ANK (990903)
+ *
+ *      Removed from softirq.c by Steven Rostedt
+ */
+#include <linux/interrupt.h>
+#include <linux/cpu.h>
+
+/* Tasklets */
+struct tasklet_head
+{
+	struct tasklet_struct *list;
+};
+
+/* Some compilers disobey section attribute on statics when not
+   initialized -- RR */
+static DEFINE_PER_CPU(struct tasklet_head, tasklet_vec) = { NULL };
+static DEFINE_PER_CPU(struct tasklet_head, tasklet_hi_vec) = { NULL };
+
+void fastcall __tasklet_schedule(struct tasklet_struct *t)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	t->next = __get_cpu_var(tasklet_vec).list;
+	__get_cpu_var(tasklet_vec).list = t;
+	raise_softirq_irqoff(TASKLET_SOFTIRQ);
+	local_irq_restore(flags);
+}
+
+EXPORT_SYMBOL(__tasklet_schedule);
+
+void fastcall __tasklet_hi_schedule(struct tasklet_struct *t)
+{
+	unsigned long flags;
+
+	local_irq_save(flags);
+	t->next = __get_cpu_var(tasklet_hi_vec).list;
+	__get_cpu_var(tasklet_hi_vec).list = t;
+	raise_softirq_irqoff(HI_SOFTIRQ);
+	local_irq_restore(flags);
+}
+
+EXPORT_SYMBOL(__tasklet_hi_schedule);
+
+static void tasklet_action(struct softirq_action *a)
+{
+	struct tasklet_struct *list;
+
+	local_irq_disable();
+	list = __get_cpu_var(tasklet_vec).list;
+	__get_cpu_var(tasklet_vec).list = NULL;
+	local_irq_enable();
+
+	while (list) {
+		struct tasklet_struct *t = list;
+
+		list = list->next;
+
+		if (tasklet_trylock(t)) {
+			if (!atomic_read(&t->count)) {
+				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
+					BUG();
+				t->func(t->data);
+				tasklet_unlock(t);
+				continue;
+			}
+			tasklet_unlock(t);
+		}
+
+		local_irq_disable();
+		t->next = __get_cpu_var(tasklet_vec).list;
+		__get_cpu_var(tasklet_vec).list = t;
+		__raise_softirq_irqoff(TASKLET_SOFTIRQ);
+		local_irq_enable();
+	}
+}
+
+static void tasklet_hi_action(struct softirq_action *a)
+{
+	struct tasklet_struct *list;
+
+	local_irq_disable();
+	list = __get_cpu_var(tasklet_hi_vec).list;
+	__get_cpu_var(tasklet_hi_vec).list = NULL;
+	local_irq_enable();
+
+	while (list) {
+		struct tasklet_struct *t = list;
+
+		list = list->next;
+
+		if (tasklet_trylock(t)) {
+			if (!atomic_read(&t->count)) {
+				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
+					BUG();
+				t->func(t->data);
+				tasklet_unlock(t);
+				continue;
+			}
+			tasklet_unlock(t);
+		}
+
+		local_irq_disable();
+		t->next = __get_cpu_var(tasklet_hi_vec).list;
+		__get_cpu_var(tasklet_hi_vec).list = t;
+		__raise_softirq_irqoff(HI_SOFTIRQ);
+		local_irq_enable();
+	}
+}
+
+
+void tasklet_init(struct tasklet_struct *t,
+		  void (*func)(unsigned long), unsigned long data)
+{
+	t->next = NULL;
+	t->state = 0;
+	atomic_set(&t->count, 0);
+	t->func = func;
+	t->data = data;
+}
+
+EXPORT_SYMBOL(tasklet_init);
+
+void tasklet_kill(struct tasklet_struct *t)
+{
+	if (in_interrupt())
+		printk("Attempt to kill tasklet from interrupt\n");
+
+	while (test_and_set_bit(TASKLET_STATE_SCHED, &t->state)) {
+		do
+			yield();
+		while (test_bit(TASKLET_STATE_SCHED, &t->state));
+	}
+	tasklet_unlock_wait(t);
+	clear_bit(TASKLET_STATE_SCHED, &t->state);
+}
+
+EXPORT_SYMBOL(tasklet_kill);
+
+void __init softirq_init(void)
+{
+	open_softirq(TASKLET_SOFTIRQ, tasklet_action, NULL);
+	open_softirq(HI_SOFTIRQ, tasklet_hi_action, NULL);
+}
+
+#ifdef CONFIG_HOTPLUG_CPU
+/*
+ * tasklet_kill_immediate is called to remove a tasklet which can already be
+ * scheduled for execution on @cpu.
+ *
+ * Unlike tasklet_kill, this function removes the tasklet
+ * _immediately_, even if the tasklet is in TASKLET_STATE_SCHED state.
+ *
+ * When this function is called, @cpu must be in the CPU_DEAD state.
+ */
+void tasklet_kill_immediate(struct tasklet_struct *t, unsigned int cpu)
+{
+	struct tasklet_struct **i;
+
+	BUG_ON(cpu_online(cpu));
+	BUG_ON(test_bit(TASKLET_STATE_RUN, &t->state));
+
+	if (!test_bit(TASKLET_STATE_SCHED, &t->state))
+		return;
+
+	/* CPU is dead, so no lock needed. */
+	for (i = &per_cpu(tasklet_vec, cpu).list; *i; i = &(*i)->next) {
+		if (*i == t) {
+			*i = t->next;
+			return;
+		}
+	}
+	BUG();
+}
+
+void takeover_tasklets(unsigned int cpu)
+{
+	struct tasklet_struct **i;
+
+	/* CPU is dead, so no lock needed. */
+	local_irq_disable();
+
+	/* Find end, append list for that CPU. */
+	for (i = &__get_cpu_var(tasklet_vec).list; *i; i = &(*i)->next);
+	*i = per_cpu(tasklet_vec, cpu).list;
+	per_cpu(tasklet_vec, cpu).list = NULL;
+	raise_softirq_irqoff(TASKLET_SOFTIRQ);
+
+	for (i = &__get_cpu_var(tasklet_hi_vec).list; *i; i = &(*i)->next);
+	*i = per_cpu(tasklet_hi_vec, cpu).list;
+	per_cpu(tasklet_hi_vec, cpu).list = NULL;
+	raise_softirq_irqoff(HI_SOFTIRQ);
+
+	local_irq_enable();
+}
+#endif /* CONFIG_HOTPLUG_CPU */
+
+

-- 

[RFC PATCH 3/5 v2] Add a tasklet is-scheduled API

[Steven Rostedt]

[-- Attachment #1: tasklet-state-api.patch --]
[-- Type: text/plain, Size: 773 bytes --]

This patch adds a tasklet_is_scheduled API to allow a driver
to know if its tasklet is already scheduled.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

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
@@ -107,6 +107,11 @@ static inline void tasklet_hi_enable(str
 	atomic_dec(&t->count);
 }
 
+static inline int tasklet_is_scheduled(struct tasklet_struct *t)
+{
+	return test_bit(TASKLET_STATE_SCHED, &t->state);
+}
+
 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,

-- 

[RFC PATCH 4/5 v2] Make DRM use the tasklet is-sched API

[Steven Rostedt]

[-- Attachment #1: tasklet-driver-hacks.patch --]
[-- Type: text/plain, Size: 737 bytes --]

Update the DRM driver to use the new tasklet API, which does not rely
on the tasklet implementation details.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>


Index: linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
===================================================================
--- linux-2.6.21-rt9.orig/drivers/char/drm/drm_irq.c
+++ linux-2.6.21-rt9/drivers/char/drm/drm_irq.c
@@ -461,7 +461,7 @@ void drm_locked_tasklet(drm_device_t *de
 	static DECLARE_TASKLET(drm_tasklet, drm_locked_tasklet_func, 0);
 
 	if (!drm_core_check_feature(dev, DRIVER_HAVE_IRQ) ||
-	    test_bit(TASKLET_STATE_SCHED, &drm_tasklet.state))
+	    tasklet_is_scheduled(&drm_tasklet))
 		return;
 
 	spin_lock_irqsave(&dev->tasklet_lock, irqflags);

-- 

[RFC PATCH 5/5 v2] Convert tasklets to work queues

[Steven Rostedt]

[-- Attachment #1: tasklets-to-workqueues.patch --]
[-- Type: text/plain, Size: 14686 bytes --]

This patch replaces the tasklet implementation with a work
queue implementation while keeping the tasklet API.
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 <rostedt@goodmis.org>


Index: linux-2.6-test/kernel/Makefile
===================================================================
--- linux-2.6-test.orig/kernel/Makefile
+++ linux-2.6-test/kernel/Makefile
@@ -21,7 +21,7 @@ obj-$(CONFIG_FUTEX) += futex.o
 ifeq ($(CONFIG_COMPAT),y)
 obj-$(CONFIG_FUTEX) += futex_compat.o
 endif
-obj-y += tasklet.o
+obj-y += tasklet_work.o
 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
@@ -120,7 +120,7 @@ EXPORT_SYMBOL(system_state);
 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);
+extern void init_tasklets(void);
 
 /* Untouched command line saved by arch-specific code. */
 char __initdata boot_command_line[COMMAND_LINE_SIZE];
@@ -563,7 +563,6 @@ asmlinkage void __init start_kernel(void
 	pidhash_init();
 	init_timers();
 	hrtimers_init();
-	softirq_init();
 	timekeeping_init();
 	time_init();
 	profile_init();
@@ -706,6 +705,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,125 +1,58 @@
-#ifndef _LINUX_TASKLET_H
-#define _LINUX_TASKLET_H
+#ifndef _LINUX_WORK_TASKLET_H
+#define _LINUX_WORK_TASKLET_H
 
-/* Tasklets --- multithreaded analogue of BHs.
+#include <linux/workqueue.h>
 
-   Main feature differing them of generic softirqs: tasklet
-   is running only on one CPU simultaneously.
-
-   Main feature differing them of BHs: different tasklets
-   may be run simultaneously on different CPUs.
-
-   Properties:
-   * If tasklet_schedule() is called, then tasklet is guaranteed
-     to be executed on some cpu at least once after this.
-   * If the tasklet is already scheduled, but its excecution is still not
-     started, it will be executed only once.
-   * If this tasklet is already running on another CPU (or schedule is called
-     from tasklet itself), it is rescheduled for later.
-   * Tasklet is strictly serialized wrt itself, but not
-     wrt another tasklets. If client needs some intertask synchronization,
-     he makes it with spinlocks.
- */
+extern void work_tasklet_exec(struct work_struct *work);
 
 struct tasklet_struct
 {
-	struct tasklet_struct *next;
+	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 = { NULL, 0, ATOMIC_INIT(0), func, data }
-
-#define DECLARE_TASKLET_DISABLED(name, func, data) \
-struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(1), func, data }
-
-
-enum
-{
-	TASKLET_STATE_SCHED,	/* Tasklet is scheduled for execution */
-	TASKLET_STATE_RUN	/* Tasklet is running (SMP only) */
-};
-
-#ifdef CONFIG_SMP
-static inline int tasklet_trylock(struct tasklet_struct *t)
-{
-	return !test_and_set_bit(TASKLET_STATE_RUN, &(t)->state);
-}
-
-static inline void tasklet_unlock(struct tasklet_struct *t)
-{
-	smp_mb__before_clear_bit(); 
-	clear_bit(TASKLET_STATE_RUN, &(t)->state);
-}
-
-static inline void tasklet_unlock_wait(struct tasklet_struct *t)
-{
-	while (test_bit(TASKLET_STATE_RUN, &(t)->state)) { barrier(); }
-}
-#else
-#define tasklet_trylock(t) 1
-#define tasklet_unlock_wait(t) do { } while (0)
-#define tasklet_unlock(t) do { } while (0)
-#endif
-
-extern void FASTCALL(__tasklet_schedule(struct tasklet_struct *t));
-
-static inline void tasklet_schedule(struct tasklet_struct *t)
-{
-	if (!test_and_set_bit(TASKLET_STATE_SCHED, &t->state))
-		__tasklet_schedule(t);
-}
-
-extern void FASTCALL(__tasklet_hi_schedule(struct tasklet_struct *t));
-
-static inline void tasklet_hi_schedule(struct tasklet_struct *t)
-{
-	if (!test_and_set_bit(TASKLET_STATE_SCHED, &t->state))
-		__tasklet_hi_schedule(t);
-}
-
-
-static inline void tasklet_disable_nosync(struct tasklet_struct *t)
-{
-	atomic_inc(&t->count);
-	smp_mb__after_atomic_inc();
-}
+#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
 
-static inline void tasklet_disable(struct tasklet_struct *t)
-{
-	tasklet_disable_nosync(t);
-	tasklet_unlock_wait(t);
-	smp_mb();
-}
-
-static inline void tasklet_enable(struct tasklet_struct *t)
-{
-	smp_mb__before_atomic_dec();
-	atomic_dec(&t->count);
-}
+void tasklet_disable_nosync(struct tasklet_struct *t);
+void tasklet_disable(struct tasklet_struct *t);
 
-static inline void tasklet_hi_enable(struct tasklet_struct *t)
-{
-	smp_mb__before_atomic_dec();
-	atomic_dec(&t->count);
-}
-
-static inline int tasklet_is_scheduled(struct tasklet_struct *t)
-{
-	return test_bit(TASKLET_STATE_SCHED, &t->state);
-}
+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);
-
-#ifdef CONFIG_HOTPLUG_CPU
 void takeover_tasklets(unsigned int cpu);
-#endif /* CONFIG_HOTPLUG_CPU */
 
-#endif /* _LINUX_TASKLET_H */
 
+#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,124 @@
+/*
+ *	linux/kernel/tasklet_work.c
+ *
+ *	Copyright (C) 2007 Steven Rostedt, Red Hat
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/tasklet.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)
+{
+	WARN_ON_ONCE(!ktaskletd_wq);
+	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);
+	return ret;
+}
+
+EXPORT_SYMBOL(tasklet_is_scheduled);
+
+void tasklet_disable_nosync(struct tasklet_struct *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);
+	flush_workqueue(ktaskletd_wq);
+	smp_mb();
+}
+
+EXPORT_SYMBOL(tasklet_disable);
+
+void fastcall tasklet_enable(struct tasklet_struct *t)
+{
+	if (!atomic_dec_and_test(&t->count))
+		return;
+	if (test_and_clear_bit(TASKLET_STATE_PENDING, &t->state))
+		tasklet_schedule(t);
+}
+
+EXPORT_SYMBOL(tasklet_enable);
+
+void tasklet_kill(struct tasklet_struct *t)
+{
+	flush_workqueue(ktaskletd_wq);
+}
+
+EXPORT_SYMBOL(tasklet_kill);
+
+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))) {
+		set_bit(TASKLET_STATE_PENDING, &t->state);
+		smp_mb();
+		/* make sure we were not just enabled */
+		if (likely(atomic_read(&t->count)))
+			goto out;
+		if (!test_and_clear_bit(TASKLET_STATE_PENDING, &t->state))
+			goto out;
+	}
+
+	local_bh_disable();
+	t->func(t->data);
+	local_bh_enable();
+
+out:
+	return;
+}
+
+EXPORT_SYMBOL(work_tasklet_exec);
+
+void __init init_tasklets(void)
+{
+	ktaskletd_wq = create_workqueue("tasklets");
+	BUG_ON(!ktaskletd_wq);
+}
+
+void takeover_tasklets(unsigned int cpu)
+{
+}
+
+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);
Index: linux-2.6-test/kernel/tasklet.c
===================================================================
--- linux-2.6-test.orig/kernel/tasklet.c
+++ /dev/null
@@ -1,203 +0,0 @@
-/*
- *	linux/kernel/softirq.c
- *
- *	Copyright (C) 1992 Linus Torvalds
- *
- * Rewritten. Old one was good in 2.2, but in 2.3 it was immoral. --ANK (990903)
- *
- *      Removed from softirq.c by Steven Rostedt
- */
-#include <linux/interrupt.h>
-#include <linux/cpu.h>
-
-/* Tasklets */
-struct tasklet_head
-{
-	struct tasklet_struct *list;
-};
-
-/* Some compilers disobey section attribute on statics when not
-   initialized -- RR */
-static DEFINE_PER_CPU(struct tasklet_head, tasklet_vec) = { NULL };
-static DEFINE_PER_CPU(struct tasklet_head, tasklet_hi_vec) = { NULL };
-
-void fastcall __tasklet_schedule(struct tasklet_struct *t)
-{
-	unsigned long flags;
-
-	local_irq_save(flags);
-	t->next = __get_cpu_var(tasklet_vec).list;
-	__get_cpu_var(tasklet_vec).list = t;
-	raise_softirq_irqoff(TASKLET_SOFTIRQ);
-	local_irq_restore(flags);
-}
-
-EXPORT_SYMBOL(__tasklet_schedule);
-
-void fastcall __tasklet_hi_schedule(struct tasklet_struct *t)
-{
-	unsigned long flags;
-
-	local_irq_save(flags);
-	t->next = __get_cpu_var(tasklet_hi_vec).list;
-	__get_cpu_var(tasklet_hi_vec).list = t;
-	raise_softirq_irqoff(HI_SOFTIRQ);
-	local_irq_restore(flags);
-}
-
-EXPORT_SYMBOL(__tasklet_hi_schedule);
-
-static void tasklet_action(struct softirq_action *a)
-{
-	struct tasklet_struct *list;
-
-	local_irq_disable();
-	list = __get_cpu_var(tasklet_vec).list;
-	__get_cpu_var(tasklet_vec).list = NULL;
-	local_irq_enable();
-
-	while (list) {
-		struct tasklet_struct *t = list;
-
-		list = list->next;
-
-		if (tasklet_trylock(t)) {
-			if (!atomic_read(&t->count)) {
-				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
-					BUG();
-				t->func(t->data);
-				tasklet_unlock(t);
-				continue;
-			}
-			tasklet_unlock(t);
-		}
-
-		local_irq_disable();
-		t->next = __get_cpu_var(tasklet_vec).list;
-		__get_cpu_var(tasklet_vec).list = t;
-		__raise_softirq_irqoff(TASKLET_SOFTIRQ);
-		local_irq_enable();
-	}
-}
-
-static void tasklet_hi_action(struct softirq_action *a)
-{
-	struct tasklet_struct *list;
-
-	local_irq_disable();
-	list = __get_cpu_var(tasklet_hi_vec).list;
-	__get_cpu_var(tasklet_hi_vec).list = NULL;
-	local_irq_enable();
-
-	while (list) {
-		struct tasklet_struct *t = list;
-
-		list = list->next;
-
-		if (tasklet_trylock(t)) {
-			if (!atomic_read(&t->count)) {
-				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
-					BUG();
-				t->func(t->data);
-				tasklet_unlock(t);
-				continue;
-			}
-			tasklet_unlock(t);
-		}
-
-		local_irq_disable();
-		t->next = __get_cpu_var(tasklet_hi_vec).list;
-		__get_cpu_var(tasklet_hi_vec).list = t;
-		__raise_softirq_irqoff(HI_SOFTIRQ);
-		local_irq_enable();
-	}
-}
-
-
-void tasklet_init(struct tasklet_struct *t,
-		  void (*func)(unsigned long), unsigned long data)
-{
-	t->next = NULL;
-	t->state = 0;
-	atomic_set(&t->count, 0);
-	t->func = func;
-	t->data = data;
-}
-
-EXPORT_SYMBOL(tasklet_init);
-
-void tasklet_kill(struct tasklet_struct *t)
-{
-	if (in_interrupt())
-		printk("Attempt to kill tasklet from interrupt\n");
-
-	while (test_and_set_bit(TASKLET_STATE_SCHED, &t->state)) {
-		do
-			yield();
-		while (test_bit(TASKLET_STATE_SCHED, &t->state));
-	}
-	tasklet_unlock_wait(t);
-	clear_bit(TASKLET_STATE_SCHED, &t->state);
-}
-
-EXPORT_SYMBOL(tasklet_kill);
-
-void __init softirq_init(void)
-{
-	open_softirq(TASKLET_SOFTIRQ, tasklet_action, NULL);
-	open_softirq(HI_SOFTIRQ, tasklet_hi_action, NULL);
-}
-
-#ifdef CONFIG_HOTPLUG_CPU
-/*
- * tasklet_kill_immediate is called to remove a tasklet which can already be
- * scheduled for execution on @cpu.
- *
- * Unlike tasklet_kill, this function removes the tasklet
- * _immediately_, even if the tasklet is in TASKLET_STATE_SCHED state.
- *
- * When this function is called, @cpu must be in the CPU_DEAD state.
- */
-void tasklet_kill_immediate(struct tasklet_struct *t, unsigned int cpu)
-{
-	struct tasklet_struct **i;
-
-	BUG_ON(cpu_online(cpu));
-	BUG_ON(test_bit(TASKLET_STATE_RUN, &t->state));
-
-	if (!test_bit(TASKLET_STATE_SCHED, &t->state))
-		return;
-
-	/* CPU is dead, so no lock needed. */
-	for (i = &per_cpu(tasklet_vec, cpu).list; *i; i = &(*i)->next) {
-		if (*i == t) {
-			*i = t->next;
-			return;
-		}
-	}
-	BUG();
-}
-
-void takeover_tasklets(unsigned int cpu)
-{
-	struct tasklet_struct **i;
-
-	/* CPU is dead, so no lock needed. */
-	local_irq_disable();
-
-	/* Find end, append list for that CPU. */
-	for (i = &__get_cpu_var(tasklet_vec).list; *i; i = &(*i)->next);
-	*i = per_cpu(tasklet_vec, cpu).list;
-	per_cpu(tasklet_vec, cpu).list = NULL;
-	raise_softirq_irqoff(TASKLET_SOFTIRQ);
-
-	for (i = &__get_cpu_var(tasklet_hi_vec).list; *i; i = &(*i)->next);
-	*i = per_cpu(tasklet_hi_vec, cpu).list;
-	per_cpu(tasklet_hi_vec, cpu).list = NULL;
-	raise_softirq_irqoff(HI_SOFTIRQ);
-
-	local_irq_enable();
-}
-#endif /* CONFIG_HOTPLUG_CPU */
-
-

-- 

Re: [RFC PATCH 1/5 v2] Convert the RCU tasklet into a softirq

[Paul E. McKenney]

On Fri, Jun 22, 2007 at 02:20:52PM -0400, Steven Rostedt wrote:
> I believe this was originally done by Dipankar Sarma. I pulled these
> changes from the -rt kernel.
> 
> For better preformance, RCU should use a softirq instead of a
> tasklet.
> 
> From: Dipankar Sarma <dipankar@in.ibm.com>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> Index: linux-2.6-test/include/linux/interrupt.h
> ===================================================================
> --- linux-2.6-test.orig/include/linux/interrupt.h
> +++ linux-2.6-test/include/linux/interrupt.h
> @@ -245,6 +245,9 @@ enum
>  #ifdef CONFIG_HIGH_RES_TIMERS
>  	HRTIMER_SOFTIRQ,
>  #endif
> +	RCU_SOFTIRQ,	/* Preferable RCU should always be the last softirq */
> +	/* Entries after this are ignored in split softirq mode */
> +	MAX_SOFTIRQ,
>  };
> 
>  /* softirq mask and active fields moved to irq_cpustat_t in
> Index: linux-2.6-test/kernel/rcupdate.c
> ===================================================================
> --- linux-2.6-test.orig/kernel/rcupdate.c
> +++ linux-2.6-test/kernel/rcupdate.c
> @@ -67,7 +67,6 @@ DEFINE_PER_CPU(struct rcu_data, rcu_data
>  DEFINE_PER_CPU(struct rcu_data, rcu_bh_data) = { 0L };
> 
>  /* Fake initialization required by compiler */
> -static DEFINE_PER_CPU(struct tasklet_struct, rcu_tasklet) = {NULL};
>  static int blimit = 10;
>  static int qhimark = 10000;
>  static int qlowmark = 100;
> @@ -253,7 +252,7 @@ static void rcu_do_batch(struct rcu_data
>  	if (!rdp->donelist)
>  		rdp->donetail = &rdp->donelist;
>  	else
> -		tasklet_schedule(&per_cpu(rcu_tasklet, rdp->cpu));
> +		raise_softirq(RCU_SOFTIRQ);
>  }
> 
>  /*
> @@ -405,7 +404,6 @@ static void rcu_offline_cpu(int cpu)
>  					&per_cpu(rcu_bh_data, cpu));
>  	put_cpu_var(rcu_data);
>  	put_cpu_var(rcu_bh_data);
> -	tasklet_kill_immediate(&per_cpu(rcu_tasklet, cpu), cpu);
>  }
> 
>  #else
> @@ -417,7 +415,7 @@ static void rcu_offline_cpu(int cpu)
>  #endif
> 
>  /*
> - * This does the RCU processing work from tasklet context. 
> + * This does the RCU processing work from softirq context.
>   */
>  static void __rcu_process_callbacks(struct rcu_ctrlblk *rcp,
>  					struct rcu_data *rdp)
> @@ -462,7 +460,7 @@ static void __rcu_process_callbacks(stru
>  		rcu_do_batch(rdp);
>  }
> 
> -static void rcu_process_callbacks(unsigned long unused)
> +static void rcu_process_callbacks(struct softirq_action *unused)
>  {
>  	__rcu_process_callbacks(&rcu_ctrlblk, &__get_cpu_var(rcu_data));
>  	__rcu_process_callbacks(&rcu_bh_ctrlblk, &__get_cpu_var(rcu_bh_data));
> @@ -526,7 +524,7 @@ void rcu_check_callbacks(int cpu, int us
>  		rcu_bh_qsctr_inc(cpu);
>  	} else if (!in_softirq())
>  		rcu_bh_qsctr_inc(cpu);
> -	tasklet_schedule(&per_cpu(rcu_tasklet, cpu));
> +	raise_softirq(RCU_SOFTIRQ);
>  }
> 
>  static void rcu_init_percpu_data(int cpu, struct rcu_ctrlblk *rcp,
> @@ -549,7 +547,7 @@ static void __devinit rcu_online_cpu(int
> 
>  	rcu_init_percpu_data(cpu, &rcu_ctrlblk, rdp);
>  	rcu_init_percpu_data(cpu, &rcu_bh_ctrlblk, bh_rdp);
> -	tasklet_init(&per_cpu(rcu_tasklet, cpu), rcu_process_callbacks, 0UL);
> +	open_softirq(RCU_SOFTIRQ, rcu_process_callbacks, NULL);
>  }
> 
>  static int __cpuinit rcu_cpu_notify(struct notifier_block *self,
> 
> -- 

Re: [RFC PATCH 5/5 v2] Convert tasklets to work queues

[Andrew Morton]

> On Fri, 22 Jun 2007 14:20:56 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:
> This patch replaces the tasklet implementation with a work
> queue implementation while keeping the tasklet API.
> The API is still the same, and the drivers don't know that a
> work queue is being used.

I still think this sucks: we end up with a fairly pointless-looking wrapper
API which we wouldn't have put in the kernel except for messy legacy
editing-is-too-hard reasons.  In an ideal world, it'd be better just to get
in and do the hard work.

Anyway.  Please fix the many correct warnings which checkpatch.pl
generates, reissue the patches with a decent overall changelog (this series
had no rationale for the changes at all) and we'll see what happens.

Thanks.

Re: [RFC PATCH 5/5 v2] Convert tasklets to work queues

[Oleg Nesterov]

minor nitpick,

On 06/22, Steven Rostedt wrote:
>
> +void takeover_tasklets(unsigned int cpu)
> +{
> +}

Since you don't use config option any longer, I think it is better
to just kill this function and fix it's only caller.

You can also kill the declaration of tasklet_kill_immediate() in
tasklet.h.

Oleg.

Re: [RFC PATCH 5/5 v2] Convert tasklets to work queues

[Steven Rostedt]

On Sat, 2007-06-23 at 09:53 -0700, Andrew Morton wrote:
> > On Fri, 22 Jun 2007 14:20:56 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:
> > This patch replaces the tasklet implementation with a work
> > queue implementation while keeping the tasklet API.
> > The API is still the same, and the drivers don't know that a
> > work queue is being used.
> 
> I still think this sucks: we end up with a fairly pointless-looking wrapper
> API which we wouldn't have put in the kernel except for messy legacy
> editing-is-too-hard reasons.  In an ideal world, it'd be better just to get
> in and do the hard work.

If I had all the hardware that tasklets touch, I'd start doing it. (look
for more patches from me that do this for the hardware I do have :-)

> 
> Anyway.  Please fix the many correct warnings which checkpatch.pl

I'll have to add a clean up patch. A lot of the output from that came
from me moving existing code.  So I'll have to clean up the existing
code first so that checkpatch.pl doesn't complain about it.  I prefer a
patch that moves code to do nothing but that, move code and not even add
clean ups to it.  So the simple solution is, add patch to clean up (will
do).

Also this:

   void tasklet_schedule(struct tasklet_struct *t)
   {
           WARN_ON_ONCE(!ktaskletd_wq);
           queue_work(ktaskletd_wq, &t->work);
   }

   EXPORT_SYMBOL(tasklet_schedule);


produces

   PATCH: tasklets-to-workqueues.patch:259:
   FILE: linux-2.6-test/kernel/tasklet_work.c:30:
   +EXPORT_SYMBOL(tasklet_schedule);


So I guess that blank line is not acceptable?
and should be:

   void tasklet_schedule(struct tasklet_struct *t)
   {
           WARN_ON_ONCE(!ktaskletd_wq);
           queue_work(ktaskletd_wq, &t->work);
   }
   EXPORT_SYMBOL(tasklet_schedule);

I'll fix that up.

> generates, reissue the patches with a decent overall changelog (this series
> had no rationale for the changes at all) and we'll see what happens.

Should I just put in the prologue from the first series, or slim it down
a bit so people don't need to read the whole thing every time? I can
just put in the stuff that's relevant and leave out the "history".

Thanks,

-- Steve

Re: [RFC PATCH 5/5 v2] Convert tasklets to work queues

[Linus Torvalds]

On Sat, 23 Jun 2007, Andrew Morton wrote:
> 
> Anyway.  Please fix the many correct warnings which checkpatch.pl
> generates

Actually, please don't.

Especially for code movement, *just* do the movement. Screw any 
checkpatch.pl crap - the code is better off not changing, because that way 
a big patch can not only be proven to not change anything at all, but 
software archeology tools can trivially find the true history of the code 
over code movement.

For example, "git blame -C" already finds copies and can annotate the 
history of a line of code past a pure code movement. But if you move *and* 
change things at the same time, it gets a lot harder to show where the 
code came from and that the movement itself caused no regressions.

So do cleanups _separately_ from movement.

(Yeah, yeah, "git blame -C -w" will generally work across whitespace 
changes too, but only whitespace _within_ a line. If you do things like 
split long lines etc, you immediately have a lot harder time to follow 
these thigns. Not impossible, but the point is that you're not *fixing* 
anything, you're just making things *worse* by doing changes and code 
movement at the same time).

Quite frankly, I personally am considering removing "checkpatch.pl". That 
thing is just a nazi dream. That hard-coded 80-character limit etc is just 
bad taste. 

Dammit, code cleanliness is not about "automated and mindless slavish 
following of rules". A process that is too inflexible is a *bad* process. 
I'd much rather have a few 80+ character lines than stupid and unreadable 
line wrapping just because the line hit 87 characters in length.

I don't have 25 lines on a screen either. 

		Linus

Re: [RFC PATCH 5/5 v2] Convert tasklets to work queues

[Randy Dunlap]

On Sat, 23 Jun 2007 11:19:18 -0700 (PDT) Linus Torvalds wrote:

> 
> 
> On Sat, 23 Jun 2007, Andrew Morton wrote:
> > 
> > Anyway.  Please fix the many correct warnings which checkpatch.pl
> > generates
> 
> Actually, please don't.
> 
> Especially for code movement, *just* do the movement. Screw any 
> checkpatch.pl crap - the code is better off not changing, because that way 
> a big patch can not only be proven to not change anything at all, but 
> software archeology tools can trivially find the true history of the code 
> over code movement.
> 
> For example, "git blame -C" already finds copies and can annotate the 
> history of a line of code past a pure code movement. But if you move *and* 
> change things at the same time, it gets a lot harder to show where the 
> code came from and that the movement itself caused no regressions.
> 
> So do cleanups _separately_ from movement.
> 
> (Yeah, yeah, "git blame -C -w" will generally work across whitespace 
> changes too, but only whitespace _within_ a line. If you do things like 
> split long lines etc, you immediately have a lot harder time to follow 
> these thigns. Not impossible, but the point is that you're not *fixing* 
> anything, you're just making things *worse* by doing changes and code 
> movement at the same time).
> 
> Quite frankly, I personally am considering removing "checkpatch.pl". That 
> thing is just a nazi dream. That hard-coded 80-character limit etc is just 
> bad taste.

Who wrote that part of CodingStyle?


The script is certainly no substitute for personal review.
And it's certainly not the final say on anything.
(neither is CodingStyle AFAIK)
It's just a helper for Andrew.

The problem IMO is that we are seeing less and less patch review
but it needs to be more and more.  Andrew is one of a handful of
people who are reviewing lots of patches.  It shouldn't be his
wheelbarrow to have to push around all the time.  So if a little
automation can help Andrew, that's a good thing.  Until people
revolt, that is.


> Dammit, code cleanliness is not about "automated and mindless slavish 
> following of rules". A process that is too inflexible is a *bad* process. 
> I'd much rather have a few 80+ character lines than stupid and unreadable 
> line wrapping just because the line hit 87 characters in length.
> 
> I don't have 25 lines on a screen either. 


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [RFC PATCH 5/5 v2] Convert tasklets to work queues

[Andrew Morton]

On Sat, 23 Jun 2007 11:52:52 -0700 Randy Dunlap <rdunlap@xenotime.net> wrote:

> > Quite frankly, I personally am considering removing "checkpatch.pl". That 
> > thing is just a nazi dream. That hard-coded 80-character limit etc is just 
> > bad taste.
> 
> Who wrote that part of CodingStyle?
> 
> 
> The script is certainly no substitute for personal review.
> And it's certainly not the final say on anything.
> (neither is CodingStyle AFAIK)
> It's just a helper for Andrew.

people are free to disagree with it - it's more of a "hey,
did you really mean to do this" thing.

It might need some fine-tuning..

Re: [RFC PATCH 5/5 v2] Convert tasklets to work queues

[Linus Torvalds]

On Sat, 23 Jun 2007, Randy Dunlap wrote:
> > 
> > Quite frankly, I personally am considering removing "checkpatch.pl". That 
> > thing is just a nazi dream. That hard-coded 80-character limit etc is just 
> > bad taste.
> 
> Who wrote that part of CodingStyle?

I don't think that the notion of "80 characters per line" is *wrong* per 
se.

So don't take this the wrong way - I think the goal really _should_ be 
that a function fits on a vt100 terminal window (*both* ways: less than 24
lines of code, and less than 80 characters wide).

So I think the notion of trying to keep lines below 80 characters is 
admirable. That part isn't the problem.

The problem is when "coding style guidelines" become "hard inviolate 
rules".

Yes, code should be less than 80 characters wide. 

But hey, sometimes it's just more readable to have one line that is 
slightly longer than it should be, than to split something that is awkward 
to split. 

The thing that scripts (and computers) have a really hard time with is 
"judgement". 

So I don't disagree with any of the checkpatch.pl things individually, but 
I do disagree with the notion that we should enforce strict rules, when 
all the guidelines are really just guidelines.

For example, the coding style also says that you should avoid indentation 
that is more than tree deep. It's _true_, but does that mean that we 
should make deeply indented code *illegal*? Obviously not. It's a "please 
try to split your functions up" kind of thing.  Not a hard rule.

The only reason I picked the 80-character thing in particular is that 
I've seen a fair number of patches that don't do anything *but* change 
that, and quite often I think it makes the code look worse if it's not 
done judiciously..

There are other things we really *could* be strict against. For example, 
the "space followed by tab" thing really is somethign where a strict rule 
(at least for C files) probably really *is* a good idea, because there 
really is never a really good reason for it. Add the fact that the problem 
is "invisible" in most editors, and having a script that checks for it 
makes even more sense.

So I'm happy with checkpatch.pl as a *guide*, but I'm not happy if it 
means that people start taking it as a *requirement*.

(And then I'm doubly unhappy when there are issues like code movement and 
file renames etc, where there are other reasons why we actually want to 
keep the code as unmodified as possible).

		Linus

Re: [RFC PATCH 5/5 v2] Convert tasklets to work queues

[Steven Rostedt]

On Sat, 2007-06-23 at 11:19 -0700, Linus Torvalds wrote:
> 
> On Sat, 23 Jun 2007, Andrew Morton wrote:
> > 
> > Anyway.  Please fix the many correct warnings which checkpatch.pl
> > generates
> 
> Actually, please don't.

You sure?  If I were to do this recommended fix, I would simply add
another patch. One to do the fixes, the other to move the code. That way
it should be trivial for something like git to recognize the code
movement, since the code movement patch would be just that, move code,
nothing else.  The clean up would be a separate change.

-- Steve

Re: [RFC PATCH 5/5 v2] Convert tasklets to work queues

[Andrew Morton]

On Sat, 23 Jun 2007 15:27:49 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 2007-06-23 at 11:19 -0700, Linus Torvalds wrote:
> > 
> > On Sat, 23 Jun 2007, Andrew Morton wrote:
> > > 
> > > Anyway.  Please fix the many correct warnings which checkpatch.pl
> > > generates
> > 
> > Actually, please don't.
> 
> You sure?  If I were to do this recommended fix, I would simply add
> another patch. One to do the fixes, the other to move the code. That way
> it should be trivial for something like git to recognize the code
> movement, since the code movement patch would be just that, move code,
> nothing else.  The clean up would be a separate change.
> 

If it's just code movement (I missed that fact) then I'd leave it be:
mucking with whitespace and stuff is unrelated to this work.

Re: [RFC PATCH 0/5 v2] Convert all tasklets to workqueues V2

[Ed Tomlinson]

Hi,

Applied this to 2.6.21-5 along with an older version of dyntick and cfs v18, durring boot I got:

[   54.154077] hci_usb_isoc_rx_submit: hci0 isoc rx submit failed urb ffff81004ec55628 err -38
[   54.154086] hci_usb_isoc_rx_submit: hci0 isoc rx submit failed urb ffff81004ec55628 err -38
[   54.168147] BUG: at kernel/mutex.c:132 __mutex_lock_common()
[   54.170801]
[   54.170802] Call Trace:
[   54.175975]  [<ffffffff801790a0>] check_preempt_curr_fair+0x70/0x90
[   54.178694]  [<ffffffff8015b11f>] __mutex_lock_slowpath+0x6f/0x200
[   54.181417]  [<ffffffff8015b2c9>] mutex_lock+0x19/0x20
[   54.184165]  [<ffffffff80185c01>] flush_workqueue+0x31/0x50
[   54.186975]  [<ffffffff80190e75>] tasklet_disable+0x15/0x20
[   54.189829]  [<ffffffff8815289f>] :bluetooth:hci_cc_host_ctl+0x17f/0x240
[   54.192767]  [<ffffffff88153e4c>] :bluetooth:hci_event_packet+0x139c/0x1560
[   54.195712]  [<ffffffff88154d24>] :bluetooth:hci_send_to_sock+0x134/0x180
[   54.198657]  [<ffffffff8815073f>] :bluetooth:hci_rx_task+0x9f/0x270
[   54.201588]  [<ffffffff80190df0>] work_tasklet_exec+0x0/0x50
[   54.204473]  [<ffffffff80190e2c>] work_tasklet_exec+0x3c/0x50
[   54.207282]  [<ffffffff801488d4>] run_workqueue+0x94/0x130
[   54.210032]  [<ffffffff80145c59>] worker_thread+0x149/0x190
[   54.212781]  [<ffffffff80177430>] default_wake_function+0x0/0x10
[   54.215539]  [<ffffffff80145b10>] worker_thread+0x0/0x190
[   54.218241]  [<ffffffff801300f3>] kthread+0xd3/0x110
[   54.220880]  [<ffffffff801581c8>] child_rip+0xa/0x12
[   54.223484]  [<ffffffff80130020>] kthread+0x0/0x110
[   54.226075]  [<ffffffff801581be>] child_rip+0x0/0x12

Has this patch uncovered a problem in bluetooth or is it a problem with the patch?

TIA,
Ed Tomlinson

On Friday 22 June 2007 14:20, Steven Rostedt wrote:
> -- 
> 
> This is version 2 of the tasklet to workqueue conversion.
> 
> Changes from version 1.
> 
> - removed config option and simply replace the old implementation
>   with the work queue one (recommended by Ingo Molnar).
> 
> - replaced clear_bit with test_and_clear_bit to avoid the race of
>   executing the tasklet function twice. (thanks to Oleg Nesterov
>   for pointing that out).
> 
> - Removed most of the pr_debug prints. (Kept one)
>   (recommended by Ingo Molnar)
> 
> - Removed call to softirq_init.
> 
> - Added Author credit to Dipankar Sarma for the RCU tasklet to
>   softirq conversion.
> 
> - Tested on my Powerbook to add another arch to the mix :-)
>   Funny that booting with this change was the first time that
>   the bcm43xx didn't get stuck for several seconds on bootup.
>   It's also one of the few drivers that use tasklet_disable_nosync.
>   So either this shows that the change fixed something, or that
>   it broke something (or was just a fluke).
> 
> 
> -- Steve
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [RFC PATCH 5/5 v2] Convert tasklets to work queues

[Linus Torvalds]

On Sat, 23 Jun 2007, Steven Rostedt wrote:
> > 
> > On Sat, 23 Jun 2007, Andrew Morton wrote:
> > > 
> > > Anyway.  Please fix the many correct warnings which checkpatch.pl
> > > generates
> > 
> > Actually, please don't.
> 
> You sure?  If I were to do this recommended fix, I would simply add
> another patch. One to do the fixes, the other to move the code.

Sure, that sounds fine.

		Linus

Re: [RFC PATCH 5/5 v2] Convert tasklets to work queues

[Jeff Garzik]

Linus Torvalds wrote:
> So do cleanups _separately_ from movement.

Definitely.  Anything else makes review more difficult, by obscuring 
changes with movement.


> Quite frankly, I personally am considering removing "checkpatch.pl". That 
> thing is just a nazi dream. That hard-coded 80-character limit etc is just 
> bad taste. 
> 
> Dammit, code cleanliness is not about "automated and mindless slavish 
> following of rules". A process that is too inflexible is a *bad* process. 
> I'd much rather have a few 80+ character lines than stupid and unreadable 
> line wrapping just because the line hit 87 characters in length.

I don't think checkpatch should be removed, but the 80-column complaint 
is -way- too obnoxious and stupid-simple.

This bugs me like the myriad recent Documentation/CodingStyle proposed 
patches...  It's STYLE dammit.  Sometimes it's best to /not/ lock down 
everything into a rule.  There is such a thing as specifying too much.

	Jeff

Coding style on function signatures (was: Convert tasklets to work queues )

[Jim Cromie]

Linus Torvalds wrote:
>
>   
> Yes, code should be less than 80 characters wide. 
>
> But hey, sometimes it's just more readable to have one line that is 
> slightly longer than it should be, than to split something that is awkward 
> to split. 
>
>   
< cc-list heavily trimmed >


could you speak to the specific case of function signatures ?
I saw nothing in CodingStyle specifically about this.
(I skimmed, and grepped for signature)

forex:

static ssize_t
store_fan_div (struct device *dev, struct device_attribute *devattr,
                    const char *buf, size_t count)
{...}


IIRC, many like the entire sig on one line, because its grep friendly.
I personally like the above, but grep-ability is hard to argue against.

The above has 2 violations (of strict-grep-ability rule)
1 - return sig is separate
2 - arg-list is split

Re: Coding style on function signatures (was: Convert tasklets to work queues )

[Randy Dunlap]

On Sat, 07 Jul 2007 18:49:07 -0600 Jim Cromie wrote:

> Linus Torvalds wrote:
> >
> >   
> > Yes, code should be less than 80 characters wide. 
> >
> > But hey, sometimes it's just more readable to have one line that is 
> > slightly longer than it should be, than to split something that is awkward 
> > to split. 
> >
> >   
> < cc-list heavily trimmed >
> 
> 
> could you speak to the specific case of function signatures ?
> I saw nothing in CodingStyle specifically about this.
> (I skimmed, and grepped for signature)
> 
> forex:
> 
> static ssize_t
> store_fan_div (struct device *dev, struct device_attribute *devattr,
>                     const char *buf, size_t count)
> {...}
> 
> 
> IIRC, many like the entire sig on one line, because its grep friendly.
> I personally like the above, but grep-ability is hard to argue against.

(I expect that "you" above means Linus, but anyway...)

Some people like the above so that the function name can be grepped
using a "^store_fan_div" regex, but that's not good enough reason
for it for others, including Linus, IIRC.  We have other tools
to find where functions are.

> The above has 2 violations (of strict-grep-ability rule)
> 1 - return sig is separate
> 2 - arg-list is split

#2 can't be helped unless someone is willing to go way beyond the
80-or-so column limit.

I would write that function sig like so:

static ssize_t store_fan_div(struct device *dev,
			struct device_attribute *devattr,
			const char *buf, size_t count)

Some people would add spaces on lines 2 & 3 so that all of the
function parameters are aligned.  I don't care very much one way
or the other about that.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: Coding style on function signatures (was: Convert tasklets to work queues )

[Oleg Verych]

>> static ssize_t
>> store_fan_div (struct device *dev, struct device_attribute *devattr,
>>                     const char *buf, size_t count)
>> {...}
>> 
>> 
>> IIRC, many like the entire sig on one line, because its grep friendly.
>> I personally like the above, but grep-ability is hard to argue against.
>
> (I expect that "you" above means Linus, but anyway...)
>
> Some people like the above so that the function name can be grepped
> using a "^store_fan_div" regex, but that's not good enough reason
> for it for others, including Linus, IIRC.

Right. I was reading some LKML archives, and what i know, that returning
type of the function is the first thing Linus cared about.

Newbies homework to think why (and care more about such things and not
of the look).

--
-o--=O`C
 #oo'L O
<___=E M

Re: Coding style on function signatures (was: Convert tasklets to work queues )

[Jan Engelhardt]

On Jul 7 2007 18:49, Jim Cromie wrote:
> forex:
>
> static ssize_t
> store_fan_div (struct device *dev, struct device_attribute *devattr,
>                   const char *buf, size_t count)
> {...}
>
> IIRC, many like the entire sig on one line, because its grep friendly.
> I personally like the above, but grep-ability is hard to argue against.
>
> The above has 2 violations (of strict-grep-ability rule)
> 1 - return sig is separate

I can only think of one case where it's ok, when the first line is quite long
already:

static __attribute__((pure)) inline const uint32_t
hashfunction(parameter1, parameter2, ...)
{
}


	Jan
--