Message-ID: <6648a8f0-0af1-4351-95b9-8380df8f336d@paulmck-laptop>
Date: Tue, 20 Aug 2024 11:49:29 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Caleb Sander Mateos <csander@...estorage.com>
Cc: Jens Axboe <axboe@...nel.dk>, Frederic Weisbecker <frederic@...nel.org>,
	Neeraj Upadhyay <neeraj.upadhyay@...nel.org>,
	Joel Fernandes <joel@...lfernandes.org>,
	Josh Triplett <josh@...htriplett.org>,
	Boqun Feng <boqun.feng@...il.com>,
	Uladzislau Rezki <urezki@...il.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Zqiang <qiang.zhang1211@...il.com>, Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Juri Lelli <juri.lelli@...hat.com>,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Dietmar Eggemann <dietmar.eggemann@....com>,
	Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
	Valentin Schneider <vschneid@...hat.com>,
	Anna-Maria Behnsen <anna-maria@...utronix.de>,
	Thomas Gleixner <tglx@...utronix.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
	rcu@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v2] softirq: remove parameter from action callback

On Thu, Aug 15, 2024 at 11:15:40AM -0600, Caleb Sander Mateos wrote:
> When softirq actions are called, they are passed a pointer to the entry
> in the softirq_vec table containing the action's function pointer. This
> pointer isn't very useful, as the action callback already knows what
> function it is. And since each callback handles a specific softirq, the
> callback also knows which softirq number is running.
> 
> No softirq action callbacks actually use this parameter, so remove it
> from the function pointer signature. This clarifies that softirq actions
> are global routines and makes it slightly cheaper to call them.
> 
> v2: use full 72 characters in commit description lines, add Reviewed-by
> 
> Signed-off-by: Caleb Sander Mateos <csander@...estorage.com>
> Reviewed-by: Jens Axboe <axboe@...nel.dk>

For the RCU pieces:

Reviewed-by: Paul E. McKenney <paulmck@...nel.org>

> ---
>  block/blk-mq.c            |  2 +-
>  include/linux/interrupt.h |  4 ++--
>  kernel/rcu/tiny.c         |  2 +-
>  kernel/rcu/tree.c         |  2 +-
>  kernel/sched/fair.c       |  2 +-
>  kernel/softirq.c          | 15 +++++++--------
>  kernel/time/hrtimer.c     |  2 +-
>  kernel/time/timer.c       |  2 +-
>  lib/irq_poll.c            |  2 +-
>  net/core/dev.c            |  4 ++--
>  10 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index e3c3c0c21b55..aa28157b1aaf 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1126,11 +1126,11 @@ static void blk_complete_reqs(struct llist_head *list)
>  
>  	llist_for_each_entry_safe(rq, next, entry, ipi_list)
>  		rq->q->mq_ops->complete(rq);
>  }
>  
> -static __latent_entropy void blk_done_softirq(struct softirq_action *h)
> +static __latent_entropy void blk_done_softirq(void)
>  {
>  	blk_complete_reqs(this_cpu_ptr(&blk_cpu_done));
>  }
>  
>  static int blk_softirq_cpu_dead(unsigned int cpu)
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 3f30c88e0b4c..694de61e0b38 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -592,11 +592,11 @@ extern const char * const softirq_to_name[NR_SOFTIRQS];
>   * asm/hardirq.h to get better cache usage.  KAO
>   */
>  
>  struct softirq_action
>  {
> -	void	(*action)(struct softirq_action *);
> +	void	(*action)(void);
>  };
>  
>  asmlinkage void do_softirq(void);
>  asmlinkage void __do_softirq(void);
>  
> @@ -607,11 +607,11 @@ static inline void do_softirq_post_smp_call_flush(unsigned int unused)
>  {
>  	do_softirq();
>  }
>  #endif
>  
> -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);
>  extern void __raise_softirq_irqoff(unsigned int nr);
>  
>  extern void raise_softirq_irqoff(unsigned int nr);
>  extern void raise_softirq(unsigned int nr);
> diff --git a/kernel/rcu/tiny.c b/kernel/rcu/tiny.c
> index 4402d6f5f857..b3b3ce34df63 100644
> --- a/kernel/rcu/tiny.c
> +++ b/kernel/rcu/tiny.c
> @@ -103,11 +103,11 @@ static inline bool rcu_reclaim_tiny(struct rcu_head *head)
>  	rcu_lock_release(&rcu_callback_map);
>  	return false;
>  }
>  
>  /* Invoke the RCU callbacks whose grace period has elapsed.  */
> -static __latent_entropy void rcu_process_callbacks(struct softirq_action *unused)
> +static __latent_entropy void rcu_process_callbacks(void)
>  {
>  	struct rcu_head *next, *list;
>  	unsigned long flags;
>  
>  	/* Move the ready-to-invoke callbacks to a local list. */
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index e641cc681901..93bd665637c0 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2853,11 +2853,11 @@ static __latent_entropy void rcu_core(void)
>  	// If strict GPs, schedule an RCU reader in a clean environment.
>  	if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD))
>  		queue_work_on(rdp->cpu, rcu_gp_wq, &rdp->strict_work);
>  }
>  
> -static void rcu_core_si(struct softirq_action *h)
> +static void rcu_core_si(void)
>  {
>  	rcu_core();
>  }
>  
>  static void rcu_wake_cond(struct task_struct *t, int status)
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 9057584ec06d..8dc9385f6da4 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -12481,11 +12481,11 @@ static int sched_balance_newidle(struct rq *this_rq, struct rq_flags *rf)
>   * - directly from the local scheduler_tick() for periodic load balancing
>   *
>   * - indirectly from a remote scheduler_tick() for NOHZ idle balancing
>   *   through the SMP cross-call nohz_csd_func()
>   */
> -static __latent_entropy void sched_balance_softirq(struct softirq_action *h)
> +static __latent_entropy void sched_balance_softirq(void)
>  {
>  	struct rq *this_rq = this_rq();
>  	enum cpu_idle_type idle = this_rq->idle_balance;
>  	/*
>  	 * If this CPU has a pending NOHZ_BALANCE_KICK, then do the
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 02582017759a..d082e7840f88 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -549,11 +549,11 @@ static void handle_softirqs(bool ksirqd)
>  		prev_count = preempt_count();
>  
>  		kstat_incr_softirqs_this_cpu(vec_nr);
>  
>  		trace_softirq_entry(vec_nr);
> -		h->action(h);
> +		h->action();
>  		trace_softirq_exit(vec_nr);
>  		if (unlikely(prev_count != preempt_count())) {
>  			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());
> @@ -698,11 +698,11 @@ void __raise_softirq_irqoff(unsigned int nr)
>  	lockdep_assert_irqs_disabled();
>  	trace_softirq_raise(nr);
>  	or_softirq_pending(1UL << nr);
>  }
>  
> -void open_softirq(int nr, void (*action)(struct softirq_action *))
> +void open_softirq(int nr, void (*action)(void))
>  {
>  	softirq_vec[nr].action = action;
>  }
>  
>  /*
> @@ -758,12 +758,11 @@ static bool tasklet_clear_sched(struct tasklet_struct *t)
>  		  t->use_callback ? (void *)t->callback : (void *)t->func);
>  
>  	return false;
>  }
>  
> -static void tasklet_action_common(struct softirq_action *a,
> -				  struct tasklet_head *tl_head,
> +static void tasklet_action_common(struct tasklet_head *tl_head,
>  				  unsigned int softirq_nr)
>  {
>  	struct tasklet_struct *list;
>  
>  	local_irq_disable();
> @@ -803,20 +802,20 @@ static void tasklet_action_common(struct softirq_action *a,
>  		__raise_softirq_irqoff(softirq_nr);
>  		local_irq_enable();
>  	}
>  }
>  
> -static __latent_entropy void tasklet_action(struct softirq_action *a)
> +static __latent_entropy void tasklet_action(void)
>  {
>  	workqueue_softirq_action(false);
> -	tasklet_action_common(a, this_cpu_ptr(&tasklet_vec), TASKLET_SOFTIRQ);
> +	tasklet_action_common(this_cpu_ptr(&tasklet_vec), TASKLET_SOFTIRQ);
>  }
>  
> -static __latent_entropy void tasklet_hi_action(struct softirq_action *a)
> +static __latent_entropy void tasklet_hi_action(void)
>  {
>  	workqueue_softirq_action(true);
> -	tasklet_action_common(a, this_cpu_ptr(&tasklet_hi_vec), HI_SOFTIRQ);
> +	tasklet_action_common(this_cpu_ptr(&tasklet_hi_vec), HI_SOFTIRQ);
>  }
>  
>  void tasklet_setup(struct tasklet_struct *t,
>  		   void (*callback)(struct tasklet_struct *))
>  {
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index b8ee320208d4..836157e09e25 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -1755,11 +1755,11 @@ static void __hrtimer_run_queues(struct hrtimer_cpu_base *cpu_base, ktime_t now,
>  				hrtimer_sync_wait_running(cpu_base, flags);
>  		}
>  	}
>  }
>  
> -static __latent_entropy void hrtimer_run_softirq(struct softirq_action *h)
> +static __latent_entropy void hrtimer_run_softirq(void)
>  {
>  	struct hrtimer_cpu_base *cpu_base = this_cpu_ptr(&hrtimer_bases);
>  	unsigned long flags;
>  	ktime_t now;
>  
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 64b0d8a0aa0f..760bbeb1f331 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -2438,11 +2438,11 @@ static void run_timer_base(int index)
>  }
>  
>  /*
>   * This function runs timers and the timer-tq in bottom half context.
>   */
> -static __latent_entropy void run_timer_softirq(struct softirq_action *h)
> +static __latent_entropy void run_timer_softirq(void)
>  {
>  	run_timer_base(BASE_LOCAL);
>  	if (IS_ENABLED(CONFIG_NO_HZ_COMMON)) {
>  		run_timer_base(BASE_GLOBAL);
>  		run_timer_base(BASE_DEF);
> diff --git a/lib/irq_poll.c b/lib/irq_poll.c
> index 2d5329a42105..08b242bbdbdf 100644
> --- a/lib/irq_poll.c
> +++ b/lib/irq_poll.c
> @@ -73,11 +73,11 @@ void irq_poll_complete(struct irq_poll *iop)
>  	__irq_poll_complete(iop);
>  	local_irq_restore(flags);
>  }
>  EXPORT_SYMBOL(irq_poll_complete);
>  
> -static void __latent_entropy irq_poll_softirq(struct softirq_action *h)
> +static void __latent_entropy irq_poll_softirq(void)
>  {
>  	struct list_head *list = this_cpu_ptr(&blk_cpu_iopoll);
>  	int rearm = 0, budget = irq_poll_budget;
>  	unsigned long start_time = jiffies;
>  
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 751d9b70e6ad..3ac02b0ca29e 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5246,11 +5246,11 @@ int netif_rx(struct sk_buff *skb)
>  		local_bh_enable();
>  	return ret;
>  }
>  EXPORT_SYMBOL(netif_rx);
>  
> -static __latent_entropy void net_tx_action(struct softirq_action *h)
> +static __latent_entropy void net_tx_action(void)
>  {
>  	struct softnet_data *sd = this_cpu_ptr(&softnet_data);
>  
>  	if (sd->completion_queue) {
>  		struct sk_buff *clist;
> @@ -6919,11 +6919,11 @@ static int napi_threaded_poll(void *data)
>  		napi_threaded_poll_loop(napi);
>  
>  	return 0;
>  }
>  
> -static __latent_entropy void net_rx_action(struct softirq_action *h)
> +static __latent_entropy void net_rx_action(void)
>  {
>  	struct softnet_data *sd = this_cpu_ptr(&softnet_data);
>  	unsigned long time_limit = jiffies +
>  		usecs_to_jiffies(READ_ONCE(net_hotdata.netdev_budget_usecs));
>  	struct bpf_net_context __bpf_net_ctx, *bpf_net_ctx;
> -- 
> 2.45.2
> 

Powered by blists - more mailing lists