Re: [dpdk-dev] [PATCH] net/i40e: fix modifying the number of queues

Hi Jia,

-----Original Message-----
From: Guo, Jia
Sent: Sunday, June 21, 2020 9:36 PM
To: Zhang, AlvinX <redacted>; dev@dpdk.org
Cc: stable@dpdk.org; Xing, Beilei <redacted>; Jiang, MaoX
[off-list ref]
Subject: Re: [PATCH] net/i40e: fix modifying the number of queues

hi, alvin

On 6/10/2020 8:07 PM, alvinx.zhang@intel.com wrote:

quoted

From: Alvin Zhang <redacted>

For the newly created VF, if the number of qps is greater than 4 at
startup, it may fail to start. This patch updates the API
`i40evf_dev_configure`.

Could you explicit explain why it limit to 4 qps, and more detail about below
code change with the purpose of the patch.

For each VF, the kernel PF driver assign 4 qps when the VF be created.

quoted

Fixes: c48eb308ed13 ("net/i40e: support VF request more queues")
Cc: stable@dpdk.org

Signed-off-by: Alvin Zhang <redacted>
---
  drivers/net/i40e/i40e_ethdev_vf.c | 32 ++++++++++++++++++++++++---

-----

quoted

  1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c

b/drivers/net/i40e/i40e_ethdev_vf.c

quoted

index bb5d28a..7500e0a 100644

--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c

@@ -1082,13 +1082,10 @@ static int i40evf_dev_xstats_get(struct

rte_eth_dev *dev,

quoted

  	args.out_buffer = vf->aq_resp;
  	args.out_size = I40E_AQ_BUF_SZ;

-	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);

Why interrupt handler is no need to cancel here and more why this change
is related with this patch according with the commit log?

Here, the handler has been cancecled by the caller.

quoted

  	err = i40evf_execute_vf_cmd(dev, &args);
  	if (err)
  		PMD_DRV_LOG(ERR, "fail to execute command

OP_REQUEST_QUEUES");

quoted

-	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL,
-			  i40evf_dev_alarm_handler, dev);
  	return err;
  }

@@ -1516,7 +1513,7 @@ static int i40evf_dev_xstats_get(struct

rte_eth_dev *dev,

quoted

  	hw->bus.device = pci_dev->addr.devid;
  	hw->bus.func = pci_dev->addr.function;
  	hw->hw_addr = (void *)pci_dev->mem_resource[0].addr;
-	hw->adapter_stopped = 0;
+	hw->adapter_stopped = 1;

Why it should be set stopped when init dev?

The Device has not been started until the API ` i40evf_dev_start ` been called.
Here we just initiate the device, so it should be set to 1. 

quoted

  	hw->adapter_closed = 0;

  	/* Pass the information to the rte_eth_dev_close() that it should

also

quoted

@@ -1612,16 +1609,35 @@ static int eth_i40evf_pci_remove(struct

rte_pci_device *pci_dev)

quoted

  	ad->tx_vec_allowed = true;

  	if (num_queue_pairs > vf->vsi_res->num_queue_pairs) {
-		int ret = 0;
+		struct i40e_hw *hw;
+		int ret;

+		hw  = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
  		PMD_DRV_LOG(INFO, "change queue pairs from %u to %u",
  			    vf->vsi_res->num_queue_pairs,

num_queue_pairs);

quoted

+		if (hw->adapter_stopped == 0) {
+			PMD_DRV_LOG(WARNING, "Device must be

stopped first!");

quoted

+			return -EINVAL;
+		}
+
+		rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
  		ret = i40evf_request_queues(dev, num_queue_pairs);
-		if (ret != 0)
+		if (ret)
  			return ret;

-		ret = i40evf_dev_reset(dev);
-		if (ret != 0)
+		/*
+		 * The device must be reinitiated after queue resources
+		 * changed
+		 */

Should you check below part is reinitialize process according to exist
dev_close and dev_init.

Yes, it stops and reinitializes the device , but if call the dev_close to do, some process is no needed and should report errors.

quoted

+		i40e_shutdown_adminq(hw);
+		i40evf_disable_irq0(hw);
+		rte_free(vf->vf_res);
+		vf->vf_res = NULL;
+		rte_free(vf->aq_resp);
+		vf->aq_resp = NULL;
+
+		ret = i40evf_dev_init(dev);
+		if (ret)
  			return ret;
  	}