net/iavf: fix vector id assignment
PATCH 1/1 net/iavf: fix vector id assignment
From: YU DAPENG <[email protected]>
When the interrupt handle instance allows none packet I/O interrupts, the max value of vector_id is set to be less than the number of msix interrupts, but not equal to it. Which may cause same vector_id is sent to PF with opcode VIRTCHNL_OP_CONFIG_IRQ_MAP to setup the cause of interrupt mapping in iavf_config_irq_map(), these are invalid parameters, cause error: VIRTCHNL_STATUS_ERR_PARAM(-5). So add code to set the max value of vector_id to be equal to the number of msix interrupts.
--- drivers/net/iavf/iavf_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/iavf/iavf_ethdev.c +1 -1
@@ -578,7 +578,7 @@ static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev, qv_map[i].queue_id = i; qv_map[i].vector_id = vec; intr_handle->intr_vec[i] = vec++; -if (vec >= vf->nb_msix) +if (vec > vf->nb_msix) vec =IAVF_RX_VEC_START ; } vf->qv_map = qv_map;
Signature · 1 line
2.27.0
Regards,
Xie Wei
Quoted 42 lines
-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of [email protected]
Sent: Wednesday, December 30, 2020 2:54 PM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Yu, DapengX <[email protected]>; [email protected]
Subject: [dpdk-dev] [PATCH] net/iavf: fix vector id assignmentFrom: YU DAPENG <[email protected]>
When the interrupt handle instance allows none packet I/O interrupts, the max value of
vector_idis set to be less than the number of msix interrupts, but not equal to it. Which may cause samevector_idis sent to PF with opcodeVIRTCHNL_OP_CONFIG_IRQ_MAPto setup the cause of interrupt mapping iniavf_config_irq_map(), these are invalid parameters, cause error:VIRTCHNL_STATUS_ERR_PARAM(-5). So add code to set the max value ofvector_idto be equal to the number of msix interrupts.Fixes d6bde6b5eae9 ("net/avf: enable Rx interrupt")Signed-off-by YU DAPENG <[email protected]>--- drivers/net/iavf/iavf_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/net/iavf/iavf_ethdev.c +1
-1@@ -578,7 +578,7 @@ static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev, qv_map[i].queue_id = i; qv_map[i].vector_id = vec; intr_handle->intr_vec[i] = vec++; -if (vec >= vf->nb_msix) +if (vec > vf->nb_msix) vec =IAVF_RX_VEC_START ; }
vf->qv_map=qv_map;
--
2.27.0
Quoted 32 lines
-----Original Message-----
From: dev <[email protected]> On Behalf Of [email protected]
Sent: Wednesday, December 30, 2020 2:54 PM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Yu, DapengX <[email protected]>; [email protected]
Subject: [dpdk-dev] [PATCH] net/iavf: fix vector id assignmentFrom: YU DAPENG <[email protected]>
When the interrupt handle instance allows none packet I/O interrupts, the max value of
vector_idis set to be less than the number of msix interrupts, but not equal to it. Which may cause samevector_idis sent to PF with opcodeVIRTCHNL_OP_CONFIG_IRQ_MAPto setup the cause of interrupt mapping iniavf_config_irq_map(), these are invalid parameters, cause error:VIRTCHNL_STATUS_ERR_PARAM(-5). So add code to set the max value ofvector_idto be equal to the number of msix interrupts.Fixes d6bde6b5eae9 ("net/avf: enable Rx interrupt")Signed-off-by YU DAPENG <[email protected]>--- drivers/net/iavf/iavf_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/net/iavf/iavf_ethdev.c index renamed from drivers/net/iavf/iavf_ethdev.c +0
-0
7e3c26a94..54ea3a36c100644
drivers/net/iavf/iavf_ethdev.c static int iavf_config_rx_queues_irqs(struct rte_eth_dev · line 582
Quoted 6 lines
drivers/net/iavf/iavf_ethdev.c +1
-1@@ -578,7 +578,7 @@ static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev, qv_map[i].queue_id = i; qv_map[i].vector_id = vec; intr_handle->intr_vec[i] = vec++; -if (vec >= vf->nb_msix) +if (vec > vf->nb_msix)
This is not correct, the nb_msix should already include the vector 0, we should not set a vector_id to a value that >= nb_msix,
Quoted 5 lines
drivers/net/iavf/iavf_ethdev.c +0
-0vec =IAVF_RX_VEC_START ; }
vf->qv_map=qv_map;
--
2.27.0
Hi Qi,
By checking the running i40e vf PMD(function:i40evf_config_irq_map()), found that the vector_id actually can be a value that is equal to nb_msix as the following.map_info->num_vectors is just equal to nb_msix(which is 2), the max vector_id is also 2. The map_info which contains vecmap[0], and vectmap[1] is sent to PF with opcode VIRTCHNL_OP_CONFIG_IRQ_MAP to setup the cause of interrupt mapping in iavf_config_irq_map(), these are valid parameters and effective at last. The interrupt can run as expected. So I deduce that vector_id can be equal to nb_msix in iavf PMD too.
(gdb) p *map_info
$6 = {num_vectors = 2, vecmap = {{vsi_id = 10, vector_id = 1, rxq_map = 1, txq_map = 0, rxitr_idx = 0,
txitr_idx = 0}}}
(gdb) p map_info->vecmap[1]
$7 = {vsi_id = 10, vector_id = 2, rxq_map = 2, txq_map = 0, rxitr_idx = 0, txitr_idx = 0} (gdb) p map_info->vecmap[0]
$8 = {vsi_id = 10, vector_id = 1, rxq_map = 1, txq_map = 0, rxitr_idx = 0, txitr_idx = 0}
-----Original Message-----
From: Zhang, Qi Z
Sent: Monday, January 4, 2021 8:26 AM
To: Yu, DapengX <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Yu, DapengX <[email protected]>; [email protected]
Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment
Quoted 31 lines
-----Original Message-----
From: dev <[email protected]> On Behalf Of [email protected]
Sent: Wednesday, December 30, 2020 2:54 PM
To: Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Yu, DapengX <[email protected]>; [email protected]
Subject: [dpdk-dev] [PATCH] net/iavf: fix vector id assignmentFrom: YU DAPENG <[email protected]>
When the interrupt handle instance allows none packet I/O interrupts,
the max value ofvector_idis set to be less than the number of msix interrupts, but not equal to it. Which may cause samevector_idis
sent to PF with opcodeVIRTCHNL_OP_CONFIG_IRQ_MAPto setup the cause
of interrupt mapping iniavf_config_irq_map(), these are invalid parameters, cause error:VIRTCHNL_STATUS_ERR_PARAM(-5). So add code to set the max value ofvector_idto be equal to the number of msix interrupts.Fixes d6bde6b5eae9 ("net/avf: enable Rx interrupt")Signed-off-by YU DAPENG <[email protected]>--- drivers/net/iavf/iavf_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)(quoted diff) +0
-0
b/drivers/net/iavf/iavf_ethdev.cindex7e3c26a94..54ea3a36c100644
drivers/net/iavf/iavf_ethdev.c static int iavf_config_rx_queues_irqs(struct · line 582
Quoted 6 lines
drivers/net/iavf/iavf_ethdev.c +1
-1@@ -578,7 +578,7 @@ static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev, qv_map[i].queue_id = i; qv_map[i].vector_id = vec; intr_handle->intr_vec[i] = vec++; -if (vec >= vf->nb_msix) +if (vec > vf->nb_msix)
This is not correct, the nb_msix should already include the vector 0, we should not set a vector_id to a value that >= nb_msix,
Quoted 5 lines
drivers/net/iavf/iavf_ethdev.c +0
-0vec =IAVF_RX_VEC_START ; }
vf->qv_map=qv_map;
--
2.27.0
PATCH 1/1 net/iavf: fix vector id assignment
In reply to [email protected]From: YU DAPENG <[email protected]>
The number of MSI-X interrupts on Rx shall be the minimal value of the number of available MSI-X interrupts per VF - 1 (the 1 is for miscellaneous interrupt) and the number of configured Rx queues. The current code break the rule because the number of available MSI-X interrupts is used as the first value, but code does not subtract 1 from it.
In normal situation, the first value is larger than the second value. So each queue can be assigned a unique vector_id.
For example: 17 available MSI-X interrupts, and 16 available Rx queues per VF; but only 4 Rx queues are configured when device is started. vector_id:0 is for misc interrupt, vector_id:1 for Rx queue0, vector_id:2 for Rx queue1, vector_id:3 for Rx queue2, vector_id:4 for Rx queue3.
Current code breaks the rule in this normal situation, because when assign vector_ids to interrupt handle, for example, it does not assign vector_id:4 to the queue3, but assign vector_id:1 to it, because the condition used causes vector_id wrap around too early.
In iavf_config_irq_map(), the current code does not write data into the last element of vecmap[], because of the previous code break. Which cause wrong data is sent to PF with opcode VIRTCHNL_OP_CONFIG_IRQ_MAP and cause error: VIRTCHNL_STATUS_ERR_PARAM(-5).
If kernel driver supports large VFs (up to 256 queues), different queues can be assigned same vector_id.
In order to adapt to large VFs and avoid wrapping early, the condition is replaced from vec >= vf->nb_msix to vec >= vf->vf_res->max_vectors.
--- drivers/net/iavf/iavf_ethdev.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/net/iavf/iavf_ethdev.c +5 -3
@@ -483,6 +483,7 @@ static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev,struct iavf_qv_map *qv_map;uint16_t interval, i;int vec; +uint16_t max_vectors;if (rte_intr_cap_multiple (intr_handle) && dev->data->dev_conf.intr_conf.rxq) {
@@ -570,15 +571,16 @@ static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev,/* If Rx interrupt is reuquired, and we can use * multi interrupts, then the vec is from 1 */ - vf->nb_msix =RTE_MIN (vf->vf_res->max_vectors, - intr_handle->nb_efd); + max_vectors = + vf->vf_res->max_vectors -IAVF_RX_VEC_START ; + vf->nb_msix =RTE_MIN (max_vectors, intr_handle->nb_efd); vf->msix_base =IAVF_RX_VEC_START ; vec =IAVF_RX_VEC_START ;for (i =0 ; i < dev->data->nb_rx_queues; i++) { qv_map[i].queue_id = i; qv_map[i].vector_id = vec; intr_handle->intr_vec[i] = vec++; -if (vec >= vf->nb_msix) +if (vec >= vf->vf_res->max_vectors) vec =IAVF_RX_VEC_START ; } vf->qv_map = qv_map;
Signature · 1 line
2.27.0
Regards,
Xie Wei
Quoted 88 lines
-----Original Message-----
From: dev [mailto:[email protected]] On Behalf Of [email protected]
Sent: Friday, January 8, 2021 6:21 PM
To: Zhang, Qi Z <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>; Xu, Ting <[email protected]>
Cc: [email protected]; Yu, DapengX <[email protected]>; [email protected]
Subject: [dpdk-dev] [PATCH] net/iavf: fix vector id assignmentFrom: YU DAPENG <[email protected]>
The number of MSI-X interrupts on Rx shall be the minimal value of the number of available MSI-X interrupts per VF - 1 (the 1 is for miscellaneous interrupt) and the number of configured Rx queues.
The current code break the rule because the number of available MSI-X interrupts is used as the first value, but code does not subtract 1 from it.In normal situation, the first value is larger than the second value. So each queue can be assigned a unique
vector_id.For example: 17 available MSI-X interrupts, and 16 available Rx queues per VF; but only 4 Rx queues are configured when device is started. vector_id:0 is for misc interrupt, vector_id:1 for Rx queue0, vector_id:2 for Rx queue1, vector_id:3 for Rx queue2, vector_id:4 for Rx queue3.
Current code breaks the rule in this normal situation, because when assign
vector_idsto interrupt handle, for example, it does not assign vector_id:4 to the queue3, but assign vector_id:1 to it, because the condition used causesvector_idwrap around too early.In
iavf_config_irq_map(), the current code does not write data into the last element of vecmap[], because of the previous code break. Which cause wrong data is sent to PF with opcodeVIRTCHNL_OP_CONFIG_IRQ_MAPand cause error:VIRTCHNL_STATUS_ERR_PARAM(-5).If kernel driver supports large VFs (up to 256 queues), different queues can be assigned same
vector_id.In order to adapt to large VFs and avoid wrapping early, the condition is replaced from vec >=
vf->nb_msixto vec >=vf->vf_res->max_vectors.Fixes d6bde6b5eae9 ("net/avf: enable Rx interrupt")Signed-off-by YU DAPENG <[email protected]>--- drivers/net/iavf/iavf_ethdev.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)drivers/net/iavf/iavf_ethdev.c +1
-0@@ -483,6 +483,7 @@ static int iavf_config_rx_queues_irqs(structrte_eth_dev *dev, struct iavf_qv_map *qv_map;uint16_t interval, i;int vec; +uint16_t max_vectors; if (rte_intr_cap_multiple(intr_handle) &&
dev->data->dev_conf.intr_conf.rxq) { @@ -570,15 +571,16 @@
static int iavf_config_rx_queues_irqs(structrte_eth_dev*dev,drivers/net/iavf/iavf_ethdev.c +2
-2/* If Rx interrupt is reuquired, and we can use * multi interrupts, then the vec is from 1 */ - vf->nb_msix =RTE_MIN (vf->vf_res->max_vectors, - intr_handle->nb_efd); + max_vectors = + vf->vf_res->max_vectors -
IAVF_RX_VEC_START;drivers/net/iavf/iavf_ethdev.c +1
-0+ vf->nb_msix = RTE_MIN(max_vectors, intr_handle-Quoted 1 line
nb_efd);drivers/net/iavf/iavf_ethdev.c +1
-2vf->msix_base =IAVF_RX_VEC_START ; vec =IAVF_RX_VEC_START ;for (i =0 ; i < dev->data->nb_rx_queues; i++) { qv_map[i].queue_id = i; qv_map[i].vector_id = vec; intr_handle->intr_vec[i] = vec++; -if (vec >= vf->nb_msix) +if (vec >= vf->vf_res->max_vectors) vec =IAVF_RX_VEC_START ; } vf->qv_map = qv_map; --2.27.0
Quoted 25 lines
-----Original Message-----
From: [email protected] <[email protected]>
Sent: Friday, January 8, 2021 6:21 PM
To: Zhang, Qi Z <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>; Xu, Ting <[email protected]>
Cc: [email protected]; Yu, DapengX <[email protected]>; [email protected]
Subject: [PATCH] net/iavf: fix vector id assignmentFrom: YU DAPENG <[email protected]>
The number of MSI-X interrupts on Rx shall be the minimal value of the number of available MSI-X interrupts per VF - 1 (the 1 is for miscellaneous interrupt) and the number of configured Rx queues.
The current code break the rule because the number of available MSI-X interrupts is used as the first value, but code does not subtract 1 from it.In normal situation, the first value is larger than the second value. So each queue can be assigned a unique
vector_id.For example: 17 available MSI-X interrupts, and 16 available Rx queues per VF; but only 4 Rx queues are configured when device is started. vector_id:0 is for misc interrupt, vector_id:1 for Rx queue0, vector_id:2 for Rx queue1, vector_id:3 for Rx queue2, vector_id:4 for Rx queue3.
Quoted 5 lines
Current code breaks the rule in this normal situation, because when assign
vector_idsto interrupt handle, for example, it does not assign vector_id:4 to the queue3, but assign vector_id:1 to it, because the condition used causesvector_idwrap around too early.
Hi, Dapeng,
Could you please further explain in which condition will this error happen? Seems it requires vf->nb_msix = 3 to make it happen, but I do not notice such situation. I know it may be an example, is there any more specific case?
Thanks.
Quoted 56 lines
In
iavf_config_irq_map(), the current code does not write data into the last element of vecmap[], because of the previous code break. Which cause wrong data is sent to PF with opcodeVIRTCHNL_OP_CONFIG_IRQ_MAPand cause
error:VIRTCHNL_STATUS_ERR_PARAM(-5).If kernel driver supports large VFs (up to 256 queues), different queues can be assigned same
vector_id.In order to adapt to large VFs and avoid wrapping early, the condition is replaced from vec >=
vf->nb_msixto vec >=vf->vf_res->max_vectors.Fixes d6bde6b5eae9 ("net/avf: enable Rx interrupt")Signed-off-by YU DAPENG <[email protected]>--- drivers/net/iavf/iavf_ethdev.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)drivers/net/iavf/iavf_ethdev.c +1
-0@@ -483,6 +483,7 @@ static int iavf_config_rx_queues_irqs(structrte_eth_dev *dev, struct iavf_qv_map *qv_map;uint16_t interval, i;int vec; +uint16_t max_vectors; if (rte_intr_cap_multiple(intr_handle) &&
dev->data->dev_conf.intr_conf.rxq) { @@ -570,15 +571,16 @@
static int iavf_config_rx_queues_irqs(structrte_eth_dev*dev,drivers/net/iavf/iavf_ethdev.c +2
-2/* If Rx interrupt is reuquired, and we can use * multi interrupts, then the vec is from 1 */ - vf->nb_msix =RTE_MIN (vf->vf_res->max_vectors, - intr_handle->nb_efd); + max_vectors = + vf->vf_res->max_vectors -
IAVF_RX_VEC_START;drivers/net/iavf/iavf_ethdev.c +1
-0+ vf->nb_msix = RTE_MIN(max_vectors, intr_handle-Quoted 1 line
nb_efd);drivers/net/iavf/iavf_ethdev.c +1
-2vf->msix_base =IAVF_RX_VEC_START ; vec =IAVF_RX_VEC_START ;for (i =0 ; i < dev->data->nb_rx_queues; i++) { qv_map[i].queue_id = i; qv_map[i].vector_id = vec; intr_handle->intr_vec[i] = vec++; -if (vec >= vf->nb_msix) +if (vec >= vf->vf_res->max_vectors) vec =IAVF_RX_VEC_START ; } vf->qv_map = qv_map; --2.27.0
Quoted 85 lines
-----Original Message-----
From: Xu, Ting <[email protected]>
Sent: Tuesday, January 12, 2021 2:27 PM
To: Yu, DapengX <[email protected]>; Zhang, Qi Z <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Yu, DapengX <[email protected]>; [email protected]
Subject: RE: [PATCH] net/iavf: fix vector id assignmentQuoted 7 lines
-----Original Message-----
From: [email protected] <[email protected]>
Sent: Friday, January 8, 2021 6:21 PM
To: Zhang, Qi Z <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>; Xu, Ting <[email protected]>
Cc: [email protected]; Yu, DapengX <[email protected]>;Quoted 20 lines
Subject: [PATCH] net/iavf: fix vector id assignment
From: YU DAPENG <[email protected]>
The number of MSI-X interrupts on Rx shall be the minimal value of the number of available MSI-X interrupts per VF - 1 (the 1 is for miscellaneous
interrupt) and the number of configured Rx queues.
The current code break the rule because the number of available MSI-X interrupts is used as the first value, but code does not subtract 1 from it.In normal situation, the first value is larger than the second value. So each queue can be assigned a unique
vector_id.For example: 17 available MSI-X interrupts, and 16 available Rx queues per VF; but only 4 Rx queues are configured when device is started. vector_id:0 is for misc interrupt, vector_id:1 for Rx queue0, vector_id:2 for Rx queue1, vector_id:3 for Rx queue2, vector_id:4 for Rx queue3.
Quoted 5 lines
Current code breaks the rule in this normal situation, because when assign
vector_idsto interrupt handle, for example, it does not assign vector_id:4 to the queue3, but assign vector_id:1 to it, because the condition used causesvector_idwrap around too early.Hi, Dapeng,
Could you please further explain in which condition will this error happen? Seems it requires
vf->nb_msix= 3 to make it happen, but I do not notice such situation.
I know it may be an example, is there any more specific case?Thanks.
Quoted 31 lines
In
iavf_config_irq_map(), the current code does not write data into the last element of vecmap[], because of the previous code break. Which cause wrong data is sent to PF with opcodeVIRTCHNL_OP_CONFIG_IRQ_MAPand cause
error:VIRTCHNL_STATUS_ERR_PARAM(-5).If kernel driver supports large VFs (up to 256 queues), different queues can be assigned same
vector_id.In order to adapt to large VFs and avoid wrapping early, the condition is replaced from vec >=
vf->nb_msixto vec >=vf->vf_res->max_vectors.Fixes d6bde6b5eae9 ("net/avf: enable Rx interrupt")Signed-off-by YU DAPENG <[email protected]>--- drivers/net/iavf/iavf_ethdev.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/net/iavf/iavf_ethdev.cindex7e3c26a94..d730bb156100644drivers/net/iavf/iavf_ethdev.c +1
-0@@ -483,6 +483,7 @@ static int iavf_config_rx_queues_irqs(structrte_eth_dev *dev, struct iavf_qv_map *qv_map;uint16_t interval, i;int vec; +uint16_t max_vectors;
Quoted 11 lines
Quoted 11 lines
drivers/net/iavf/iavf_ethdev.c +0
-0if (rte_intr_cap_multiple(intr_handle) &&
dev->data->dev_conf.intr_conf.rxq) { @@ -570,15 +571,16 @@
static int iavf_config_rx_queues_irqs(structrte_eth_dev*dev,drivers/net/iavf/iavf_ethdev.c +2
-2/* If Rx interrupt is reuquired, and we can use * multi interrupts, then the vec is from 1 */ - vf->nb_msix =RTE_MIN (vf->vf_res->max_vectors, - intr_handle->nb_efd); + max_vectors = + vf->vf_res->max_vectors -
IAVF_RX_VEC_START;
Looks it is the same fix as http://patchwork.dpdk.org/patch/86118 /.
I think this line need to be moved to ahead of RTE_MIN? And the RTE_MIN(max_vectors, intr_handle->nb_efd);
Quoted 15 lines
Quoted 15 lines
drivers/net/iavf/iavf_ethdev.c +1
-0+ vf->nb_msix = RTE_MIN(max_vectors, intr_handle-Quoted 1 line
nb_efd);drivers/net/iavf/iavf_ethdev.c +1
-2vf->msix_base =IAVF_RX_VEC_START ; vec =IAVF_RX_VEC_START ;for (i =0 ; i < dev->data->nb_rx_queues; i++) { qv_map[i].queue_id = i; qv_map[i].vector_id = vec; intr_handle->intr_vec[i] = vec++; -if (vec >= vf->nb_msix) +if (vec >= vf->vf_res->max_vectors) vec =IAVF_RX_VEC_START ; } vf->qv_map = qv_map; --2.27.0
Hi Jingjing,
I double checked that the max_vectors assignment statement is already ahead of RTE_MIN.
And this patch do the same thing as http://patchwork.dpdk.org/patch/86118/. Since patch 86118 is ahead of this one, I guess merging 86118 is preferred.
-----Original Message-----
From: Wu, Jingjing
Sent: Tuesday, January 12, 2021 2:44 PM
To: Xu, Ting <[email protected]>; Yu, DapengX <[email protected]>; Zhang, Qi Z <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Yu, DapengX <[email protected]>; [email protected]
Subject: RE: [PATCH] net/iavf: fix vector id assignment
Quoted 82 lines
-----Original Message-----
From: Xu, Ting <[email protected]>
Sent: Tuesday, January 12, 2021 2:27 PM
To: Yu, DapengX <[email protected]>; Zhang, Qi Z <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Yu, DapengX <[email protected]>; [email protected]
Subject: RE: [PATCH] net/iavf: fix vector id assignmentQuoted 7 lines
-----Original Message-----
From: [email protected] <[email protected]>
Sent: Friday, January 8, 2021 6:21 PM
To: Zhang, Qi Z <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>; Xu, Ting <[email protected]>
Cc: [email protected]; Yu, DapengX <[email protected]>;Quoted 20 lines
Subject: [PATCH] net/iavf: fix vector id assignment
From: YU DAPENG <[email protected]>
The number of MSI-X interrupts on Rx shall be the minimal value of the number of available MSI-X interrupts per VF - 1 (the 1 is for miscellaneous
interrupt) and the number of configured Rx queues.
The current code break the rule because the number of available
MSI-X interrupts is used as the first value, but code does not subtract 1 from it.In normal situation, the first value is larger than the second value. So each queue can be assigned a unique
vector_id.For example: 17 available MSI-X interrupts, and 16 available Rx queues per VF; but only 4 Rx queues are configured when device is started. vector_id:0 is for misc interrupt, vector_id:1 for Rx queue0, vector_id:2 for Rx queue1, vector_id:3 for Rx queue2, vector_id:4 for Rx queue3.
Quoted 6 lines
Current code breaks the rule in this normal situation, because when assign
vector_idsto interrupt handle, for example, it does not assign
vector_id:4 to the queue3, but assign vector_id:1 to it, because the condition used causesvector_idwrap around too early.Hi, Dapeng,
Could you please further explain in which condition will this error happen? Seems it requires
vf->nb_msix= 3 to make it happen, but I do not notice such situation.
I know it may be an example, is there any more specific case?Thanks.
Quoted 29 lines
In
iavf_config_irq_map(), the current code does not write data into the last element of vecmap[], because of the previous code break. Which cause wrong data is sent to PF with opcodeVIRTCHNL_OP_CONFIG_IRQ_MAPand cause
error:VIRTCHNL_STATUS_ERR_PARAM(-5).If kernel driver supports large VFs (up to 256 queues), different queues can be assigned same
vector_id.In order to adapt to large VFs and avoid wrapping early, the condition is replaced from vec >=
vf->nb_msixto vec >=vf->vf_res->max_vectors.Fixes d6bde6b5eae9 ("net/avf: enable Rx interrupt")Signed-off-by YU DAPENG <[email protected]>--- drivers/net/iavf/iavf_ethdev.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/net/iavf/iavf_ethdev.cindex7e3c26a94..d730bb156100644drivers/net/iavf/iavf_ethdev.c +1
-0@@ -483,6 +483,7 @@ static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev, struct iavf_qv_map *qv_map; uint16_t interval, i;int vec; +uint16_t max_vectors;
Quoted 11 lines
Quoted 11 lines
drivers/net/iavf/iavf_ethdev.c +0
-0if (rte_intr_cap_multiple (intr_handle) && dev->data->dev_conf.intr_conf.rxq) { @@ -570,15 +571,16 @@ static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev,/* If Rx interrupt is reuquired, and we can use
* multi interrupts, then the vec is from 1
*/drivers/net/iavf/iavf_ethdev.c +2
-2-vf->nb_msix =RTE_MIN (vf->vf_res->max_vectors, - intr_handle->nb_efd); +max_vectors = +vf->vf_res->max_vectors -
IAVF_RX_VEC_START;
Looks it is the same fix as http://patchwork.dpdk.org/patch/86118 /.
I think this line need to be moved to ahead of RTE_MIN? And the RTE_MIN(max_vectors, intr_handle->nb_efd);
Quoted 13 lines
Quoted 13 lines
+vf->nb_msix = RTE_MIN(max_vectors, intr_handle-
Quoted 1 line
nb_efd);
vf->msix_base=IAVF_RX_VEC_START;
vec =IAVF_RX_VEC_START;
for (i = 0; i <dev->data->nb_rx_queues; i++) { qv_map[i].queue_id
= i; qv_map[i].vector_id = vec; intr_handle->intr_vec[i] = vec++;drivers/net/iavf/iavf_ethdev.c +1
-2-if (vec >= vf->nb_msix) +if (vec >= vf->vf_res->max_vectors) vec =IAVF_RX_VEC_START ; } vf->qv_map = qv_map; --2.27.0
Hi Ting,
The following test case is for i40e. if you want to test the case on ice, just ignore step 1.
Step1: switch i40evf to iavf
# sed -i '/{ RTE_PCI_DEVICE(IAVF_INTEL_VENDOR_ID, IAVF_DEV_ID_ADAPTIVE_VF) },/a { RTE_PCI_DEVICE(IAVF_INTEL_VENDOR_ID, IAVF_DEV_ID_VF) },' drivers/net/iavf/iavf_ethdev.c # sed -i -e '/I40E_DEV_ID_VF/s/0x154C/0x164C/g' drivers/net/i40e/base/i40e_devids.h
# sed -i -e '/DEV_RX_OFFLOAD_CHECKSUM,/d' ./examples/l3fwd-power/main.c
Step2. build dpdk and l3fwd-power
Step3. create 1 vf
# echo 1 > /sys/bus/pci/devices/0000\:81\:00.0/sriov_numvfs
Step4. bind vf to vfio-pci
# usertools/dpdk-devbind.py --force --bind=vfio-pci 81:02.0
Step5. launch l3fwd-power
# ./x86_64-native-linuxapp-gcc/examples/dpdk-l3fwd-power -l 0-1 -n 4 -- -P -p 0x1 --config='(0,0,0),(0,1,1)'
Expected result:
start l3fwd-power successfully
Actual result:
fail to start l3fwd-power
-----Original Message-----
From: Xu, Ting
Sent: Tuesday, January 12, 2021 2:27 PM
To: Yu, DapengX <[email protected]>; Zhang, Qi Z <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>
Cc: [email protected]; Yu, DapengX <[email protected]>; [email protected]
Subject: RE: [PATCH] net/iavf: fix vector id assignment
Quoted 27 lines
-----Original Message-----
From: [email protected] <[email protected]>
Sent: Friday, January 8, 2021 6:21 PM
To: Zhang, Qi Z <[email protected]>; Wu, Jingjing <[email protected]>; Xing, Beilei <[email protected]>; Xu, Ting <[email protected]>
Cc: [email protected]; Yu, DapengX <[email protected]>; [email protected]
Subject: [PATCH] net/iavf: fix vector id assignmentFrom: YU DAPENG <[email protected]>
The number of MSI-X interrupts on Rx shall be the minimal value of the number of available MSI-X interrupts per VF - 1 (the 1 is for miscellaneous
interrupt) and the number of configured Rx queues.
The current code break the rule because the number of available MSI-X interrupts is used as the first value, but code does not subtract 1 from it.In normal situation, the first value is larger than the second value. So each queue can be assigned a unique
vector_id.For example: 17 available MSI-X interrupts, and 16 available Rx queues per VF; but only 4 Rx queues are configured when device is started. vector_id:0 is for misc interrupt, vector_id:1 for Rx queue0, vector_id:2 for Rx queue1, vector_id:3 for Rx queue2, vector_id:4 for Rx queue3.
Quoted 5 lines
Current code breaks the rule in this normal situation, because when assign
vector_idsto interrupt handle, for example, it does not assign vector_id:4 to the queue3, but assign vector_id:1 to it, because the condition used causesvector_idwrap around too early.
Hi, Dapeng,
Could you please further explain in which condition will this error happen? Seems it requires vf->nb_msix = 3 to make it happen, but I do not notice such situation. I know it may be an example, is there any more specific case?
Thanks.
Quoted 53 lines
In
iavf_config_irq_map(), the current code does not write data into the last element of vecmap[], because of the previous code break. Which cause wrong data is sent to PF with opcodeVIRTCHNL_OP_CONFIG_IRQ_MAPand cause
error:VIRTCHNL_STATUS_ERR_PARAM(-5).If kernel driver supports large VFs (up to 256 queues), different queues can be assigned same
vector_id.In order to adapt to large VFs and avoid wrapping early, the condition is replaced from vec >=
vf->nb_msixto vec >=vf->vf_res->max_vectors.Fixes d6bde6b5eae9 ("net/avf: enable Rx interrupt")Signed-off-by YU DAPENG <[email protected]>--- drivers/net/iavf/iavf_ethdev.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/net/iavf/iavf_ethdev.cindex7e3c26a94..d730bb156100644drivers/net/iavf/iavf_ethdev.c +1
-0@@ -483,6 +483,7 @@ static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev, struct iavf_qv_map *qv_map; uint16_t interval, i;int vec; +uint16_t max_vectors;if (rte_intr_cap_multiple (intr_handle) && dev->data->dev_conf.intr_conf.rxq) { @@ -570,15 +571,16 @@ static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev,/* If Rx interrupt is reuquired, and we can use
* multi interrupts, then the vec is from 1
*/drivers/net/iavf/iavf_ethdev.c +2
-2-vf->nb_msix =RTE_MIN (vf->vf_res->max_vectors, - intr_handle->nb_efd); +max_vectors = +vf->vf_res->max_vectors -
IAVF_RX_VEC_START;
+vf->nb_msix = RTE_MIN(max_vectors, intr_handle-Quoted 1 line
nb_efd);
vf->msix_base=IAVF_RX_VEC_START;
vec =IAVF_RX_VEC_START;
for (i = 0; i <dev->data->nb_rx_queues; i++) { qv_map[i].queue_id =
i; qv_map[i].vector_id = vec; intr_handle->intr_vec[i] = vec++; -if (vec >=vf->nb_msix)
+if (vec >=vf->vf_res->max_vectors)
vec =IAVF_RX_VEC_START;
}
vf->qv_map=qv_map;
--
2.27.0