diff mbox

[dpdk-dev,3/3] net/mlx5: rebuild flows on updating RETA

Message ID 20170316224056.19685-4-yskoh@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

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

Commit Message

Yongseok Koh March 16, 2017, 10:40 p.m. UTC 
  Currently mlx5_dev_rss_reta_update() just updates tables in the host,
therefore it isn't immediately effective until restarting the device by
calling mlx5_dev_stop()/mlx5_dev_start() to update the changes in the
device side. This patch adds rebuilding the device-specific datastructure
and applying it to the device right away.

Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
---
 drivers/net/mlx5/mlx5_rss.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Nélio Laranjeiro March 17, 2017, 9:11 a.m. UTC | #1 
On Thu, Mar 16, 2017 at 03:40:56PM -0700, Yongseok Koh wrote:
> Currently mlx5_dev_rss_reta_update() just updates tables in the host,
> therefore it isn't immediately effective until restarting the device by
> calling mlx5_dev_stop()/mlx5_dev_start() to update the changes in the
> device side. This patch adds rebuilding the device-specific datastructure
> and applying it to the device right away.
> 
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_rss.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/mlx5/mlx5_rss.c b/drivers/net/mlx5/mlx5_rss.c
> index 0702f1a63..30e59faa5 100644
> --- a/drivers/net/mlx5/mlx5_rss.c
> +++ b/drivers/net/mlx5/mlx5_rss.c
> @@ -357,8 +357,11 @@ mlx5_dev_rss_reta_update(struct rte_eth_dev *dev,
>  	int ret;
>  	struct priv *priv = dev->data->dev_private;
>  
> +	mlx5_dev_stop(dev);
>  	priv_lock(priv);
>  	ret = priv_dev_rss_reta_update(priv, reta_conf, reta_size);
>  	priv_unlock(priv);
> +	if (!ret)
> +		ret = (unsigned int)mlx5_dev_start(dev);
>  	return -ret;
>  }
> -- 
> 2.11.0
> 

Hi Yongseok,

I don't understand why you need the cast for the returned value of
mlx5_dev_start() as it already returns an int and your final variable is
also an int.

Thanks,
Yongseok Koh March 17, 2017, 5:14 p.m. UTC | #2 
Hi Nelio,

On Fri, Mar 17, 2017 at 02:11:43AM -0700, Nélio Laranjeiro wrote:
> On Thu, Mar 16, 2017 at 03:40:56PM -0700, Yongseok Koh wrote:
> > Currently mlx5_dev_rss_reta_update() just updates tables in the host,
> > therefore it isn't immediately effective until restarting the device by
> > calling mlx5_dev_stop()/mlx5_dev_start() to update the changes in the
> > device side. This patch adds rebuilding the device-specific datastructure
> > and applying it to the device right away.
> >
> > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> > ---
> >  drivers/net/mlx5/mlx5_rss.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/net/mlx5/mlx5_rss.c b/drivers/net/mlx5/mlx5_rss.c
> > index 0702f1a63..30e59faa5 100644
> > --- a/drivers/net/mlx5/mlx5_rss.c
> > +++ b/drivers/net/mlx5/mlx5_rss.c
> > @@ -357,8 +357,11 @@ mlx5_dev_rss_reta_update(struct rte_eth_dev *dev,
> >        int ret;
> >        struct priv *priv = dev->data->dev_private;
> > 
> > +     mlx5_dev_stop(dev);
> >        priv_lock(priv);
> >        ret = priv_dev_rss_reta_update(priv, reta_conf, reta_size);
> >        priv_unlock(priv);
> > +     if (!ret)
> > +             ret = (unsigned int)mlx5_dev_start(dev);
> >        return -ret;
> >  }
> > --
> > 2.11.0
> >
> 
> Hi Yongseok,
> 
> I don't understand why you need the cast for the returned value of
> mlx5_dev_start() as it already returns an int and your final variable is
> also an int.

For some reason, in mlx5 PMD code, priv_* calls return positive error numbers
and corresponding mlx5_* APIs return negative values by adding (-) sign the the
return value from priv_* calls.

To follow this tacit rule in the existing code, I casted the return value to
unsigned.

Thanks,
Yongseok
Nélio Laranjeiro March 20, 2017, 7:56 a.m. UTC | #3 
On Fri, Mar 17, 2017 at 10:14:56AM -0700, Yongseok Koh wrote:
> Hi Nelio,
> 
> On Fri, Mar 17, 2017 at 02:11:43AM -0700, Nélio Laranjeiro wrote:
> > On Thu, Mar 16, 2017 at 03:40:56PM -0700, Yongseok Koh wrote:
> > > Currently mlx5_dev_rss_reta_update() just updates tables in the host,
> > > therefore it isn't immediately effective until restarting the device by
> > > calling mlx5_dev_stop()/mlx5_dev_start() to update the changes in the
> > > device side. This patch adds rebuilding the device-specific datastructure
> > > and applying it to the device right away.
> > >
> > > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> > > ---
> > >  drivers/net/mlx5/mlx5_rss.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/net/mlx5/mlx5_rss.c b/drivers/net/mlx5/mlx5_rss.c
> > > index 0702f1a63..30e59faa5 100644
> > > --- a/drivers/net/mlx5/mlx5_rss.c
> > > +++ b/drivers/net/mlx5/mlx5_rss.c
> > > @@ -357,8 +357,11 @@ mlx5_dev_rss_reta_update(struct rte_eth_dev *dev,
> > >        int ret;
> > >        struct priv *priv = dev->data->dev_private;
> > > 
> > > +     mlx5_dev_stop(dev);
> > >        priv_lock(priv);
> > >        ret = priv_dev_rss_reta_update(priv, reta_conf, reta_size);
> > >        priv_unlock(priv);
> > > +     if (!ret)
> > > +             ret = (unsigned int)mlx5_dev_start(dev);
> > >        return -ret;
> > >  }
> > > --
> > > 2.11.0
> > >
> > 
> > Hi Yongseok,
> > 
> > I don't understand why you need the cast for the returned value of
> > mlx5_dev_start() as it already returns an int and your final variable is
> > also an int.
> 
> For some reason, in mlx5 PMD code, priv_* calls return positive error numbers
> and corresponding mlx5_* APIs return negative values by adding (-) sign the the
> return value from priv_* calls.

This is due to the IOCTL calls which returns only positives errno, we
decided to keep only positives errors internally and negate them in when
they are returned to DPDK.

> To follow this tacit rule in the existing code, I casted the return value to
> unsigned.

I see more this cast as something confusing than helping, knowing that
it could be avoided with something like:

 if (ret)
 	return -ret;
 return mlx5_dev_start(dev);

Regards,
diff mbox

Patch

diff --git a/drivers/net/mlx5/mlx5_rss.c b/drivers/net/mlx5/mlx5_rss.c
index 0702f1a63..30e59faa5 100644
--- a/drivers/net/mlx5/mlx5_rss.c
+++ b/drivers/net/mlx5/mlx5_rss.c
@@ -357,8 +357,11 @@  mlx5_dev_rss_reta_update(struct rte_eth_dev *dev,
 	int ret;
 	struct priv *priv = dev->data->dev_private;
 
+	mlx5_dev_stop(dev);
 	priv_lock(priv);
 	ret = priv_dev_rss_reta_update(priv, reta_conf, reta_size);
 	priv_unlock(priv);
+	if (!ret)
+		ret = (unsigned int)mlx5_dev_start(dev);
 	return -ret;
 }