Re: [PATCH 22.11] Revert "net/iavf: fix abnormal disable HW interrupt"

["Chris Brezovec (cbrezove)" <cbrezove@cisco.com>]

[-- Attachment #1: Type: text/plain, Size: 6672 bytes --]

Kevin,

I have a question on this issue.  The commit you are looking to back out was committed into v22.11.3.  Does that mean that the issue exists in v22.11.3?  Or were there some other commits between v22.11.3 and v22.11.4 that exposed this issue, or made it more likely to hit?

Thanks!
-Chris

From: Kevin Traynor <ktraynor@redhat.com>
Date: Friday, January 12, 2024 at 6:21 AM
To: stable@dpdk.org <stable@dpdk.org>, xuemingl@nvidia.com <xuemingl@nvidia.com>
Cc: Kevin Traynor <ktraynor@redhat.com>
Subject: [PATCH 22.11] Revert "net/iavf: fix abnormal disable HW interrupt"
This reverts commit 2bdeeb53da0dda4e36a022553d3403c1217d677e.

The change in behaviour [0] for handling some admin queue messages
from polling to interrupt mode has changed the behaviour and is leading
to a deadlock with OVS.

[0]
commit 2bdeeb53da0dda4e36a022553d3403c1217d677e
Author: Mingjin Ye <mingjinx.ye@intel.com>
Date:   Wed Jun 14 09:53:03 2023 +0000

    net/iavf: fix abnormal disable HW interrupt

    [ upstream commit 675a104e2e940ec476e8b469725e8465d01c0098 ]

Bugzilla ID: 1337
Reported-by: Kevin Traynor <ktraynor@redhat.com>
Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
 drivers/net/iavf/iavf_ethdev.c | 25 +++++++++---------
 drivers/net/iavf/iavf_vchnl.c  | 48 +++++++++++++++++++++++++++-------
 2 files changed, 50 insertions(+), 23 deletions(-)

diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
index 62d72a0155..a12ea39444 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -2760,17 +2760,4 @@ iavf_dev_close(struct rte_eth_dev *dev)

         ret = iavf_dev_stop(dev);
-
-       /*
-        * Release redundant queue resource when close the dev
-        * so that other vfs can re-use the queues.
-        */
-       if (vf->lv_enabled) {
-               ret = iavf_request_queues(dev, IAVF_MAX_NUM_QUEUES_DFLT);
-               if (ret)
-                       PMD_DRV_LOG(ERR, "Reset the num of queues failed");
-
-               vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
-       }
-
         adapter->closed = true;

@@ -2789,4 +2776,16 @@ iavf_dev_close(struct rte_eth_dev *dev)
                 iavf_config_promisc(adapter, false, false);

+       /*
+        * Release redundant queue resource when close the dev
+        * so that other vfs can re-use the queues.
+        */
+       if (vf->lv_enabled) {
+               ret = iavf_request_queues(dev, IAVF_MAX_NUM_QUEUES_DFLT);
+               if (ret)
+                       PMD_DRV_LOG(ERR, "Reset the num of queues failed");
+
+               vf->max_rss_qregion = IAVF_MAX_NUM_QUEUES_DFLT;
+       }
+
         iavf_shutdown_adminq(hw);
         if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) {
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index c2f6b617d6..aeffb07cca 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -319,5 +319,4 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter, struct iavf_cmd_info *args,
         switch (args->ops) {
         case VIRTCHNL_OP_RESET_VF:
-       case VIRTCHNL_OP_REQUEST_QUEUES:
                 /*no need to wait for response */
                 _clear_cmd(vf);
@@ -343,4 +342,31 @@ iavf_execute_vf_cmd(struct iavf_adapter *adapter, struct iavf_cmd_info *args,
                 _clear_cmd(vf);
                 break;
+       case VIRTCHNL_OP_REQUEST_QUEUES:
+               /*
+                * ignore async reply, only wait for system message,
+                * vf_reset = true if get VIRTCHNL_EVENT_RESET_IMPENDING,
+                * if not, means request queues failed.
+                */
+               do {
+                       result = iavf_read_msg_from_pf(adapter, args->out_size,
+                                                  args->out_buffer);
+                       if (result == IAVF_MSG_SYS && vf->vf_reset) {
+                               break;
+                       } else if (result == IAVF_MSG_CMD ||
+                               result == IAVF_MSG_ERR) {
+                               err = -1;
+                               break;
+                       }
+                       iavf_msec_delay(ASQ_DELAY_MS);
+                       /* If don't read msg or read sys event, continue */
+               } while (i++ < MAX_TRY_TIMES);
+               if (i >= MAX_TRY_TIMES ||
+                       vf->cmd_retval != VIRTCHNL_STATUS_SUCCESS) {
+                       err = -1;
+                       PMD_DRV_LOG(ERR, "No response or return failure (%d)"
+                                   " for cmd %d", vf->cmd_retval, args->ops);
+               }
+               _clear_cmd(vf);
+               break;
         default:
                 if (rte_thread_is_intr()) {
@@ -2042,9 +2068,9 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
                 IAVF_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);
         struct iavf_info *vf =  IAVF_DEV_PRIVATE_TO_VF(adapter);
+       struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
         struct virtchnl_vf_res_request vfres;
         struct iavf_cmd_info args;
         uint16_t num_queue_pairs;
         int err;
-       int i = 0;

         if (!(vf->vf_res->vf_cap_flags &
@@ -2067,5 +2093,14 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)

         if (vf->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) {
-               err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
+               /* disable interrupt to avoid the admin queue message to be read
+                * before iavf_read_msg_from_pf.
+                *
+                * don't disable interrupt handler until ready to execute vf cmd.
+                */
+               rte_spinlock_lock(&vf->aq_lock);
+               rte_intr_disable(pci_dev->intr_handle);
+               err = iavf_execute_vf_cmd(adapter, &args, 0);
+               rte_intr_enable(pci_dev->intr_handle);
+               rte_spinlock_unlock(&vf->aq_lock);
         } else {
                 rte_eal_alarm_cancel(iavf_dev_alarm_handler, dev);
@@ -2080,11 +2115,4 @@ iavf_request_queues(struct rte_eth_dev *dev, uint16_t num)
         }

-       /* wait for interrupt notification vf is resetting */
-       while (i++ < MAX_TRY_TIMES) {
-               if (vf->vf_reset)
-                       break;
-               iavf_msec_delay(ASQ_DELAY_MS);
-       }
-
         /* request queues succeeded, vf is resetting */
         if (vf->vf_reset) {
--
2.43.0

[-- Attachment #2: Type: text/html, Size: 16233 bytes --]