lkml.org 
[lkml]   [2022]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [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
>

\
 
 \ /
  Last update: 2022-06-30 12:26    [W:1.670 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site