diff mbox

[dpdk-dev,v2] net/ixgbe: fix interrupt block issue

Message ID 1484589732-13537-1-git-send-email-qi.z.zhang@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel compilation success Compilation OK

Commit Message

Qi Zhang Jan. 16, 2017, 6:02 p.m. UTC 
  When handle link status change interrupt, interrupt
will be blocked until delayed handler finish, the
duration is at least 1 second, this may cause following
VF to PF mailbox traffic be blocked and sometimes PF
can't ack to VF in time before VF think it's time out.
This patch remove this limitation, interrupt will be
enabled before interrupt handler finish, and a flag is
used to prevent re-entering delayed handler.

Fixes: 0a45657a6794 ("pci: rework interrupt handling")

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
v2:
- rebase to dpdk-next-net

 drivers/net/ixgbe/ixgbe_ethdev.c | 19 +++++++++----------
 drivers/net/ixgbe/ixgbe_ethdev.h |  2 ++
 2 files changed, 11 insertions(+), 10 deletions(-)

Comments

Zhang, Helin Jan. 17, 2017, 1:31 a.m. UTC | #1 
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Tuesday, January 17, 2017 2:02 AM
> To: Lu, Wenzhuo; Zhang, Helin
> Cc: dev@dpdk.org; Zhang, Qi Z
> Subject: [PATCH v2] net/ixgbe: fix interrupt block issue
> 
> When handle link status change interrupt, interrupt will be blocked until
> delayed handler finish, the duration is at least 1 second, this may cause
> following VF to PF mailbox traffic be blocked and sometimes PF can't ack to
> VF in time before VF think it's time out.
> This patch remove this limitation, interrupt will be enabled before interrupt
> handler finish, and a flag is used to prevent re-entering delayed handler.
> 
> Fixes: 0a45657a6794 ("pci: rework interrupt handling")
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
> v2:
> - rebase to dpdk-next-net
> 
>  drivers/net/ixgbe/ixgbe_ethdev.c | 19 +++++++++----------
> drivers/net/ixgbe/ixgbe_ethdev.h |  2 ++
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> b/drivers/net/ixgbe/ixgbe_ethdev.c
> index c66f432..f36ce89 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -3762,7 +3762,6 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev
> *dev,
>  		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
>  	int64_t timeout;
>  	struct rte_eth_link link;
> -	int intr_enable_delay = false;
>  	struct ixgbe_hw *hw =
>  		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> 
> @@ -3778,7 +3777,7 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev
> *dev,
>  		intr->flags &= ~IXGBE_FLAG_PHY_INTERRUPT;
>  	}
> 
> -	if (intr->flags & IXGBE_FLAG_NEED_LINK_UPDATE) {
> +	if (intr->flags & IXGBE_FLAG_NEED_LINK_UPDATE && !intr->delay) {
>  		/* get the link status before link update, for predicting later
> */
>  		memset(&link, 0, sizeof(link));
>  		rte_ixgbe_dev_atomic_read_link_status(dev, &link); @@ -
> 3795,20 +3794,16 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,
>  			timeout = IXGBE_LINK_DOWN_CHECK_TIMEOUT;
> 
>  		ixgbe_dev_link_status_print(dev);
> +		intr->delay = true;
> 
> -		intr_enable_delay = true;
> -	}
> -
> -	if (intr_enable_delay) {
>  		if (rte_eal_alarm_set(timeout * 1000,
>  				      ixgbe_dev_interrupt_delayed_handler,
> (void *)dev) < 0)
>  			PMD_DRV_LOG(ERR, "Error setting alarm");
> -	} else {
> -		PMD_DRV_LOG(DEBUG, "enable intr immediately");
> -		ixgbe_enable_intr(dev);
> -		rte_intr_enable(intr_handle);
>  	}
> 
> +	PMD_DRV_LOG(DEBUG, "enable intr immediately");
> +	ixgbe_enable_intr(dev);
> +	rte_intr_enable(intr_handle);
I think we should still disable LSC interrupt before the delayed handling, to
avoid any chaos on potential too many interrupts being reported.
Of cause other interrupts can be enabled here, to fix the issue you found.

Thanks,
Helin

> 
>  	return 0;
>  }
> @@ -3839,6 +3834,8 @@ ixgbe_dev_interrupt_delayed_handler(void
> *param)
>  		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>  	uint32_t eicr;
> 
> +	ixgbe_disable_intr(hw);
> +
>  	eicr = IXGBE_READ_REG(hw, IXGBE_EICR);
>  	if (eicr & IXGBE_EICR_MAILBOX)
>  		ixgbe_pf_mbx_process(dev);
> @@ -3861,6 +3858,8 @@ ixgbe_dev_interrupt_delayed_handler(void
> *param)
>  		intr->flags &= ~IXGBE_FLAG_MACSEC;
>  	}
> 
> +	intr->delay = false;
> +
>  	PMD_DRV_LOG(DEBUG, "enable intr in delayed handler S[%08x]",
> eicr);
>  	ixgbe_enable_intr(dev);
>  	rte_intr_enable(intr_handle);
> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h
> b/drivers/net/ixgbe/ixgbe_ethdev.h
> index 6695b68..b931108 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.h
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.h
> @@ -200,6 +200,8 @@ struct ixgbe_hw_fdir_info {  struct ixgbe_interrupt {
>  	uint32_t flags;
>  	uint32_t mask;
> +	/* to prevent re-enter delayed handler */
> +	uint8_t delay;
Use bool to replace uint8_t?

Thanks,
Helin

>  };
> 
>  struct ixgbe_stat_mapping_registers {
> --
> 2.7.4
Qi Zhang Jan. 17, 2017, 2:16 a.m. UTC | #2 
Hi Helin:

> -----Original Message-----
> From: Zhang, Helin
> Sent: Tuesday, January 17, 2017 9:31 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v2] net/ixgbe: fix interrupt block issue
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Tuesday, January 17, 2017 2:02 AM
> > To: Lu, Wenzhuo; Zhang, Helin
> > Cc: dev@dpdk.org; Zhang, Qi Z
> > Subject: [PATCH v2] net/ixgbe: fix interrupt block issue
> >
> > When handle link status change interrupt, interrupt will be blocked
> > until delayed handler finish, the duration is at least 1 second, this
> > may cause following VF to PF mailbox traffic be blocked and sometimes
> > PF can't ack to VF in time before VF think it's time out.
> > This patch remove this limitation, interrupt will be enabled before
> > interrupt handler finish, and a flag is used to prevent re-entering delayed
> handler.
> >
> > Fixes: 0a45657a6794 ("pci: rework interrupt handling")
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> > v2:
> > - rebase to dpdk-next-net
> >
> >  drivers/net/ixgbe/ixgbe_ethdev.c | 19 +++++++++----------
> > drivers/net/ixgbe/ixgbe_ethdev.h |  2 ++
> >  2 files changed, 11 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c
> > b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index c66f432..f36ce89 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -3762,7 +3762,6 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev
> > *dev,
> >  		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
> >  	int64_t timeout;
> >  	struct rte_eth_link link;
> > -	int intr_enable_delay = false;
> >  	struct ixgbe_hw *hw =
> >  		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >
> > @@ -3778,7 +3777,7 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev
> > *dev,
> >  		intr->flags &= ~IXGBE_FLAG_PHY_INTERRUPT;
> >  	}
> >
> > -	if (intr->flags & IXGBE_FLAG_NEED_LINK_UPDATE) {
> > +	if (intr->flags & IXGBE_FLAG_NEED_LINK_UPDATE && !intr->delay) {
> >  		/* get the link status before link update, for predicting later */
> >  		memset(&link, 0, sizeof(link));
> >  		rte_ixgbe_dev_atomic_read_link_status(dev, &link); @@ -
> > 3795,20 +3794,16 @@ ixgbe_dev_interrupt_action(struct rte_eth_dev
> *dev,
> >  			timeout = IXGBE_LINK_DOWN_CHECK_TIMEOUT;
> >
> >  		ixgbe_dev_link_status_print(dev);
> > +		intr->delay = true;
> >
> > -		intr_enable_delay = true;
> > -	}
> > -
> > -	if (intr_enable_delay) {
> >  		if (rte_eal_alarm_set(timeout * 1000,
> >  				      ixgbe_dev_interrupt_delayed_handler,
> > (void *)dev) < 0)
> >  			PMD_DRV_LOG(ERR, "Error setting alarm");
> > -	} else {
> > -		PMD_DRV_LOG(DEBUG, "enable intr immediately");
> > -		ixgbe_enable_intr(dev);
> > -		rte_intr_enable(intr_handle);
> >  	}
> >
> > +	PMD_DRV_LOG(DEBUG, "enable intr immediately");
> > +	ixgbe_enable_intr(dev);
> > +	rte_intr_enable(intr_handle);
> I think we should still disable LSC interrupt before the delayed handling, to
> avoid any chaos on potential too many interrupts being reported.
> Of cause other interrupts can be enabled here, to fix the issue you found.

Agree, will add this in v3.
> 
> Thanks,
> Helin
diff mbox

Patch

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index c66f432..f36ce89 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -3762,7 +3762,6 @@  ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,
 		IXGBE_DEV_PRIVATE_TO_INTR(dev->data->dev_private);
 	int64_t timeout;
 	struct rte_eth_link link;
-	int intr_enable_delay = false;
 	struct ixgbe_hw *hw =
 		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 
@@ -3778,7 +3777,7 @@  ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,
 		intr->flags &= ~IXGBE_FLAG_PHY_INTERRUPT;
 	}
 
-	if (intr->flags & IXGBE_FLAG_NEED_LINK_UPDATE) {
+	if (intr->flags & IXGBE_FLAG_NEED_LINK_UPDATE && !intr->delay) {
 		/* get the link status before link update, for predicting later */
 		memset(&link, 0, sizeof(link));
 		rte_ixgbe_dev_atomic_read_link_status(dev, &link);
@@ -3795,20 +3794,16 @@  ixgbe_dev_interrupt_action(struct rte_eth_dev *dev,
 			timeout = IXGBE_LINK_DOWN_CHECK_TIMEOUT;
 
 		ixgbe_dev_link_status_print(dev);
+		intr->delay = true;
 
-		intr_enable_delay = true;
-	}
-
-	if (intr_enable_delay) {
 		if (rte_eal_alarm_set(timeout * 1000,
 				      ixgbe_dev_interrupt_delayed_handler, (void *)dev) < 0)
 			PMD_DRV_LOG(ERR, "Error setting alarm");
-	} else {
-		PMD_DRV_LOG(DEBUG, "enable intr immediately");
-		ixgbe_enable_intr(dev);
-		rte_intr_enable(intr_handle);
 	}
 
+	PMD_DRV_LOG(DEBUG, "enable intr immediately");
+	ixgbe_enable_intr(dev);
+	rte_intr_enable(intr_handle);
 
 	return 0;
 }
@@ -3839,6 +3834,8 @@  ixgbe_dev_interrupt_delayed_handler(void *param)
 		IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	uint32_t eicr;
 
+	ixgbe_disable_intr(hw);
+
 	eicr = IXGBE_READ_REG(hw, IXGBE_EICR);
 	if (eicr & IXGBE_EICR_MAILBOX)
 		ixgbe_pf_mbx_process(dev);
@@ -3861,6 +3858,8 @@  ixgbe_dev_interrupt_delayed_handler(void *param)
 		intr->flags &= ~IXGBE_FLAG_MACSEC;
 	}
 
+	intr->delay = false;
+
 	PMD_DRV_LOG(DEBUG, "enable intr in delayed handler S[%08x]", eicr);
 	ixgbe_enable_intr(dev);
 	rte_intr_enable(intr_handle);
diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h b/drivers/net/ixgbe/ixgbe_ethdev.h
index 6695b68..b931108 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.h
+++ b/drivers/net/ixgbe/ixgbe_ethdev.h
@@ -200,6 +200,8 @@  struct ixgbe_hw_fdir_info {
 struct ixgbe_interrupt {
 	uint32_t flags;
 	uint32_t mask;
+	/* to prevent re-enter delayed handler */
+	uint8_t delay;
 };
 
 struct ixgbe_stat_mapping_registers {