* [dpdk-dev] [PATCH] net/iavf: fix vector id assignment @ 2020-12-30 6:53 dapengx.yu 2020-12-30 7:31 ` Xie, WeiX ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: dapengx.yu @ 2020-12-30 6:53 UTC (permalink / raw) To: jingjing.wu, beilei.xing; +Cc: dev, YU DAPENG, stable From: YU DAPENG <dapengx.yu@intel.com> 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. Fixes: d6bde6b5eae9 ("net/avf: enable Rx interrupt") Cc: stable@dpdk.org Signed-off-by: YU DAPENG <dapengx.yu@intel.com> --- drivers/net/iavf/iavf_ethdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c index 7e3c26a94..54ea3a36c 100644 --- a/drivers/net/iavf/iavf_ethdev.c +++ b/drivers/net/iavf/iavf_ethdev.c @@ -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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment 2020-12-30 6:53 [dpdk-dev] [PATCH] net/iavf: fix vector id assignment dapengx.yu @ 2020-12-30 7:31 ` Xie, WeiX 2021-01-04 0:25 ` Zhang, Qi Z 2021-01-08 10:21 ` dapengx.yu 2 siblings, 0 replies; 10+ messages in thread From: Xie, WeiX @ 2020-12-30 7:31 UTC (permalink / raw) To: Yu, DapengX, Wu, Jingjing, Xing, Beilei; +Cc: dev, Yu, DapengX, stable Tested-by: Xie,WeiX < weix.xie@intel.com> Regards, Xie Wei > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of > dapengx.yu@intel.com > Sent: Wednesday, December 30, 2020 2:54 PM > To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com> > Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org > Subject: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment > > From: YU DAPENG <dapengx.yu@intel.com> > > 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. > > Fixes: d6bde6b5eae9 ("net/avf: enable Rx interrupt") > Cc: stable@dpdk.org > > Signed-off-by: YU DAPENG <dapengx.yu@intel.com> > --- > drivers/net/iavf/iavf_ethdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c > index 7e3c26a94..54ea3a36c 100644 > --- a/drivers/net/iavf/iavf_ethdev.c > +++ b/drivers/net/iavf/iavf_ethdev.c > @@ -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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment 2020-12-30 6:53 [dpdk-dev] [PATCH] net/iavf: fix vector id assignment dapengx.yu 2020-12-30 7:31 ` Xie, WeiX @ 2021-01-04 0:25 ` Zhang, Qi Z 2021-01-04 2:01 ` Yu, DapengX 2021-01-08 10:21 ` dapengx.yu 2 siblings, 1 reply; 10+ messages in thread From: Zhang, Qi Z @ 2021-01-04 0:25 UTC (permalink / raw) To: Yu, DapengX, Wu, Jingjing, Xing, Beilei; +Cc: dev, Yu, DapengX, stable > -----Original Message----- > From: dev <dev-bounces@dpdk.org> On Behalf Of dapengx.yu@intel.com > Sent: Wednesday, December 30, 2020 2:54 PM > To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com> > Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org > Subject: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment > > From: YU DAPENG <dapengx.yu@intel.com> > > 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. > > Fixes: d6bde6b5eae9 ("net/avf: enable Rx interrupt") > Cc: stable@dpdk.org > > Signed-off-by: YU DAPENG <dapengx.yu@intel.com> > --- > drivers/net/iavf/iavf_ethdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c index > 7e3c26a94..54ea3a36c 100644 > --- a/drivers/net/iavf/iavf_ethdev.c > +++ b/drivers/net/iavf/iavf_ethdev.c > @@ -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, > vec = IAVF_RX_VEC_START; > } > vf->qv_map = qv_map; > -- > 2.27.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment 2021-01-04 0:25 ` Zhang, Qi Z @ 2021-01-04 2:01 ` Yu, DapengX 0 siblings, 0 replies; 10+ messages in thread From: Yu, DapengX @ 2021-01-04 2:01 UTC (permalink / raw) To: Zhang, Qi Z, Wu, Jingjing, Xing, Beilei; +Cc: dev, stable 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 <dapengx.yu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment > -----Original Message----- > From: dev <dev-bounces@dpdk.org> On Behalf Of dapengx.yu@intel.com > Sent: Wednesday, December 30, 2020 2:54 PM > To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei > <beilei.xing@intel.com> > Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org > Subject: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment > > From: YU DAPENG <dapengx.yu@intel.com> > > 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. > > Fixes: d6bde6b5eae9 ("net/avf: enable Rx interrupt") > Cc: stable@dpdk.org > > Signed-off-by: YU DAPENG <dapengx.yu@intel.com> > --- > drivers/net/iavf/iavf_ethdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/iavf/iavf_ethdev.c > b/drivers/net/iavf/iavf_ethdev.c index 7e3c26a94..54ea3a36c 100644 > --- a/drivers/net/iavf/iavf_ethdev.c > +++ b/drivers/net/iavf/iavf_ethdev.c > @@ -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, > vec = IAVF_RX_VEC_START; > } > vf->qv_map = qv_map; > -- > 2.27.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [dpdk-dev] [PATCH] net/iavf: fix vector id assignment 2020-12-30 6:53 [dpdk-dev] [PATCH] net/iavf: fix vector id assignment dapengx.yu 2020-12-30 7:31 ` Xie, WeiX 2021-01-04 0:25 ` Zhang, Qi Z @ 2021-01-08 10:21 ` dapengx.yu 2021-01-11 9:06 ` Xie, WeiX 2021-01-12 6:26 ` Xu, Ting 2 siblings, 2 replies; 10+ messages in thread From: dapengx.yu @ 2021-01-08 10:21 UTC (permalink / raw) To: qi.z.zhang, jingjing.wu, beilei.xing, ting.xu; +Cc: dev, YU DAPENG, stable From: YU DAPENG <dapengx.yu@intel.com> 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. Fixes: d6bde6b5eae9 ("net/avf: enable Rx interrupt") Cc: stable@dpdk.org Signed-off-by: YU DAPENG <dapengx.yu@intel.com> --- drivers/net/iavf/iavf_ethdev.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c index 7e3c26a94..d730bb156 100644 --- a/drivers/net/iavf/iavf_ethdev.c +++ b/drivers/net/iavf/iavf_ethdev.c @@ -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; -- 2.27.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment 2021-01-08 10:21 ` dapengx.yu @ 2021-01-11 9:06 ` Xie, WeiX 2021-01-12 6:26 ` Xu, Ting 1 sibling, 0 replies; 10+ messages in thread From: Xie, WeiX @ 2021-01-11 9:06 UTC (permalink / raw) To: Yu, DapengX, Zhang, Qi Z, Wu, Jingjing, Xing, Beilei, Xu, Ting Cc: dev, Yu, DapengX, stable Tested-by: Xie,WeiX < weix.xie@intel.com> Regards, Xie Wei > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of > dapengx.yu@intel.com > Sent: Friday, January 8, 2021 6:21 PM > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing > <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Xu, Ting > <ting.xu@intel.com> > Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org > Subject: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment > > From: YU DAPENG <dapengx.yu@intel.com> > > 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. > > Fixes: d6bde6b5eae9 ("net/avf: enable Rx interrupt") > Cc: stable@dpdk.org > > Signed-off-by: YU DAPENG <dapengx.yu@intel.com> > --- > drivers/net/iavf/iavf_ethdev.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c > index 7e3c26a94..d730bb156 100644 > --- a/drivers/net/iavf/iavf_ethdev.c > +++ b/drivers/net/iavf/iavf_ethdev.c > @@ -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; > -- > 2.27.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment 2021-01-08 10:21 ` dapengx.yu 2021-01-11 9:06 ` Xie, WeiX @ 2021-01-12 6:26 ` Xu, Ting 2021-01-12 6:44 ` Wu, Jingjing 2021-01-12 6:46 ` Yu, DapengX 1 sibling, 2 replies; 10+ messages in thread From: Xu, Ting @ 2021-01-12 6:26 UTC (permalink / raw) To: Yu, DapengX, Zhang, Qi Z, Wu, Jingjing, Xing, Beilei Cc: dev, Yu, DapengX, stable > -----Original Message----- > From: dapengx.yu@intel.com <dapengx.yu@intel.com> > Sent: Friday, January 8, 2021 6:21 PM > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; > Xing, Beilei <beilei.xing@intel.com>; Xu, Ting <ting.xu@intel.com> > Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org > Subject: [PATCH] net/iavf: fix vector id assignment > > From: YU DAPENG <dapengx.yu@intel.com> > > 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. > 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. > 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. > > Fixes: d6bde6b5eae9 ("net/avf: enable Rx interrupt") > Cc: stable@dpdk.org > > Signed-off-by: YU DAPENG <dapengx.yu@intel.com> > --- > drivers/net/iavf/iavf_ethdev.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c > index 7e3c26a94..d730bb156 100644 > --- a/drivers/net/iavf/iavf_ethdev.c > +++ b/drivers/net/iavf/iavf_ethdev.c > @@ -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; > -- > 2.27.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment 2021-01-12 6:26 ` Xu, Ting @ 2021-01-12 6:44 ` Wu, Jingjing 2021-01-12 7:08 ` Yu, DapengX 2021-01-12 6:46 ` Yu, DapengX 1 sibling, 1 reply; 10+ messages in thread From: Wu, Jingjing @ 2021-01-12 6:44 UTC (permalink / raw) To: Xu, Ting, Yu, DapengX, Zhang, Qi Z, Xing, Beilei; +Cc: dev, Yu, DapengX, stable > -----Original Message----- > From: Xu, Ting <ting.xu@intel.com> > Sent: Tuesday, January 12, 2021 2:27 PM > To: Yu, DapengX <dapengx.yu@intel.com>; Zhang, Qi Z > <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei > <beilei.xing@intel.com> > Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org > Subject: RE: [PATCH] net/iavf: fix vector id assignment > > > -----Original Message----- > > From: dapengx.yu@intel.com <dapengx.yu@intel.com> > > Sent: Friday, January 8, 2021 6:21 PM > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing > > <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Xu, > > Ting <ting.xu@intel.com> > > Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; > stable@dpdk.org > > Subject: [PATCH] net/iavf: fix vector id assignment > > > > From: YU DAPENG <dapengx.yu@intel.com> > > > > 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. > > > > 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. > > > 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. > > > > Fixes: d6bde6b5eae9 ("net/avf: enable Rx interrupt") > > Cc: stable@dpdk.org > > > > Signed-off-by: YU DAPENG <dapengx.yu@intel.com> > > --- > > drivers/net/iavf/iavf_ethdev.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/iavf/iavf_ethdev.c > > b/drivers/net/iavf/iavf_ethdev.c index 7e3c26a94..d730bb156 100644 > > --- a/drivers/net/iavf/iavf_ethdev.c > > +++ b/drivers/net/iavf/iavf_ethdev.c > > @@ -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; 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); > > + 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; > > -- > > 2.27.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment 2021-01-12 6:44 ` Wu, Jingjing @ 2021-01-12 7:08 ` Yu, DapengX 0 siblings, 0 replies; 10+ messages in thread From: Yu, DapengX @ 2021-01-12 7:08 UTC (permalink / raw) To: Wu, Jingjing, Xu, Ting, Zhang, Qi Z, Xing, Beilei; +Cc: dev, stable 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 <ting.xu@intel.com>; Yu, DapengX <dapengx.yu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei <beilei.xing@intel.com> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org Subject: RE: [PATCH] net/iavf: fix vector id assignment > -----Original Message----- > From: Xu, Ting <ting.xu@intel.com> > Sent: Tuesday, January 12, 2021 2:27 PM > To: Yu, DapengX <dapengx.yu@intel.com>; Zhang, Qi Z > <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Xing, > Beilei <beilei.xing@intel.com> > Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org > Subject: RE: [PATCH] net/iavf: fix vector id assignment > > > -----Original Message----- > > From: dapengx.yu@intel.com <dapengx.yu@intel.com> > > Sent: Friday, January 8, 2021 6:21 PM > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing > > <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Xu, > > Ting <ting.xu@intel.com> > > Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; > stable@dpdk.org > > Subject: [PATCH] net/iavf: fix vector id assignment > > > > From: YU DAPENG <dapengx.yu@intel.com> > > > > 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. > > > > 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. > > > 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. > > > > Fixes: d6bde6b5eae9 ("net/avf: enable Rx interrupt") > > Cc: stable@dpdk.org > > > > Signed-off-by: YU DAPENG <dapengx.yu@intel.com> > > --- > > drivers/net/iavf/iavf_ethdev.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/iavf/iavf_ethdev.c > > b/drivers/net/iavf/iavf_ethdev.c index 7e3c26a94..d730bb156 100644 > > --- a/drivers/net/iavf/iavf_ethdev.c > > +++ b/drivers/net/iavf/iavf_ethdev.c > > @@ -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; 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); > > +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; > > -- > > 2.27.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [dpdk-dev] [PATCH] net/iavf: fix vector id assignment 2021-01-12 6:26 ` Xu, Ting 2021-01-12 6:44 ` Wu, Jingjing @ 2021-01-12 6:46 ` Yu, DapengX 1 sibling, 0 replies; 10+ messages in thread From: Yu, DapengX @ 2021-01-12 6:46 UTC (permalink / raw) To: Xu, Ting, Zhang, Qi Z, Wu, Jingjing, Xing, Beilei; +Cc: dev, stable 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 <dapengx.yu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com> Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org Subject: RE: [PATCH] net/iavf: fix vector id assignment > -----Original Message----- > From: dapengx.yu@intel.com <dapengx.yu@intel.com> > Sent: Friday, January 8, 2021 6:21 PM > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing > <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Xu, > Ting <ting.xu@intel.com> > Cc: dev@dpdk.org; Yu, DapengX <dapengx.yu@intel.com>; stable@dpdk.org > Subject: [PATCH] net/iavf: fix vector id assignment > > From: YU DAPENG <dapengx.yu@intel.com> > > 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. > 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. > 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. > > Fixes: d6bde6b5eae9 ("net/avf: enable Rx interrupt") > Cc: stable@dpdk.org > > Signed-off-by: YU DAPENG <dapengx.yu@intel.com> > --- > drivers/net/iavf/iavf_ethdev.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/iavf/iavf_ethdev.c > b/drivers/net/iavf/iavf_ethdev.c index 7e3c26a94..d730bb156 100644 > --- a/drivers/net/iavf/iavf_ethdev.c > +++ b/drivers/net/iavf/iavf_ethdev.c > @@ -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; > -- > 2.27.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-01-12 7:08 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-30 6:53 [dpdk-dev] [PATCH] net/iavf: fix vector id assignment dapengx.yu 2020-12-30 7:31 ` Xie, WeiX 2021-01-04 0:25 ` Zhang, Qi Z 2021-01-04 2:01 ` Yu, DapengX 2021-01-08 10:21 ` dapengx.yu 2021-01-11 9:06 ` Xie, WeiX 2021-01-12 6:26 ` Xu, Ting 2021-01-12 6:44 ` Wu, Jingjing 2021-01-12 7:08 ` Yu, DapengX 2021-01-12 6:46 ` Yu, DapengX
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).