diff mbox series

net/iavf: fix vertor interrupt number configuration error

Message ID 1552964680-45860-1-git-send-email-wei.zhao1@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series net/iavf: fix vertor interrupt number configuration error |

Checks

Context Check Description
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Zhao1, Wei March 19, 2019, 3:04 a.m. UTC 
  There is a issue when iavf do vertor interrupt configuration,
it will miss one interrupt vector which set admin queue interrupt
when communicate with host pf.

Fixes: 69dd4c3d0898 ("net/avf: enable queue and device")
Cc: stable@dpdk.org

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
Signed-off-by: Zhao Wei <wei.zhao1@intel.com>
---
 drivers/net/iavf/iavf_ethdev.c | 4 ++--
 drivers/net/iavf/iavf_vchnl.c  | 8 +++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

Comments

Qi Zhang March 19, 2019, 4 a.m. UTC | #1 
> -----Original Message-----
> From: Zhao1, Wei
> Sent: Tuesday, March 19, 2019 11:05 AM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Zhao1, Wei
> <wei.zhao1@intel.com>
> Subject: [PATCH] net/iavf: fix vertor interrupt number configuration error
> 
> There is a issue when iavf do vertor interrupt configuration, it will miss one
> interrupt vector which set admin queue interrupt when communicate with host
> pf.
> 
> Fixes: 69dd4c3d0898 ("net/avf: enable queue and device")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Signed-off-by: Zhao Wei <wei.zhao1@intel.com>
> ---
>  drivers/net/iavf/iavf_ethdev.c | 4 ++--  drivers/net/iavf/iavf_vchnl.c  | 8
> +++++++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c index
> 846e604..49c9499 100644
> --- a/drivers/net/iavf/iavf_ethdev.c
> +++ b/drivers/net/iavf/iavf_ethdev.c
> @@ -308,7 +308,7 @@ static int iavf_config_rx_queues_irqs(struct rte_eth_dev
> *dev,
>  	if (!dev->data->dev_conf.intr_conf.rxq ||
>  	    !rte_intr_dp_is_en(intr_handle)) {
>  		/* Rx interrupt disabled, Map interrupt only for writeback */
> -		vf->nb_msix = 1;
> +		vf->nb_msix = 2;
>  		if (vf->vf_res->vf_cap_flags &
>  		    VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) {
>  			/* If WB_ON_ITR supports, enable it */ @@ -338,7 +338,7 @@
> static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev,
>  			vf->rxq_map[vf->msix_base] |= 1 << i;
>  	} else {
>  		if (!rte_intr_allow_others(intr_handle)) {
> -			vf->nb_msix = 1;
> +			vf->nb_msix = 2;
>  			vf->msix_base = IAVF_MISC_VEC_ID;
>  			for (i = 0; i < dev->data->nb_rx_queues; i++) {
>  				vf->rxq_map[vf->msix_base] |= 1 << i; diff --git


Looks like something missing in below else branch

			} else {
                       /* 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);
                        vf->msix_base = IAVF_RX_VEC_START;
                        vec = IAVF_RX_VEC_START;
                        for (i = 0; i < dev->data->nb_rx_queues; i++) {
                                vf->rxq_map[vec] |= 1 << i;
                                intr_handle->intr_vec[i] = vec++;
                                if (vec >= vf->nb_msix)
                                        vec = IAVF_RX_VEC_START;
                        }
                        PMD_DRV_LOG(DEBUG,
                                    "%u vectors are mapping to %u Rx queues",
                                    vf->nb_msix, dev->data->nb_rx_queues);
                }
Should we also reserve 1 vector for admin queue in this case, 
or looks like some Rx queue will share the irq vector with admin queue during iavf_config_irq_map which is not expected?


> a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c index
> 6381fb6..d9a376e 100644
> --- a/drivers/net/iavf/iavf_vchnl.c
> +++ b/drivers/net/iavf/iavf_vchnl.c
> @@ -609,7 +609,7 @@ iavf_config_irq_map(struct iavf_adapter *adapter)
>  		return -ENOMEM;
> 
>  	map_info->num_vectors = vf->nb_msix;
> -	for (i = 0; i < vf->nb_msix; i++) {
> +	for (i = 0; i < vf->nb_msix - 1; i++) {
>  		vecmap = &map_info->vecmap[i];
>  		vecmap->vsi_id = vf->vsi_res->vsi_id;
>  		vecmap->rxitr_idx = IAVF_ITR_INDEX_DEFAULT; @@ -618,6 +618,12
> @@ iavf_config_irq_map(struct iavf_adapter *adapter)
>  		vecmap->rxq_map = vf->rxq_map[vf->msix_base + i];
>  	}
> 
> +	vecmap = &map_info->vecmap[i];
> +	vecmap->vsi_id = vf->vsi_res->vsi_id;
> +	vecmap->vector_id = 0;
> +	vecmap->txq_map = 0;
> +	vecmap->rxq_map = 0;
> +
>  	args.ops = VIRTCHNL_OP_CONFIG_IRQ_MAP;
>  	args.in_args = (u8 *)map_info;
>  	args.in_args_size = len;
> --
> 2.7.5
Stillwell Jr, Paul M March 19, 2019, 4:16 a.m. UTC | #2 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Zhao
> Sent: Monday, March 18, 2019 8:05 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Zhao1, Wei
> <wei.zhao1@intel.com>
> Subject: [dpdk-dev] [PATCH] net/iavf: fix vertor interrupt number
> configuration error
> 

Should the patch name be net/avf?

> There is a issue when iavf do vertor interrupt configuration, it will miss one
> interrupt vector which set admin queue interrupt when communicate with
> host pf.
> 

I would reword this to be clearer, something like:

The AVF driver needs to send an extra vector number to the Linux PF to
Work around an issue with interrupt vector mapping. The admin queue
needs to be added to the number of queues sent to the PF.

> Fixes: 69dd4c3d0898 ("net/avf: enable queue and device")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Signed-off-by: Zhao Wei <wei.zhao1@intel.com>
> ---
>  drivers/net/iavf/iavf_ethdev.c | 4 ++--  drivers/net/iavf/iavf_vchnl.c  | 8
> +++++++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
> index 846e604..49c9499 100644
> --- a/drivers/net/iavf/iavf_ethdev.c
> +++ b/drivers/net/iavf/iavf_ethdev.c
> @@ -308,7 +308,7 @@ static int iavf_config_rx_queues_irqs(struct
> rte_eth_dev *dev,
>  	if (!dev->data->dev_conf.intr_conf.rxq ||
>  	    !rte_intr_dp_is_en(intr_handle)) {
>  		/* Rx interrupt disabled, Map interrupt only for writeback */
> -		vf->nb_msix = 1;
> +		vf->nb_msix = 2;
>  		if (vf->vf_res->vf_cap_flags &
>  		    VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) {
>  			/* If WB_ON_ITR supports, enable it */ @@ -338,7
> +338,7 @@ static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev,
>  			vf->rxq_map[vf->msix_base] |= 1 << i;
>  	} else {
>  		if (!rte_intr_allow_others(intr_handle)) {
> -			vf->nb_msix = 1;
> +			vf->nb_msix = 2;
>  			vf->msix_base = IAVF_MISC_VEC_ID;
>  			for (i = 0; i < dev->data->nb_rx_queues; i++) {
>  				vf->rxq_map[vf->msix_base] |= 1 << i; diff --
> git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c index
> 6381fb6..d9a376e 100644
> --- a/drivers/net/iavf/iavf_vchnl.c
> +++ b/drivers/net/iavf/iavf_vchnl.c
> @@ -609,7 +609,7 @@ iavf_config_irq_map(struct iavf_adapter *adapter)
>  		return -ENOMEM;
> 
>  	map_info->num_vectors = vf->nb_msix;
> -	for (i = 0; i < vf->nb_msix; i++) {
> +	for (i = 0; i < vf->nb_msix - 1; i++) {
>  		vecmap = &map_info->vecmap[i];
>  		vecmap->vsi_id = vf->vsi_res->vsi_id;
>  		vecmap->rxitr_idx = IAVF_ITR_INDEX_DEFAULT; @@ -618,6
> +618,12 @@ iavf_config_irq_map(struct iavf_adapter *adapter)
>  		vecmap->rxq_map = vf->rxq_map[vf->msix_base + i];
>  	}
> 
> +	vecmap = &map_info->vecmap[i];
> +	vecmap->vsi_id = vf->vsi_res->vsi_id;
> +	vecmap->vector_id = 0;
> +	vecmap->txq_map = 0;
> +	vecmap->rxq_map = 0;
> +
>  	args.ops = VIRTCHNL_OP_CONFIG_IRQ_MAP;
>  	args.in_args = (u8 *)map_info;
>  	args.in_args_size = len;
> --
> 2.7.5
Zhao1, Wei March 19, 2019, 5:57 a.m. UTC | #3 
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Tuesday, March 19, 2019 12:01 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH] net/iavf: fix vertor interrupt number configuration error
> 
> 
> 
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Tuesday, March 19, 2019 11:05 AM
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Zhao1, Wei
> > <wei.zhao1@intel.com>
> > Subject: [PATCH] net/iavf: fix vertor interrupt number configuration
> > error
> >
> > There is a issue when iavf do vertor interrupt configuration, it will
> > miss one interrupt vector which set admin queue interrupt when
> > communicate with host pf.
> >
> > Fixes: 69dd4c3d0898 ("net/avf: enable queue and device")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > Signed-off-by: Zhao Wei <wei.zhao1@intel.com>
> > ---
> >  drivers/net/iavf/iavf_ethdev.c | 4 ++--
> > drivers/net/iavf/iavf_vchnl.c  | 8
> > +++++++-
> >  2 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/iavf/iavf_ethdev.c
> > b/drivers/net/iavf/iavf_ethdev.c index
> > 846e604..49c9499 100644
> > --- a/drivers/net/iavf/iavf_ethdev.c
> > +++ b/drivers/net/iavf/iavf_ethdev.c
> > @@ -308,7 +308,7 @@ static int iavf_config_rx_queues_irqs(struct
> > rte_eth_dev *dev,
> >  	if (!dev->data->dev_conf.intr_conf.rxq ||
> >  	    !rte_intr_dp_is_en(intr_handle)) {
> >  		/* Rx interrupt disabled, Map interrupt only for writeback */
> > -		vf->nb_msix = 1;
> > +		vf->nb_msix = 2;
> >  		if (vf->vf_res->vf_cap_flags &
> >  		    VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) {
> >  			/* If WB_ON_ITR supports, enable it */ @@ -338,7
> +338,7 @@ static
> > int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev,
> >  			vf->rxq_map[vf->msix_base] |= 1 << i;
> >  	} else {
> >  		if (!rte_intr_allow_others(intr_handle)) {
> > -			vf->nb_msix = 1;
> > +			vf->nb_msix = 2;
> >  			vf->msix_base = IAVF_MISC_VEC_ID;
> >  			for (i = 0; i < dev->data->nb_rx_queues; i++) {
> >  				vf->rxq_map[vf->msix_base] |= 1 << i; diff --
> git
> 
> 
> Looks like something missing in below else branch
> 
> 			} else {
>                        /* 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);
>                         vf->msix_base = IAVF_RX_VEC_START;
>                         vec = IAVF_RX_VEC_START;
>                         for (i = 0; i < dev->data->nb_rx_queues; i++) {
>                                 vf->rxq_map[vec] |= 1 << i;
>                                 intr_handle->intr_vec[i] = vec++;
>                                 if (vec >= vf->nb_msix)
>                                         vec = IAVF_RX_VEC_START;
>                         }
>                         PMD_DRV_LOG(DEBUG,
>                                     "%u vectors are mapping to %u Rx queues",
>                                     vf->nb_msix, dev->data->nb_rx_queues);
>                 }
> Should we also reserve 1 vector for admin queue in this case, or looks like
> some Rx queue will share the irq vector with admin queue during
> iavf_config_irq_map which is not expected?


Yes,  we should change to
"
			vf->nb_msix = RTE_MIN(vf->vf_res->max_vectors,
					      intr_handle->nb_efd) + 1;
"
in  v2 

> 
> 
> > a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c index
> > 6381fb6..d9a376e 100644
> > --- a/drivers/net/iavf/iavf_vchnl.c
> > +++ b/drivers/net/iavf/iavf_vchnl.c
> > @@ -609,7 +609,7 @@ iavf_config_irq_map(struct iavf_adapter *adapter)
> >  		return -ENOMEM;
> >
> >  	map_info->num_vectors = vf->nb_msix;
> > -	for (i = 0; i < vf->nb_msix; i++) {
> > +	for (i = 0; i < vf->nb_msix - 1; i++) {
> >  		vecmap = &map_info->vecmap[i];
> >  		vecmap->vsi_id = vf->vsi_res->vsi_id;
> >  		vecmap->rxitr_idx = IAVF_ITR_INDEX_DEFAULT; @@ -618,6
> +618,12
> > @@ iavf_config_irq_map(struct iavf_adapter *adapter)
> >  		vecmap->rxq_map = vf->rxq_map[vf->msix_base + i];
> >  	}
> >
> > +	vecmap = &map_info->vecmap[i];
> > +	vecmap->vsi_id = vf->vsi_res->vsi_id;
> > +	vecmap->vector_id = 0;
> > +	vecmap->txq_map = 0;
> > +	vecmap->rxq_map = 0;
> > +
> >  	args.ops = VIRTCHNL_OP_CONFIG_IRQ_MAP;
> >  	args.in_args = (u8 *)map_info;
> >  	args.in_args_size = len;
> > --
> > 2.7.5
Zhao1, Wei March 21, 2019, 3:30 a.m. UTC | #4 
> -----Original Message-----
> From: Stillwell Jr, Paul M
> Sent: Tuesday, March 19, 2019 12:17 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Zhao1, Wei
> <wei.zhao1@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] net/iavf: fix vertor interrupt number
> configuration error
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Wei Zhao
> > Sent: Monday, March 18, 2019 8:05 PM
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Zhao1, Wei
> > <wei.zhao1@intel.com>
> > Subject: [dpdk-dev] [PATCH] net/iavf: fix vertor interrupt number
> > configuration error
> >
> 
> Should the patch name be net/avf?

No, DPDK has change name to iavf in recent days.
 
> 
> > There is a issue when iavf do vertor interrupt configuration, it will
> > miss one interrupt vector which set admin queue interrupt when
> > communicate with host pf.
> >
> 
> I would reword this to be clearer, something like:
> 
> The AVF driver needs to send an extra vector number to the Linux PF to
> Work around an issue with interrupt vector mapping. The admin queue
> needs to be added to the number of queues sent to the PF.

Good, add in v2 later

> 
> > Fixes: 69dd4c3d0898 ("net/avf: enable queue and device")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > Signed-off-by: Zhao Wei <wei.zhao1@intel.com>
> > ---
> >  drivers/net/iavf/iavf_ethdev.c | 4 ++--
> > drivers/net/iavf/iavf_vchnl.c  | 8
> > +++++++-
> >  2 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/iavf/iavf_ethdev.c
> > b/drivers/net/iavf/iavf_ethdev.c index 846e604..49c9499 100644
> > --- a/drivers/net/iavf/iavf_ethdev.c
> > +++ b/drivers/net/iavf/iavf_ethdev.c
> > @@ -308,7 +308,7 @@ static int iavf_config_rx_queues_irqs(struct
> > rte_eth_dev *dev,
> >  	if (!dev->data->dev_conf.intr_conf.rxq ||
> >  	    !rte_intr_dp_is_en(intr_handle)) {
> >  		/* Rx interrupt disabled, Map interrupt only for writeback */
> > -		vf->nb_msix = 1;
> > +		vf->nb_msix = 2;
> >  		if (vf->vf_res->vf_cap_flags &
> >  		    VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) {
> >  			/* If WB_ON_ITR supports, enable it */ @@ -338,7
> > +338,7 @@ static int iavf_config_rx_queues_irqs(struct rte_eth_dev
> > +*dev,
> >  			vf->rxq_map[vf->msix_base] |= 1 << i;
> >  	} else {
> >  		if (!rte_intr_allow_others(intr_handle)) {
> > -			vf->nb_msix = 1;
> > +			vf->nb_msix = 2;
> >  			vf->msix_base = IAVF_MISC_VEC_ID;
> >  			for (i = 0; i < dev->data->nb_rx_queues; i++) {
> >  				vf->rxq_map[vf->msix_base] |= 1 << i; diff --
> git
> > a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c index
> > 6381fb6..d9a376e 100644
> > --- a/drivers/net/iavf/iavf_vchnl.c
> > +++ b/drivers/net/iavf/iavf_vchnl.c
> > @@ -609,7 +609,7 @@ iavf_config_irq_map(struct iavf_adapter *adapter)
> >  		return -ENOMEM;
> >
> >  	map_info->num_vectors = vf->nb_msix;
> > -	for (i = 0; i < vf->nb_msix; i++) {
> > +	for (i = 0; i < vf->nb_msix - 1; i++) {
> >  		vecmap = &map_info->vecmap[i];
> >  		vecmap->vsi_id = vf->vsi_res->vsi_id;
> >  		vecmap->rxitr_idx = IAVF_ITR_INDEX_DEFAULT; @@ -618,6
> > +618,12 @@ iavf_config_irq_map(struct iavf_adapter *adapter)
> >  		vecmap->rxq_map = vf->rxq_map[vf->msix_base + i];
> >  	}
> >
> > +	vecmap = &map_info->vecmap[i];
> > +	vecmap->vsi_id = vf->vsi_res->vsi_id;
> > +	vecmap->vector_id = 0;
> > +	vecmap->txq_map = 0;
> > +	vecmap->rxq_map = 0;
> > +
> >  	args.ops = VIRTCHNL_OP_CONFIG_IRQ_MAP;
> >  	args.in_args = (u8 *)map_info;
> >  	args.in_args_size = len;
> > --
> > 2.7.5
diff mbox series

Patch

diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
index 846e604..49c9499 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -308,7 +308,7 @@  static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev,
 	if (!dev->data->dev_conf.intr_conf.rxq ||
 	    !rte_intr_dp_is_en(intr_handle)) {
 		/* Rx interrupt disabled, Map interrupt only for writeback */
-		vf->nb_msix = 1;
+		vf->nb_msix = 2;
 		if (vf->vf_res->vf_cap_flags &
 		    VIRTCHNL_VF_OFFLOAD_WB_ON_ITR) {
 			/* If WB_ON_ITR supports, enable it */
@@ -338,7 +338,7 @@  static int iavf_config_rx_queues_irqs(struct rte_eth_dev *dev,
 			vf->rxq_map[vf->msix_base] |= 1 << i;
 	} else {
 		if (!rte_intr_allow_others(intr_handle)) {
-			vf->nb_msix = 1;
+			vf->nb_msix = 2;
 			vf->msix_base = IAVF_MISC_VEC_ID;
 			for (i = 0; i < dev->data->nb_rx_queues; i++) {
 				vf->rxq_map[vf->msix_base] |= 1 << i;
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index 6381fb6..d9a376e 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -609,7 +609,7 @@  iavf_config_irq_map(struct iavf_adapter *adapter)
 		return -ENOMEM;
 
 	map_info->num_vectors = vf->nb_msix;
-	for (i = 0; i < vf->nb_msix; i++) {
+	for (i = 0; i < vf->nb_msix - 1; i++) {
 		vecmap = &map_info->vecmap[i];
 		vecmap->vsi_id = vf->vsi_res->vsi_id;
 		vecmap->rxitr_idx = IAVF_ITR_INDEX_DEFAULT;
@@ -618,6 +618,12 @@  iavf_config_irq_map(struct iavf_adapter *adapter)
 		vecmap->rxq_map = vf->rxq_map[vf->msix_base + i];
 	}
 
+	vecmap = &map_info->vecmap[i];
+	vecmap->vsi_id = vf->vsi_res->vsi_id;
+	vecmap->vector_id = 0;
+	vecmap->txq_map = 0;
+	vecmap->rxq_map = 0;
+
 	args.ops = VIRTCHNL_OP_CONFIG_IRQ_MAP;
 	args.in_args = (u8 *)map_info;
 	args.in_args_size = len;