[PATCH 0/2] net: Fix/prevent napi_schedule() call from bare task context

[PATCH 0/2] net: Fix/prevent napi_schedule() call from bare task context

[Frederic Weisbecker]

Hi,

Here is a fix to a bad context calling napi_schedule() and a lockdep
assertion to prevent from that in the future. I've tried to produce
a relevant Fixes tag but I'm not confident enough with this codebase.
This call is there for many years and yet the issue got reported only
recently, so I may be missing something in the history of this driver
or in net/usb infrastructure...

Thanks.

Frederic Weisbecker (2):
  net: Assert proper context while calling napi_schedule()
  r8152: Call napi_schedule() from proper context

 drivers/net/usb/r8152.c |  5 ++++-
 include/linux/lockdep.h | 12 ++++++++++++
 net/core/dev.c          |  1 +
 3 files changed, 17 insertions(+), 1 deletion(-)

-- 
2.46.0

[PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Frederic Weisbecker]

napi_schedule() is expected to be called either:

* From an interrupt, where raised softirqs are handled on IRQ exit

* From a softirq disabled section, where raised softirqs are handled on
  the next call to local_bh_enable().

* From a softirq handler, where raised softirqs are handled on the next
  round in do_softirq(), or further deferred to a dedicated kthread.

Other bare tasks context may end up ignoring the raised NET_RX vector
until the next random softirq handling opportunity, which may not
happen before a while if the CPU goes idle afterwards with the tick
stopped.

Report inappropriate calling contexts when neither of the three above
conditions are met.

Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 include/linux/lockdep.h | 12 ++++++++++++
 net/core/dev.c          |  1 +
 2 files changed, 13 insertions(+)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index 67964dc4db95..1bd730b881f0 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -619,6 +619,17 @@ do {									\
 		     (!in_softirq() || in_irq() || in_nmi()));		\
 } while (0)
 
+/*
+ * Assert to be either in hardirq or in serving softirq or with
+ * softirqs disabled. Verifies a safe context to queue a softirq
+ * with __raise_softirq_irqoff().
+ */
+#define lockdep_assert_in_interrupt()				\
+do {								\
+	WARN_ON_ONCE(__lockdep_enabled && !in_interrupt());	\
+} while (0)
+
+
 extern void lockdep_assert_in_softirq_func(void);
 
 #else
@@ -634,6 +645,7 @@ extern void lockdep_assert_in_softirq_func(void);
 # define lockdep_assert_preemption_enabled() do { } while (0)
 # define lockdep_assert_preemption_disabled() do { } while (0)
 # define lockdep_assert_in_softirq() do { } while (0)
+# define lockdep_assert_in_interrupt() do { } while (0)
 # define lockdep_assert_in_softirq_func() do { } while (0)
 #endif
 
diff --git a/net/core/dev.c b/net/core/dev.c
index c0021cbd28fc..80e415ccf2c8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4666,6 +4666,7 @@ static inline void ____napi_schedule(struct softnet_data *sd,
 	struct task_struct *thread;
 
 	lockdep_assert_irqs_disabled();
+	lockdep_assert_in_interrupt();
 
 	if (test_bit(NAPI_STATE_THREADED, &napi->state)) {
 		/* Paired with smp_mb__before_atomic() in
-- 
2.46.0

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Jakub Kicinski]

On Wed, 12 Feb 2025 18:43:28 +0100 Frederic Weisbecker wrote:
> napi_schedule() is expected to be called either:
> 
> * From an interrupt, where raised softirqs are handled on IRQ exit
> 
> * From a softirq disabled section, where raised softirqs are handled on
>   the next call to local_bh_enable().
> 
> * From a softirq handler, where raised softirqs are handled on the next
>   round in do_softirq(), or further deferred to a dedicated kthread.
> 
> Other bare tasks context may end up ignoring the raised NET_RX vector
> until the next random softirq handling opportunity, which may not
> happen before a while if the CPU goes idle afterwards with the tick
> stopped.
> 
> Report inappropriate calling contexts when neither of the three above
> conditions are met.

Looks like netcons is hitting this warning in netdevsim:

[   16.063196][  T219]  nsim_start_xmit+0x4e0/0x6f0 [netdevsim]
[   16.063219][  T219]  ? netif_skb_features+0x23e/0xa80
[   16.063237][  T219]  netpoll_start_xmit+0x3c3/0x670
[   16.063258][  T219]  __netpoll_send_skb+0x3e9/0x800
[   16.063287][  T219]  netpoll_send_skb+0x2a/0xa0
[   16.063298][  T219]  send_ext_msg_udp+0x286/0x350 [netconsole]
[   16.063325][  T219]  write_ext_msg+0x1c6/0x230 [netconsole]
[   16.063346][  T219]  console_emit_next_record+0x20d/0x430

https://netdev-3.bots.linux.dev/vmksft-net-drv-dbg/results/990261/7-netcons-basic-sh/stderr

We gotta fix that first.

Please post the fixes for net, and then the warning in net-next.
So that we have some time to fix the uncovered warnings before
users are broadly exposed to them.
-- 
pw-bot: cr

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Breno Leitao]

On Wed, Feb 12, 2025 at 07:48:20PM -0800, Jakub Kicinski wrote:
> On Wed, 12 Feb 2025 18:43:28 +0100 Frederic Weisbecker wrote:
> > napi_schedule() is expected to be called either:
> > 
> > * From an interrupt, where raised softirqs are handled on IRQ exit
> > 
> > * From a softirq disabled section, where raised softirqs are handled on
> >   the next call to local_bh_enable().
> > 
> > * From a softirq handler, where raised softirqs are handled on the next
> >   round in do_softirq(), or further deferred to a dedicated kthread.
> > 
> > Other bare tasks context may end up ignoring the raised NET_RX vector
> > until the next random softirq handling opportunity, which may not
> > happen before a while if the CPU goes idle afterwards with the tick
> > stopped.
> > 
> > Report inappropriate calling contexts when neither of the three above
> > conditions are met.
> 
> Looks like netcons is hitting this warning in netdevsim:
> 
> [   16.063196][  T219]  nsim_start_xmit+0x4e0/0x6f0 [netdevsim]
> [   16.063219][  T219]  ? netif_skb_features+0x23e/0xa80
> [   16.063237][  T219]  netpoll_start_xmit+0x3c3/0x670
> [   16.063258][  T219]  __netpoll_send_skb+0x3e9/0x800
> [   16.063287][  T219]  netpoll_send_skb+0x2a/0xa0
> [   16.063298][  T219]  send_ext_msg_udp+0x286/0x350 [netconsole]
> [   16.063325][  T219]  write_ext_msg+0x1c6/0x230 [netconsole]
> [   16.063346][  T219]  console_emit_next_record+0x20d/0x430
> 
> https://netdev-3.bots.linux.dev/vmksft-net-drv-dbg/results/990261/7-netcons-basic-sh/stderr
> 
> We gotta fix that first.

Thanks Jakub,

I understand that it will be fixed by this patchset, right?

https://lore.kernel.org/all/20250212-netdevsim-v1-1-20ece94daae8@debian.org/

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Jakub Kicinski]

On Thu, 13 Feb 2025 01:58:17 -0800 Breno Leitao wrote:
> > Looks like netcons is hitting this warning in netdevsim:
> > 
> > [   16.063196][  T219]  nsim_start_xmit+0x4e0/0x6f0 [netdevsim]
> > [   16.063219][  T219]  ? netif_skb_features+0x23e/0xa80
> > [   16.063237][  T219]  netpoll_start_xmit+0x3c3/0x670
> > [   16.063258][  T219]  __netpoll_send_skb+0x3e9/0x800
> > [   16.063287][  T219]  netpoll_send_skb+0x2a/0xa0
> > [   16.063298][  T219]  send_ext_msg_udp+0x286/0x350 [netconsole]
> > [   16.063325][  T219]  write_ext_msg+0x1c6/0x230 [netconsole]
> > [   16.063346][  T219]  console_emit_next_record+0x20d/0x430
> > 
> > https://netdev-3.bots.linux.dev/vmksft-net-drv-dbg/results/990261/7-netcons-basic-sh/stderr
> > 
> > We gotta fix that first.  
> 
> Thanks Jakub,
> 
> I understand that it will be fixed by this patchset, right?

The problem is a bit nasty, on a closer look. We don't know if netcons
is called in IRQ context or not. How about we add an hrtimer to netdevsim,
schedule it to fire 5usec in the future instead of scheduling NAPI
immediately? We can call napi_schedule() from a timer safely.

Unless there's another driver which schedules NAPI from xmit.
Then we'd need to try harder to fix this in netpoll.
veth does use NAPI on xmit but it sets IFF_DISABLE_NETPOLL already.

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Breno Leitao]

Hello Jakub,

On Thu, Feb 13, 2025 at 07:14:26AM -0800, Jakub Kicinski wrote:
> On Thu, 13 Feb 2025 01:58:17 -0800 Breno Leitao wrote:
> > > Looks like netcons is hitting this warning in netdevsim:
> > > 
> > > [   16.063196][  T219]  nsim_start_xmit+0x4e0/0x6f0 [netdevsim]
> > > [   16.063219][  T219]  ? netif_skb_features+0x23e/0xa80
> > > [   16.063237][  T219]  netpoll_start_xmit+0x3c3/0x670
> > > [   16.063258][  T219]  __netpoll_send_skb+0x3e9/0x800
> > > [   16.063287][  T219]  netpoll_send_skb+0x2a/0xa0
> > > [   16.063298][  T219]  send_ext_msg_udp+0x286/0x350 [netconsole]
> > > [   16.063325][  T219]  write_ext_msg+0x1c6/0x230 [netconsole]
> > > [   16.063346][  T219]  console_emit_next_record+0x20d/0x430
> > > 
> > > https://netdev-3.bots.linux.dev/vmksft-net-drv-dbg/results/990261/7-netcons-basic-sh/stderr
> > > 
> > > We gotta fix that first.  
> > 
> > Thanks Jakub,
> > 
> > I understand that it will be fixed by this patchset, right?
> 
> The problem is a bit nasty, on a closer look. We don't know if netcons
> is called in IRQ context or not. How about we add an hrtimer to netdevsim,
> schedule it to fire 5usec in the future instead of scheduling NAPI
> immediately? We can call napi_schedule() from a timer safely.
> 
> Unless there's another driver which schedules NAPI from xmit.
> Then we'd need to try harder to fix this in netpoll.
> veth does use NAPI on xmit but it sets IFF_DISABLE_NETPOLL already.

Just to make sure I follow the netpoll issue. What would you like to fix
in netpoll exactly?

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Jakub Kicinski]

On Thu, 13 Feb 2025 10:14:02 -0800 Breno Leitao wrote:
> > The problem is a bit nasty, on a closer look. We don't know if netcons
> > is called in IRQ context or not. How about we add an hrtimer to netdevsim,
> > schedule it to fire 5usec in the future instead of scheduling NAPI
> > immediately? We can call napi_schedule() from a timer safely.
> > 
> > Unless there's another driver which schedules NAPI from xmit.
> > Then we'd need to try harder to fix this in netpoll.
> > veth does use NAPI on xmit but it sets IFF_DISABLE_NETPOLL already.  
> 
> Just to make sure I follow the netpoll issue. What would you like to fix
> in netpoll exactly?

Nothing in netpoll, the problem is that netdevsim calls napi_schedule
from the xmit path. That's incompatible with netpoll. We should fix
netdevsim instead (unless more real drivers need napi-from-xmit to
work).

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Frederic Weisbecker]

Le Thu, Feb 13, 2025 at 11:04:52AM -0800, Jakub Kicinski a écrit :
> On Thu, 13 Feb 2025 10:14:02 -0800 Breno Leitao wrote:
> > > The problem is a bit nasty, on a closer look. We don't know if netcons
> > > is called in IRQ context or not. How about we add an hrtimer to netdevsim,
> > > schedule it to fire 5usec in the future instead of scheduling NAPI
> > > immediately? We can call napi_schedule() from a timer safely.
> > > 
> > > Unless there's another driver which schedules NAPI from xmit.
> > > Then we'd need to try harder to fix this in netpoll.
> > > veth does use NAPI on xmit but it sets IFF_DISABLE_NETPOLL already.  
> > 
> > Just to make sure I follow the netpoll issue. What would you like to fix
> > in netpoll exactly?
> 
> Nothing in netpoll, the problem is that netdevsim calls napi_schedule
> from the xmit path. That's incompatible with netpoll. We should fix
> netdevsim instead (unless more real drivers need napi-from-xmit to
> work).

Let me clarify, because I don't know much this area. If the problem is that xmit
can't call napi_schedule() by design, then I defer to you. But if the problem is that
napi_schedule() may or may not be called from an interrupt, please note that
local_bh_enable() won't run softirqs from a hardirq and will instead defer to
IRQ tail. So it's fine to do an unconditional pair of local_bh_disable() / local_bh_enable().

Thanks.

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Jakub Kicinski]

On Thu, 13 Feb 2025 21:38:12 +0100 Frederic Weisbecker wrote:
> > > Just to make sure I follow the netpoll issue. What would you like to fix
> > > in netpoll exactly?  
> > 
> > Nothing in netpoll, the problem is that netdevsim calls napi_schedule
> > from the xmit path. That's incompatible with netpoll. We should fix
> > netdevsim instead (unless more real drivers need napi-from-xmit to
> > work).  
> 
> Let me clarify, because I don't know much this area. If the problem is that xmit
> can't call napi_schedule() by design, then I defer to you. But if the problem is that
> napi_schedule() may or may not be called from an interrupt, please note that
> local_bh_enable() won't run softirqs from a hardirq and will instead defer to
> IRQ tail. So it's fine to do an unconditional pair of local_bh_disable() / local_bh_enable().

I don't know where this is in the code TBH, but my understanding is
that HW IRQs - yes, as you say it'd be safe; the problem is that 
we have local_irq_save() all over the place. And that is neither
protected from local_bh_enable(), not does irq_restore execute softirqs.

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Frederic Weisbecker]

Le Fri, Feb 14, 2025 at 02:00:45PM -0800, Jakub Kicinski a écrit :
> On Thu, 13 Feb 2025 21:38:12 +0100 Frederic Weisbecker wrote:
> > > > Just to make sure I follow the netpoll issue. What would you like to fix
> > > > in netpoll exactly?  
> > > 
> > > Nothing in netpoll, the problem is that netdevsim calls napi_schedule
> > > from the xmit path. That's incompatible with netpoll. We should fix
> > > netdevsim instead (unless more real drivers need napi-from-xmit to
> > > work).  
> > 
> > Let me clarify, because I don't know much this area. If the problem is that xmit
> > can't call napi_schedule() by design, then I defer to you. But if the problem is that
> > napi_schedule() may or may not be called from an interrupt, please note that
> > local_bh_enable() won't run softirqs from a hardirq and will instead defer to
> > IRQ tail. So it's fine to do an unconditional pair of local_bh_disable() / local_bh_enable().
> 
> I don't know where this is in the code TBH, but my understanding is
> that HW IRQs - yes, as you say it'd be safe; the problem is that 
> we have local_irq_save() all over the place. And that is neither
> protected from local_bh_enable(), not does irq_restore execute softirqs.

Yeah actually checking local_bh_enable() again, it's not safe to call within
a hardirq. Ok I've been thinking some more and how about this instead?

diff --git a/net/core/dev.c b/net/core/dev.c
index c0021cbd28fc..2419cc558a64 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4692,7 +4692,7 @@ static inline void ____napi_schedule(struct softnet_data *sd,
 	 * we have to raise NET_RX_SOFTIRQ.
 	 */
 	if (!sd->in_net_rx_action)
-		__raise_softirq_irqoff(NET_RX_SOFTIRQ);
+		raise_softirq_irqoff(NET_RX_SOFTIRQ);
 }
 
 #ifdef CONFIG_RPS


This will simply wake up ksoftirqd if called from a non-IRQ. I expect such
callers to be rare enough to not impact performances and it has the advantage
to work for everyone.

Thanks.

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Breno Leitao]

On Thu, Feb 13, 2025 at 11:04:52AM -0800, Jakub Kicinski wrote:
> On Thu, 13 Feb 2025 10:14:02 -0800 Breno Leitao wrote:
> > > The problem is a bit nasty, on a closer look. We don't know if netcons
> > > is called in IRQ context or not. How about we add an hrtimer to netdevsim,
> > > schedule it to fire 5usec in the future instead of scheduling NAPI
> > > immediately? We can call napi_schedule() from a timer safely.
> > > 
> > > Unless there's another driver which schedules NAPI from xmit.
> > > Then we'd need to try harder to fix this in netpoll.
> > > veth does use NAPI on xmit but it sets IFF_DISABLE_NETPOLL already.  
> > 
> > Just to make sure I follow the netpoll issue. What would you like to fix
> > in netpoll exactly?
> 
> Nothing in netpoll, the problem is that netdevsim calls napi_schedule

Hm, you said the following above: 

	Then we'd need to try harder to fix this in netpoll.

I was curious about the meaning of that statement?

Thanks
--breno

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Breno Leitao]

Hello Jakub,

On Thu, Feb 13, 2025 at 07:14:26AM -0800, Jakub Kicinski wrote:
> ... How about we add an hrtimer to netdevsim,
> schedule it to fire 5usec in the future instead of scheduling NAPI
> immediately? We can call napi_schedule() from a timer safely.

I hacked a way to do so. Is this what you had in mind?

Author: Breno Leitao <leitao@debian.org>
Date:   Wed Feb 12 09:50:51 2025 -0800

    netdevsim: call napi_schedule from a timer context
    
    The netdevsim driver was experiencing NOHZ tick-stop errors during packet
    transmission due to pending softirq work when calling napi_schedule().
    This issue was observed when running the netconsole selftest, which
    triggered the following error message:
    
      NOHZ tick-stop error: local softirq work is pending, handler #08!!!
    
    To fix this issue, introduce a timer that schedules napi_schedule()
    from a timer context instead of calling it directly from the TX path.
    
    Create an hrtimer for each queue and kick it from the TX path,
    which then schedules napi_schedule() from the timer context.
    
    Suggested-by: Jakub Kicinski <kuba@kernel.org>
    Signed-off-by: Breno Leitao <leitao@debian.org>

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 42f247cbdceec..cd56904a39049 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -87,7 +87,7 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (unlikely(nsim_forward_skb(peer_dev, skb, rq) == NET_RX_DROP))
 		goto out_drop_cnt;
 
-	napi_schedule(&rq->napi);
+	hrtimer_start(&rq->napi_timer, ns_to_ktime(5), HRTIMER_MODE_REL);
 
 	rcu_read_unlock();
 	u64_stats_update_begin(&ns->syncp);
@@ -426,6 +426,25 @@ static int nsim_init_napi(struct netdevsim *ns)
 	return err;
 }
 
+static enum hrtimer_restart nsim_napi_schedule(struct hrtimer *timer)
+{
+	struct nsim_rq *rq;
+
+	rq = container_of(timer, struct nsim_rq, napi_timer);
+	napi_schedule(&rq->napi);
+	/* TODO: Should HRTIMER_RESTART be returned if napi_schedule returns
+	 * false?
+	 */
+
+	return HRTIMER_NORESTART;
+}
+
+static void nsim_rq_timer_init(struct nsim_rq *rq)
+{
+	hrtimer_init(&rq->napi_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	rq->napi_timer.function = nsim_napi_schedule;
+}
+
 static void nsim_enable_napi(struct netdevsim *ns)
 {
 	struct net_device *dev = ns->netdev;
@@ -436,6 +455,7 @@ static void nsim_enable_napi(struct netdevsim *ns)
 
 		netif_queue_set_napi(dev, i, NETDEV_QUEUE_TYPE_RX, &rq->napi);
 		napi_enable(&rq->napi);
+		nsim_rq_timer_init(rq);
 	}
 }
 
@@ -461,6 +481,7 @@ static void nsim_del_napi(struct netdevsim *ns)
 	for (i = 0; i < dev->num_rx_queues; i++) {
 		struct nsim_rq *rq = ns->rq[i];
 
+		hrtimer_cancel(&rq->napi_timer);
 		napi_disable(&rq->napi);
 		__netif_napi_del(&rq->napi);
 	}
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index dcf073bc4802e..2b396c517ac1d 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -97,6 +97,7 @@ struct nsim_rq {
 	struct napi_struct napi;
 	struct sk_buff_head skb_queue;
 	struct page_pool *page_pool;
+	struct hrtimer napi_timer;
 };
 
 struct netdevsim {

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Jakub Kicinski]

On Fri, 14 Feb 2025 08:43:28 -0800 Breno Leitao wrote:
> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> index 42f247cbdceec..cd56904a39049 100644
> --- a/drivers/net/netdevsim/netdev.c
> +++ b/drivers/net/netdevsim/netdev.c
> @@ -87,7 +87,7 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  	if (unlikely(nsim_forward_skb(peer_dev, skb, rq) == NET_RX_DROP))
>  		goto out_drop_cnt;
>  
> -	napi_schedule(&rq->napi);
> +	hrtimer_start(&rq->napi_timer, ns_to_ktime(5), HRTIMER_MODE_REL);

ns -> us

We want to leave the timer be in case it's already scheduled.
Otherwise we'll keep postponing forever under load.
Double check that hrtime_start() does not reset the time if already
pending. Maybe hrtimer_start_range_ns(..., 0, us_to_ktime(5), ...)
would work?

>  	rcu_read_unlock();
>  	u64_stats_update_begin(&ns->syncp);
> @@ -426,6 +426,25 @@ static int nsim_init_napi(struct netdevsim *ns)
>  	return err;
>  }
>  
> +static enum hrtimer_restart nsim_napi_schedule(struct hrtimer *timer)
> +{
> +	struct nsim_rq *rq;
> +
> +	rq = container_of(timer, struct nsim_rq, napi_timer);
> +	napi_schedule(&rq->napi);
> +	/* TODO: Should HRTIMER_RESTART be returned if napi_schedule returns
> +	 * false?
> +	 */

I think not, ignore the return value

> +	return HRTIMER_NORESTART;
> +}

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Breno Leitao]

On Fri, Feb 14, 2025 at 02:10:11PM -0800, Jakub Kicinski wrote:
> On Fri, 14 Feb 2025 08:43:28 -0800 Breno Leitao wrote:
> > diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
> > index 42f247cbdceec..cd56904a39049 100644
> > --- a/drivers/net/netdevsim/netdev.c
> > +++ b/drivers/net/netdevsim/netdev.c
> > @@ -87,7 +87,7 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >  	if (unlikely(nsim_forward_skb(peer_dev, skb, rq) == NET_RX_DROP))
> >  		goto out_drop_cnt;
> >  
> > -	napi_schedule(&rq->napi);
> > +	hrtimer_start(&rq->napi_timer, ns_to_ktime(5), HRTIMER_MODE_REL);
> 
> ns -> us
> 
> We want to leave the timer be in case it's already scheduled.
> Otherwise we'll keep postponing forever under load.
> Double check that hrtime_start() does not reset the time if already
> pending. Maybe hrtimer_start_range_ns(..., 0, us_to_ktime(5), ...)
> would work?

Reading the code, I got the impression that
hrtimer_start_range_ns(..., 0, us_to_ktime(5), ...) will reprogram the
timer anyway.

	hrtimer_start_range_ns() {
		__hrtimer_start_range_ns() {
			remove_hrtimer(timer, base, true, force_local);
			enqueue_hrtimer(timer, new_base, mode);
			...
		}	
	}
	
I think a better solution is to do something as:

	if (!hrtimer_active(&rq->napi_timer))
		hrtimer_start(&rq->napi_timer, us_to_ktime(5), HRTIMER_MODE_REL);

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Jakub Kicinski]

On Mon, 17 Feb 2025 08:46:21 -0800 Breno Leitao wrote:
> I think a better solution is to do something as:
> 
> 	if (!hrtimer_active(&rq->napi_timer))
> 		hrtimer_start(&rq->napi_timer, us_to_ktime(5), HRTIMER_MODE_REL);

sounds good

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Frederic Weisbecker]

Hi Jakub,

Le Wed, Feb 12, 2025 at 07:48:20PM -0800, Jakub Kicinski a écrit :
> On Wed, 12 Feb 2025 18:43:28 +0100 Frederic Weisbecker wrote:
> Please post the fixes for net, and then the warning in net-next.
> So that we have some time to fix the uncovered warnings before
> users are broadly exposed to them.

How do I define the patch target? Is it just about patch prefix?

Thanks.

> -- 
> pw-bot: cr

Re: [PATCH 1/2] net: Assert proper context while calling napi_schedule()

[Jakub Kicinski]

On Thu, 13 Feb 2025 21:39:51 +0100 Frederic Weisbecker wrote:
> Le Wed, Feb 12, 2025 at 07:48:20PM -0800, Jakub Kicinski a écrit :
> > On Wed, 12 Feb 2025 18:43:28 +0100 Frederic Weisbecker wrote:
> > Please post the fixes for net, and then the warning in net-next.
> > So that we have some time to fix the uncovered warnings before
> > users are broadly exposed to them.  
> 
> How do I define the patch target? Is it just about patch prefix?

Yes, --subject-prefix="PATCH net" for fixes
     --subject-prefix="PATCH net-next" for -next stuff

But even that isn't really a super hard requirement. My main ask
was to post patch 2, then wait for it to propagate (we merge the 
trees every Thu afternoon) then post patch 1.

Sorry for the delay.

[PATCH 2/2] r8152: Call napi_schedule() from proper context

[Frederic Weisbecker]

napi_schedule() is expected to be called either:

* From an interrupt, where raised softirqs are handled on IRQ exit

* Fom a softirq disabled section, where raised softirqs are handled on
  the next call to local_bh_enable().

* From a softirq handler, where raised softirqs are handled on the next
  round in do_softirq(), or further deferred to a dedicated kthread.

r8152 may call napi_schedule() on device resume time from a bare task
context without disabling softirqs as the following trace shows:

	__raise_softirq_irqoff
	__napi_schedule
	rtl8152_runtime_resume.isra.0
	rtl8152_resume
	usb_resume_interface.isra.0
	usb_resume_both
	__rpm_callback
	rpm_callback
	rpm_resume
	__pm_runtime_resume
	usb_autoresume_device
	usb_remote_wakeup
	hub_event
	process_one_work
	worker_thread
	kthread
	ret_from_fork
	ret_from_fork_asm

This may result in the NET_RX softirq vector to be ignored until the
next interrupt or softirq handling. The delay can be long if the
above kthread leaves the CPU idle and the tick is stopped for a while,
as reported with the following message:

	NOHZ tick-stop error: local softirq work is pending, handler #08!!!

Fix this with disabling softirqs while calling napi_schedule(). The
call to local_bh_enable() will take care of the NET_RX raised vector.

Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Closes: 354a2690-9bbf-4ccb-8769-fa94707a9340@molgen.mpg.de
Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
---
 drivers/net/usb/r8152.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 468c73974046..1325460ae457 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -8537,8 +8537,11 @@ static int rtl8152_runtime_resume(struct r8152 *tp)
 		clear_bit(SELECTIVE_SUSPEND, &tp->flags);
 		smp_mb__after_atomic();
 
-		if (!list_empty(&tp->rx_done))
+		if (!list_empty(&tp->rx_done)) {
+			local_bh_disable();
 			napi_schedule(&tp->napi);
+			local_bh_enable();
+		}
 
 		usb_submit_urb(tp->intr_urb, GFP_NOIO);
 	} else {
-- 
2.46.0

Re: [PATCH 2/2] r8152: Call napi_schedule() from proper context

[Francois Romieu]

Frederic Weisbecker <frederic@kernel.org> :
[...]
> r8152 may call napi_schedule() on device resume time from a bare task
> context without disabling softirqs as the following trace shows:
[...]
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 468c73974046..1325460ae457 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -8537,8 +8537,11 @@ static int rtl8152_runtime_resume(struct r8152 *tp)
>  		clear_bit(SELECTIVE_SUSPEND, &tp->flags);
>  		smp_mb__after_atomic();
>  
> -		if (!list_empty(&tp->rx_done))
> +		if (!list_empty(&tp->rx_done)) {
> +			local_bh_disable();
>  			napi_schedule(&tp->napi);
> +			local_bh_enable();
> +		}

AFAIU drivers/net/usb/r8152.c::rtl_work_func_t exhibits the same
problem.

-- 
Ueimor

Re: [PATCH 2/2] r8152: Call napi_schedule() from proper context

[Frederic Weisbecker]

Le Wed, Feb 12, 2025 at 09:49:29PM +0100, Francois Romieu a écrit :
> Frederic Weisbecker <frederic@kernel.org> :
> [...]
> > r8152 may call napi_schedule() on device resume time from a bare task
> > context without disabling softirqs as the following trace shows:
> [...]
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > index 468c73974046..1325460ae457 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -8537,8 +8537,11 @@ static int rtl8152_runtime_resume(struct r8152 *tp)
> >  		clear_bit(SELECTIVE_SUSPEND, &tp->flags);
> >  		smp_mb__after_atomic();
> >  
> > -		if (!list_empty(&tp->rx_done))
> > +		if (!list_empty(&tp->rx_done)) {
> > +			local_bh_disable();
> >  			napi_schedule(&tp->napi);
> > +			local_bh_enable();
> > +		}
> 
> AFAIU drivers/net/usb/r8152.c::rtl_work_func_t exhibits the same
> problem.

It's a workqueue function and softirqs don't seem to be disabled.
Looks like a goot catch!

Thanks.

> 
> -- 
> Ueimor

Re: [PATCH 2/2] r8152: Call napi_schedule() from proper context

[Paul Menzel]

Dear Frederic, dear Francois,


Thank you for the patch and review.


Am 12.02.25 um 21:58 schrieb Frederic Weisbecker:
> Le Wed, Feb 12, 2025 at 09:49:29PM +0100, Francois Romieu a écrit :
>> Frederic Weisbecker <frederic@kernel.org> :
>> [...]
>>> r8152 may call napi_schedule() on device resume time from a bare task
>>> context without disabling softirqs as the following trace shows:
>> [...]
>>> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
>>> index 468c73974046..1325460ae457 100644
>>> --- a/drivers/net/usb/r8152.c
>>> +++ b/drivers/net/usb/r8152.c
>>> @@ -8537,8 +8537,11 @@ static int rtl8152_runtime_resume(struct r8152 *tp)
>>>   		clear_bit(SELECTIVE_SUSPEND, &tp->flags);
>>>   		smp_mb__after_atomic();
>>>   
>>> -		if (!list_empty(&tp->rx_done))
>>> +		if (!list_empty(&tp->rx_done)) {
>>> +			local_bh_disable();
>>>   			napi_schedule(&tp->napi);
>>> +			local_bh_enable();
>>> +		}
>>
>> AFAIU drivers/net/usb/r8152.c::rtl_work_func_t exhibits the same
>> problem.
> 
> It's a workqueue function and softirqs don't seem to be disabled.
> Looks like a goot catch!

Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>

Are you going to send a v2, so it might get into Linux 6.14, or is it 
too late anyway?


Kind regards,

Paul

Re: [PATCH 2/2] r8152: Call napi_schedule() from proper context

[Tobias Jakobi]

Hello everyone,

I believe this is still a problem, correct? I've been carrying the two 
patches in my tree for some weeks now -- otherwise the network adapter 
in my USB-C dock stops working randomly.

Anything I can do to get these merged?

With best wishes,
Tobias

Re: [PATCH 2/2] r8152: Call napi_schedule() from proper context

[Jakub Kicinski]

On Sun, 27 Apr 2025 22:50:34 +0200 Tobias Jakobi wrote:
> Hello everyone,
> 
> I believe this is still a problem, correct? I've been carrying the two 
> patches in my tree for some weeks now -- otherwise the network adapter 
> in my USB-C dock stops working randomly.
> 
> Anything I can do to get these merged?

Which kernel are you suffering on? 
The fix is in 6.14 -- commit 77e45145e3039a
It went in without Fixes and stable annotation, tho

Re: [PATCH 2/2] r8152: Call napi_schedule() from proper context

[Tobias Jakobi]

On 4/28/25 7:49 PM, Jakub Kicinski wrote:
> On Sun, 27 Apr 2025 22:50:34 +0200 Tobias Jakobi wrote:
>> Hello everyone,
>>
>> I believe this is still a problem, correct? I've been carrying the two
>> patches in my tree for some weeks now -- otherwise the network adapter
>> in my USB-C dock stops working randomly.
>>
>> Anything I can do to get these merged?
> 
> Which kernel are you suffering on?
> The fix is in 6.14 -- commit 77e45145e3039a
> It went in without Fixes and stable annotation, tho
Hello Jakub,

thanks for pointing of the commit hash. I was looking for the wrong 
commit in 6.14.y. More specifically I was searching for the one named 
"r8152: Call napi_schedule() from proper context", touching 
drivers/net/usb/r8152.c.

But it seems the problem was solved in a different manner.

Sorry for the noise!

With best wishes,
Tobias