diff mbox series

net/i40e: fix modifying the number of queues

Message ID 20200610120703.8268-1-alvinx.zhang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series net/i40e: fix modifying the number of queues |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Alvin Zhang June 10, 2020, 12:07 p.m. UTC 
  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(-)

Comments

Guo, Jia June 21, 2020, 1:36 p.m. UTC | #1 
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;
>   	}
>
Alvin Zhang June 29, 2020, 3:16 a.m. UTC | #2 
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;
> >   	}
> >
Guo, Jia June 30, 2020, 3:13 a.m. UTC | #3 
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;
>>>    	}
>>>
diff mbox series

Patch

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