Re: [PATCH net V2] net/mlx5e: SHAMPO: Do not merge PSP packets

[Daniel Zahka]

On Wed Oct 21, 2026 at 2:28 AM EDT, Tariq Toukan wrote:
> From: Cosmin Ratiu <cratiu@xxxxxxxxxx>
>
> PSP packets are UDP-encapsulated, and enabling HW GRO (SHAMPO in mlx5)
> makes it possible to merge those packets in a silly way:
> IP|UDP|PSP1|payload1|PSP2|payload2|...
>
> That happens because the HW GRO machinery doesn't know about PSP and
> considers it part of the UDP payload.
>
> Reject merging when the incoming CQE has PSP marker or the pending skb
> has the PSP extension.
>
> Fixes: 29d7f433fcec ("net/mlx5e: Add Rx data path offload")
> Signed-off-by: Cosmin Ratiu <cratiu@xxxxxxxxxx>
> Reviewed-by: Dragos Tatulea <dtatulea@xxxxxxxxxx>
> Signed-off-by: Tariq Toukan <tariqt@xxxxxxxxxx>
> ---
>  .../ethernet/mellanox/mlx5/core/en_accel/en_accel.h |  6 ++++++
>  .../ethernet/mellanox/mlx5/core/en_accel/psp_rxtx.h | 13 +++++++++++++
>  drivers/net/ethernet/mellanox/mlx5/core/en_rx.c     |  6 ++++--
>  3 files changed, 23 insertions(+), 2 deletions(-)
>
> V2:
> - Separated from the patchset.
> - Moved the PSP merge check into a helper in psp_rxtx.h, with a stub
>   returning true when CONFIG_MLX5_EN_PSP is off, and an en_accel.h
>   wrapper, replacing the #ifdef in en_rx.c.
> - Also reject merging when the pending skb already has the PSP
>   extension, not only when the incoming CQE has the PSP marker.
> - Fixed the commit message diagram and capitalized SHAMPO in the
>   subject.
>
> V1:
> https://lore.kernel.org/all/20260917175433.4090878-8-tariqt@xxxxxxxxxx/
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/en_accel.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/en_accel.h
> index 3f212e46fc2f..5a340f1435e6 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/en_accel.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/en_accel.h
> @@ -218,6 +218,12 @@ static inline void mlx5e_accel_tx_finish(struct mlx5e_txqsq *sq,
>  #endif
>  }
>  
> +static inline bool
> +mlx5e_accel_shampo_can_merge(struct mlx5e_rq *rq, struct mlx5_cqe64 *cqe)
> +{
> +	return mlx5e_psp_shampo_can_merge(rq->hw_gro_data->skb, cqe);
> +}
> +
>  static inline int mlx5e_accel_init_rx(struct mlx5e_priv *priv)
>  {
>  	return mlx5e_ktls_init_rx(priv);
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp_rxtx.h b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp_rxtx.h
> index 2b080c39cc37..9fd1ee789de0 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp_rxtx.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp_rxtx.h
> @@ -85,6 +85,13 @@ static inline bool mlx5e_psp_is_rx_flow(struct mlx5_cqe64 *cqe)
>  	return MLX5_PSP_METADATA_MARKER(be32_to_cpu(cqe->ft_metadata));
>  }
>  
> +static inline bool
> +mlx5e_psp_shampo_can_merge(struct sk_buff *skb, struct mlx5_cqe64 *cqe)
> +{
> +	/* PSP packets cannot be merged. */
> +	return !skb_ext_exist(skb, SKB_EXT_PSP) && !mlx5e_psp_is_rx_flow(cqe);
> +}
> +
>  bool mlx5e_psp_offload_handle_rx_skb(struct net_device *netdev, struct sk_buff *skb,
>  				     struct mlx5_cqe64 *cqe);
>  #else
> @@ -110,6 +117,12 @@ static inline bool mlx5e_psp_is_rx_flow(struct mlx5_cqe64 *cqe)
>  	return false;
>  }
>  
> +static inline bool
> +mlx5e_psp_shampo_can_merge(struct sk_buff *skb, struct mlx5_cqe64 *cqe)
> +{
> +	return true;
> +}
> +
>  static inline bool mlx5e_psp_offload_handle_rx_skb(struct net_device *netdev,
>  						   struct sk_buff *skb,
>  						   struct mlx5_cqe64 *cqe)
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index 903af3b3e779..d49f26323b1e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -49,6 +49,7 @@
>  #include "en_rep.h"
>  #include "en/rep/tc.h"
>  #include "ipoib/ipoib.h"
> +#include "en_accel/en_accel.h"
>  #include "en_accel/ipsec.h"
>  #include "en_accel/macsec.h"
>  #include "en_accel/psp_rxtx.h"
> @@ -2268,8 +2269,9 @@ static void mlx5e_handle_rx_cqe_mpwrq_shampo(struct mlx5e_rq *rq, struct mlx5_cq
>  	}
>  
>  	if (*skb &&
> -	    !(match && mlx5e_hw_gro_skb_has_enough_space(*skb, data_bcnt,
> -							 page_size))) {
> +	    !(match &&
> +	      mlx5e_hw_gro_skb_has_enough_space(*skb, data_bcnt, page_size) &&
> +	      mlx5e_accel_shampo_can_merge(rq, cqe))) {

nit: we already have the skb from the line above, why go back to the rq
just to find the same skb again?

>  		match = false;
>  		mlx5e_shampo_flush_skb(rq, cqe, match);
>  	}
>
> base-commit: b6ef0d965fe8ce0ee4b4dcb58b3511f83003e42d

Makes sense to me:

Reviewed-by: Daniel Zahka <daniel.zahka@xxxxxxxxx>