hi, alvin
The processing of the queue pairs configure looks better than prior
version, but still have some comment.
On 7/15/2020 3:28 PM, alvinx.zh...@intel.com wrote:
From: Alvin Zhang <alvinx.zh...@intel.com>
If a VF request PF to allocate more number of queue pairs, the PF will
free the queue pairs which have been allocated and reset the VF. So,
VF should stop to work until all the process is done. This patch modify
the process of the request queue pairs. To improve efficiency and
eliminate code redundancy, the promiscuous ops were also updated.
Fixes: c48eb308ed13 ("net/i40e: support VF request more queues")
Cc: sta...@dpdk.org
Signed-off-by: Alvin Zhang <alvinx.zh...@intel.com>
---
V2: Update git log and modify codes according to comments.
V3: All the code was refactored.
---
drivers/net/i40e/i40e_ethdev_vf.c | 109 +++++++++++++++++++++-----------------
1 file changed, 59 insertions(+), 50 deletions(-)
diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
b/drivers/net/i40e/i40e_ethdev_vf.c
index eca716a..758d444 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -91,7 +91,8 @@ static int i40evf_vlan_filter_set(struct rte_eth_dev *dev,
uint16_t vlan_id, int on);
static int i40evf_vlan_offload_set(struct rte_eth_dev *dev, int mask);
static void i40evf_dev_close(struct rte_eth_dev *dev);
-static int i40evf_dev_reset(struct rte_eth_dev *dev);
+static int i40evf_dev_reset(struct rte_eth_dev *dev);
+static int i40evf_check_vf_reset_done(struct rte_eth_dev *dev);
static int i40evf_dev_promiscuous_enable(struct rte_eth_dev *dev);
static int i40evf_dev_promiscuous_disable(struct rte_eth_dev *dev);
static int i40evf_dev_allmulticast_enable(struct rte_eth_dev *dev);
@@ -519,10 +520,17 @@ struct rte_i40evf_xstats_name_off {
err = i40evf_execute_vf_cmd(dev, &args);
- if (err)
- PMD_DRV_LOG(ERR, "fail to execute command "
- "CONFIG_PROMISCUOUS_MODE");
- return err;
+ if (err) {
+ if (err == I40E_NOT_SUPPORTED)
+ return -ENOTSUP;
+
+ PMD_DRV_LOG(ERR, "Failed to set promiscuous mode");
+ return -EAGAIN;
I think below is better, please try to keep the log align in driver if
there is no special reason.
if (err) {
PMD_DRV_LOG(ERR, "fail to execute command "
"CONFIG_PROMISCUOUS_MODE");
if (err == I40E_NOT_SUPPORTED)
return -ENOTSUP;
else
return -EAGAIN;
+ }
+
+ vf->promisc_unicast_enabled = enable_unicast;
+ vf->promisc_multicast_enabled = enable_multicast;
+ return 0;
}
static int
@@ -1081,12 +1089,31 @@ static int i40evf_dev_xstats_get(struct rte_eth_dev
*dev,
args.out_size = I40E_AQ_BUF_SZ;
rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
+
err = i40evf_execute_vf_cmd(dev, &args);
- if (err)
+ if (err != I40E_SUCCESS) {
+ rte_eal_alarm_set(I40EVF_ALARM_INTERVAL,
+ i40evf_dev_alarm_handler, dev);
PMD_DRV_LOG(ERR, "fail to execute command OP_REQUEST_QUEUES");
+ return -EIO;
+ }
rte_eal_alarm_set(I40EVF_ALARM_INTERVAL,
i40evf_dev_alarm_handler, dev);
+
+ /*
+ * The PF will issue a reset to the VF when change the number of
+ * queues. The PF will set I40E_VFGEN_RSTAT to COMPLETE first, then
+ * wait 10ms and set it to ACTIVE. In this duration, vf may not catch
+ * the moment that COMPLETE is set. So, for vf, we'll try to wait a
+ * long time.
+ */
+ rte_delay_ms(100);
+
+ err = i40evf_check_vf_reset_done(dev);
+ if (err)
+ PMD_DRV_LOG(ERR, "VF is still resetting");
+
Please keep the return value and eliminate the duplicate, do you think
below is better, you could ref
err = i40evf_execute_vf_cmd(dev, &args);
rte_eal_alarm_set(I40EVF_ALARM_INTERVAL,
i40evf_dev_alarm_handler, dev);
if (!err) {
check vf reset done....
} else {
PMD_DRV_LOG(ERR, "fail to execute command OP_REQUEST_QUEUES");
}
return err;
}
@@ -1514,7 +1541,7 @@ static int i40evf_dev_xstats_get(struct rte_eth_dev *dev,
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;
hw->adapter_closed = 0;
/* Pass the information to the rte_eth_dev_close() that it should also
@@ -1610,10 +1637,27 @@ static int eth_i40evf_pci_remove(struct rte_pci_device
*pci_dev)
ad->tx_vec_allowed = true;
if (num_queue_pairs > vf->vsi_res->num_queue_pairs) {
- int ret = 0;
+ struct i40e_hw *hw;
+ int ret;
+
+ /*
+ * All VF resources will be reallocated, so change queue pairs
+ * in secondary processes is forbidden.
+ */
Please don't use an empty /* line, use /* Comment....
And seems that the word "forbidden" is not very make sense. you could
ref other place about secondary process checking, simply like below
/ * for secondary processes, we don't configure queue pairs
any further
* as primary has already done this work.
*/
+ if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+ PMD_DRV_LOG(ERR,
+ "For secondary processes, change queue pairs is
forbidden!");
Alignment should match open parenthesis, you could use the checkpatch.pl
to check it.
Add please check if below i40evf_init_vlan also no need in secondary
process. If so, i suggest split this part into other specific fixing
patch for secondary process configuration.
+ return -ENOTSUP;
+ }
+ 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);
+ if (hw->adapter_stopped == 0) {
+ PMD_DRV_LOG(WARNING, "Device must be stopped first!");
+ return -EINVAL;
I think ERR but not warning should be return here, and do you think -EBUSY
should be better than -EINVAL?
+ }
+
ret = i40evf_request_queues(dev, num_queue_pairs);
if (ret != 0)
return ret;
@@ -2182,68 +2226,32 @@ static int eth_i40evf_pci_remove(struct rte_pci_device
*pci_dev)
i40evf_dev_promiscuous_enable(struct rte_eth_dev *dev)
{
struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
- int ret;
-
- ret = i40evf_config_promisc(dev, 1, vf->promisc_multicast_enabled);
- if (ret == 0)
- vf->promisc_unicast_enabled = TRUE;
- else if (ret == I40E_NOT_SUPPORTED)
- ret = -ENOTSUP;
- else
- ret = -EAGAIN;
- return ret;
+ return i40evf_config_promisc(dev, true, vf->promisc_multicast_enabled);
}
static int
i40evf_dev_promiscuous_disable(struct rte_eth_dev *dev)
{
struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
- int ret;
-
- ret = i40evf_config_promisc(dev, 0, vf->promisc_multicast_enabled);
- if (ret == 0)
- vf->promisc_unicast_enabled = FALSE;
- else if (ret == I40E_NOT_SUPPORTED)
- ret = -ENOTSUP;
- else
- ret = -EAGAIN;
- return ret;
+ return i40evf_config_promisc(dev, false, vf->promisc_multicast_enabled);
}
static int
i40evf_dev_allmulticast_enable(struct rte_eth_dev *dev)
{
struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
- int ret;
-
- ret = i40evf_config_promisc(dev, vf->promisc_unicast_enabled, 1);
- if (ret == 0)
- vf->promisc_multicast_enabled = TRUE;
- else if (ret == I40E_NOT_SUPPORTED)
- ret = -ENOTSUP;
- else
- ret = -EAGAIN;
- return ret;
+ return i40evf_config_promisc(dev, vf->promisc_unicast_enabled, true);
}
static int
i40evf_dev_allmulticast_disable(struct rte_eth_dev *dev)
{
struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
- int ret;
-
- ret = i40evf_config_promisc(dev, vf->promisc_unicast_enabled, 0);
- if (ret == 0)
- vf->promisc_multicast_enabled = FALSE;
- else if (ret == I40E_NOT_SUPPORTED)
- ret = -ENOTSUP;
- else
- ret = -EAGAIN;
- return ret;
+ return i40evf_config_promisc(dev, vf->promisc_unicast_enabled, false);
}
static int
@@ -2365,8 +2373,9 @@ static int eth_i40evf_pci_remove(struct rte_pci_device
*pci_dev)
* it is a workaround solution when work with kernel driver
* and it is not the normal way
*/
- i40evf_dev_promiscuous_disable(dev);
- i40evf_dev_allmulticast_disable(dev);
+ if (vf->promisc_unicast_enabled || vf->promisc_multicast_enabled)
+ i40evf_config_promisc(dev, false, false);
Need checking the return status when i40evf_config_promisc return failed?
+
rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
i40evf_reset_vf(dev);
