Re: [PATCH] Remove argument from open_softirq which is always NULL

Previous thread: Suspend/Resume bug in PCI layer wrt quirks by Arjan van de Ven on Thursday, May 15, 2008 - 7:05 am. (9 messages)

Next thread: [PATCHv2] [POWERPC] Add i2c pins to dts and board setup by Jochen Friedrich on Thursday, May 15, 2008 - 7:44 am. (1 message)
From: Carlos R. Mafra
Date: Thursday, May 15, 2008 - 7:15 am

From 9371c45e92c308425c0aad4794f61acb6fe6d140 Mon Sep 17 00:00:00 2001
From: Carlos R. Mafra <crmafra@ift.unesp.br>
Date: Wed, 14 May 2008 18:26:06 -0300
Subject: [PATCH] Remove argument from open_softirq which is always NULL

As git-grep shows, open_softirq() is always called with the last argument
being NULL

block/blk-core.c:       open_softirq(BLOCK_SOFTIRQ, blk_done_softirq, NULL);
kernel/hrtimer.c:       open_softirq(HRTIMER_SOFTIRQ, run_hrtimer_softirq, NULL);
kernel/rcuclassic.c:    open_softirq(RCU_SOFTIRQ, rcu_process_callbacks, NULL);
kernel/rcupreempt.c:    open_softirq(RCU_SOFTIRQ, rcu_process_callbacks, NULL);
kernel/sched.c: open_softirq(SCHED_SOFTIRQ, run_rebalance_domains, NULL);
kernel/softirq.c:       open_softirq(TASKLET_SOFTIRQ, tasklet_action, NULL);
kernel/softirq.c:       open_softirq(HI_SOFTIRQ, tasklet_hi_action, NULL);
kernel/timer.c: open_softirq(TIMER_SOFTIRQ, run_timer_softirq, NULL);
net/core/dev.c: open_softirq(NET_TX_SOFTIRQ, net_tx_action, NULL);
net/core/dev.c: open_softirq(NET_RX_SOFTIRQ, net_rx_action, NULL);

This observation has already been made by Matthew Wilcox in June 2002
(http://www.cs.helsinki.fi/linux/linux-kernel/2002-25/0687.html)

"I notice that none of the current softirq routines use the data element
passed to them."

and the situation hasn't changed since them. So it appears we can safely
remove that extra argument to save 128 (54) bytes of kernel data (text).

Signed-off-by: Carlos R. Mafra <crmafra@ift.unesp.br>
---
 block/blk-core.c          |    2 +-
 include/linux/interrupt.h |    3 +--
 kernel/hrtimer.c          |    2 +-
 kernel/rcuclassic.c       |    2 +-
 kernel/rcupreempt.c       |    2 +-
 kernel/sched.c            |    2 +-
 kernel/softirq.c          |    7 +++----
 kernel/timer.c            |    2 +-
 net/core/dev.c            |    4 ++--
 9 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2987fe4..667efc4 100644
--- a/block/blk-core.c
+++ ...
From: Matthew Wilcox
Date: Thursday, May 15, 2008 - 8:22 am

Reviewed-by: Matthew Wilcox <willy@linux.intel.com>

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
--

From: Ingo Molnar
Date: Friday, May 16, 2008 - 4:57 am

applied to -tip, thanks Carlos. Nice find!

	Ingo
--

From: Paul E. McKenney
Date: Friday, May 16, 2008 - 11:28 am

Good stuff!!!

Will there also be a similar set of patches that removes the extra
argument from the invoked function?  For example:

	static void rcu_process_callbacks(struct softirq_action *unused)

could now become:

	static void rcu_process_callbacks(void)

--

From: Carlos R. Mafra
Date: Friday, May 16, 2008 - 12:52 pm

That was my intention at first when I came acroos 
run_hrtimer_softirq(struct softirq_action *h) in hrtimer.c, which then led me to
read more code and notice that I could do the cleanup in this patch.

You've just gave me the motivation I needed to keep cleaning (and learning) further, 
as the original bug which motivated me to look at those things does not
happen anymore in my laptop :-)

I will try to do another patch soon, but I will travel tomorrow so it

Thanks,
Carlos

--

From: Carlos R. Mafra
Date: Friday, May 16, 2008 - 9:24 pm

Remove unused argument from functions invoked by open_softirq

Simplify struct softirq_action to avoid the need of an explicit unused
argument in the functions invoked by open_softirq(). For example,

static void rcu_process_callbacks(struct softirq_action *unused)

can now become

static void rcu_process_callbacks(void)

Signed-off-by: Carlos R. Mafra <crmafra@ift.unesp.br>
---
 block/blk-core.c          |    2 +-
 include/linux/interrupt.h |    4 ++--
 kernel/hrtimer.c          |    2 +-
 kernel/rcuclassic.c       |    2 +-
 kernel/rcupreempt.c       |    2 +-
 kernel/sched.c            |    2 +-
 kernel/softirq.c          |    8 ++++----
 kernel/timer.c            |    2 +-
 net/core/dev.c            |    4 ++--
 9 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 75fdc65..20847b0 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1630,7 +1630,7 @@ static int __end_that_request_first(struct request *req, int error,
  * splice the completion data to a local structure and hand off to
  * process_completion_queue() to complete the requests
  */
-static void blk_done_softirq(struct softirq_action *h)
+static void blk_done_softirq(void)
 {
 	struct list_head *cpu_list, local_list;
 
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index a86186d..0fed9e9 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -284,12 +284,12 @@ enum
 
 struct softirq_action
 {
-	void	(*action)(struct softirq_action *);
+	void	(*action)(void);
 };
 
 asmlinkage void do_softirq(void);
 asmlinkage void __do_softirq(void);
-extern void open_softirq(int nr, void (*action)(struct softirq_action *));
+extern void open_softirq(int nr, void (*action)(void));
 extern void softirq_init(void);
 #define __raise_softirq_irqoff(nr) do { or_softirq_pending(1UL << (nr)); } while (0)
 extern void raise_softirq_irqoff(unsigned int nr);
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index ...
From: Paul E. McKenney
Date: Friday, May 16, 2008 - 11:03 pm

Looks good to me!

--

Previous thread: Suspend/Resume bug in PCI layer wrt quirks by Arjan van de Ven on Thursday, May 15, 2008 - 7:05 am. (9 messages)

Next thread: [PATCHv2] [POWERPC] Add i2c pins to dts and board setup by Jochen Friedrich on Thursday, May 15, 2008 - 7:44 am. (1 message)