Message-ID: <1bdc12d6-ac23-49ff-a235-5ea54ca2ddaa@roeck-us.net>
Date: Wed, 27 Nov 2024 14:48:06 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Joe Damato <jdamato@...tly.com>, netdev@...r.kernel.org,
 mkarsten@...terloo.ca, skhawaja@...gle.com, sdf@...ichev.me,
 bjorn@...osinc.com, amritha.nambiar@...el.com, sridhar.samudrala@...el.com,
 willemdebruijn.kernel@...il.com, edumazet@...gle.com,
 Jakub Kicinski <kuba@...nel.org>, "David S. Miller" <davem@...emloft.net>,
 Paolo Abeni <pabeni@...hat.com>, Jonathan Corbet <corbet@....net>,
 Jiri Pirko <jiri@...nulli.us>,
 Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
 Lorenzo Bianconi <lorenzo@...nel.org>,
 Johannes Berg <johannes.berg@...el.com>,
 "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
 open list <linux-kernel@...r.kernel.org>, pcnet32@...ntier.com
Subject: Re: [net-next v6 5/9] net: napi: Add napi_config

On 11/27/24 12:00, Joe Damato wrote:
> On Wed, Nov 27, 2024 at 10:51:16AM -0800, Joe Damato wrote:
>> On Wed, Nov 27, 2024 at 09:43:54AM -0800, Guenter Roeck wrote:
>>> Hi,
>>>
>>> On Fri, Oct 11, 2024 at 06:45:00PM +0000, Joe Damato wrote:
>>>> Add a persistent NAPI config area for NAPI configuration to the core.
>>>> Drivers opt-in to setting the persistent config for a NAPI by passing an
>>>> index when calling netif_napi_add_config.
>>>>
>>>> napi_config is allocated in alloc_netdev_mqs, freed in free_netdev
>>>> (after the NAPIs are deleted).
>>>>
>>>> Drivers which call netif_napi_add_config will have persistent per-NAPI
>>>> settings: NAPI IDs, gro_flush_timeout, and defer_hard_irq settings.
>>>>
>>>> Per-NAPI settings are saved in napi_disable and restored in napi_enable.
>>>>
>>>> Co-developed-by: Martin Karsten <mkarsten@...terloo.ca>
>>>> Signed-off-by: Martin Karsten <mkarsten@...terloo.ca>
>>>> Signed-off-by: Joe Damato <jdamato@...tly.com>
>>>> Reviewed-by: Jakub Kicinski <kuba@...nel.org>
>>>
>>> This patch triggers a lock inversion message on pcnet Ethernet adapters.
>>
>> Thanks for the report. I am not familiar with the pcnet driver, but
>> took some time now to read the report below and the driver code.
>>
>> I could definitely be reading the output incorrectly (if so please
>> let me know), but it seems like the issue can be triggered in this
>> case:
> 
> Sorry, my apologies, I both:
>    - read the report incorrectly, and
>    - proposed a bad patch that would result in a deadlock :)
> 
> After re-reading it and running this by Martin (who is CC'd), the
> inversion is actually:
> 
> CPU 0:
> pcnet32_open
>     lock(lp->lock)
>       napi_enable
>         napi_hash_add <- before this executes, CPU 1 proceeds
>           lock(napi_hash_lock)
> CPU 1:
>    pcnet32_close
>      napi_disable
>        napi_hash_del
>          lock(napi_hash_lock)
>           < INTERRUPT >
>              pcnet32_interrupt
>                lock(lp->lock)
> 
> This is now an inversion because:
> 
> CPU 0: holds lp->lock and is about to take napi_hash_lock
> CPU 1: holds napi_hashlock and an IRQ firing on CPU 1 tries to take
>         lp->lock (which CPU 0 already holds)
> 
> Neither side can proceed:
>    - CPU 0 is stuck waiting for napi_hash_lock
>    - CPU 1 is stuck waiting for lp->lock
> 
> I think the below explanation is still correct as to why the
> identified commit causes the issue:
> 
>> It seems this was triggered because before the identified commit,
>> napi_enable did not call napi_hash_add (and thus did not take the
>> napi_hash_lock).
> 
> However, the previous patch I proposed for pcnet32 would also cause
> a deadlock as the watchdog timer's function also needs lp->lock.
> 
> A corrected patch for pcnet32 can be found below.
> 
> Guenter: Sorry, would you mind testing the below instead of the
> previous patch?
> 
> Don: Let me know what you think about the below?
> 
> Netdev maintainers, there is an alternate locking solution I can
> propose as an RFC that might avoid this class of problem if this
> sort of issue is more widespread than just pcnet32:
>    - add the NAPI to the hash in netif_napi_add_weight (instead of napi_enable)
>    - remove the NAPI from the hash in __netif_napi_del (instead of
>      napi_disable)
> 
> If changing the locking order in core is the desired route, than the
> patch below should be unnecessary, but:
> 
> diff --git a/drivers/net/ethernet/amd/pcnet32.c b/drivers/net/ethernet/amd/pcnet32.c
> index 72db9f9e7bee..2e0077e68883 100644
> --- a/drivers/net/ethernet/amd/pcnet32.c
> +++ b/drivers/net/ethernet/amd/pcnet32.c
> @@ -2625,11 +2625,10 @@ static int pcnet32_close(struct net_device *dev)
> 
>          del_timer_sync(&lp->watchdog_timer);
> 
> +       spin_lock_irqsave(&lp->lock, flags);
>          netif_stop_queue(dev);
>          napi_disable(&lp->napi);
> 

That is what I did, actually. Problem with that is that napi_disable()
wants to be able to sleep, thus the above triggers:

BUG: sleeping function called from invalid context at net/core/dev.c:6775
in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 1817, name: ip
preempt_count: 1, expected: 0
2 locks held by ip/1817:
#0: ffffffff81ded990 (rtnl_mutex){+.+.}-{4:4}, at: rtnl_newlink+0x22a/0x74c
#1: ff6000000498ccb0 (&lp->lock){-.-.}-{3:3}, at: pcnet32_close+0x40/0x126
irq event stamp: 3720
hardirqs last  enabled at (3719): [<ffffffff80decaf4>] _raw_spin_unlock_irqrestore+0x54/0x62
hardirqs last disabled at (3720): [<ffffffff80dec8a2>] _raw_spin_lock_irqsave+0x5e/0x64
softirqs last  enabled at (3712): [<ffffffff8001efca>] handle_softirqs+0x3e6/0x4a2
softirqs last disabled at (3631): [<ffffffff80ded6cc>] __do_softirq+0x12/0x1a
CPU: 0 UID: 0 PID: 1817 Comm: ip Tainted: G                 N 6.12.0-10313-g7d4050728c83-dirty #1
Tainted: [N]=TEST
Hardware name: riscv-virtio,qemu (DT)
Call Trace:
[<ffffffff80006d42>] dump_backtrace+0x1c/0x24
[<ffffffff80dc8d94>] show_stack+0x2c/0x38
[<ffffffff80de00b0>] dump_stack_lvl+0x74/0xac
[<ffffffff80de00fc>] dump_stack+0x14/0x1c
[<ffffffff8004da18>] __might_resched+0x23e/0x248
[<ffffffff8004da60>] __might_sleep+0x3e/0x62
[<ffffffff80b8d370>] napi_disable+0x24/0x10c
[<ffffffff809a06fe>] pcnet32_close+0x6c/0x126

Guenter

Powered by blists - more mailing lists