DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/i40e: fix modifying the number of queues
@ 2020-06-10 12:07 alvinx.zhang
  2020-06-21 13:36 ` Jeff Guo
  2020-07-02  3:26 ` [dpdk-dev] [PATCH v2] net/i40e: fix modify the number of qps in VF alvinx.zhang
  0 siblings, 2 replies; 16+ messages in thread
From: alvinx.zhang @ 2020-06-10 12:07 UTC (permalink / raw)
  To: dev; +Cc: stable, beilei.xing, jia.guo, maox.jiang

From: Alvin Zhang <alvinx.zhang@intel.com>

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`.

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

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---
 drivers/net/i40e/i40e_ethdev_vf.c | 32 ++++++++++++++++++++++++--------
 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
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,
 	args.out_buffer = vf->aq_resp;
 	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)
 		PMD_DRV_LOG(ERR, "fail to execute command OP_REQUEST_QUEUES");
 
-	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,
 	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
@@ -1612,16 +1609,35 @@ 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;
 
+		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;
+		}
+
+		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
+		 */
+		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;
 	}
 
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] net/i40e: fix modifying the number of queues
  2020-06-10 12:07 [dpdk-dev] [PATCH] net/i40e: fix modifying the number of queues alvinx.zhang
@ 2020-06-21 13:36 ` Jeff Guo
  2020-06-29  3:16   ` Zhang, AlvinX
  2020-07-02  3:26 ` [dpdk-dev] [PATCH v2] net/i40e: fix modify the number of qps in VF alvinx.zhang
  1 sibling, 1 reply; 16+ messages in thread
From: Jeff Guo @ 2020-06-21 13:36 UTC (permalink / raw)
  To: alvinx.zhang, dev; +Cc: stable, beilei.xing, maox.jiang

hi, alvin

On 6/10/2020 8:07 PM, alvinx.zhang@intel.com wrote:
> From: Alvin Zhang <alvinx.zhang@intel.com>
>
> 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.


> Fixes: c48eb308ed13 ("net/i40e: support VF request more queues")
> Cc: stable@dpdk.org
>
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> ---
>   drivers/net/i40e/i40e_ethdev_vf.c | 32 ++++++++++++++++++++++++--------
>   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
> 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,
>   	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?


>   	err = i40evf_execute_vf_cmd(dev, &args);
>   	if (err)
>   		PMD_DRV_LOG(ERR, "fail to execute command OP_REQUEST_QUEUES");
>   
> -	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,
>   	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?


>   	hw->adapter_closed = 0;
>   
>   	/* Pass the information to the rte_eth_dev_close() that it should also
> @@ -1612,16 +1609,35 @@ 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;
>   
> +		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;
> +		}
> +
> +		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.


> +		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;
>   	}
>   

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] net/i40e: fix modifying the number of queues
  2020-06-21 13:36 ` Jeff Guo
@ 2020-06-29  3:16   ` Zhang, AlvinX
  2020-06-30  3:13     ` Jeff Guo
  0 siblings, 1 reply; 16+ messages in thread
From: Zhang, AlvinX @ 2020-06-29  3:16 UTC (permalink / raw)
  To: Guo, Jia, dev; +Cc: stable, Xing, Beilei, Jiang, MaoX

Hi Jia,

> -----Original Message-----
> From: Guo, Jia
> Sent: Sunday, June 21, 2020 9:36 PM
> To: Zhang, AlvinX <alvinx.zhang@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Jiang, MaoX
> <maox.jiang@intel.com>
> 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:
> > From: Alvin Zhang <alvinx.zhang@intel.com>
> >
> > 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.

> 
> 
> > Fixes: c48eb308ed13 ("net/i40e: support VF request more queues")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > ---
> >   drivers/net/i40e/i40e_ethdev_vf.c | 32 ++++++++++++++++++++++++---
> -----
> >   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
> > 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,
> >   	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.

> 
> 
> >   	err = i40evf_execute_vf_cmd(dev, &args);
> >   	if (err)
> >   		PMD_DRV_LOG(ERR, "fail to execute command
> OP_REQUEST_QUEUES");
> >
> > -	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,
> >   	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. 

> 
> 
> >   	hw->adapter_closed = 0;
> >
> >   	/* Pass the information to the rte_eth_dev_close() that it should
> also
> > @@ -1612,16 +1609,35 @@ 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;
> >
> > +		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;
> > +		}
> > +
> > +		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.

> 
> 
> > +		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;
> >   	}
> >

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH] net/i40e: fix modifying the number of queues
  2020-06-29  3:16   ` Zhang, AlvinX
@ 2020-06-30  3:13     ` Jeff Guo
  0 siblings, 0 replies; 16+ messages in thread
From: Jeff Guo @ 2020-06-30  3:13 UTC (permalink / raw)
  To: Zhang, AlvinX, dev; +Cc: stable, Xing, Beilei, Jiang, MaoX

hi, alvin

On 6/29/2020 11:16 AM, Zhang, AlvinX wrote:
> Hi Jia,
>
>> -----Original Message-----
>> From: Guo, Jia
>> Sent: Sunday, June 21, 2020 9:36 PM
>> To: Zhang, AlvinX <alvinx.zhang@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Jiang, MaoX
>> <maox.jiang@intel.com>
>> 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:
>>> From: Alvin Zhang <alvinx.zhang@intel.com>
>>>
>>> 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.


It would be better also add the detail info replace of "updates".


>>
>>> Fixes: c48eb308ed13 ("net/i40e: support VF request more queues")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
>>> ---
>>>    drivers/net/i40e/i40e_ethdev_vf.c | 32 ++++++++++++++++++++++++---
>> -----
>>>    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
>>> 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,
>>>    	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.


  If it related with this patch please add the fix info into the commit 
log and delete the useless statement in the begin.


>>
>>>    	err = i40evf_execute_vf_cmd(dev, &args);
>>>    	if (err)
>>>    		PMD_DRV_LOG(ERR, "fail to execute command
>> OP_REQUEST_QUEUES");
>>> -	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,
>>>    	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.


make sense, and what about below "hw->adapter_closed = 0;", should it be 
after the success of the init process.


>>
>>>    	hw->adapter_closed = 0;
>>>
>>>    	/* Pass the information to the rte_eth_dev_close() that it should
>> also
>>> @@ -1612,16 +1609,35 @@ 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;
>>>
>>> +		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;
>>> +		}
>>> +
>>> +		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.


When close dev, it will stop dev and free queues, but you don't involve 
the process of free queues here,  and you check the 
"hw->adapter_stopped" before, so if it had stopped and then close, why 
it will report some errors of some useless process?


>>
>>> +		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;
>>>    	}
>>>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-dev] [PATCH v2] net/i40e: fix modify the number of qps in VF
  2020-06-10 12:07 [dpdk-dev] [PATCH] net/i40e: fix modifying the number of queues alvinx.zhang
  2020-06-21 13:36 ` Jeff Guo
@ 2020-07-02  3:26 ` alvinx.zhang
  2020-07-15  7:28   ` [dpdk-dev] [PATCH v3] " alvinx.zhang
  1 sibling, 1 reply; 16+ messages in thread
From: alvinx.zhang @ 2020-07-02  3:26 UTC (permalink / raw)
  To: dev; +Cc: stable, beilei.xing, jia.guo, maox.jiang

From: Alvin Zhang <alvinx.zhang@intel.com>

For a VF, if the number of request queue pairs is greater than that
has been allocated by the kernel driver, the app may fail to start.
This patch modify the request queue process.

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

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---

V2: Update git log and modify codes according to comments.
---
 drivers/net/i40e/i40e_ethdev_vf.c | 57 +++++++++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 14 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index eca716a..55dd237 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -90,6 +90,7 @@ static int i40evf_dev_xstats_get_names(struct rte_eth_dev *dev,
 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_release(struct rte_eth_dev *dev);
 static void i40evf_dev_close(struct rte_eth_dev *dev);
 static int  i40evf_dev_reset(struct rte_eth_dev *dev);
 static int i40evf_dev_promiscuous_enable(struct rte_eth_dev *dev);
@@ -1080,13 +1081,10 @@ static int i40evf_dev_xstats_get(struct rte_eth_dev *dev,
 	args.out_buffer = vf->aq_resp;
 	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)
 		PMD_DRV_LOG(ERR, "fail to execute command OP_REQUEST_QUEUES");
 
-	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL,
-			  i40evf_dev_alarm_handler, dev);
 	return err;
 }
 
@@ -1514,7 +1512,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,16 +1608,39 @@ 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.
+		 */
+		if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+			PMD_DRV_LOG(ERR,
+				"For secondary processes, change queue pairs is forbidden!");
+			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;
+		}
+
+		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 has been reseted after queue resources changed
+		 * and must be reinitiated.
+		 */
+		i40evf_dev_release(dev);
+		ret = i40evf_dev_init(dev);
+		if (ret)
 			return ret;
 	}
 
@@ -2356,10 +2377,7 @@ static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
 i40evf_dev_close(struct rte_eth_dev *dev)
 {
 	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
-
 	i40evf_dev_stop(dev);
-	i40e_dev_free_queues(dev);
 	/*
 	 * disable promiscuous mode before reset vf
 	 * it is a workaround solution when work with kernel driver
@@ -2370,19 +2388,30 @@ static int eth_i40evf_pci_remove(struct rte_pci_device *pci_dev)
 	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
 
 	i40evf_reset_vf(dev);
-	i40e_shutdown_adminq(hw);
-	i40evf_disable_irq0(hw);
+	i40evf_dev_release(dev);
 
 	dev->dev_ops = NULL;
 	dev->rx_pkt_burst = NULL;
 	dev->tx_pkt_burst = NULL;
 
+	hw->adapter_closed = 1;
+}
+
+static void
+i40evf_dev_release(struct rte_eth_dev *dev)
+{
+	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
+
+	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;
 
-	hw->adapter_closed = 1;
+	i40e_dev_free_queues(dev);
 }
 
 /*
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-dev] [PATCH v3] net/i40e: fix modify the number of qps in VF
  2020-07-02  3:26 ` [dpdk-dev] [PATCH v2] net/i40e: fix modify the number of qps in VF alvinx.zhang
@ 2020-07-15  7:28   ` alvinx.zhang
  2020-07-15 10:17     ` Jeff Guo
  2020-07-16  1:52     ` [dpdk-dev] [PATCH v4] " alvinx.zhang
  0 siblings, 2 replies; 16+ messages in thread
From: alvinx.zhang @ 2020-07-15  7:28 UTC (permalink / raw)
  To: jia.guo; +Cc: dev, beilei.xing

From: Alvin Zhang <alvinx.zhang@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: stable@dpdk.org

Signed-off-by: Alvin Zhang <alvinx.zhang@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;
+	}
+
+	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");
+
 	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.
+		 */
+		if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+			PMD_DRV_LOG(ERR,
+				"For secondary processes, change queue pairs is forbidden!");
+			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;
+		}
+
 		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);
+
 	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
 
 	i40evf_reset_vf(dev);
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v3] net/i40e: fix modify the number of qps in VF
  2020-07-15  7:28   ` [dpdk-dev] [PATCH v3] " alvinx.zhang
@ 2020-07-15 10:17     ` Jeff Guo
  2020-07-16  1:38       ` Zhang, AlvinX
  2020-07-16  1:52     ` [dpdk-dev] [PATCH v4] " alvinx.zhang
  1 sibling, 1 reply; 16+ messages in thread
From: Jeff Guo @ 2020-07-15 10:17 UTC (permalink / raw)
  To: alvinx.zhang; +Cc: dev, beilei.xing

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.zhang@intel.com wrote:
> From: Alvin Zhang <alvinx.zhang@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: stable@dpdk.org
>
> Signed-off-by: Alvin Zhang <alvinx.zhang@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);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v3] net/i40e: fix modify the number of qps in VF
  2020-07-15 10:17     ` Jeff Guo
@ 2020-07-16  1:38       ` Zhang, AlvinX
  0 siblings, 0 replies; 16+ messages in thread
From: Zhang, AlvinX @ 2020-07-16  1:38 UTC (permalink / raw)
  To: Guo, Jia; +Cc: dev, Xing, Beilei

Hi Guojia,

Thanks a lot.
I'll update it.

BR,
Alvin

> -----Original Message-----
> From: Guo, Jia <jia.guo@intel.com>
> Sent: Wednesday, July 15, 2020 6:17 PM
> To: Zhang, AlvinX <alvinx.zhang@intel.com>
> Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>
> Subject: Re: [PATCH v3] net/i40e: fix modify the number of qps in VF
> 
> 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.zhang@intel.com wrote:
> > From: Alvin Zhang <alvinx.zhang@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: stable@dpdk.org
> >
> > Signed-off-by: Alvin Zhang <alvinx.zhang@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);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-dev] [PATCH v4] net/i40e: fix modify the number of qps in VF
  2020-07-15  7:28   ` [dpdk-dev] [PATCH v3] " alvinx.zhang
  2020-07-15 10:17     ` Jeff Guo
@ 2020-07-16  1:52     ` alvinx.zhang
  2020-07-16  4:24       ` [dpdk-dev] [PATCH v5] " alvinx.zhang
  1 sibling, 1 reply; 16+ messages in thread
From: alvinx.zhang @ 2020-07-16  1:52 UTC (permalink / raw)
  To: jia.guo; +Cc: beilei.xing, dev, Alvin Zhang, stable

From: Alvin Zhang <alvinx.zhang@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: stable@dpdk.org

Signed-off-by: Alvin Zhang <alvinx.zhang@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 | 106 +++++++++++++++++++++-----------------
 1 file changed, 58 insertions(+), 48 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index eca716a..faff77e 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,19 @@ struct rte_i40evf_xstats_name_off {
 
 	err = i40evf_execute_vf_cmd(dev, &args);
 
-	if (err)
+	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command "
 			    "CONFIG_PROMISCUOUS_MODE");
-	return err;
+
+		if (err == I40E_NOT_SUPPORTED)
+			return -ENOTSUP;
+
+		return -EAGAIN;
+	}
+
+	vf->promisc_unicast_enabled = enable_unicast;
+	vf->promisc_multicast_enabled = enable_multicast;
+	return 0;
 }
 
 static int
@@ -1081,12 +1091,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 err;
+	}
 
 	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");
+
 	return err;
 }
 
@@ -1514,7 +1543,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 +1639,26 @@ 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;
+
+		/* 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 not supported!");
+			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(ERR, "Device must be stopped first!");
+			return -EBUSY;
+		}
+
 		ret = i40evf_request_queues(dev, num_queue_pairs);
 		if (ret != 0)
 			return ret;
@@ -2182,68 +2227,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 +2374,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);
+
 	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
 
 	i40evf_reset_vf(dev);
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-dev] [PATCH v5] net/i40e: fix modify the number of qps in VF
  2020-07-16  1:52     ` [dpdk-dev] [PATCH v4] " alvinx.zhang
@ 2020-07-16  4:24       ` alvinx.zhang
  2020-07-16  4:57         ` Yang, Qiming
  2020-07-16  5:12         ` alvinx.zhang
  0 siblings, 2 replies; 16+ messages in thread
From: alvinx.zhang @ 2020-07-16  4:24 UTC (permalink / raw)
  To: jia.guo; +Cc: beilei.xing, dev, Alvin Zhang, stable

From: Alvin Zhang <alvinx.zhang@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: stable@dpdk.org

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---

V2: Update git log and modify codes according to comments.
V3: All the code was refactored.
V4, V5: Modify codes according to comments.
---
 drivers/net/i40e/i40e_ethdev_vf.c | 104 ++++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 50 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index eca716a..69cab8e 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,19 @@ struct rte_i40evf_xstats_name_off {
 
 	err = i40evf_execute_vf_cmd(dev, &args);
 
-	if (err)
+	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command "
 			    "CONFIG_PROMISCUOUS_MODE");
-	return err;
+
+		if (err == I40E_NOT_SUPPORTED)
+			return -ENOTSUP;
+
+		return -EAGAIN;
+	}
+
+	vf->promisc_unicast_enabled = enable_unicast;
+	vf->promisc_multicast_enabled = enable_multicast;
+	return 0;
 }
 
 static int
@@ -1081,12 +1091,28 @@ 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)
+
+	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL, i40evf_dev_alarm_handler, dev);
+
+	if (err != I40E_SUCCESS) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_REQUEST_QUEUES");
+		return err;
+	}
+
+	/* 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");
 
-	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL,
-			  i40evf_dev_alarm_handler, dev);
 	return err;
 }
 
@@ -1514,7 +1540,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,7 +1636,20 @@ 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;
+
+		if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+			PMD_DRV_LOG(ERR,
+				    "For secondary processes, change queue pairs is not supported!");
+			return -ENOTSUP;
+		}
+
+		hw  = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+		if (!hw->adapter_stopped) {
+			PMD_DRV_LOG(ERR, "Device must be stopped first!");
+			return -EBUSY;
+		}
 
 		PMD_DRV_LOG(INFO, "change queue pairs from %u to %u",
 			    vf->vsi_res->num_queue_pairs, num_queue_pairs);
@@ -2182,68 +2221,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 +2368,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);
+
 	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
 
 	i40evf_reset_vf(dev);
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v5] net/i40e: fix modify the number of qps in VF
  2020-07-16  4:24       ` [dpdk-dev] [PATCH v5] " alvinx.zhang
@ 2020-07-16  4:57         ` Yang, Qiming
  2020-07-16  5:13           ` Zhang, AlvinX
  2020-07-16  5:12         ` alvinx.zhang
  1 sibling, 1 reply; 16+ messages in thread
From: Yang, Qiming @ 2020-07-16  4:57 UTC (permalink / raw)
  To: Zhang, AlvinX, Guo, Jia; +Cc: Xing, Beilei, dev, Zhang, AlvinX, stable



> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of alvinx.zhang@intel.com
> Sent: Thursday, July 16, 2020 12:24
> To: Guo, Jia <jia.guo@intel.com>
> Cc: Xing, Beilei <beilei.xing@intel.com>; dev@dpdk.org; Zhang, AlvinX
> <alvinx.zhang@intel.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v5] net/i40e: fix modify the number of qps in VF

Fix and modify are all verbs, I think you want to fix the queue pair modify process.

> From: Alvin Zhang <alvinx.zhang@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: stable@dpdk.org
> 
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> ---
> 
> V2: Update git log and modify codes according to comments.
> V3: All the code was refactored.
> V4, V5: Modify codes according to comments.
> ---
>  drivers/net/i40e/i40e_ethdev_vf.c | 104 ++++++++++++++++++++-----------
> -------
>  1 file changed, 54 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> b/drivers/net/i40e/i40e_ethdev_vf.c
> index eca716a..69cab8e 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,19 @@ struct rte_i40evf_xstats_name_off {
> 
>  	err = i40evf_execute_vf_cmd(dev, &args);
> 
> -	if (err)
> +	if (err) {
>  		PMD_DRV_LOG(ERR, "fail to execute command "
>  			    "CONFIG_PROMISCUOUS_MODE");
> -	return err;
> +
> +		if (err == I40E_NOT_SUPPORTED)
> +			return -ENOTSUP;
> +
> +		return -EAGAIN;
> +	}
> +
> +	vf->promisc_unicast_enabled = enable_unicast;
> +	vf->promisc_multicast_enabled = enable_multicast;
> +	return 0;
>  }
> 
>  static int
> @@ -1081,12 +1091,28 @@ 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)
> +
> +	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL,
> i40evf_dev_alarm_handler,
> +dev);
> +
> +	if (err != I40E_SUCCESS) {
>  		PMD_DRV_LOG(ERR, "fail to execute command
> OP_REQUEST_QUEUES");
> +		return err;
> +	}
> +
> +	/* 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");
> 
> -	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL,
> -			  i40evf_dev_alarm_handler, dev);
>  	return err;
>  }
> 
> @@ -1514,7 +1540,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,7 +1636,20 @@ 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;
> +
> +		if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> +			PMD_DRV_LOG(ERR,
> +				    "For secondary processes, change queue
> pairs is not supported!");
> +			return -ENOTSUP;
> +		}
> +
> +		hw  = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +		if (!hw->adapter_stopped) {
> +			PMD_DRV_LOG(ERR, "Device must be stopped
> first!");
> +			return -EBUSY;
> +		}
> 
>  		PMD_DRV_LOG(INFO, "change queue pairs from %u to %u",
>  			    vf->vsi_res->num_queue_pairs,
> num_queue_pairs); @@ -2182,68 +2221,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 +2368,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);
> +
>  	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
> 
>  	i40evf_reset_vf(dev);
> --
> 1.8.3.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-dev] [PATCH v5] net/i40e: fix modify the number of qps in VF
  2020-07-16  4:24       ` [dpdk-dev] [PATCH v5] " alvinx.zhang
  2020-07-16  4:57         ` Yang, Qiming
@ 2020-07-16  5:12         ` alvinx.zhang
  2020-07-16  6:33           ` [dpdk-dev] [PATCH v5] net/i40e: fix qps configuration " alvinx.zhang
  1 sibling, 1 reply; 16+ messages in thread
From: alvinx.zhang @ 2020-07-16  5:12 UTC (permalink / raw)
  To: jia.guo; +Cc: beilei.xing, dev, Alvin Zhang, stable

From: Alvin Zhang <alvinx.zhang@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: stable@dpdk.org

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---

V2: Update git log and modify codes according to comments.
V3: All the code was refactored.
V4, V5: Modify codes according to comments.
---
 drivers/net/i40e/i40e_ethdev_vf.c | 104 ++++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 50 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index eca716a..69cab8e 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,19 @@ struct rte_i40evf_xstats_name_off {
 
 	err = i40evf_execute_vf_cmd(dev, &args);
 
-	if (err)
+	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command "
 			    "CONFIG_PROMISCUOUS_MODE");
-	return err;
+
+		if (err == I40E_NOT_SUPPORTED)
+			return -ENOTSUP;
+
+		return -EAGAIN;
+	}
+
+	vf->promisc_unicast_enabled = enable_unicast;
+	vf->promisc_multicast_enabled = enable_multicast;
+	return 0;
 }
 
 static int
@@ -1081,12 +1091,28 @@ 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)
+
+	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL, i40evf_dev_alarm_handler, dev);
+
+	if (err != I40E_SUCCESS) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_REQUEST_QUEUES");
+		return err;
+	}
+
+	/* 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");
 
-	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL,
-			  i40evf_dev_alarm_handler, dev);
 	return err;
 }
 
@@ -1514,7 +1540,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,7 +1636,20 @@ 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;
+
+		if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+			PMD_DRV_LOG(ERR,
+				    "For secondary processes, change queue pairs is not supported!");
+			return -ENOTSUP;
+		}
+
+		hw  = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+		if (!hw->adapter_stopped) {
+			PMD_DRV_LOG(ERR, "Device must be stopped first!");
+			return -EBUSY;
+		}
 
 		PMD_DRV_LOG(INFO, "change queue pairs from %u to %u",
 			    vf->vsi_res->num_queue_pairs, num_queue_pairs);
@@ -2182,68 +2221,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 +2368,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);
+
 	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
 
 	i40evf_reset_vf(dev);
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v5] net/i40e: fix modify the number of qps in VF
  2020-07-16  4:57         ` Yang, Qiming
@ 2020-07-16  5:13           ` Zhang, AlvinX
  0 siblings, 0 replies; 16+ messages in thread
From: Zhang, AlvinX @ 2020-07-16  5:13 UTC (permalink / raw)
  To: Yang, Qiming, Guo, Jia; +Cc: Xing, Beilei, dev, stable

Thanks Qiming,
I have updated it and resubmitted the patch.

> -----Original Message-----
> From: Yang, Qiming <qiming.yang@intel.com>
> Sent: Thursday, July 16, 2020 12:58 PM
> To: Zhang, AlvinX <alvinx.zhang@intel.com>; Guo, Jia <jia.guo@intel.com>
> Cc: Xing, Beilei <beilei.xing@intel.com>; dev@dpdk.org; Zhang, AlvinX
> <alvinx.zhang@intel.com>; stable@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v5] net/i40e: fix modify the number of qps in VF
> 
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of alvinx.zhang@intel.com
> > Sent: Thursday, July 16, 2020 12:24
> > To: Guo, Jia <jia.guo@intel.com>
> > Cc: Xing, Beilei <beilei.xing@intel.com>; dev@dpdk.org; Zhang, AlvinX
> > <alvinx.zhang@intel.com>; stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH v5] net/i40e: fix modify the number of qps
> > in VF
> 
> Fix and modify are all verbs, I think you want to fix the queue pair modify
> process.
> 
> > From: Alvin Zhang <alvinx.zhang@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: stable@dpdk.org
> >
> > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> > ---
> >
> > V2: Update git log and modify codes according to comments.
> > V3: All the code was refactored.
> > V4, V5: Modify codes according to comments.
> > ---
> >  drivers/net/i40e/i40e_ethdev_vf.c | 104
> > ++++++++++++++++++++-----------
> > -------
> >  1 file changed, 54 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > b/drivers/net/i40e/i40e_ethdev_vf.c
> > index eca716a..69cab8e 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,19 @@ struct rte_i40evf_xstats_name_off {
> >
> >  	err = i40evf_execute_vf_cmd(dev, &args);
> >
> > -	if (err)
> > +	if (err) {
> >  		PMD_DRV_LOG(ERR, "fail to execute command "
> >  			    "CONFIG_PROMISCUOUS_MODE");
> > -	return err;
> > +
> > +		if (err == I40E_NOT_SUPPORTED)
> > +			return -ENOTSUP;
> > +
> > +		return -EAGAIN;
> > +	}
> > +
> > +	vf->promisc_unicast_enabled = enable_unicast;
> > +	vf->promisc_multicast_enabled = enable_multicast;
> > +	return 0;
> >  }
> >
> >  static int
> > @@ -1081,12 +1091,28 @@ 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)
> > +
> > +	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL,
> > i40evf_dev_alarm_handler,
> > +dev);
> > +
> > +	if (err != I40E_SUCCESS) {
> >  		PMD_DRV_LOG(ERR, "fail to execute command
> OP_REQUEST_QUEUES");
> > +		return err;
> > +	}
> > +
> > +	/* 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");
> >
> > -	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL,
> > -			  i40evf_dev_alarm_handler, dev);
> >  	return err;
> >  }
> >
> > @@ -1514,7 +1540,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,7 +1636,20 @@ 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;
> > +
> > +		if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> > +			PMD_DRV_LOG(ERR,
> > +				    "For secondary processes, change queue
> > pairs is not supported!");
> > +			return -ENOTSUP;
> > +		}
> > +
> > +		hw  = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > +		if (!hw->adapter_stopped) {
> > +			PMD_DRV_LOG(ERR, "Device must be stopped
> > first!");
> > +			return -EBUSY;
> > +		}
> >
> >  		PMD_DRV_LOG(INFO, "change queue pairs from %u to %u",
> >  			    vf->vsi_res->num_queue_pairs,
> > num_queue_pairs); @@ -2182,68 +2221,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 +2368,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);
> > +
> >  	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
> >
> >  	i40evf_reset_vf(dev);
> > --
> > 1.8.3.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [dpdk-dev] [PATCH v5] net/i40e: fix qps configuration in VF
  2020-07-16  5:12         ` alvinx.zhang
@ 2020-07-16  6:33           ` alvinx.zhang
  2020-07-16  7:30             ` Jeff Guo
  0 siblings, 1 reply; 16+ messages in thread
From: alvinx.zhang @ 2020-07-16  6:33 UTC (permalink / raw)
  To: jia.guo, qiming.yang; +Cc: dev, Alvin Zhang, stable

From: Alvin Zhang <alvinx.zhang@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: stable@dpdk.org

Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
---

V2: Update git log and modify codes according to comments.
V3: All the code was refactored.
V4, V5: Modify codes according to comments.
---
 drivers/net/i40e/i40e_ethdev_vf.c | 104 ++++++++++++++++++++------------------
 1 file changed, 54 insertions(+), 50 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index eca716a..69cab8e 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,19 @@ struct rte_i40evf_xstats_name_off {
 
 	err = i40evf_execute_vf_cmd(dev, &args);
 
-	if (err)
+	if (err) {
 		PMD_DRV_LOG(ERR, "fail to execute command "
 			    "CONFIG_PROMISCUOUS_MODE");
-	return err;
+
+		if (err == I40E_NOT_SUPPORTED)
+			return -ENOTSUP;
+
+		return -EAGAIN;
+	}
+
+	vf->promisc_unicast_enabled = enable_unicast;
+	vf->promisc_multicast_enabled = enable_multicast;
+	return 0;
 }
 
 static int
@@ -1081,12 +1091,28 @@ 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)
+
+	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL, i40evf_dev_alarm_handler, dev);
+
+	if (err != I40E_SUCCESS) {
 		PMD_DRV_LOG(ERR, "fail to execute command OP_REQUEST_QUEUES");
+		return err;
+	}
+
+	/* 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");
 
-	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL,
-			  i40evf_dev_alarm_handler, dev);
 	return err;
 }
 
@@ -1514,7 +1540,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,7 +1636,20 @@ 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;
+
+		if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+			PMD_DRV_LOG(ERR,
+				    "For secondary processes, change queue pairs is not supported!");
+			return -ENOTSUP;
+		}
+
+		hw  = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+		if (!hw->adapter_stopped) {
+			PMD_DRV_LOG(ERR, "Device must be stopped first!");
+			return -EBUSY;
+		}
 
 		PMD_DRV_LOG(INFO, "change queue pairs from %u to %u",
 			    vf->vsi_res->num_queue_pairs, num_queue_pairs);
@@ -2182,68 +2221,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 +2368,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);
+
 	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
 
 	i40evf_reset_vf(dev);
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v5] net/i40e: fix qps configuration in VF
  2020-07-16  6:33           ` [dpdk-dev] [PATCH v5] net/i40e: fix qps configuration " alvinx.zhang
@ 2020-07-16  7:30             ` Jeff Guo
  2020-07-16  9:14               ` Zhang, Qi Z
  0 siblings, 1 reply; 16+ messages in thread
From: Jeff Guo @ 2020-07-16  7:30 UTC (permalink / raw)
  To: alvinx.zhang, qiming.yang; +Cc: dev, stable

Acked-by: Jeff Guo <jia.guo@intel.com>

On 7/16/2020 2:33 PM, alvinx.zhang@intel.com wrote:
> From: Alvin Zhang <alvinx.zhang@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: stable@dpdk.org
>
> Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>
> ---
>
> V2: Update git log and modify codes according to comments.
> V3: All the code was refactored.
> V4, V5: Modify codes according to comments.
> ---
>   drivers/net/i40e/i40e_ethdev_vf.c | 104 ++++++++++++++++++++------------------
>   1 file changed, 54 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
> index eca716a..69cab8e 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,19 @@ struct rte_i40evf_xstats_name_off {
>   
>   	err = i40evf_execute_vf_cmd(dev, &args);
>   
> -	if (err)
> +	if (err) {
>   		PMD_DRV_LOG(ERR, "fail to execute command "
>   			    "CONFIG_PROMISCUOUS_MODE");
> -	return err;
> +
> +		if (err == I40E_NOT_SUPPORTED)
> +			return -ENOTSUP;
> +
> +		return -EAGAIN;
> +	}
> +
> +	vf->promisc_unicast_enabled = enable_unicast;
> +	vf->promisc_multicast_enabled = enable_multicast;
> +	return 0;
>   }
>   
>   static int
> @@ -1081,12 +1091,28 @@ 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)
> +
> +	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL, i40evf_dev_alarm_handler, dev);
> +
> +	if (err != I40E_SUCCESS) {
>   		PMD_DRV_LOG(ERR, "fail to execute command OP_REQUEST_QUEUES");
> +		return err;
> +	}
> +
> +	/* 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");
>   
> -	rte_eal_alarm_set(I40EVF_ALARM_INTERVAL,
> -			  i40evf_dev_alarm_handler, dev);
>   	return err;
>   }
>   
> @@ -1514,7 +1540,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,7 +1636,20 @@ 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;
> +
> +		if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> +			PMD_DRV_LOG(ERR,
> +				    "For secondary processes, change queue pairs is not supported!");
> +			return -ENOTSUP;
> +		}
> +
> +		hw  = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +		if (!hw->adapter_stopped) {
> +			PMD_DRV_LOG(ERR, "Device must be stopped first!");
> +			return -EBUSY;
> +		}
>   
>   		PMD_DRV_LOG(INFO, "change queue pairs from %u to %u",
>   			    vf->vsi_res->num_queue_pairs, num_queue_pairs);
> @@ -2182,68 +2221,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 +2368,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);
> +
>   	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
>   
>   	i40evf_reset_vf(dev);

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [dpdk-dev] [PATCH v5] net/i40e: fix qps configuration in VF
  2020-07-16  7:30             ` Jeff Guo
@ 2020-07-16  9:14               ` Zhang, Qi Z
  0 siblings, 0 replies; 16+ messages in thread
From: Zhang, Qi Z @ 2020-07-16  9:14 UTC (permalink / raw)
  To: Guo, Jia, Zhang, AlvinX, Yang, Qiming; +Cc: dev, stable



> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Jeff Guo
> Sent: Thursday, July 16, 2020 3:31 PM
> To: Zhang, AlvinX <alvinx.zhang@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v5] net/i40e: fix qps configuration in VF
> 
> Acked-by: Jeff Guo <jia.guo@intel.com>
> 
> On 7/16/2020 2:33 PM, alvinx.zhang@intel.com wrote:
> > From: Alvin Zhang <alvinx.zhang@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: stable@dpdk.org
> >
> > Signed-off-by: Alvin Zhang <alvinx.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-07-16  9:14 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-10 12:07 [dpdk-dev] [PATCH] net/i40e: fix modifying the number of queues alvinx.zhang
2020-06-21 13:36 ` Jeff Guo
2020-06-29  3:16   ` Zhang, AlvinX
2020-06-30  3:13     ` Jeff Guo
2020-07-02  3:26 ` [dpdk-dev] [PATCH v2] net/i40e: fix modify the number of qps in VF alvinx.zhang
2020-07-15  7:28   ` [dpdk-dev] [PATCH v3] " alvinx.zhang
2020-07-15 10:17     ` Jeff Guo
2020-07-16  1:38       ` Zhang, AlvinX
2020-07-16  1:52     ` [dpdk-dev] [PATCH v4] " alvinx.zhang
2020-07-16  4:24       ` [dpdk-dev] [PATCH v5] " alvinx.zhang
2020-07-16  4:57         ` Yang, Qiming
2020-07-16  5:13           ` Zhang, AlvinX
2020-07-16  5:12         ` alvinx.zhang
2020-07-16  6:33           ` [dpdk-dev] [PATCH v5] net/i40e: fix qps configuration " alvinx.zhang
2020-07-16  7:30             ` Jeff Guo
2020-07-16  9:14               ` Zhang, Qi Z

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).