Re: [PATCH] net: hinic: avoid kernel hung in hinic_get_stats64()

[maqiao <mqaio@linux.alibaba.com>]

在 2022/6/30 下午6:23, Eric Dumazet 写道:
> 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.Thanks for reminding, and I noticed that nic_tx_stats/nic_rx_stats has 
been protected by u64_stats_sync in update_t/rx_stats(), it seems that 
it's unnecessary to use spinlock in update_nic_stats().

I will send v2 as soon as possible, thanks.
> 
>>          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
>>