[PATCH net] mlx4: xdp_prog becomes inactive after ethtool '-L' or '-G'

[PATCH net] mlx4: xdp_prog becomes inactive after ethtool '-L' or '-G'

[Martin KaFai Lau]

If the rx-queues ever get re-initialized (e.g. by changing the
number of rx-queues with ethtool -L), the existing xdp_prog becomes
inactive.

The bug is that the xdp_prog ptr has not been carried over from
the old rx-queues to the new rx-queues

Fixes: 47a38e155037 ("net/mlx4_en: add support for fast rx drop bpf program")
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_ethtool.c |  4 +-
 drivers/net/ethernet/mellanox/mlx4/en_netdev.c  | 52 ++++++++++++++++++++-----
 drivers/net/ethernet/mellanox/mlx4/mlx4_en.h    |  3 +-
 3 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index d5a9372ed84d..9aa422691954 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -1099,7 +1099,7 @@ static int mlx4_en_set_ringparam(struct net_device *dev,
 	memcpy(&new_prof, priv->prof, sizeof(struct mlx4_en_port_profile));
 	new_prof.tx_ring_size = tx_size;
 	new_prof.rx_ring_size = rx_size;
-	err = mlx4_en_try_alloc_resources(priv, tmp, &new_prof);
+	err = mlx4_en_try_alloc_resources(priv, tmp, &new_prof, true);
 	if (err)
 		goto out;
 
@@ -1774,7 +1774,7 @@ static int mlx4_en_set_channels(struct net_device *dev,
 	new_prof.tx_ring_num[TX_XDP] = xdp_count;
 	new_prof.rx_ring_num = channel->rx_count;
 
-	err = mlx4_en_try_alloc_resources(priv, tmp, &new_prof);
+	err = mlx4_en_try_alloc_resources(priv, tmp, &new_prof, true);
 	if (err)
 		goto out;
 
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
index 761f8b12399c..f4179086b3c6 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
@@ -2184,23 +2184,57 @@ static void mlx4_en_update_priv(struct mlx4_en_priv *dst,
 
 int mlx4_en_try_alloc_resources(struct mlx4_en_priv *priv,
 				struct mlx4_en_priv *tmp,
-				struct mlx4_en_port_profile *prof)
+				struct mlx4_en_port_profile *prof,
+				bool carry_xdp_prog)
 {
-	int t;
+	struct bpf_prog *xdp_prog = NULL;
+	int err;
+	int i;
 
 	mlx4_en_copy_priv(tmp, priv, prof);
 
+	if (carry_xdp_prog) {
+		/* All rx_rings has the same xdp_prog.  Pick the first one */
+		xdp_prog = rcu_dereference_protected(
+			priv->rx_ring[0]->xdp_prog,
+			lockdep_is_held(&priv->mdev->state_lock));
+
+		if (xdp_prog) {
+			xdp_prog = bpf_prog_add(xdp_prog, tmp->rx_ring_num);
+			if (IS_ERR(xdp_prog)) {
+				err = PTR_ERR(xdp_prog);
+				xdp_prog = NULL;
+				goto err_free;
+			}
+		}
+	}
+
 	if (mlx4_en_alloc_resources(tmp)) {
 		en_warn(priv,
 			"%s: Resource allocation failed, using previous configuration\n",
 			__func__);
-		for (t = 0; t < MLX4_EN_NUM_TX_TYPES; t++) {
-			kfree(tmp->tx_ring[t]);
-			kfree(tmp->tx_cq[t]);
-		}
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto err_free;
+	}
+
+	if (xdp_prog) {
+		for (i = 0; i < tmp->rx_ring_num; i++)
+			rcu_assign_pointer(tmp->rx_ring[i]->xdp_prog,
+					   xdp_prog);
 	}
+
 	return 0;
+
+err_free:
+	if (xdp_prog)
+		bpf_prog_sub(xdp_prog, tmp->rx_ring_num);
+
+	for (i = 0; i < MLX4_EN_NUM_TX_TYPES; i++) {
+		kfree(tmp->tx_ring[i]);
+		kfree(tmp->tx_cq[i]);
+	}
+
+	return err;
 }
 
 void mlx4_en_safe_replace_resources(struct mlx4_en_priv *priv,
@@ -2755,7 +2789,7 @@ static int mlx4_xdp_set(struct net_device *dev, struct bpf_prog *prog)
 		en_warn(priv, "Reducing the number of TX rings, to not exceed the max total rings number.\n");
 	}
 
-	err = mlx4_en_try_alloc_resources(priv, tmp, &new_prof);
+	err = mlx4_en_try_alloc_resources(priv, tmp, &new_prof, false);
 	if (err) {
 		if (prog)
 			bpf_prog_sub(prog, priv->rx_ring_num - 1);
@@ -3499,7 +3533,7 @@ int mlx4_en_reset_config(struct net_device *dev,
 	memcpy(&new_prof, priv->prof, sizeof(struct mlx4_en_port_profile));
 	memcpy(&new_prof.hwtstamp_config, &ts_config, sizeof(ts_config));
 
-	err = mlx4_en_try_alloc_resources(priv, tmp, &new_prof);
+	err = mlx4_en_try_alloc_resources(priv, tmp, &new_prof, true);
 	if (err)
 		goto out;
 
diff --git a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
index ba1c6cd0cc79..cec59bc264c9 100644
--- a/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
+++ b/drivers/net/ethernet/mellanox/mlx4/mlx4_en.h
@@ -679,7 +679,8 @@ void mlx4_en_set_stats_bitmap(struct mlx4_dev *dev,
 
 int mlx4_en_try_alloc_resources(struct mlx4_en_priv *priv,
 				struct mlx4_en_priv *tmp,
-				struct mlx4_en_port_profile *prof);
+				struct mlx4_en_port_profile *prof,
+				bool carry_xdp_prog);
 void mlx4_en_safe_replace_resources(struct mlx4_en_priv *priv,
 				    struct mlx4_en_priv *tmp);
 
-- 
2.5.1

Re: [PATCH net] mlx4: xdp_prog becomes inactive after ethtool '-L' or '-G'

[David Miller]

From: Martin KaFai Lau <kafai@fb.com>
Date: Fri, 27 Jan 2017 23:40:51 -0800

> If the rx-queues ever get re-initialized (e.g. by changing the
> number of rx-queues with ethtool -L), the existing xdp_prog becomes
> inactive.
> 
> The bug is that the xdp_prog ptr has not been carried over from
> the old rx-queues to the new rx-queues
> 
> Fixes: 47a38e155037 ("net/mlx4_en: add support for fast rx drop bpf program")
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>

Mellanox folks, please review.

Re: [PATCH net] mlx4: xdp_prog becomes inactive after ethtool '-L' or '-G'

[Tariq Toukan]

Hi Martin,

Thanks for your patch.

It looks good to me, in general.
I just have one small comment below.

On 28/01/2017 9:40 AM, Martin KaFai Lau wrote:
> If the rx-queues ever get re-initialized (e.g. by changing the
> number of rx-queues with ethtool -L), the existing xdp_prog becomes
> inactive.
>
> The bug is that the xdp_prog ptr has not been carried over from
> the old rx-queues to the new rx-queues
>
> Fixes: 47a38e155037 ("net/mlx4_en: add support for fast rx drop bpf program")
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
...
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> index 761f8b12399c..f4179086b3c6 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> @@ -2184,23 +2184,57 @@ static void mlx4_en_update_priv(struct mlx4_en_priv *dst,
>   
>   int mlx4_en_try_alloc_resources(struct mlx4_en_priv *priv,
>   				struct mlx4_en_priv *tmp,
> -				struct mlx4_en_port_profile *prof)
> +				struct mlx4_en_port_profile *prof,
> +				bool carry_xdp_prog)
>   {
> -	int t;
> +	struct bpf_prog *xdp_prog = NULL;
> +	int err;
> +	int i;
>   
>   	mlx4_en_copy_priv(tmp, priv, prof);
>   
> +	if (carry_xdp_prog) {
> +		/* All rx_rings has the same xdp_prog.  Pick the first one */
> +		xdp_prog = rcu_dereference_protected(
> +			priv->rx_ring[0]->xdp_prog,
> +			lockdep_is_held(&priv->mdev->state_lock));
> +
> +		if (xdp_prog) {
> +			xdp_prog = bpf_prog_add(xdp_prog, tmp->rx_ring_num);
> +			if (IS_ERR(xdp_prog)) {
> +				err = PTR_ERR(xdp_prog);
> +				xdp_prog = NULL;
> +				goto err_free;
> +			}
> +		}
> +	}
Why do you prefer dealing with xdp_prog in two stages? You can handle it 
all at once, after "mlx4_en_alloc_resources()" succeeds.
> +
>   	if (mlx4_en_alloc_resources(tmp)) {
>   		en_warn(priv,
>   			"%s: Resource allocation failed, using previous configuration\n",
>   			__func__);
> -		for (t = 0; t < MLX4_EN_NUM_TX_TYPES; t++) {
> -			kfree(tmp->tx_ring[t]);
> -			kfree(tmp->tx_cq[t]);
> -		}
> -		return -ENOMEM;
> +		err = -ENOMEM;
> +		goto err_free;
> +	}
> +
> +	if (xdp_prog) {
> +		for (i = 0; i < tmp->rx_ring_num; i++)
> +			rcu_assign_pointer(tmp->rx_ring[i]->xdp_prog,
> +					   xdp_prog);
>   	}
> +
>   	return 0;
> +
> +err_free:
> +	if (xdp_prog)
> +		bpf_prog_sub(xdp_prog, tmp->rx_ring_num);
> +
> +	for (i = 0; i < MLX4_EN_NUM_TX_TYPES; i++) {
> +		kfree(tmp->tx_ring[i]);
> +		kfree(tmp->tx_cq[i]);
> +	}
> +
> +	return err;
>   }
>   
>   void mlx4_en_safe_replace_resources(struct mlx4_en_priv *priv,
>
Regards,
Tariq Toukan.

Re: [PATCH net] mlx4: xdp_prog becomes inactive after ethtool '-L' or '-G'

[David Miller]

From: Tariq Toukan <tariqt@mellanox.com>
Date: Mon, 30 Jan 2017 19:18:28 +0200

> It looks good to me, in general.
> I just have one small comment below.

Martin, please address Tariq's feedback.

Thank you.

Re: [PATCH net] mlx4: xdp_prog becomes inactive after ethtool '-L' or '-G'

[Martin KaFai Lau]

On Tue, Jan 31, 2017 at 01:11:40PM -0500, David Miller wrote:
> From: Tariq Toukan <tariqt@mellanox.com>
> Date: Mon, 30 Jan 2017 19:18:28 +0200
>
> > It looks good to me, in general.
> > I just have one small comment below.
>
> Martin, please address Tariq's feedback.
Sorry for the delay.  I am on PTO for this week and
have been travelling.

I will take a closer look when I get
a more stable intenet connection tonight.

Thanks,
--Martin

Re: [PATCH net] mlx4: xdp_prog becomes inactive after ethtool '-L' or '-G'

[Martin KaFai Lau]

On Mon, Jan 30, 2017 at 07:18:28PM +0200, Tariq Toukan wrote:
> Hi Martin,
>
> Thanks for your patch.
>
> It looks good to me, in general.
> I just have one small comment below.
Thanks for your feedback and sorry for the delay.

>
> On 28/01/2017 9:40 AM, Martin KaFai Lau wrote:
> > If the rx-queues ever get re-initialized (e.g. by changing the
> > number of rx-queues with ethtool -L), the existing xdp_prog becomes
> > inactive.
> >
> > The bug is that the xdp_prog ptr has not been carried over from
> > the old rx-queues to the new rx-queues
> >
> > Fixes: 47a38e155037 ("net/mlx4_en: add support for fast rx drop bpf program")
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > ---
> ...
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> > index 761f8b12399c..f4179086b3c6 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> > +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> > @@ -2184,23 +2184,57 @@ static void mlx4_en_update_priv(struct mlx4_en_priv *dst,
> >   int mlx4_en_try_alloc_resources(struct mlx4_en_priv *priv,
> >   				struct mlx4_en_priv *tmp,
> > -				struct mlx4_en_port_profile *prof)
> > +				struct mlx4_en_port_profile *prof,
> > +				bool carry_xdp_prog)
> >   {
> > -	int t;
> > +	struct bpf_prog *xdp_prog = NULL;
> > +	int err;
> > +	int i;
> >   	mlx4_en_copy_priv(tmp, priv, prof);
> > +	if (carry_xdp_prog) {
> > +		/* All rx_rings has the same xdp_prog.  Pick the first one */
> > +		xdp_prog = rcu_dereference_protected(
> > +			priv->rx_ring[0]->xdp_prog,
> > +			lockdep_is_held(&priv->mdev->state_lock));
> > +
> > +		if (xdp_prog) {
> > +			xdp_prog = bpf_prog_add(xdp_prog, tmp->rx_ring_num);
> > +			if (IS_ERR(xdp_prog)) {
> > +				err = PTR_ERR(xdp_prog);
> > +				xdp_prog = NULL;
> > +				goto err_free;
> > +			}
> > +		}
> > +	}
> Why do you prefer dealing with xdp_prog in two stages? You can handle it all
> at once, after "mlx4_en_alloc_resources()" succeeds.
If bpf_prog_add() did fail, resources allocated for tmp had to
be freed.  I was thinking it is not safe to call mlx4_en_free_resources()
at this point.  Since your feedback, I took another look and re-read the
'goto err' path in mlx4_en_alloc_resources(), I realized we can use
mlx4_en_free_resources() here for the bpf_prog_add() error case.  Hence,
agree with your suggestion.

A side note though:
after taking another look at mlx4_en_free_resources(), I might have found
another issue.  I need to run some tests to confirm first to avoid any false
alarm.

I will post v2.

Thanks,
--Martin


> > +
> >   	if (mlx4_en_alloc_resources(tmp)) {
> >   		en_warn(priv,
> >   			"%s: Resource allocation failed, using previous configuration\n",
> >   			__func__);
> > -		for (t = 0; t < MLX4_EN_NUM_TX_TYPES; t++) {
> > -			kfree(tmp->tx_ring[t]);
> > -			kfree(tmp->tx_cq[t]);
> > -		}
> > -		return -ENOMEM;
> > +		err = -ENOMEM;
> > +		goto err_free;
> > +	}
> > +
> > +	if (xdp_prog) {
> > +		for (i = 0; i < tmp->rx_ring_num; i++)
> > +			rcu_assign_pointer(tmp->rx_ring[i]->xdp_prog,
> > +					   xdp_prog);
> >   	}
> > +
> >   	return 0;
> > +
> > +err_free:
> > +	if (xdp_prog)
> > +		bpf_prog_sub(xdp_prog, tmp->rx_ring_num);
> > +
> > +	for (i = 0; i < MLX4_EN_NUM_TX_TYPES; i++) {
> > +		kfree(tmp->tx_ring[i]);
> > +		kfree(tmp->tx_cq[i]);
> > +	}
> > +
> > +	return err;
> >   }
> >   void mlx4_en_safe_replace_resources(struct mlx4_en_priv *priv,
> >
> Regards,
> Tariq Toukan.