netdev - Re: [PATCH v1 net 2/4] net: ena: Wrong missing IO completions check order

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e4ff2cce-904b-45fb-8bf9-4ebfd14271c4@amd.com>
Date: Wed, 10 Apr 2024 14:51:32 -0700
From: "Nelson, Shannon" <shannon.nelson@....com>
To: darinzon@...zon.com, David Miller <davem@...emloft.net>,
 Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Cc: "Woodhouse, David" <dwmw@...zon.com>, "Machulsky, Zorik"
 <zorik@...zon.com>, "Matushevsky, Alexander" <matua@...zon.com>,
 Saeed Bshara <saeedb@...zon.com>, "Wilson, Matt" <msw@...zon.com>,
 "Liguori, Anthony" <aliguori@...zon.com>, "Bshara, Nafea"
 <nafea@...zon.com>, "Belgazal, Netanel" <netanel@...zon.com>,
 "Saidi, Ali" <alisaidi@...zon.com>, "Herrenschmidt, Benjamin"
 <benh@...zon.com>, "Kiyanovski, Arthur" <akiyano@...zon.com>,
 "Dagan, Noam" <ndagan@...zon.com>, "Agroskin, Shay" <shayagr@...zon.com>,
 "Itzko, Shahar" <itzko@...zon.com>, "Abboud, Osama" <osamaabb@...zon.com>,
 "Ostrovsky, Evgeny" <evostrov@...zon.com>, "Tabachnik, Ofir"
 <ofirt@...zon.com>, Netanel Belgazal <netanel@...apurnalabs.com>,
 Sameeh Jubran <sameehj@...zon.com>, Amit Bernstein <amitbern@...zon.com>
Subject: Re: [PATCH v1 net 2/4] net: ena: Wrong missing IO completions check
 order

On 4/10/2024 2:13 AM, darinzon@...zon.com wrote:
> From: David Arinzon <darinzon@...zon.com>
> 
> Missing IO completions check is called every second (HZ jiffies).
> This commit fixes several issues with this check:
> 
> 1. Duplicate queues check:
>     Max of 4 queues are scanned on each check due to monitor budget.
>     Once reaching the budget, this check exits under the assumption that
>     the next check will continue to scan the remainder of the queues,
>     but in practice, next check will first scan the last already scanned
>     queue which is not necessary and may cause the full queue scan to
>     last a couple of seconds longer.
>     The fix is to start every check with the next queue to scan.
>     For example, on 8 IO queues:
>     Bug: [0,1,2,3], [3,4,5,6], [6,7]
>     Fix: [0,1,2,3], [4,5,6,7]
> 
> 2. Unbalanced queues check:
>     In case the number of active IO queues is not a multiple of budget,
>     there will be checks which don't utilize the full budget
>     because the full scan exits when reaching the last queue id.
>     The fix is to run every TX completion check with exact queue budget
>     regardless of the queue id.
>     For example, on 7 IO queues:
>     Bug: [0,1,2,3], [4,5,6], [0,1,2,3]
>     Fix: [0,1,2,3], [4,5,6,0], [1,2,3,4]
>     The budget may be lowered in case the number of IO queues is less
>     than the budget (4) to make sure there are no duplicate queues on
>     the same check.
>     For example, on 3 IO queues:
>     Bug: [0,1,2,0], [1,2,0,1]
>     Fix: [0,1,2], [0,1,2]
> 
> Fixes: 1738cd3ed342 ("net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)")
> Signed-off-by: Amit Bernstein <amitbern@...zon.com>
> Signed-off-by: David Arinzon <darinzon@...zon.com>


Reviewed-by: Shannon Nelson <shannon.nelson@....com>


> ---
>   drivers/net/ethernet/amazon/ena/ena_netdev.c | 21 +++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index 09e7da1a..59befc0f 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -3481,10 +3481,11 @@ static void check_for_missing_completions(struct ena_adapter *adapter)
>   {
>          struct ena_ring *tx_ring;
>          struct ena_ring *rx_ring;
> -       int i, budget, rc;
> +       int qid, budget, rc;
>          int io_queue_count;
> 
>          io_queue_count = adapter->xdp_num_queues + adapter->num_io_queues;
> +
>          /* Make sure the driver doesn't turn the device in other process */
>          smp_rmb();
> 
> @@ -3497,27 +3498,29 @@ static void check_for_missing_completions(struct ena_adapter *adapter)
>          if (adapter->missing_tx_completion_to == ENA_HW_HINTS_NO_TIMEOUT)
>                  return;
> 
> -       budget = ENA_MONITORED_TX_QUEUES;
> +       budget = min_t(u32, io_queue_count, ENA_MONITORED_TX_QUEUES);
> 
> -       for (i = adapter->last_monitored_tx_qid; i < io_queue_count; i++) {
> -               tx_ring = &adapter->tx_ring[i];
> -               rx_ring = &adapter->rx_ring[i];
> +       qid = adapter->last_monitored_tx_qid;
> +
> +       while (budget) {
> +               qid = (qid + 1) % io_queue_count;
> +
> +               tx_ring = &adapter->tx_ring[qid];
> +               rx_ring = &adapter->rx_ring[qid];
> 
>                  rc = check_missing_comp_in_tx_queue(adapter, tx_ring);
>                  if (unlikely(rc))
>                          return;
> 
> -               rc =  !ENA_IS_XDP_INDEX(adapter, i) ?
> +               rc =  !ENA_IS_XDP_INDEX(adapter, qid) ?
>                          check_for_rx_interrupt_queue(adapter, rx_ring) : 0;
>                  if (unlikely(rc))
>                          return;
> 
>                  budget--;
> -               if (!budget)
> -                       break;
>          }
> 
> -       adapter->last_monitored_tx_qid = i % io_queue_count;
> +       adapter->last_monitored_tx_qid = qid;
>   }
> 
>   /* trigger napi schedule after 2 consecutive detections */
> --
> 2.40.1
> 
>