[PATCH net] ionic: use dev_consume_skb_any outside of napi

[PATCH net] ionic: use dev_consume_skb_any outside of napi

[Shannon Nelson]

If we're not in a NAPI softirq context, we need to be careful
about how we call napi_consume_skb(), specifically we need to
call it with budget==0 to signal to it that we're not in a
safe context.

This was found while running some configuration stress testing
of traffic and a change queue config loop running, and this
curious note popped out:

[ 4371.402645] BUG: using smp_processor_id() in preemptible [00000000] code: ethtool/20545
[ 4371.402897] caller is napi_skb_cache_put+0x16/0x80
[ 4371.403120] CPU: 25 PID: 20545 Comm: ethtool Kdump: loaded Tainted: G           OE      6.10.0-rc3-netnext+ #8
[ 4371.403302] Hardware name: HPE ProLiant DL360 Gen10/ProLiant DL360 Gen10, BIOS U32 01/23/2021
[ 4371.403460] Call Trace:
[ 4371.403613]  <TASK>
[ 4371.403758]  dump_stack_lvl+0x4f/0x70
[ 4371.403904]  check_preemption_disabled+0xc1/0xe0
[ 4371.404051]  napi_skb_cache_put+0x16/0x80
[ 4371.404199]  ionic_tx_clean+0x18a/0x240 [ionic]
[ 4371.404354]  ionic_tx_cq_service+0xc4/0x200 [ionic]
[ 4371.404505]  ionic_tx_flush+0x15/0x70 [ionic]
[ 4371.404653]  ? ionic_lif_qcq_deinit.isra.23+0x5b/0x70 [ionic]
[ 4371.404805]  ionic_txrx_deinit+0x71/0x190 [ionic]
[ 4371.404956]  ionic_reconfigure_queues+0x5f5/0xff0 [ionic]
[ 4371.405111]  ionic_set_ringparam+0x2e8/0x3e0 [ionic]
[ 4371.405265]  ethnl_set_rings+0x1f1/0x300
[ 4371.405418]  ethnl_default_set_doit+0xbb/0x160
[ 4371.405571]  genl_family_rcv_msg_doit+0xff/0x130
	[...]

I found that ionic_tx_clean() calls napi_consume_skb() which calls
napi_skb_cache_put(), but before that last call is the note
    /* Zero budget indicate non-NAPI context called us, like netpoll */
and
    DEBUG_NET_WARN_ON_ONCE(!in_softirq());

Those are pretty big hints that we're doing it wrong.  So, let's pass a 0
when we know we're not in a napi context.

Fixes: 386e69865311 ("ionic: Make use napi_consume_skb")
Reviewed-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_txrx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
index 2427610f4306..7fbea9c346eb 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
@@ -1204,7 +1204,7 @@ static void ionic_tx_clean(struct ionic_queue *q,
 	desc_info->bytes = skb->len;
 	stats->clean++;
 
-	napi_consume_skb(skb, 1);
+	napi_consume_skb(skb, likely(softirq_count()) ? 1 : 0);
 }
 
 static bool ionic_tx_service(struct ionic_cq *cq,
-- 
2.17.1

Re: [PATCH net] ionic: use dev_consume_skb_any outside of napi

[Jakub Kicinski]

On Wed, 19 Jun 2024 14:20:22 -0700 Shannon Nelson wrote:
> I found that ionic_tx_clean() calls napi_consume_skb() which calls
> napi_skb_cache_put(), but before that last call is the note
>     /* Zero budget indicate non-NAPI context called us, like netpoll */
> and
>     DEBUG_NET_WARN_ON_ONCE(!in_softirq());
> 
> Those are pretty big hints that we're doing it wrong.  So, let's pass a 0
> when we know we're not in a napi context.

Just pass the NAPI budget in, and if not in NAPI pass 0.
A bit more plumbing thru, but a lot less thinking required during
review..
-- 
pw-bot: cr

Re: [PATCH net] ionic: use dev_consume_skb_any outside of napi

[Nelson, Shannon]

On 6/19/2024 4:44 PM, Jakub Kicinski wrote:
> On Wed, 19 Jun 2024 14:20:22 -0700 Shannon Nelson wrote:
>> I found that ionic_tx_clean() calls napi_consume_skb() which calls
>> napi_skb_cache_put(), but before that last call is the note
>>      /* Zero budget indicate non-NAPI context called us, like netpoll */
>> and
>>      DEBUG_NET_WARN_ON_ONCE(!in_softirq());
>>
>> Those are pretty big hints that we're doing it wrong.  So, let's pass a 0
>> when we know we're not in a napi context.
> 
> Just pass the NAPI budget in, and if not in NAPI pass 0.
> A bit more plumbing thru, but a lot less thinking required during
> review..
> --
> pw-bot: cr

I had a plumb-it-through solution at one point, but this is so much 
cleaner with a simple one line fix, so much less to go wrong...

Thanks, I'll look at it again tomorrow.
sln

Re: [PATCH net] ionic: use dev_consume_skb_any outside of napi

[Jakub Kicinski]

On Wed, 19 Jun 2024 17:11:08 -0700 Nelson, Shannon wrote:
> > Just pass the NAPI budget in, and if not in NAPI pass 0.
> > A bit more plumbing thru, but a lot less thinking required during
> > review..
> > --
> > pw-bot: cr  
> 
> I had a plumb-it-through solution at one point, but this is so much 
> cleaner with a simple one line fix, so much less to go wrong...

So much less to go wrong you say? I'm 75% sure the fix is buggy,
it won't work if hard irq comes during soft irq ;)