[dpdk-dev] [PATCH] librte: Link status interrupt race condition, IGB E1000

[dpdk-dev] [PATCH] librte: Link status interrupt race condition, IGB E1000

[Tim Shearer]

I encountered an issue with DPDK 2.1.0  which occasionally causes the link status interrupt callback not to be called after the interface is started for the first time. I traced the problem back to the function eth_igb_link_update(), which is used to determine if the link has changed state since the previous time it was called. It appears that this function can be called simultaneously from two different threads:

(1) From the main application/configuration thread, via rte_eth_dev_start() - pointed to by (*dev->dev_ops->link_update)
(2) From the eal interrupt thread, via eth_igb_interrupt_action(), to check if the link state has transitioned up or down. The user callback is only executed if the link has changed state.

The race condition manifests itself as follows:
 - Main thread configures the interface with link status interrupt (LSI) enabled, sets up the queues etc.
 - Main thread calls rte_eth_dev_start. The interface is started and then we call eth_igb_link_update()
 - While in this call, the link goes up. Accordingly, we  detect the transition, and write the new link state (up) into the global rte_eth_dev struct
 - The interrupt fires, which also drops into the eth_igb_link_update function, finds that the global link status has already been set to up (no change)
 - Therefore, the handler thinks the interrupt was spurious, and the callback doesn't get called.

I suspect that rte_eth_dev_start shouldn't be checking the link state if interrupts are enabled. Would someone mind taking a quick look at the patch below?

Thanks!
Tim

--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1300,7 +1300,7 @@ rte_eth_dev_start(uint8_t port_id)
 
        rte_eth_dev_config_restore(port_id);
 
-       if (dev->data->dev_conf.intr_conf.lsc != 0) {
+       if (dev->data->dev_conf.intr_conf.lsc == 0) {
                FUNC_PTR_OR_ERR_RET(*dev->dev_ops->link_update, -ENOTSUP);
                (*dev->dev_ops->link_update)(dev, 0);
        }

Re: [dpdk-dev] [PATCH] librte: Link status interrupt race condition, IGB E1000

[Thomas Monjalon]

Wenzhuo,
Please could you have a look?
Thanks

2015-09-24 20:44, Tim Shearer:
> I encountered an issue with DPDK 2.1.0  which occasionally causes the link status interrupt callback not to be called after the interface is started for the first time. I traced the problem back to the function eth_igb_link_update(), which is used to determine if the link has changed state since the previous time it was called. It appears that this function can be called simultaneously from two different threads:
> 
> (1) From the main application/configuration thread, via rte_eth_dev_start() - pointed to by (*dev->dev_ops->link_update)
> (2) From the eal interrupt thread, via eth_igb_interrupt_action(), to check if the link state has transitioned up or down. The user callback is only executed if the link has changed state.
> 
> The race condition manifests itself as follows:
>  - Main thread configures the interface with link status interrupt (LSI) enabled, sets up the queues etc.
>  - Main thread calls rte_eth_dev_start. The interface is started and then we call eth_igb_link_update()
>  - While in this call, the link goes up. Accordingly, we  detect the transition, and write the new link state (up) into the global rte_eth_dev struct
>  - The interrupt fires, which also drops into the eth_igb_link_update function, finds that the global link status has already been set to up (no change)
>  - Therefore, the handler thinks the interrupt was spurious, and the callback doesn't get called.
> 
> I suspect that rte_eth_dev_start shouldn't be checking the link state if interrupts are enabled. Would someone mind taking a quick look at the patch below?
> 
> Thanks!
> Tim
> 
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1300,7 +1300,7 @@ rte_eth_dev_start(uint8_t port_id)
>  
>         rte_eth_dev_config_restore(port_id);
>  
> -       if (dev->data->dev_conf.intr_conf.lsc != 0) {
> +       if (dev->data->dev_conf.intr_conf.lsc == 0) {
>                 FUNC_PTR_OR_ERR_RET(*dev->dev_ops->link_update, -ENOTSUP);
>                 (*dev->dev_ops->link_update)(dev, 0);
>         }
> 
> 

Re: [dpdk-dev] [PATCH] librte: Link status interrupt race condition, IGB E1000

[Lu, Wenzhuo]

Hi Tim,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, October 26, 2015 6:56 AM
> To: Lu, Wenzhuo
> Cc: dev@dpdk.org; Tim Shearer
> Subject: Re: [dpdk-dev] [PATCH] librte: Link status interrupt race condition,
> IGB E1000
> 
> Wenzhuo,
> Please could you have a look?
> Thanks
> 
> 2015-09-24 20:44, Tim Shearer:
> > I encountered an issue with DPDK 2.1.0  which occasionally causes the link
> status interrupt callback not to be called after the interface is started for the
> first time. I traced the problem back to the function eth_igb_link_update(),
> which is used to determine if the link has changed state since the previous
> time it was called. It appears that this function can be called simultaneously
> from two different threads:
> >
> > (1) From the main application/configuration thread, via rte_eth_dev_start()
> - pointed to by (*dev->dev_ops->link_update)
> > (2) From the eal interrupt thread, via eth_igb_interrupt_action(), to check
> if the link state has transitioned up or down. The user callback is only
> executed if the link has changed state.
> >
> > The race condition manifests itself as follows:
> >  - Main thread configures the interface with link status interrupt (LSI)
> enabled, sets up the queues etc.
> >  - Main thread calls rte_eth_dev_start. The interface is started and then we
> call eth_igb_link_update()
> >  - While in this call, the link goes up. Accordingly, we  detect the transition,
> and write the new link state (up) into the global rte_eth_dev struct
> >  - The interrupt fires, which also drops into the eth_igb_link_update
> function, finds that the global link status has already been set to up (no
> change)
> >  - Therefore, the handler thinks the interrupt was spurious, and the
> callback doesn't get called.
> >
> > I suspect that rte_eth_dev_start shouldn't be checking the link state if
> interrupts are enabled. Would someone mind taking a quick look at the
> patch below?
> >
> > Thanks!
> > Tim
> >
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -1300,7 +1300,7 @@ rte_eth_dev_start(uint8_t port_id)
> >
> >         rte_eth_dev_config_restore(port_id);
> >
> > -       if (dev->data->dev_conf.intr_conf.lsc != 0) {
> > +       if (dev->data->dev_conf.intr_conf.lsc == 0) {
> >                 FUNC_PTR_OR_ERR_RET(*dev->dev_ops->link_update, -
> ENOTSUP);
> >                 (*dev->dev_ops->link_update)(dev, 0);
> >         }
> >
> >
> 
I think you're right. To my opinion, this if is added to avoid the race condition. So, it should be " dev->data->dev_conf.intr_conf.lsc == 0". It means if the interrupts are not enabled, we'd update the link when starting, if not we can leave it the interrupt handler.
Seems it's not an igb specific but common issue. 

Re: [dpdk-dev] [PATCH] librte: Link status interrupt race condition, IGB E1000

[Thomas Monjalon]

2015-10-26 05:25, Lu, Wenzhuo:
> I think you're right. To my opinion, this if is added to avoid the race condition. So, it should be " dev->data->dev_conf.intr_conf.lsc == 0". It means if the interrupts are not enabled, we'd update the link when starting, if not we can leave it the interrupt handler.
> Seems it's not an igb specific but common issue. 

Tim, please could you send an appropriate patch?
The procedure is described in http://dpdk.org/dev#send

Could you also check if other PMDs have the same bug?
Thanks

[dpdk-dev] [PATCH] lib/librte_ether: Prevent link status race condition when LSI enabled

[Tim Shearer]

Calling the Ethernet driver's link_update function from rte_eth_dev_start can result in a race condition if the NIC raises the link interrupt at the same time. Depending on the interrupt handler implementation, the race can cause the it to think that it received two consecutive link up interrupts, and it exits without calling the user callback. Appears to impact E1000/IGB and virtio drivers only.

Signed-off-by: Tim Shearer <tim.shearer@overturenetworks.com>
---
 lib/librte_ether/rte_ethdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index f593f6e..a821a1f 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1300,7 +1300,7 @@ rte_eth_dev_start(uint8_t port_id)
 
 	rte_eth_dev_config_restore(port_id);
 
-	if (dev->data->dev_conf.intr_conf.lsc != 0) {
+	if (dev->data->dev_conf.intr_conf.lsc == 0) {
 		FUNC_PTR_OR_ERR_RET(*dev->dev_ops->link_update, -ENOTSUP);
 		(*dev->dev_ops->link_update)(dev, 0);
 	}
-- 
1.7.2.3

Re: [dpdk-dev] [PATCH] lib/librte_ether: Prevent link status race condition when LSI enabled

[Thomas Monjalon]

2015-10-27 17:38, Tim Shearer:
> Calling the Ethernet driver's link_update function from rte_eth_dev_start can result in a race condition if the NIC raises the link interrupt at the same time. Depending on the interrupt handler implementation, the race can cause the it to think that it received two consecutive link up interrupts, and it exits without calling the user callback. Appears to impact E1000/IGB and virtio drivers only.
> 
> Signed-off-by: Tim Shearer <tim.shearer@overturenetworks.com>

Applied, thanks