[PATCH net 0/2] xdp_rxq_info_reg fixes for mlx5e

[PATCH net 0/2] xdp_rxq_info_reg fixes for mlx5e

[Maxim Mikityanskiy]

Two small fixes that add parameters to xdp_rxq_info_reg missed in older
commits.

Maxim Mikityanskiy (2):
  net/mlx5e: XDP, Allow growing tail for XDP multi buffer
  net/mlx5e: xsk: Set napi_id to support busy polling on XSK RQ

 .../net/ethernet/mellanox/mlx5/core/en/xsk/setup.c    |  2 +-
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c     | 11 ++++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

-- 
2.39.1

[PATCH net 1/2] net/mlx5e: XDP, Allow growing tail for XDP multi buffer

[Maxim Mikityanskiy]

The cited commits missed passing frag_size to __xdp_rxq_info_reg, which
is required by bpf_xdp_adjust_tail to support growing the tail pointer
in fragmented packets. Pass the missing parameter when the current RQ
mode allows XDP multi buffer.

Fixes: ea5d49bdae8b ("net/mlx5e: Add XDP multi buffer support to the non-linear legacy RQ")
Fixes: 9cb9482ef10e ("net/mlx5e: Use fragments of the same size in non-linear legacy RQ with XDP")
Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
Cc: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index abcc614b6191..cdd1e47e18f9 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -576,9 +576,10 @@ static void mlx5e_free_mpwqe_rq_drop_page(struct mlx5e_rq *rq)
 }
 
 static int mlx5e_init_rxq_rq(struct mlx5e_channel *c, struct mlx5e_params *params,
-			     struct mlx5e_rq *rq)
+			     struct mlx5e_rq_param *rq_params, struct mlx5e_rq *rq)
 {
 	struct mlx5_core_dev *mdev = c->mdev;
+	u32 xdp_frag_size = 0;
 	int err;
 
 	rq->wq_type      = params->rq_wq_type;
@@ -599,7 +600,11 @@ static int mlx5e_init_rxq_rq(struct mlx5e_channel *c, struct mlx5e_params *param
 	if (err)
 		return err;
 
-	return xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq->ix, c->napi.napi_id);
+	if (rq->wq_type == MLX5_WQ_TYPE_CYCLIC && rq_params->frags_info.num_frags > 1)
+		xdp_frag_size = rq_params->frags_info.arr[1].frag_stride;
+
+	return __xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq->ix, c->napi.napi_id,
+				  xdp_frag_size);
 }
 
 static int mlx5_rq_shampo_alloc(struct mlx5_core_dev *mdev,
@@ -2214,7 +2219,7 @@ static int mlx5e_open_rxq_rq(struct mlx5e_channel *c, struct mlx5e_params *param
 {
 	int err;
 
-	err = mlx5e_init_rxq_rq(c, params, &c->rq);
+	err = mlx5e_init_rxq_rq(c, params, rq_params, &c->rq);
 	if (err)
 		return err;
 
-- 
2.39.1

[PATCH net 2/2] net/mlx5e: xsk: Set napi_id to support busy polling on XSK RQ

[Maxim Mikityanskiy]

The cited commit missed setting napi_id on XSK RQs, it only affected
regular RQs. Add the missing part to support socket busy polling on XSK
RQs.

Fixes: a2740f529da2 ("net/mlx5e: xsk: Set napi_id to support busy polling")
Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
index ff03c43833bb..53c93d1daa7e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
@@ -71,7 +71,7 @@ static int mlx5e_init_xsk_rq(struct mlx5e_channel *c,
 	if (err)
 		return err;
 
-	return  xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq_xdp_ix, 0);
+	return xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq_xdp_ix, c->napi.napi_id);
 }
 
 static int mlx5e_open_xsk_rq(struct mlx5e_channel *c, struct mlx5e_params *params,
-- 
2.39.1

Re: [PATCH net 1/2] net/mlx5e: XDP, Allow growing tail for XDP multi buffer

[Tariq Toukan]

On 26/01/2023 21:10, Maxim Mikityanskiy wrote:
> The cited commits missed passing frag_size to __xdp_rxq_info_reg, which
> is required by bpf_xdp_adjust_tail to support growing the tail pointer
> in fragmented packets. Pass the missing parameter when the current RQ
> mode allows XDP multi buffer.
> 
> Fixes: ea5d49bdae8b ("net/mlx5e: Add XDP multi buffer support to the non-linear legacy RQ")
> Fixes: 9cb9482ef10e ("net/mlx5e: Use fragments of the same size in non-linear legacy RQ with XDP")
> Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
> Cc: Tariq Toukan <tariqt@nvidia.com>
> ---
>   drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index abcc614b6191..cdd1e47e18f9 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -576,9 +576,10 @@ static void mlx5e_free_mpwqe_rq_drop_page(struct mlx5e_rq *rq)
>   }
>   
>   static int mlx5e_init_rxq_rq(struct mlx5e_channel *c, struct mlx5e_params *params,
> -			     struct mlx5e_rq *rq)
> +			     struct mlx5e_rq_param *rq_params, struct mlx5e_rq *rq)
>   {
>   	struct mlx5_core_dev *mdev = c->mdev;
> +	u32 xdp_frag_size = 0;
>   	int err;
>   
>   	rq->wq_type      = params->rq_wq_type;
> @@ -599,7 +600,11 @@ static int mlx5e_init_rxq_rq(struct mlx5e_channel *c, struct mlx5e_params *param
>   	if (err)
>   		return err;
>   
> -	return xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq->ix, c->napi.napi_id);
> +	if (rq->wq_type == MLX5_WQ_TYPE_CYCLIC && rq_params->frags_info.num_frags > 1)

How about a more generic check? like:
if (params->xdp_prog && params->xdp_prog->aux->xdp_has_frags)

So we won't have to maintain this when Stridng RQ support is added.

> +		xdp_frag_size = rq_params->frags_info.arr[1].frag_stride;

Again, in order to not maintain this (frags_info.arr[1].frag_stride not 
relevant for Striding RQ), isn't the value always PAGE_SIZE?

Another idea is to introduce something like
#define XDP_MB_FRAG_SZ (PAGE_SIZE)
use it here and in mlx5e_build_rq_frags_info ::
if (byte_count > max_mtu || params->xdp_prog) {
	frag_size_max = XDP_MB_FRAG_SZ;
Not sure it's worth it...

Both ways we save passing rq_params in the callstack.

> +
> +	return __xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq->ix, c->napi.napi_id,
> +				  xdp_frag_size);
>   }
>   
>   static int mlx5_rq_shampo_alloc(struct mlx5_core_dev *mdev,
> @@ -2214,7 +2219,7 @@ static int mlx5e_open_rxq_rq(struct mlx5e_channel *c, struct mlx5e_params *param
>   {
>   	int err;
>   
> -	err = mlx5e_init_rxq_rq(c, params, &c->rq);
> +	err = mlx5e_init_rxq_rq(c, params, rq_params, &c->rq);
>   	if (err)
>   		return err;
>   

Re: [PATCH net 2/2] net/mlx5e: xsk: Set napi_id to support busy polling on XSK RQ

[Tariq Toukan]

On 26/01/2023 21:10, Maxim Mikityanskiy wrote:
> The cited commit missed setting napi_id on XSK RQs, it only affected
> regular RQs. Add the missing part to support socket busy polling on XSK
> RQs.
> 
> Fixes: a2740f529da2 ("net/mlx5e: xsk: Set napi_id to support busy polling")
> Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
> ---
>   drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
> index ff03c43833bb..53c93d1daa7e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/setup.c
> @@ -71,7 +71,7 @@ static int mlx5e_init_xsk_rq(struct mlx5e_channel *c,
>   	if (err)
>   		return err;
>   
> -	return  xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq_xdp_ix, 0);
> +	return xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq_xdp_ix, c->napi.napi_id);
>   }
>   
>   static int mlx5e_open_xsk_rq(struct mlx5e_channel *c, struct mlx5e_params *params,

Reviewed-by: Tariq Toukan <tariqt@nvidia.com>
Thanks for your patch!

Re: [PATCH net 1/2] net/mlx5e: XDP, Allow growing tail for XDP multi buffer

[Maxim Mikityanskiy]

On Thu, Jan 26, 2023 at 10:41:30PM +0200, Tariq Toukan wrote:
> 
> 
> On 26/01/2023 21:10, Maxim Mikityanskiy wrote:
> > The cited commits missed passing frag_size to __xdp_rxq_info_reg, which
> > is required by bpf_xdp_adjust_tail to support growing the tail pointer
> > in fragmented packets. Pass the missing parameter when the current RQ
> > mode allows XDP multi buffer.
> > 
> > Fixes: ea5d49bdae8b ("net/mlx5e: Add XDP multi buffer support to the non-linear legacy RQ")
> > Fixes: 9cb9482ef10e ("net/mlx5e: Use fragments of the same size in non-linear legacy RQ with XDP")
> > Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
> > Cc: Tariq Toukan <tariqt@nvidia.com>
> > ---
> >   drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > index abcc614b6191..cdd1e47e18f9 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > @@ -576,9 +576,10 @@ static void mlx5e_free_mpwqe_rq_drop_page(struct mlx5e_rq *rq)
> >   }
> >   static int mlx5e_init_rxq_rq(struct mlx5e_channel *c, struct mlx5e_params *params,
> > -			     struct mlx5e_rq *rq)
> > +			     struct mlx5e_rq_param *rq_params, struct mlx5e_rq *rq)
> >   {
> >   	struct mlx5_core_dev *mdev = c->mdev;
> > +	u32 xdp_frag_size = 0;
> >   	int err;
> >   	rq->wq_type      = params->rq_wq_type;
> > @@ -599,7 +600,11 @@ static int mlx5e_init_rxq_rq(struct mlx5e_channel *c, struct mlx5e_params *param
> >   	if (err)
> >   		return err;
> > -	return xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq->ix, c->napi.napi_id);
> > +	if (rq->wq_type == MLX5_WQ_TYPE_CYCLIC && rq_params->frags_info.num_frags > 1)
> 
> How about a more generic check? like:
> if (params->xdp_prog && params->xdp_prog->aux->xdp_has_frags)
> 
> So we won't have to maintain this when Stridng RQ support is added.

The check is specific, because below I use rq_params->frags_info, which
is specific to legacy RQ. If we change the input for xdp_frag_size, the
check can also be changed, but the condition that you suggested can't be
used anyway, because the XDP program can be hot-swapped without
recreating channels (i.e. without calling into mlx5e_init_rxq_rq), and
xdp_has_frags can change after the hot-swap.

It's actually valid to pass a non-zero value unconditionally, it just
won't be used if the driver doesn't pass any multi-buffer frames to XDP.
I added a reasonable condition solely for extra robustness, but we can
drop the `if` altogether if we don't agree on the condition.

> > +		xdp_frag_size = rq_params->frags_info.arr[1].frag_stride;
> 
> Again, in order to not maintain this (frags_info.arr[1].frag_stride not
> relevant for Striding RQ), isn't the value always PAGE_SIZE?

It's always PAGE_SIZE for the current implementation of legacy RQ, but
the kernel doesn't fix it to PAGE_SIZE, it's possible for a driver to
choose a different memory allocation scheme with fragments of another
size, that's why this parameter exists.

Setting it to PAGE_SIZE to be "future-proof" may be problematic: if
striding RQ uses a different frag_size, and the author forgets to update
this code, it may lead to a memory corruption on adjust_tail.

There is an obvious robustness problem with this place in code: it's
easy to forget about updating it. I forgot to set the right non-zero
value when I added XDP multi buffer, the next developer risks forgetting
updating this code when XDP multi buffer support is extended to striding
RQ, or the memory allocation scheme is somehow changed. So, it's not
possible to avoid maintaining it: either way it might need changes in
the future. I wanted to add some WARN_ON or BUILD_BUG_ON to simplify
such maintenance, but couldn't think of a good check...

> 
> Another idea is to introduce something like
> #define XDP_MB_FRAG_SZ (PAGE_SIZE)
> use it here and in mlx5e_build_rq_frags_info ::
> if (byte_count > max_mtu || params->xdp_prog) {
> 	frag_size_max = XDP_MB_FRAG_SZ;
> Not sure it's worth it...

IMO, it doesn't fit to mlx5e_build_rq_frags_info, because that function
heavily relies on its value being PAGE_SIZE, and hiding it under a
different name may give false impression that it can be changed.
Moreover, there is a chance that striding RQ will use a different value
for XDP frag_size. Also, it rather doesn't make sense even in the code
that you quoted: if byte_count > max_mtu, using XDP_MB_FRAG_SZ doesn't
make sense.

Using this constant only here, but not in mlx5e_build_rq_frags_info
doesn't make sense either, because it won't help remind developers to
update this part of code.

I think I got a better idea: move the logic to en/params.c, it knows
everything about the memory allocation scheme, about the XDP multi
buffer support, so let it calculate the right value and assign it to
some field (let's say, rq_params->xdp_frag_size), which is passed to
mlx5e_init_rxq_rq and used here as is. mlx5e_init_rxq_rq won't need to
dig into implementation details of each mode, instead the functions that
contain these details will calculate the value for XDP. What do you
think?

> Both ways we save passing rq_params in the callstack.

I don't think the number of parameters is crucial for non-datapath,
especially given that it's still fewer than 6.

> 
> > +
> > +	return __xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq->ix, c->napi.napi_id,
> > +				  xdp_frag_size);
> >   }
> >   static int mlx5_rq_shampo_alloc(struct mlx5_core_dev *mdev,
> > @@ -2214,7 +2219,7 @@ static int mlx5e_open_rxq_rq(struct mlx5e_channel *c, struct mlx5e_params *param
> >   {
> >   	int err;
> > -	err = mlx5e_init_rxq_rq(c, params, &c->rq);
> > +	err = mlx5e_init_rxq_rq(c, params, rq_params, &c->rq);
> >   	if (err)
> >   		return err;

Re: [PATCH net 1/2] net/mlx5e: XDP, Allow growing tail for XDP multi buffer

[Tariq Toukan]

On 27/01/2023 0:43, Maxim Mikityanskiy wrote:
> On Thu, Jan 26, 2023 at 10:41:30PM +0200, Tariq Toukan wrote:
>>
>>
>> On 26/01/2023 21:10, Maxim Mikityanskiy wrote:
>>> The cited commits missed passing frag_size to __xdp_rxq_info_reg, which
>>> is required by bpf_xdp_adjust_tail to support growing the tail pointer
>>> in fragmented packets. Pass the missing parameter when the current RQ
>>> mode allows XDP multi buffer.
>>>
>>> Fixes: ea5d49bdae8b ("net/mlx5e: Add XDP multi buffer support to the non-linear legacy RQ")
>>> Fixes: 9cb9482ef10e ("net/mlx5e: Use fragments of the same size in non-linear legacy RQ with XDP")
>>> Signed-off-by: Maxim Mikityanskiy <maxtram95@gmail.com>
>>> Cc: Tariq Toukan <tariqt@nvidia.com>
>>> ---
>>>    drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 11 ++++++++---
>>>    1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>> index abcc614b6191..cdd1e47e18f9 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>> @@ -576,9 +576,10 @@ static void mlx5e_free_mpwqe_rq_drop_page(struct mlx5e_rq *rq)
>>>    }
>>>    static int mlx5e_init_rxq_rq(struct mlx5e_channel *c, struct mlx5e_params *params,
>>> -			     struct mlx5e_rq *rq)
>>> +			     struct mlx5e_rq_param *rq_params, struct mlx5e_rq *rq)
>>>    {
>>>    	struct mlx5_core_dev *mdev = c->mdev;
>>> +	u32 xdp_frag_size = 0;
>>>    	int err;
>>>    	rq->wq_type      = params->rq_wq_type;
>>> @@ -599,7 +600,11 @@ static int mlx5e_init_rxq_rq(struct mlx5e_channel *c, struct mlx5e_params *param
>>>    	if (err)
>>>    		return err;
>>> -	return xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq->ix, c->napi.napi_id);
>>> +	if (rq->wq_type == MLX5_WQ_TYPE_CYCLIC && rq_params->frags_info.num_frags > 1)
>>
>> How about a more generic check? like:
>> if (params->xdp_prog && params->xdp_prog->aux->xdp_has_frags)
>>
>> So we won't have to maintain this when Stridng RQ support is added.
> 
> The check is specific, because below I use rq_params->frags_info, which
> is specific to legacy RQ. If we change the input for xdp_frag_size, the
> check can also be changed, but the condition that you suggested can't be
> used anyway, because the XDP program can be hot-swapped without
> recreating channels (i.e. without calling into mlx5e_init_rxq_rq), and
> xdp_has_frags can change after the hot-swap.
> 
> It's actually valid to pass a non-zero value unconditionally, it just
> won't be used if the driver doesn't pass any multi-buffer frames to XDP.
> I added a reasonable condition solely for extra robustness, but we can
> drop the `if` altogether if we don't agree on the condition.
> 
>>> +		xdp_frag_size = rq_params->frags_info.arr[1].frag_stride;
>>
>> Again, in order to not maintain this (frags_info.arr[1].frag_stride not
>> relevant for Striding RQ), isn't the value always PAGE_SIZE?
> 
> It's always PAGE_SIZE for the current implementation of legacy RQ, but
> the kernel doesn't fix it to PAGE_SIZE, it's possible for a driver to
> choose a different memory allocation scheme with fragments of another
> size, that's why this parameter exists.
> 
> Setting it to PAGE_SIZE to be "future-proof" may be problematic: if
> striding RQ uses a different frag_size, and the author forgets to update
> this code, it may lead to a memory corruption on adjust_tail.
> 
> There is an obvious robustness problem with this place in code: it's
> easy to forget about updating it. I forgot to set the right non-zero
> value when I added XDP multi buffer, the next developer risks forgetting
> updating this code when XDP multi buffer support is extended to striding
> RQ, or the memory allocation scheme is somehow changed. So, it's not
> possible to avoid maintaining it: either way it might need changes in
> the future. I wanted to add some WARN_ON or BUILD_BUG_ON to simplify
> such maintenance, but couldn't think of a good check...
> 
>>
>> Another idea is to introduce something like
>> #define XDP_MB_FRAG_SZ (PAGE_SIZE)
>> use it here and in mlx5e_build_rq_frags_info ::
>> if (byte_count > max_mtu || params->xdp_prog) {
>> 	frag_size_max = XDP_MB_FRAG_SZ;
>> Not sure it's worth it...
> 
> IMO, it doesn't fit to mlx5e_build_rq_frags_info, because that function
> heavily relies on its value being PAGE_SIZE, and hiding it under a
> different name may give false impression that it can be changed.
> Moreover, there is a chance that striding RQ will use a different value
> for XDP frag_size. Also, it rather doesn't make sense even in the code
> that you quoted: if byte_count > max_mtu, using XDP_MB_FRAG_SZ doesn't
> make sense.
> 
> Using this constant only here, but not in mlx5e_build_rq_frags_info
> doesn't make sense either, because it won't help remind developers to
> update this part of code.
> 

Agree.

> I think I got a better idea: move the logic to en/params.c, it knows
> everything about the memory allocation scheme, about the XDP multi
> buffer support, so let it calculate the right value and assign it to
> some field (let's say, rq_params->xdp_frag_size), which is passed to
> mlx5e_init_rxq_rq and used here as is. mlx5e_init_rxq_rq won't need to
> dig into implementation details of each mode, instead the functions that
> contain these details will calculate the value for XDP. What do you
> think?
> 

Yes, that would be best.

>> Both ways we save passing rq_params in the callstack.
> 
> I don't think the number of parameters is crucial for non-datapath,
> especially given that it's still fewer than 6.
> 
>>
>>> +
>>> +	return __xdp_rxq_info_reg(&rq->xdp_rxq, rq->netdev, rq->ix, c->napi.napi_id,
>>> +				  xdp_frag_size);
>>>    }
>>>    static int mlx5_rq_shampo_alloc(struct mlx5_core_dev *mdev,
>>> @@ -2214,7 +2219,7 @@ static int mlx5e_open_rxq_rq(struct mlx5e_channel *c, struct mlx5e_params *param
>>>    {
>>>    	int err;
>>> -	err = mlx5e_init_rxq_rq(c, params, &c->rq);
>>> +	err = mlx5e_init_rxq_rq(c, params, rq_params, &c->rq);
>>>    	if (err)
>>>    		return err;