[dpdk-dev,v6,0/4] support reset of VF link

  On Mon, 2016-07-11 at 13:02 +0100, Luca Boccassi wrote:
> On Mon, 2016-07-11 at 01:32 +0000, Lu, Wenzhuo wrote:

> > > 

> > > Unfortunately I found one issue: if PF is down, and then the VF on the guest is

> > > down as well (ip link down) and then goes back up before the PF, then calling

> > > rte_eth_dev_reset will return 0 (success), even though the PF is still down and it

> > > should fail. This is with ixgbe. Any idea what could be the problem?

> > I've found this interesting thing. I believe it’s the HW difference between igb and ixgbe. When the link is down, ixgbe VF can be reset successfully but igb VF cannot. The expression is the  registers of the ixgbe VF can be accessed when the PF link is down but igb VF cannot.

> > It means, on ixgbe, when PF link is down, we reset the VF link. Then PF link is up, we receive the message again and reset the VF link again. 

> 

> What message do you refer to here? I am seeing the RESET callback only

> when the PF goes down, not when it goes up.

> 

> At the moment, with ixgbe, this happens:

> 

> PF down -> reset notification, rte_eth_dev_reset keeps failing -> VF

> down -> VF up -> rte_eth_dev_reset in a loop/timer succeeds -> PF up ->

> VF link has no-carrier, and traffic does NOT go through

> 

> The problem is that there is just no way of being notified that PF is

> up, and if rte_eth_dev_reset succeeds I have no way of knowing that I

> need to run it again.


I was now able to solve this use case, by having the rte_eth_dev_reset
implementations return -EAGAIN if the dev is not up. This way I know, in
the application, that I have to try again. What do you think?

IMHO it makes sense, as the reset does not actually succeeds, and the
caller should try again. The diff is very trivial, and attached for
reference.

-- 
Kind regards,
Luca Boccassi


Make rte_eth_dev_reset return EAGAIN if VF down

If VF is down the reset will not happen, so the driver should return
EAGAIN to signal the application that it needs to call again
rte_eth_dev_reset.

Signed-off-by: Luca Boccassi <lboccass@brocade.com

---
 drivers/net/e1000/igb_ethdev.c    |    2 +-
 drivers/net/i40e/i40e_ethdev_vf.c |    2 +-
 drivers/net/ixgbe/ixgbe_ethdev.c  |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
  

> -----Original Message-----

> From: Luca Boccassi [mailto:lboccass@Brocade.com]

> Sent: Monday, July 11, 2016 11:43 PM

> To: Lu, Wenzhuo

> Cc: dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH v6 0/4] support reset of VF link

> 

> On Mon, 2016-07-11 at 13:02 +0100, Luca Boccassi wrote:

> > On Mon, 2016-07-11 at 01:32 +0000, Lu, Wenzhuo wrote:

> > > >

> > > > Unfortunately I found one issue: if PF is down, and then the VF on

> > > > the guest is down as well (ip link down) and then goes back up

> > > > before the PF, then calling rte_eth_dev_reset will return 0

> > > > (success), even though the PF is still down and it should fail. This is with

> ixgbe. Any idea what could be the problem?

> > > I've found this interesting thing. I believe it’s the HW difference between igb

> and ixgbe. When the link is down, ixgbe VF can be reset successfully but igb VF

> cannot. The expression is the  registers of the ixgbe VF can be accessed when

> the PF link is down but igb VF cannot.

> > > It means, on ixgbe, when PF link is down, we reset the VF link. Then PF link is

> up, we receive the message again and reset the VF link again.

> >

> > What message do you refer to here? I am seeing the RESET callback only

> > when the PF goes down, not when it goes up.

> >

> > At the moment, with ixgbe, this happens:

> >

> > PF down -> reset notification, rte_eth_dev_reset keeps failing -> VF

> > down -> VF up -> rte_eth_dev_reset in a loop/timer succeeds -> PF up

> > -> VF link has no-carrier, and traffic does NOT go through

> >

> > The problem is that there is just no way of being notified that PF is

> > up, and if rte_eth_dev_reset succeeds I have no way of knowing that I

> > need to run it again.

> 

> I was now able to solve this use case, by having the rte_eth_dev_reset

> implementations return -EAGAIN if the dev is not up. This way I know, in the

> application, that I have to try again. What do you think?

> 

> IMHO it makes sense, as the reset does not actually succeeds, and the caller

> should try again. The diff is very trivial, and attached for reference.

Yes, I think the change is reasonable. Sorry, I didn’t realize you're talking about the code you have changed. Maybe we're not on the same page when discussing before :)

> 

> --

> Kind regards,

> Luca Boccassi

> 

> 

> Make rte_eth_dev_reset return EAGAIN if VF down

> 

> If VF is down the reset will not happen, so the driver should return

> EAGAIN to signal the application that it needs to call again

> rte_eth_dev_reset.

> 

> Signed-off-by: Luca Boccassi <lboccass@brocade.com

> ---

>  drivers/net/e1000/igb_ethdev.c    |    2 +-

>  drivers/net/i40e/i40e_ethdev_vf.c |    2 +-

>  drivers/net/ixgbe/ixgbe_ethdev.c  |    2 +-

>  3 files changed, 3 insertions(+), 3 deletions(-)

> 

> --- a/drivers/net/ixgbe/ixgbe_ethdev.c

> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c

> @@ -6235,7 +6235,7 @@ ixgbevf_dev_reset(struct rte_eth_dev *de

> 

>  	/* Nothing needs to be done if the device is not started. */

>  	if (!dev->data->dev_started)

> -		 return 0;

> +		 return -EAGAIN;

> 

>  	PMD_DRV_LOG(DEBUG, "Link up/down event detected.");

> 

> --- a/drivers/net/i40e/i40e_ethdev_vf.c

> +++ b/drivers/net/i40e/i40e_ethdev_vf.c

> @@ -1504,7 +1504,7 @@ i40evf_handle_vf_reset(struct rte_eth_de

>  		 I40E_DEV_PRIVATE_TO_ADAPTER(dev->data->dev_private);

> 

>  	if (!dev->data->dev_started)

> -		 return 0;

> +		 return -EAGAIN;

> 

>  	adapter->reset_number = 1;

>  	i40e_vf_reset_dev(dev);

> --- a/drivers/net/e1000/igb_ethdev.c

> +++ b/drivers/net/e1000/igb_ethdev.c

> @@ -2609,7 +2609,7 @@ igbvf_dev_reset(struct rte_eth_dev *dev,

> 

>  	/* Nothing needs to be done if the device is not started. */

>  	if (!dev->data->dev_started)

> -		 return 0;

> +		 return -EAGAIN;

> 

>  	PMD_DRV_LOG(DEBUG, "Link up/down event detected.");

>
  

On Mon, 2016-07-11 at 15:43 +0000, Luca Boccassi wrote:
> On Mon, 2016-07-11 at 13:02 +0100, Luca Boccassi wrote:

> > On Mon, 2016-07-11 at 01:32 +0000, Lu, Wenzhuo wrote:

> > > > 

> > > > Unfortunately I found one issue: if PF is down, and then the VF on the guest is

> > > > down as well (ip link down) and then goes back up before the PF, then calling

> > > > rte_eth_dev_reset will return 0 (success), even though the PF is still down and it

> > > > should fail. This is with ixgbe. Any idea what could be the problem?

> > > I've found this interesting thing. I believe it’s the HW difference between igb and ixgbe. When the link is down, ixgbe VF can be reset successfully but igb VF cannot. The expression is the  registers of the ixgbe VF can be accessed when the PF link is down but igb VF cannot.

> > > It means, on ixgbe, when PF link is down, we reset the VF link. Then PF link is up, we receive the message again and reset the VF link again. 

> > 

> > What message do you refer to here? I am seeing the RESET callback only

> > when the PF goes down, not when it goes up.

> > 

> > At the moment, with ixgbe, this happens:

> > 

> > PF down -> reset notification, rte_eth_dev_reset keeps failing -> VF

> > down -> VF up -> rte_eth_dev_reset in a loop/timer succeeds -> PF up ->

> > VF link has no-carrier, and traffic does NOT go through

> > 

> > The problem is that there is just no way of being notified that PF is

> > up, and if rte_eth_dev_reset succeeds I have no way of knowing that I

> > need to run it again.

> 

> I was now able to solve this use case, by having the rte_eth_dev_reset

> implementations return -EAGAIN if the dev is not up. This way I know, in

> the application, that I have to try again. What do you think?

> 

> IMHO it makes sense, as the reset does not actually succeeds, and the

> caller should try again. The diff is very trivial, and attached for

> reference.


Hi,

Is there any update on resubmitting this patchset for 16.11? Thanks!

-- 
Kind regards,
Luca Boccassi
  

Hi Luca,


> -----Original Message-----

> From: Luca Boccassi [mailto:lboccass@Brocade.com]

> Sent: Friday, August 26, 2016 8:58 PM

> To: Lu, Wenzhuo

> Cc: dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH v6 0/4] support reset of VF link

> 

> On Mon, 2016-07-11 at 15:43 +0000, Luca Boccassi wrote:

> > On Mon, 2016-07-11 at 13:02 +0100, Luca Boccassi wrote:

> > > On Mon, 2016-07-11 at 01:32 +0000, Lu, Wenzhuo wrote:

> > > > >

> > > > > Unfortunately I found one issue: if PF is down, and then the VF

> > > > > on the guest is down as well (ip link down) and then goes back

> > > > > up before the PF, then calling rte_eth_dev_reset will return 0

> > > > > (success), even though the PF is still down and it should fail. This is with

> ixgbe. Any idea what could be the problem?

> > > > I've found this interesting thing. I believe it’s the HW difference between

> igb and ixgbe. When the link is down, ixgbe VF can be reset successfully but igb

> VF cannot. The expression is the  registers of the ixgbe VF can be accessed when

> the PF link is down but igb VF cannot.

> > > > It means, on ixgbe, when PF link is down, we reset the VF link. Then PF link

> is up, we receive the message again and reset the VF link again.

> > >

> > > What message do you refer to here? I am seeing the RESET callback

> > > only when the PF goes down, not when it goes up.

> > >

> > > At the moment, with ixgbe, this happens:

> > >

> > > PF down -> reset notification, rte_eth_dev_reset keeps failing -> VF

> > > down -> VF up -> rte_eth_dev_reset in a loop/timer succeeds -> PF up

> > > -> VF link has no-carrier, and traffic does NOT go through

> > >

> > > The problem is that there is just no way of being notified that PF

> > > is up, and if rte_eth_dev_reset succeeds I have no way of knowing

> > > that I need to run it again.

> >

> > I was now able to solve this use case, by having the rte_eth_dev_reset

> > implementations return -EAGAIN if the dev is not up. This way I know,

> > in the application, that I have to try again. What do you think?

> >

> > IMHO it makes sense, as the reset does not actually succeeds, and the

> > caller should try again. The diff is very trivial, and attached for

> > reference.

> 

> Hi,

> 

> Is there any update on resubmitting this patchset for 16.11? Thanks!

Sorry, we're short of hands, so this feature is planned to R17.02.

> 

> --

> Kind regards,

> Luca Boccassi