[PATCH 0/3] softirq: possible speedup and neatenings

[PATCH 0/3] softirq: possible speedup and neatenings

[Joe Perches]

The first one boots, but it's barely tested.

Joe Perches (3):
  softirq: Use ffs in __do_softirq
  softirq: Convert printks to pr_<level>
  softirq: Use const char * const for softirq_to_name, whitespace neatening

 include/linux/interrupt.h |  2 +-
 kernel/softirq.c          | 75 +++++++++++++++++++++--------------------------
 2 files changed, 35 insertions(+), 42 deletions(-)

-- 
1.8.1.2.459.gbcd45b4.dirty

[PATCH 1/3] softirq: Use ffs in __do_softirq

[Joe Perches]

Possible speed improvement of the __do_softirq function by using ffs
instead of using a while loop with an & 1 test then single bit shift.

Signed-off-by: Joe Perches <joe@perches.com>
---
 kernel/softirq.c | 43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 11025cc..7be95d7 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -219,6 +219,7 @@ asmlinkage void __do_softirq(void)
 	int cpu;
 	unsigned long old_flags = current->flags;
 	int max_restart = MAX_SOFTIRQ_RESTART;
+	int softirq_bit;
 
 	/*
 	 * Mask out PF_MEMALLOC s current task context is borrowed for the
@@ -242,30 +243,30 @@ restart:
 
 	h = softirq_vec;
 
-	do {
-		if (pending & 1) {
-			unsigned int vec_nr = h - softirq_vec;
-			int prev_count = preempt_count();
-
-			kstat_incr_softirqs_this_cpu(vec_nr);
-
-			trace_softirq_entry(vec_nr);
-			h->action(h);
-			trace_softirq_exit(vec_nr);
-			if (unlikely(prev_count != preempt_count())) {
-				printk(KERN_ERR "huh, entered softirq %u %s %p"
-				       "with preempt_count %08x,"
-				       " exited with %08x?\n", vec_nr,
-				       softirq_to_name[vec_nr], h->action,
-				       prev_count, preempt_count());
-				preempt_count_set(prev_count);
-			}
+	while ((softirq_bit = ffs(pending))) {
+		unsigned int vec_nr;
+		int prev_count;
+
+		h += softirq_bit - 1;
+
+		vec_nr = h - softirq_vec;
+		prev_count = preempt_count();
 
-			rcu_bh_qs(cpu);
+		kstat_incr_softirqs_this_cpu(vec_nr);
+
+		trace_softirq_entry(vec_nr);
+		h->action(h);
+		trace_softirq_exit(vec_nr);
+		if (unlikely(prev_count != preempt_count())) {
+			printk(KERN_ERR "huh, entered softirq %u %s %p with preempt_count %08x, exited with %08x?\n",
+			       vec_nr, softirq_to_name[vec_nr], h->action,
+			       prev_count, preempt_count());
+			preempt_count_set(prev_count);
 		}
+		rcu_bh_qs(cpu);
 		h++;
-		pending >>= 1;
-	} while (pending);
+		pending >>= softirq_bit;
+	}
 
 	local_irq_disable();
 
-- 
1.8.1.2.459.gbcd45b4.dirty

[PATCH 2/3] softirq: Convert printks to pr_<level>

[Joe Perches]

Use a more current logging style.

Signed-off-by: Joe Perches <joe@perches.com>
---
 kernel/softirq.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 7be95d7..49a44cb 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -8,6 +8,8 @@
  *	Rewritten. Old one was good in 2.2, but in 2.3 it was immoral. --ANK (990903)
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/export.h>
 #include <linux/kernel_stat.h>
 #include <linux/interrupt.h>
@@ -258,7 +260,7 @@ restart:
 		h->action(h);
 		trace_softirq_exit(vec_nr);
 		if (unlikely(prev_count != preempt_count())) {
-			printk(KERN_ERR "huh, entered softirq %u %s %p with preempt_count %08x, exited with %08x?\n",
+			pr_err("huh, entered softirq %u %s %p with preempt_count %08x, exited with %08x?\n",
 			       vec_nr, softirq_to_name[vec_nr], h->action,
 			       prev_count, preempt_count());
 			preempt_count_set(prev_count);
@@ -562,7 +564,7 @@ EXPORT_SYMBOL(tasklet_init);
 void tasklet_kill(struct tasklet_struct *t)
 {
 	if (in_interrupt())
-		printk("Attempt to kill tasklet from interrupt\n");
+		pr_notice("Attempt to kill tasklet from interrupt\n");
 
 	while (test_and_set_bit(TASKLET_STATE_SCHED, &t->state)) {
 		do {
-- 
1.8.1.2.459.gbcd45b4.dirty

[PATCH 3/3] softirq: Use const char * const for softirq_to_name, whitespace neatening

[Joe Perches]

Reduce data size a little.
Reduce checkpatch noise.

$ size kernel/softirq.o*
   text	   data	    bss	    dec	    hex	filename
  11554	   6013	   4008	  21575	   5447	kernel/softirq.o.new
  11474	   6093	   4008	  21575	   5447	kernel/softirq.o.old

Signed-off-by: Joe Perches <joe@perches.com>
---
 include/linux/interrupt.h |  2 +-
 kernel/softirq.c          | 28 +++++++++-------------------
 2 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index db43b58..0053add 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -360,7 +360,7 @@ enum
 /* map softirq index to softirq name. update 'softirq_to_name' in
  * kernel/softirq.c when adding a new softirq.
  */
-extern char *softirq_to_name[NR_SOFTIRQS];
+extern const char * const softirq_to_name[NR_SOFTIRQS];
 
 /* softirq mask and active fields moved to irq_cpustat_t in
  * asm/hardirq.h to get better cache usage.  KAO
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 49a44cb..c828df5 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -56,7 +56,7 @@ static struct softirq_action softirq_vec[NR_SOFTIRQS] __cacheline_aligned_in_smp
 
 DEFINE_PER_CPU(struct task_struct *, ksoftirqd);
 
-char *softirq_to_name[NR_SOFTIRQS] = {
+const char * const softirq_to_name[NR_SOFTIRQS] = {
 	"HI", "TIMER", "NET_TX", "NET_RX", "BLOCK", "BLOCK_IOPOLL",
 	"TASKLET", "SCHED", "HRTIMER", "RCU"
 };
@@ -128,7 +128,6 @@ void local_bh_disable(void)
 {
 	__local_bh_disable(_RET_IP_, SOFTIRQ_DISABLE_OFFSET);
 }
-
 EXPORT_SYMBOL(local_bh_disable);
 
 static void __local_bh_enable(unsigned int cnt)
@@ -150,7 +149,6 @@ void _local_bh_enable(void)
 	WARN_ON_ONCE(in_irq());
 	__local_bh_enable(SOFTIRQ_DISABLE_OFFSET);
 }
-
 EXPORT_SYMBOL(_local_bh_enable);
 
 static inline void _local_bh_enable_ip(unsigned long ip)
@@ -167,7 +165,7 @@ static inline void _local_bh_enable_ip(unsigned long ip)
 	/*
 	 * Keep preemption disabled until we are done with
 	 * softirq processing:
- 	 */
+	 */
 	preempt_count_sub(SOFTIRQ_DISABLE_OFFSET - 1);
 
 	if (unlikely(!in_interrupt() && local_softirq_pending())) {
@@ -289,8 +287,6 @@ restart:
 	tsk_restore_flags(current, old_flags, PF_MEMALLOC);
 }
 
-
-
 asmlinkage void do_softirq(void)
 {
 	__u32 pending;
@@ -430,8 +426,7 @@ void open_softirq(int nr, void (*action)(struct softirq_action *))
 /*
  * Tasklets
  */
-struct tasklet_head
-{
+struct tasklet_head {
 	struct tasklet_struct *head;
 	struct tasklet_struct **tail;
 };
@@ -450,7 +445,6 @@ void __tasklet_schedule(struct tasklet_struct *t)
 	raise_softirq_irqoff(TASKLET_SOFTIRQ);
 	local_irq_restore(flags);
 }
-
 EXPORT_SYMBOL(__tasklet_schedule);
 
 void __tasklet_hi_schedule(struct tasklet_struct *t)
@@ -464,7 +458,6 @@ void __tasklet_hi_schedule(struct tasklet_struct *t)
 	raise_softirq_irqoff(HI_SOFTIRQ);
 	local_irq_restore(flags);
 }
-
 EXPORT_SYMBOL(__tasklet_hi_schedule);
 
 void __tasklet_hi_schedule_first(struct tasklet_struct *t)
@@ -475,7 +468,6 @@ void __tasklet_hi_schedule_first(struct tasklet_struct *t)
 	__this_cpu_write(tasklet_hi_vec.head, t);
 	__raise_softirq_irqoff(HI_SOFTIRQ);
 }
-
 EXPORT_SYMBOL(__tasklet_hi_schedule_first);
 
 static void tasklet_action(struct softirq_action *a)
@@ -495,7 +487,8 @@ static void tasklet_action(struct softirq_action *a)
 
 		if (tasklet_trylock(t)) {
 			if (!atomic_read(&t->count)) {
-				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
+				if (!test_and_clear_bit(TASKLET_STATE_SCHED,
+							&t->state))
 					BUG();
 				t->func(t->data);
 				tasklet_unlock(t);
@@ -530,7 +523,8 @@ static void tasklet_hi_action(struct softirq_action *a)
 
 		if (tasklet_trylock(t)) {
 			if (!atomic_read(&t->count)) {
-				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
+				if (!test_and_clear_bit(TASKLET_STATE_SCHED,
+							&t->state))
 					BUG();
 				t->func(t->data);
 				tasklet_unlock(t);
@@ -548,7 +542,6 @@ static void tasklet_hi_action(struct softirq_action *a)
 	}
 }
 
-
 void tasklet_init(struct tasklet_struct *t,
 		  void (*func)(unsigned long), unsigned long data)
 {
@@ -558,7 +551,6 @@ void tasklet_init(struct tasklet_struct *t,
 	t->func = func;
 	t->data = data;
 }
-
 EXPORT_SYMBOL(tasklet_init);
 
 void tasklet_kill(struct tasklet_struct *t)
@@ -574,7 +566,6 @@ void tasklet_kill(struct tasklet_struct *t)
 	tasklet_unlock_wait(t);
 	clear_bit(TASKLET_STATE_SCHED, &t->state);
 }
-
 EXPORT_SYMBOL(tasklet_kill);
 
 /*
@@ -724,9 +715,8 @@ static void takeover_tasklets(unsigned int cpu)
 }
 #endif /* CONFIG_HOTPLUG_CPU */
 
-static int cpu_callback(struct notifier_block *nfb,
-				  unsigned long action,
-				  void *hcpu)
+static int cpu_callback(struct notifier_block *nfb, unsigned long action,
+			void *hcpu)
 {
 	switch (action) {
 #ifdef CONFIG_HOTPLUG_CPU
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [PATCH 0/3] softirq: possible speedup and neatenings

[Joe Perches]

On Sun, 2013-11-17 at 01:55 -0800, Joe Perches wrote:
> The first one boots, but it's barely tested.
> 
> Joe Perches (3):
>   softirq: Use ffs in __do_softirq
>   softirq: Convert printks to pr_<level>
>   softirq: Use const char * const for softirq_to_name, whitespace neatening
> 
>  include/linux/interrupt.h |  2 +-
>  kernel/softirq.c          | 75 +++++++++++++++++++++--------------------------
>  2 files changed, 35 insertions(+), 42 deletions(-)

ping?

Re: [PATCH 1/3] softirq: Use ffs in __do_softirq

[Frederic Weisbecker]

On Sun, Nov 17, 2013 at 01:55:10AM -0800, Joe Perches wrote:
> Possible speed improvement of the __do_softirq function by using ffs
> instead of using a while loop with an & 1 test then single bit shift.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  kernel/softirq.c | 43 ++++++++++++++++++++++---------------------
>  1 file changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 11025cc..7be95d7 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -219,6 +219,7 @@ asmlinkage void __do_softirq(void)
>  	int cpu;
>  	unsigned long old_flags = current->flags;
>  	int max_restart = MAX_SOFTIRQ_RESTART;
> +	int softirq_bit;
>  
>  	/*
>  	 * Mask out PF_MEMALLOC s current task context is borrowed for the
> @@ -242,30 +243,30 @@ restart:
>  
>  	h = softirq_vec;
>  
> -	do {
> -		if (pending & 1) {
> -			unsigned int vec_nr = h - softirq_vec;
> -			int prev_count = preempt_count();
> -
> -			kstat_incr_softirqs_this_cpu(vec_nr);
> -
> -			trace_softirq_entry(vec_nr);
> -			h->action(h);
> -			trace_softirq_exit(vec_nr);
> -			if (unlikely(prev_count != preempt_count())) {
> -				printk(KERN_ERR "huh, entered softirq %u %s %p"
> -				       "with preempt_count %08x,"
> -				       " exited with %08x?\n", vec_nr,
> -				       softirq_to_name[vec_nr], h->action,
> -				       prev_count, preempt_count());
> -				preempt_count_set(prev_count);
> -			}
> +	while ((softirq_bit = ffs(pending))) {
> +		unsigned int vec_nr;
> +		int prev_count;
> +
> +		h += softirq_bit - 1;

Perhaps using for_each_set_bit() would simplify that more?

> +
> +		vec_nr = h - softirq_vec;
> +		prev_count = preempt_count();
>  
> -			rcu_bh_qs(cpu);
> +		kstat_incr_softirqs_this_cpu(vec_nr);
> +
> +		trace_softirq_entry(vec_nr);
> +		h->action(h);
> +		trace_softirq_exit(vec_nr);
> +		if (unlikely(prev_count != preempt_count())) {
> +			printk(KERN_ERR "huh, entered softirq %u %s %p with preempt_count %08x, exited with %08x?\n",
> +			       vec_nr, softirq_to_name[vec_nr], h->action,
> +			       prev_count, preempt_count());
> +			preempt_count_set(prev_count);
>  		}
> +		rcu_bh_qs(cpu);
>  		h++;
> -		pending >>= 1;
> -	} while (pending);
> +		pending >>= softirq_bit;
> +	}
>  
>  	local_irq_disable();
>  
> -- 
> 1.8.1.2.459.gbcd45b4.dirty
> 

Re: [PATCH 1/3] softirq: Use ffs in __do_softirq

[Joe Perches]

On Mon, 2013-12-09 at 19:44 +0100, Frederic Weisbecker wrote:
> On Sun, Nov 17, 2013 at 01:55:10AM -0800, Joe Perches wrote:
> > Possible speed improvement of the __do_softirq function by using ffs
> > instead of using a while loop with an & 1 test then single bit shift.
[]
> Perhaps using for_each_set_bit() would simplify that more?

It might simplify the appearance of the code but it
would/could expand the amount of generated code because
for_each_set_bit uses an address_of(unsigned long) and
the value tested is an unsigned int.

extra dereferences, can't be in a register, etc...

Re: [PATCH 1/3] softirq: Use ffs in __do_softirq

[Frederic Weisbecker]

On Mon, Dec 09, 2013 at 10:56:46AM -0800, Joe Perches wrote:
> On Mon, 2013-12-09 at 19:44 +0100, Frederic Weisbecker wrote:
> > On Sun, Nov 17, 2013 at 01:55:10AM -0800, Joe Perches wrote:
> > > Possible speed improvement of the __do_softirq function by using ffs
> > > instead of using a while loop with an & 1 test then single bit shift.
> []
> > Perhaps using for_each_set_bit() would simplify that more?
> 
> It might simplify the appearance of the code but it
> would/could expand the amount of generated code because
> for_each_set_bit uses an address_of(unsigned long) and
> the value tested is an unsigned int.
> 
> extra dereferences, can't be in a register, etc...

I'm not sure that would matter that much. But yeah it appears that find_first_bit/find_next_bit
aren't even overriden in x86. So they are function calls. Although I guess that most
of the time only one softirq is pending at a time.

But anyway perhaps we want that path to stay very optimized, so you're
patch look ok.

Thanks.

Re: [PATCH 3/3] softirq: Use const char * const for softirq_to_name, whitespace neatening

[Wang YanQing]

On Sun, Nov 17, 2013 at 01:55:12AM -0800, Joe Perches wrote:
> Reduce data size a little.
> Reduce checkpatch noise.
> 
> $ size kernel/softirq.o*
>    text	   data	    bss	    dec	    hex	filename
>   11554	   6013	   4008	  21575	   5447	kernel/softirq.o.new
>   11474	   6093	   4008	  21575	   5447	kernel/softirq.o.old

Hi, could you tell me why this patch could reduce data size?

Thanks.

Re: [PATCH 3/3] softirq: Use const char * const for softirq_to_name, whitespace neatening

[Joe Perches]

On Tue, 2013-12-24 at 23:19 +0800, Wang YanQing wrote:
> On Sun, Nov 17, 2013 at 01:55:12AM -0800, Joe Perches wrote:
> > Reduce data size a little.
> > Reduce checkpatch noise.
> > 
> > $ size kernel/softirq.o*
> >    text	   data	    bss	    dec	    hex	filename
> >   11554	   6013	   4008	  21575	   5447	kernel/softirq.o.new
> >   11474	   6093	   4008	  21575	   5447	kernel/softirq.o.old
> 
> Hi, could you tell me why this patch could reduce data size?

It moves the softirq_to_name array of 10 char *
from data (non-const) to text (const).

-char *softirq_to_name[NR_SOFTIRQS] = {
+const char * const softirq_to_name[NR_SOFTIRQS] = {
        "HI", "TIMER", "NET_TX", "NET_RX", "BLOCK", "BLOCK_IOPOLL",
        "TASKLET", "SCHED", "HRTIMER", "RCU"
 };

Use objdump or "make kernel/softirq.lst" and inspect
the object code produced to see for yourself.

cheers, Joe