[dpdk-dev] [PATCH] net/mlx5: fix link status initialization

[dpdk-dev] [PATCH] net/mlx5: fix link status initialization

[Shahaf Shuler]

Following commit 7ba5320baa32 ("net/mlx5: fix link status behavior")

The initial link status is no longer set as part of the port start.
This may cause application to query the link as down while in fact it was
already up before the DPDK application start.

Fixes: 7ba5320baa32 ("net/mlx5: fix link status behavior")
Cc: nelio.laranjeiro@6wind.com

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
Acked-by: Yongseok Koh <yskoh@mellanox.com>
---
 drivers/net/mlx5/mlx5.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 7d58d66bb9..f954ea2862 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -961,6 +961,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		DRV_LOG(DEBUG, "port %u forcing Ethernet interface up",
 			eth_dev->data->port_id);
 		mlx5_set_link_up(eth_dev);
+		mlx5_link_update(eth_dev, 1);
 		/* Store device configuration on private structure. */
 		priv->config = config;
 		continue;
-- 
2.12.0

Re: [dpdk-dev] [PATCH] net/mlx5: fix link status initialization

[Nélio Laranjeiro]

On Tue, Apr 03, 2018 at 07:48:17AM +0300, Shahaf Shuler wrote:
> Following commit 7ba5320baa32 ("net/mlx5: fix link status behavior")
> 
> The initial link status is no longer set as part of the port start.
> This may cause application to query the link as down while in fact it was
> already up before the DPDK application start.

There is something wrong in this explanation, the application should
query the link using this same callback, why the PMD should call it?

> Fixes: 7ba5320baa32 ("net/mlx5: fix link status behavior")
> Cc: nelio.laranjeiro@6wind.com
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> Acked-by: Yongseok Koh <yskoh@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index 7d58d66bb9..f954ea2862 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -961,6 +961,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>  		DRV_LOG(DEBUG, "port %u forcing Ethernet interface up",
>  			eth_dev->data->port_id);
>  		mlx5_set_link_up(eth_dev);
> +		mlx5_link_update(eth_dev, 1);
>  		/* Store device configuration on private structure. */
>  		priv->config = config;
>  		continue;
> -- 
> 2.12.0


-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix link status initialization

[Shahaf Shuler]

Wednesday, April 4, 2018 10:30 AM, Nélio Laranjeiro:
> Subject: Re: [PATCH] net/mlx5: fix link status initialization
> 
> On Tue, Apr 03, 2018 at 07:48:17AM +0300, Shahaf Shuler wrote:
> > Following commit 7ba5320baa32 ("net/mlx5: fix link status behavior")
> >
> > The initial link status is no longer set as part of the port start.
> > This may cause application to query the link as down while in fact it
> > was already up before the DPDK application start.
> 
> There is something wrong in this explanation, the application should query
> the link using this same callback, why the PMD should call it?

It is how ethdev is implemented. The application is doing nothing wrong, it queries the link status using rte_eth_link_get_nowait

When the application works with LSC interrupts the ethdev layer skips the PMD callback and just update according to the link status exists on device data.
It is because it assumes the link status on the device data is the correct one since any link change is processed by the application.

The issue is with the initial state of the link. If the link is already up when the PMD starts there will be no callback for the application. 

I think this logic is OK, and it is also a good practice to initialize the link status to the actual state of the link as part of the port probing. 

> 
> > Fixes: 7ba5320baa32 ("net/mlx5: fix link status behavior")
> > Cc: nelio.laranjeiro@6wind.com
> >
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > Acked-by: Yongseok Koh <yskoh@mellanox.com>
> > ---
> >  drivers/net/mlx5/mlx5.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > 7d58d66bb9..f954ea2862 100644
> > --- a/drivers/net/mlx5/mlx5.c
> > +++ b/drivers/net/mlx5/mlx5.c
> > @@ -961,6 +961,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> >  		DRV_LOG(DEBUG, "port %u forcing Ethernet interface up",
> >  			eth_dev->data->port_id);
> >  		mlx5_set_link_up(eth_dev);
> > +		mlx5_link_update(eth_dev, 1);
> >  		/* Store device configuration on private structure. */
> >  		priv->config = config;
> >  		continue;
> > --
> > 2.12.0
> 
> 
> --
> Nélio Laranjeiro
> 6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix link status initialization

[Nélio Laranjeiro]

On Wed, Apr 04, 2018 at 09:58:33AM +0000, Shahaf Shuler wrote:
> Wednesday, April 4, 2018 10:30 AM, Nélio Laranjeiro:
> > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > 
> > On Tue, Apr 03, 2018 at 07:48:17AM +0300, Shahaf Shuler wrote:
> > > Following commit 7ba5320baa32 ("net/mlx5: fix link status behavior")
> > >
> > > The initial link status is no longer set as part of the port start.
> > > This may cause application to query the link as down while in fact it
> > > was already up before the DPDK application start.
> > 
> > There is something wrong in this explanation, the application should query
> > the link using this same callback, why the PMD should call it?
> 
> It is how ethdev is implemented. The application is doing nothing
> wrong, it queries the link status using rte_eth_link_get_nowait
> 
> When the application works with LSC interrupts the ethdev layer skips
> the PMD callback and just update according to the link status exists
> on device data.
> It is because it assumes the link status on the device data is the
> correct one since any link change is processed by the application.
> 
> The issue is with the initial state of the link. If the link is
> already up when the PMD starts there will be no callback for the
> application. 
> 
> I think this logic is OK, and it is also a good practice to initialize
> the link status to the actual state of the link as part of the port
> probing. 

The commit log should be re-worded to include this explanation.

> > > Fixes: 7ba5320baa32 ("net/mlx5: fix link status behavior")
> > > Cc: nelio.laranjeiro@6wind.com
> > >
> > > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > > Acked-by: Yongseok Koh <yskoh@mellanox.com>
> > > ---
> > >  drivers/net/mlx5/mlx5.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > > 7d58d66bb9..f954ea2862 100644
> > > --- a/drivers/net/mlx5/mlx5.c
> > > +++ b/drivers/net/mlx5/mlx5.c
> > > @@ -961,6 +961,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> > __rte_unused,
> > >  		DRV_LOG(DEBUG, "port %u forcing Ethernet interface up",
> > >  			eth_dev->data->port_id);
> > >  		mlx5_set_link_up(eth_dev);
> > > +		mlx5_link_update(eth_dev, 1);
> > >  		/* Store device configuration on private structure. */
> > >  		priv->config = config;
> > >  		continue;
> > > --
> > > 2.12.0

According to your analysis, this is only necessary when the LCS is
configured in the device.  Why not adding this call to
mlx5_dev_interrupt_handler_install() which is responsible to install the
LCS callback.

Another point, the wait to complete flag is useless, if the link is up,
the status and speed will be accurate, if not, it will receive an LSC
event later.

Regards,

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix link status initialization

[Shahaf Shuler]

Wednesday, April 4, 2018 3:11 PM, Nélio Laranjeiro:
> Subject: Re: [PATCH] net/mlx5: fix link status initialization
> 
> On Wed, Apr 04, 2018 at 09:58:33AM +0000, Shahaf Shuler wrote:
> > Wednesday, April 4, 2018 10:30 AM, Nélio Laranjeiro:
> > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > >
> > > On Tue, Apr 03, 2018 at 07:48:17AM +0300, Shahaf Shuler wrote:
> > > > Following commit 7ba5320baa32 ("net/mlx5: fix link status
> > > > behavior")
> > > >
> > > > The initial link status is no longer set as part of the port start.
> > > > This may cause application to query the link as down while in fact
> > > > it was already up before the DPDK application start.
> > >
> > > There is something wrong in this explanation, the application should
> > > query the link using this same callback, why the PMD should call it?
> >
> > It is how ethdev is implemented. The application is doing nothing
> > wrong, it queries the link status using rte_eth_link_get_nowait
> >
> > When the application works with LSC interrupts the ethdev layer skips
> > the PMD callback and just update according to the link status exists
> > on device data.
> > It is because it assumes the link status on the device data is the
> > correct one since any link change is processed by the application.
> >
> > The issue is with the initial state of the link. If the link is
> > already up when the PMD starts there will be no callback for the
> > application.
> >
> > I think this logic is OK, and it is also a good practice to initialize
> > the link status to the actual state of the link as part of the port
> > probing.
> 
> The commit log should be re-worded to include this explanation.

Will add.

> 
> > > > Fixes: 7ba5320baa32 ("net/mlx5: fix link status behavior")
> > > > Cc: nelio.laranjeiro@6wind.com
> > > >
> > > > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > > > Acked-by: Yongseok Koh <yskoh@mellanox.com>
> > > > ---
> > > >  drivers/net/mlx5/mlx5.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> > > > index
> > > > 7d58d66bb9..f954ea2862 100644
> > > > --- a/drivers/net/mlx5/mlx5.c
> > > > +++ b/drivers/net/mlx5/mlx5.c
> > > > @@ -961,6 +961,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> > > __rte_unused,
> > > >  		DRV_LOG(DEBUG, "port %u forcing Ethernet interface up",
> > > >  			eth_dev->data->port_id);
> > > >  		mlx5_set_link_up(eth_dev);
> > > > +		mlx5_link_update(eth_dev, 1);
> > > >  		/* Store device configuration on private structure. */
> > > >  		priv->config = config;
> > > >  		continue;
> > > > --
> > > > 2.12.0
> 
> According to your analysis, this is only necessary when the LCS is configured
> in the device.  Why not adding this call to
> mlx5_dev_interrupt_handler_install() which is responsible to install the LCS
> callback.

I think it is good practice whether or not LSC is set. 
The link status should be initialized to the correct value after the probe.

> 
> Another point, the wait to complete flag is useless, if the link is up, the status
> and speed will be accurate, if not, it will receive an LSC event later.

Agree.

So how about keeping the code on the current place, just removing the wait_to_complete? 

> 
> Regards,
> 
> --
> Nélio Laranjeiro
> 6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix link status initialization

[Nélio Laranjeiro]

On Thu, Apr 05, 2018 at 05:35:57AM +0000, Shahaf Shuler wrote:
> Wednesday, April 4, 2018 3:11 PM, Nélio Laranjeiro:
> > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > 
> > On Wed, Apr 04, 2018 at 09:58:33AM +0000, Shahaf Shuler wrote:
> > > Wednesday, April 4, 2018 10:30 AM, Nélio Laranjeiro:
> > > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > > >
> > > > On Tue, Apr 03, 2018 at 07:48:17AM +0300, Shahaf Shuler wrote:
> > > > > Following commit 7ba5320baa32 ("net/mlx5: fix link status
> > > > > behavior")
> > > > >
> > > > > The initial link status is no longer set as part of the port start.
> > > > > This may cause application to query the link as down while in fact
> > > > > it was already up before the DPDK application start.
> > > >
> > > > There is something wrong in this explanation, the application should
> > > > query the link using this same callback, why the PMD should call it?
> > >
> > > It is how ethdev is implemented. The application is doing nothing
> > > wrong, it queries the link status using rte_eth_link_get_nowait
> > >
> > > When the application works with LSC interrupts the ethdev layer skips
> > > the PMD callback and just update according to the link status exists
> > > on device data.
> > > It is because it assumes the link status on the device data is the
> > > correct one since any link change is processed by the application.
> > >
> > > The issue is with the initial state of the link. If the link is
> > > already up when the PMD starts there will be no callback for the
> > > application.
> > >
> > > I think this logic is OK, and it is also a good practice to initialize
> > > the link status to the actual state of the link as part of the port
> > > probing.
> > 
> > The commit log should be re-worded to include this explanation.
> 
> Will add.
> 
> > 
> > > > > Fixes: 7ba5320baa32 ("net/mlx5: fix link status behavior")
> > > > > Cc: nelio.laranjeiro@6wind.com
> > > > >
> > > > > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > > > > Acked-by: Yongseok Koh <yskoh@mellanox.com>
> > > > > ---
> > > > >  drivers/net/mlx5/mlx5.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> > > > > index
> > > > > 7d58d66bb9..f954ea2862 100644
> > > > > --- a/drivers/net/mlx5/mlx5.c
> > > > > +++ b/drivers/net/mlx5/mlx5.c
> > > > > @@ -961,6 +961,7 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> > > > __rte_unused,
> > > > >  		DRV_LOG(DEBUG, "port %u forcing Ethernet interface up",
> > > > >  			eth_dev->data->port_id);
> > > > >  		mlx5_set_link_up(eth_dev);
> > > > > +		mlx5_link_update(eth_dev, 1);
> > > > >  		/* Store device configuration on private structure. */
> > > > >  		priv->config = config;
> > > > >  		continue;
> > > > > --
> > > > > 2.12.0
> > 
> > According to your analysis, this is only necessary when the LCS is configured
> > in the device.  Why not adding this call to
> > mlx5_dev_interrupt_handler_install() which is responsible to install the LCS
> > callback.
> 
> I think it is good practice whether or not LSC is set. 
> The link status should be initialized to the correct value after the probe.

There is no guarantee the link will be accurate, at probe time the link
may be up so internal information has a status up with a speed with this
patch.
The application probes a second port, in the mean time the link of the
first port goes down, the interrupt is still not installed and the
internal status becomes wrong (still up whereas the port is down).

Finally at start, the device installs the handler, but the link is still
down whereas internally it is up, the application will call
rte_eth_link_get_nowait() which will directly copy the wrong internal
status to the application.

There is also another situation, when the application stops the port,
the interrupt is also removed, which means during this time, the
internal status may be wrong as it won't be updated anymore.
This comes to the same possible situation as above.

> > Another point, the wait to complete flag is useless, if the link is up, the status
> > and speed will be accurate, if not, it will receive an LSC event later.
> 
> Agree.
> 
> So how about keeping the code on the current place, just removing the
> wait_to_complete? 

The current place is not fixing the issue as there is still a
possibility to have a wrong value.

Regards,

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix link status initialization

[Shahaf Shuler]

Thursday, April 5, 2018 9:51 AM, Nélio Laranjeiro:
> Subject: Re: [PATCH] net/mlx5: fix link status initialization
> 
> On Thu, Apr 05, 2018 at 05:35:57AM +0000, Shahaf Shuler wrote:
> > Wednesday, April 4, 2018 3:11 PM, Nélio Laranjeiro:
> > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > >
> > > On Wed, Apr 04, 2018 at 09:58:33AM +0000, Shahaf Shuler wrote:
> > > > Wednesday, April 4, 2018 10:30 AM, Nélio Laranjeiro:
> > > > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > > > >
> > > > > On Tue, Apr 03, 2018 at 07:48:17AM +0300, Shahaf Shuler wrote:

[..]

> > >
> > > According to your analysis, this is only necessary when the LCS is
> > > configured in the device.  Why not adding this call to
> > > mlx5_dev_interrupt_handler_install() which is responsible to install
> > > the LCS callback.
> >
> > I think it is good practice whether or not LSC is set.
> > The link status should be initialized to the correct value after the probe.
> 
> There is no guarantee the link will be accurate, at probe time the link may be
> up so internal information has a status up with a speed with this patch.
> The application probes a second port, in the mean time the link of the first
> port goes down, the interrupt is still not installed and the internal status
> becomes wrong (still up whereas the port is down).
> 
> Finally at start, the device installs the handler, but the link is still down
> whereas internally it is up, the application will call
> rte_eth_link_get_nowait() which will directly copy the wrong internal status
> to the application.

This is not correct.
Using Verbs, the async_fd on which link status interrupts are reported is created on probe. 
Even if the interrupt handler is not installed, interrupts still trigger on this fd. They will be processed when the interrupt handler will be installed as part of the port start. 
So in fact you have the whole trace on the link status changes waiting to be processed upon port start. 

Please try and see. 

> 
> There is also another situation, when the application stops the port, the
> interrupt is also removed, which means during this time, the internal status
> may be wrong as it won't be updated anymore.
> This comes to the same possible situation as above.

Same comment. 

> 
> > > Another point, the wait to complete flag is useless, if the link is
> > > up, the status and speed will be accurate, if not, it will receive an LSC
> event later.
> >
> > Agree.
> >
> > So how about keeping the code on the current place, just removing the
> > wait_to_complete?
> 
> The current place is not fixing the issue as there is still a possibility to have a
> wrong value.
> 
> Regards,
> 
> --
> Nélio Laranjeiro
> 6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix link status initialization

[Nélio Laranjeiro]

On Sun, Apr 08, 2018 at 01:09:27PM +0000, Shahaf Shuler wrote:
> Thursday, April 5, 2018 9:51 AM, Nélio Laranjeiro:
> > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > 
> > On Thu, Apr 05, 2018 at 05:35:57AM +0000, Shahaf Shuler wrote:
> > > Wednesday, April 4, 2018 3:11 PM, Nélio Laranjeiro:
> > > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > > >
> > > > On Wed, Apr 04, 2018 at 09:58:33AM +0000, Shahaf Shuler wrote:
> > > > > Wednesday, April 4, 2018 10:30 AM, Nélio Laranjeiro:
> > > > > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > > > > >
> > > > > > On Tue, Apr 03, 2018 at 07:48:17AM +0300, Shahaf Shuler wrote:
> 
> [..]
> 
> > > >
> > > > According to your analysis, this is only necessary when the LCS is
> > > > configured in the device.  Why not adding this call to
> > > > mlx5_dev_interrupt_handler_install() which is responsible to install
> > > > the LCS callback.
> > >
> > > I think it is good practice whether or not LSC is set.
> > > The link status should be initialized to the correct value after the probe.
> > 
> > There is no guarantee the link will be accurate, at probe time the link may be
> > up so internal information has a status up with a speed with this patch.
> > The application probes a second port, in the mean time the link of the first
> > port goes down, the interrupt is still not installed and the internal status
> > becomes wrong (still up whereas the port is down).
> > 
> > Finally at start, the device installs the handler, but the link is still down
> > whereas internally it is up, the application will call
> > rte_eth_link_get_nowait() which will directly copy the wrong internal status
> > to the application.
> 
> This is not correct.
> Using Verbs, the async_fd on which link status interrupts are reported is created on probe. 
> Even if the interrupt handler is not installed, interrupts still
> trigger on this fd. They will be processed when the interrupt handler
> will be installed as part of the port start. 
> So in fact you have the whole trace on the link status changes waiting
> to be processed upon port start. 

Right, but in such case, this patch still does not solves the issue.
Until the dev_start() is called, the link may be inconsistent with the
real status.

example: 

 pci_probe --> link is up.
 leaving pci probe, the link goes downs --> internally the PMD has a link up.

Until the dev_start() is called any call to rte_eth_link_get_nowait()
will copy the internal PMD status which is not accurate.

>From this point, the issue seems to fully come from
rte_eth_link_get_nowait() which should not make any copy until the port
is not started, until then the link may be inconsistent between the
driver and the device.

Regards,

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix link status initialization

[Shahaf Shuler]

Monday, April 9, 2018 11:28 AM, Nélio Laranjeiro:
> Subject: Re: [PATCH] net/mlx5: fix link status initialization
> 
> On Sun, Apr 08, 2018 at 01:09:27PM +0000, Shahaf Shuler wrote:
> > Thursday, April 5, 2018 9:51 AM, Nélio Laranjeiro:
> > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > >
> > > On Thu, Apr 05, 2018 at 05:35:57AM +0000, Shahaf Shuler wrote:
> > > > Wednesday, April 4, 2018 3:11 PM, Nélio Laranjeiro:
> > > > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > > > >
> > > > > On Wed, Apr 04, 2018 at 09:58:33AM +0000, Shahaf Shuler wrote:
> > > > > > Wednesday, April 4, 2018 10:30 AM, Nélio Laranjeiro:
> > > > > > > Subject: Re: [PATCH] net/mlx5: fix link status
> > > > > > > initialization
> > > > > > >
> > > > > > > On Tue, Apr 03, 2018 at 07:48:17AM +0300, Shahaf Shuler wrote:
> >
> > [..]
> >
> > > > >
> > > > > According to your analysis, this is only necessary when the LCS
> > > > > is configured in the device.  Why not adding this call to
> > > > > mlx5_dev_interrupt_handler_install() which is responsible to
> > > > > install the LCS callback.
> > > >
> > > > I think it is good practice whether or not LSC is set.
> > > > The link status should be initialized to the correct value after the probe.
> > >
> > > There is no guarantee the link will be accurate, at probe time the
> > > link may be up so internal information has a status up with a speed with
> this patch.
> > > The application probes a second port, in the mean time the link of
> > > the first port goes down, the interrupt is still not installed and
> > > the internal status becomes wrong (still up whereas the port is down).
> > >
> > > Finally at start, the device installs the handler, but the link is
> > > still down whereas internally it is up, the application will call
> > > rte_eth_link_get_nowait() which will directly copy the wrong
> > > internal status to the application.
> >
> > This is not correct.
> > Using Verbs, the async_fd on which link status interrupts are reported is
> created on probe.
> > Even if the interrupt handler is not installed, interrupts still
> > trigger on this fd. They will be processed when the interrupt handler
> > will be installed as part of the port start.
> > So in fact you have the whole trace on the link status changes waiting
> > to be processed upon port start.
> 
> Right, but in such case, this patch still does not solves the issue.
> Until the dev_start() is called, the link may be inconsistent with the real
> status.
> 
> example:
> 
>  pci_probe --> link is up.
>  leaving pci probe, the link goes downs --> internally the PMD has a link up.
> 
> Until the dev_start() is called any call to rte_eth_link_get_nowait() will copy
> the internal PMD status which is not accurate.
> 
> From this point, the issue seems to fully come from
> rte_eth_link_get_nowait() which should not make any copy until the port is
> not started, until then the link may be inconsistent between the driver and
> the device.

Right. However fixing it only on the ethdev layer is not enough.
Assume the link was up from the beginning. 
For the case were the call for rte_eth_link_get_nowait happens only after the port start, the link will still be wrong as it will be reported as down (and no interrupt to fix it). 

So to summarize I plan to have this patch (with modification of the wait_to_complete) along with another fix for ethdev. 
Do we have an agreement? 


> 
> Regards,
> 
> --
> Nélio Laranjeiro
> 6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix link status initialization

[Nélio Laranjeiro]

On Mon, Apr 09, 2018 at 12:28:04PM +0000, Shahaf Shuler wrote:
> Monday, April 9, 2018 11:28 AM, Nélio Laranjeiro:
> > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > 
> > On Sun, Apr 08, 2018 at 01:09:27PM +0000, Shahaf Shuler wrote:
> > > Thursday, April 5, 2018 9:51 AM, Nélio Laranjeiro:
> > > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > > >
> > > > On Thu, Apr 05, 2018 at 05:35:57AM +0000, Shahaf Shuler wrote:
> > > > > Wednesday, April 4, 2018 3:11 PM, Nélio Laranjeiro:
> > > > > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > > > > >
> > > > > > On Wed, Apr 04, 2018 at 09:58:33AM +0000, Shahaf Shuler wrote:
> > > > > > > Wednesday, April 4, 2018 10:30 AM, Nélio Laranjeiro:
> > > > > > > > Subject: Re: [PATCH] net/mlx5: fix link status
> > > > > > > > initialization
> > > > > > > >
> > > > > > > > On Tue, Apr 03, 2018 at 07:48:17AM +0300, Shahaf Shuler wrote:
> > >
> > > [..]
> > >
> > > > > >
> > > > > > According to your analysis, this is only necessary when the LCS
> > > > > > is configured in the device.  Why not adding this call to
> > > > > > mlx5_dev_interrupt_handler_install() which is responsible to
> > > > > > install the LCS callback.
> > > > >
> > > > > I think it is good practice whether or not LSC is set.
> > > > > The link status should be initialized to the correct value after the probe.
> > > >
> > > > There is no guarantee the link will be accurate, at probe time the
> > > > link may be up so internal information has a status up with a speed with
> > this patch.
> > > > The application probes a second port, in the mean time the link of
> > > > the first port goes down, the interrupt is still not installed and
> > > > the internal status becomes wrong (still up whereas the port is down).
> > > >
> > > > Finally at start, the device installs the handler, but the link is
> > > > still down whereas internally it is up, the application will call
> > > > rte_eth_link_get_nowait() which will directly copy the wrong
> > > > internal status to the application.
> > >
> > > This is not correct.
> > > Using Verbs, the async_fd on which link status interrupts are reported is
> > created on probe.
> > > Even if the interrupt handler is not installed, interrupts still
> > > trigger on this fd. They will be processed when the interrupt handler
> > > will be installed as part of the port start.
> > > So in fact you have the whole trace on the link status changes waiting
> > > to be processed upon port start.
> > 
> > Right, but in such case, this patch still does not solves the issue.
> > Until the dev_start() is called, the link may be inconsistent with the real
> > status.
> > 
> > example:
> > 
> >  pci_probe --> link is up.
> >  leaving pci probe, the link goes downs --> internally the PMD has a link up.
> > 
> > Until the dev_start() is called any call to rte_eth_link_get_nowait() will copy
> > the internal PMD status which is not accurate.
> > 
> > From this point, the issue seems to fully come from
> > rte_eth_link_get_nowait() which should not make any copy until the port is
> > not started, until then the link may be inconsistent between the driver and
> > the device.
> 
> Right. However fixing it only on the ethdev layer is not enough.
>
> Assume the link was up from the beginning.
> For the case were the call for rte_eth_link_get_nowait happens only
> after the port start, the link will still be wrong as it will be
> reported as down (and no interrupt to fix it). 
>
> So to summarize I plan to have this patch (with modification of the
> wait_to_complete) along with another fix for ethdev. 
> Do we have an agreement? 

Sorry but no, as MLX5 is not the only PMD impacted by this issue and a
solution can be done for all of them (ixgbe, virtio, MLX5 I did not
check the others).

Your proposition does not solves the following case neither, when the
link goes down and the application stops the port due to that, if the
application waits using the same API to restart the port, it will never
occur as the interrupts handlers are no more installed and thus the
internal link will never be updated remaining indefinitely down.  This
also impact the three PMD above.

Commit b77d21cc2364 ("ethdev: add link status get/set helper functions")
has been integrated a little too fast introducing this bug.

All those issues can be fixed by adding another statement in the if of
the function rte_eth_link_get_nowait()

-       if (dev->data->dev_conf.intr_conf.lsc)
+       if (dev->data->dev_conf.intr_conf.lsc && dev->data->dev_started)
                rte_eth_linkstatus_get(dev, eth_link);
        else {
                RTE_FUNC_PTR_OR_RET(*dev->dev_ops->link_update);
                (*dev->dev_ops->link_update)(dev, 1);
                *eth_link = dev->data->dev_link;
        }

Doing this fixes all PMD having the same bug, and remove your specific
patch for MLX5 which becomes useless.

Regards,

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix link status initialization

[Nélio Laranjeiro]

On Mon, Apr 09, 2018 at 03:26:41PM +0200, Nélio Laranjeiro wrote:
> On Mon, Apr 09, 2018 at 12:28:04PM +0000, Shahaf Shuler wrote:
> > Monday, April 9, 2018 11:28 AM, Nélio Laranjeiro:
> > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > > 
> > > On Sun, Apr 08, 2018 at 01:09:27PM +0000, Shahaf Shuler wrote:
> > > > Thursday, April 5, 2018 9:51 AM, Nélio Laranjeiro:
> > > > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > > > >
> > > > > On Thu, Apr 05, 2018 at 05:35:57AM +0000, Shahaf Shuler wrote:
> > > > > > Wednesday, April 4, 2018 3:11 PM, Nélio Laranjeiro:
> > > > > > > Subject: Re: [PATCH] net/mlx5: fix link status initialization
> > > > > > >
> > > > > > > On Wed, Apr 04, 2018 at 09:58:33AM +0000, Shahaf Shuler wrote:
> > > > > > > > Wednesday, April 4, 2018 10:30 AM, Nélio Laranjeiro:
> > > > > > > > > Subject: Re: [PATCH] net/mlx5: fix link status
> > > > > > > > > initialization
> > > > > > > > >
> > > > > > > > > On Tue, Apr 03, 2018 at 07:48:17AM +0300, Shahaf Shuler wrote:
> > > >
> > > > [..]
> > > >
> > > > > > >
> > > > > > > According to your analysis, this is only necessary when the LCS
> > > > > > > is configured in the device.  Why not adding this call to
> > > > > > > mlx5_dev_interrupt_handler_install() which is responsible to
> > > > > > > install the LCS callback.
> > > > > >
> > > > > > I think it is good practice whether or not LSC is set.
> > > > > > The link status should be initialized to the correct value after the probe.
> > > > >
> > > > > There is no guarantee the link will be accurate, at probe time the
> > > > > link may be up so internal information has a status up with a speed with
> > > this patch.
> > > > > The application probes a second port, in the mean time the link of
> > > > > the first port goes down, the interrupt is still not installed and
> > > > > the internal status becomes wrong (still up whereas the port is down).
> > > > >
> > > > > Finally at start, the device installs the handler, but the link is
> > > > > still down whereas internally it is up, the application will call
> > > > > rte_eth_link_get_nowait() which will directly copy the wrong
> > > > > internal status to the application.
> > > >
> > > > This is not correct.
> > > > Using Verbs, the async_fd on which link status interrupts are reported is
> > > created on probe.
> > > > Even if the interrupt handler is not installed, interrupts still
> > > > trigger on this fd. They will be processed when the interrupt handler
> > > > will be installed as part of the port start.
> > > > So in fact you have the whole trace on the link status changes waiting
> > > > to be processed upon port start.
> > > 
> > > Right, but in such case, this patch still does not solves the issue.
> > > Until the dev_start() is called, the link may be inconsistent with the real
> > > status.
> > > 
> > > example:
> > > 
> > >  pci_probe --> link is up.
> > >  leaving pci probe, the link goes downs --> internally the PMD has a link up.
> > > 
> > > Until the dev_start() is called any call to rte_eth_link_get_nowait() will copy
> > > the internal PMD status which is not accurate.
> > > 
> > > From this point, the issue seems to fully come from
> > > rte_eth_link_get_nowait() which should not make any copy until the port is
> > > not started, until then the link may be inconsistent between the driver and
> > > the device.
> > 
> > Right. However fixing it only on the ethdev layer is not enough.
> >
> > Assume the link was up from the beginning.
> > For the case were the call for rte_eth_link_get_nowait happens only
> > after the port start, the link will still be wrong as it will be
> > reported as down (and no interrupt to fix it). 
> >
> > So to summarize I plan to have this patch (with modification of the
> > wait_to_complete) along with another fix for ethdev. 
> > Do we have an agreement? 
> 
> Sorry but no, as MLX5 is not the only PMD impacted by this issue and a
> solution can be done for all of them (ixgbe, virtio, MLX5 I did not
> check the others).
> 
> Your proposition does not solves the following case neither, when the
> link goes down and the application stops the port due to that, if the
> application waits using the same API to restart the port, it will never
> occur as the interrupts handlers are no more installed and thus the
> internal link will never be updated remaining indefinitely down.  This
> also impact the three PMD above.
> 
> Commit b77d21cc2364 ("ethdev: add link status get/set helper functions")
> has been integrated a little too fast introducing this bug.
> 
> All those issues can be fixed by adding another statement in the if of
> the function rte_eth_link_get_nowait()
> 
> -       if (dev->data->dev_conf.intr_conf.lsc)
> +       if (dev->data->dev_conf.intr_conf.lsc && dev->data->dev_started)
>                 rte_eth_linkstatus_get(dev, eth_link);
>         else {
>                 RTE_FUNC_PTR_OR_RET(*dev->dev_ops->link_update);
>                 (*dev->dev_ops->link_update)(dev, 1);
>                 *eth_link = dev->data->dev_link;
>         }
> 
> Doing this fixes all PMD having the same bug, and remove your specific
> patch for MLX5 which becomes useless.

Making our private little discussion public.

Indeed with my proposal MLX5 still needs to make a first link update to
have the internal value set in case the link is consistent until the
dev_start() (i.e. up before the probing), otherwise, the returned value
from rte_eth_link_get_nowait() will be down when the device is started.

Regards,

-- 
Nélio Laranjeiro
6WIND

[dpdk-dev] [PATCH v2] net/mlx5: fix link status initialization

[Shahaf Shuler]

Following commit 7ba5320baa32 ("net/mlx5: fix link status behavior")
The initial link status is no longer set as part of the port start.

When LSC interrupts are enabled, ethdev layer reads the link status
directly from the device data instead of using the PMD callback.
This may cause application to query the link as down while in fact it was
already up before the DPDK application start (and no interrupt to fix
it).

Fixes: 7ba5320baa32 ("net/mlx5: fix link status behavior")
Cc: nelio.laranjeiro@6wind.com

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
Acked-by: Yongseok Koh <yskoh@mellanox.com>
---

On v2:
 - Reworded commit log.
 - Cleared the wait_to_complete on the link update call.

---
 drivers/net/mlx5/mlx5.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index cfab558979..3d222df707 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -991,6 +991,12 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		DRV_LOG(DEBUG, "port %u forcing Ethernet interface up",
 			eth_dev->data->port_id);
 		mlx5_set_link_up(eth_dev);
+		/*
+		 * Even though the interrupt handler is not installed yet,
+		 * interrupts will still trigger on the asyn_fd from
+		 * Verbs context returned by ibv_open_device().
+		 */
+		mlx5_link_update(eth_dev, 0);
 		/* Store device configuration on private structure. */
 		priv->config = config;
 		continue;
-- 
2.12.0

Re: [dpdk-dev] [PATCH v2] net/mlx5: fix link status initialization

[Nélio Laranjeiro]

On Tue, Apr 10, 2018 at 09:13:36AM +0300, Shahaf Shuler wrote:
> Following commit 7ba5320baa32 ("net/mlx5: fix link status behavior")
> The initial link status is no longer set as part of the port start.
> 
> When LSC interrupts are enabled, ethdev layer reads the link status
> directly from the device data instead of using the PMD callback.
> This may cause application to query the link as down while in fact it was
> already up before the DPDK application start (and no interrupt to fix
> it).
> 
> Fixes: 7ba5320baa32 ("net/mlx5: fix link status behavior")
> Cc: nelio.laranjeiro@6wind.com
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> Acked-by: Yongseok Koh <yskoh@mellanox.com>
> ---
> 
> On v2:
>  - Reworded commit log.
>  - Cleared the wait_to_complete on the link update call.
> 
> ---
>  drivers/net/mlx5/mlx5.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
> index cfab558979..3d222df707 100644
> --- a/drivers/net/mlx5/mlx5.c
> +++ b/drivers/net/mlx5/mlx5.c
> @@ -991,6 +991,12 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>  		DRV_LOG(DEBUG, "port %u forcing Ethernet interface up",
>  			eth_dev->data->port_id);
>  		mlx5_set_link_up(eth_dev);
> +		/*
> +		 * Even though the interrupt handler is not installed yet,
> +		 * interrupts will still trigger on the asyn_fd from
> +		 * Verbs context returned by ibv_open_device().
> +		 */
> +		mlx5_link_update(eth_dev, 0);
>  		/* Store device configuration on private structure. */
>  		priv->config = config;
>  		continue;
> -- 
> 2.12.0

You should have linked this patch with the ethdev one[1], if the ethdev is
refused, this patch won't solve anything.

With a reserves of acceptance of [1]: 
Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

Thanks,

[1] https://dpdk.org/ml/archives/dev/2018-April/096387.html
-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH v2] net/mlx5: fix link status initialization

[Shahaf Shuler]

Tuesday, April 10, 2018 11:18 AM, Nélio Laranjeiro:
> Subject: Re: [PATCH v2] net/mlx5: fix link status initialization
> 
> On Tue, Apr 10, 2018 at 09:13:36AM +0300, Shahaf Shuler wrote:
> > Following commit 7ba5320baa32 ("net/mlx5: fix link status behavior")
> > The initial link status is no longer set as part of the port start.
> >
> > When LSC interrupts are enabled, ethdev layer reads the link status
> > directly from the device data instead of using the PMD callback.
> > This may cause application to query the link as down while in fact it
> > was already up before the DPDK application start (and no interrupt to
> > fix it).
> >
> > Fixes: 7ba5320baa32 ("net/mlx5: fix link status behavior")
> > Cc: nelio.laranjeiro@6wind.com
> >
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > Acked-by: Yongseok Koh <yskoh@mellanox.com>
> > ---
> >
> > On v2:
> >  - Reworded commit log.
> >  - Cleared the wait_to_complete on the link update call.
> >
> > ---
> >  drivers/net/mlx5/mlx5.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index
> > cfab558979..3d222df707 100644
> > --- a/drivers/net/mlx5/mlx5.c
> > +++ b/drivers/net/mlx5/mlx5.c
> > @@ -991,6 +991,12 @@ mlx5_pci_probe(struct rte_pci_driver *pci_drv
> __rte_unused,
> >  		DRV_LOG(DEBUG, "port %u forcing Ethernet interface up",
> >  			eth_dev->data->port_id);
> >  		mlx5_set_link_up(eth_dev);
> > +		/*
> > +		 * Even though the interrupt handler is not installed yet,
> > +		 * interrupts will still trigger on the asyn_fd from
> > +		 * Verbs context returned by ibv_open_device().
> > +		 */
> > +		mlx5_link_update(eth_dev, 0);
> >  		/* Store device configuration on private structure. */
> >  		priv->config = config;
> >  		continue;
> > --
> > 2.12.0
> 
> You should have linked this patch with the ethdev one[1], if the ethdev is
> refused, this patch won't solve anything.
> 
> With a reserves of acceptance of [1]:
> Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>

As the ethdev pathc applied -> Applied to next-net-mlx, thanks