Messages in this thread Patch in this message | | | From | Eric Dumazet <> | Date | Thu, 30 Jun 2022 12:23:26 +0200 | Subject | Re: [PATCH] net: hinic: avoid kernel hung in hinic_get_stats64() |
| |
On Wed, Jun 29, 2022 at 9:28 AM Qiao Ma <mqaio@linux.alibaba.com> wrote: > > When using hinic device as a bond slave device, and reading device stats of > master bond device, the kernel may hung. > > The kernel panic calltrace as follows: > Kernel panic - not syncing: softlockup: hung tasks > Call trace: > native_queued_spin_lock_slowpath+0x1ec/0x31c > dev_get_stats+0x60/0xcc > dev_seq_printf_stats+0x40/0x120 > dev_seq_show+0x1c/0x40 > seq_read_iter+0x3c8/0x4dc > seq_read+0xe0/0x130 > proc_reg_read+0xa8/0xe0 > vfs_read+0xb0/0x1d4 > ksys_read+0x70/0xfc > __arm64_sys_read+0x20/0x30 > el0_svc_common+0x88/0x234 > do_el0_svc+0x2c/0x90 > el0_svc+0x1c/0x30 > el0_sync_handler+0xa8/0xb0 > el0_sync+0x148/0x180 > > And the calltrace of task that actually caused kernel hungs as follows: > __switch_to+124 > __schedule+548 > schedule+72 > schedule_timeout+348 > __down_common+188 > __down+24 > down+104 > hinic_get_stats64+44 [hinic] > dev_get_stats+92 > bond_get_stats+172 [bonding] > dev_get_stats+92 > dev_seq_printf_stats+60 > dev_seq_show+24 > seq_read_iter+964 > seq_read+220 > proc_reg_read+164 > vfs_read+172 > ksys_read+108 > __arm64_sys_read+28 > el0_svc_common+132 > do_el0_svc+40 > el0_svc+24 > el0_sync_handler+164 > el0_sync+324 > > When getting device stats from bond, kernel will call bond_get_stats(). > It first holds the spinlock bond->stats_lock, and then call > hinic_get_stats64() to collect hinic device's stats. > However, hinic_get_stats64() calls `down(&nic_dev->mgmt_lock)` to > protect its critical section, which may schedule current task out. > And if system is under high pressure, the task cannot be woken up > immediately, which eventually triggers kernel hung panic. > > Fixes: edd384f682cc ("net-next/hinic: Add ethtool and stats") > Signed-off-by: Qiao Ma <mqaio@linux.alibaba.com> > --- > drivers/net/ethernet/huawei/hinic/hinic_dev.h | 1 + > drivers/net/ethernet/huawei/hinic/hinic_main.c | 7 +++---- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/huawei/hinic/hinic_dev.h b/drivers/net/ethernet/huawei/hinic/hinic_dev.h > index fb3e89141a0d..1fb343d03fd5 100644 > --- a/drivers/net/ethernet/huawei/hinic/hinic_dev.h > +++ b/drivers/net/ethernet/huawei/hinic/hinic_dev.h > @@ -97,6 +97,7 @@ struct hinic_dev { > > struct hinic_txq_stats tx_stats; > struct hinic_rxq_stats rx_stats; > + spinlock_t stats_lock; > > u8 rss_tmpl_idx; > u8 rss_hash_engine; > diff --git a/drivers/net/ethernet/huawei/hinic/hinic_main.c b/drivers/net/ethernet/huawei/hinic/hinic_main.c > index 56a89793f47d..32a3d700ad26 100644 > --- a/drivers/net/ethernet/huawei/hinic/hinic_main.c > +++ b/drivers/net/ethernet/huawei/hinic/hinic_main.c > @@ -125,11 +125,13 @@ static void update_nic_stats(struct hinic_dev *nic_dev) > { > int i, num_qps = hinic_hwdev_num_qps(nic_dev->hwdev); > > + spin_lock(&nic_dev->stats_lock); > for (i = 0; i < num_qps; i++) > update_rx_stats(nic_dev, &nic_dev->rxqs[i]); > > for (i = 0; i < num_qps; i++) > update_tx_stats(nic_dev, &nic_dev->txqs[i]); > + spin_unlock(&nic_dev->stats_lock); > } > > /** > @@ -859,13 +861,9 @@ static void hinic_get_stats64(struct net_device *netdev, > nic_rx_stats = &nic_dev->rx_stats; > nic_tx_stats = &nic_dev->tx_stats; > > - down(&nic_dev->mgmt_lock); > - > if (nic_dev->flags & HINIC_INTF_UP)) > update_nic_stats(nic_dev); > > - up(&nic_dev->mgmt_lock); > -
Note: The following is racy, because multiple threads can call hinic_get_stats64() at the same time. It needs a loop, see include/linux/u64_stats_sync.h for detail.
> stats->rx_bytes = nic_rx_stats->bytes; > stats->rx_packets = nic_rx_stats->pkts; > stats->rx_errors = nic_rx_stats->errors;
Same remark for nic_tx_stats->{bytes|pkts|errors}
> @@ -1239,6 +1237,7 @@ static int nic_dev_init(struct pci_dev *pdev) > > u64_stats_init(&tx_stats->syncp); > u64_stats_init(&rx_stats->syncp); > + spin_lock_init(&nic_dev->stats_lock); > > nic_dev->vlan_bitmap = devm_bitmap_zalloc(&pdev->dev, VLAN_N_VID, > GFP_KERNEL); > -- > 1.8.3.1 >
| |