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

[Tariq Toukan <tariqt@nvidia.com>]

On 21/10/2026 13:30, Daniel Zahka wrote:
> On Wed Oct 21, 2026 at 2:28 AM EDT, Tariq Toukan wrote:
>> From: Cosmin Ratiu <cratiu@nvidia.com>
>>
>> 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@nvidia.com>
>> Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
>> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
>> ---
>>   .../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@nvidia.com/
>>
>> 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?
> 

Because we have a followup patch to be submitted soon, that adds a check 
for TLS. It needs the RQ, not the SKB.

>>   		match = false;
>>   		mlx5e_shampo_flush_skb(rq, cqe, match);
>>   	}
>>
>> base-commit: b6ef0d965fe8ce0ee4b4dcb58b3511f83003e42d
> 
> Makes sense to me:
> 
> Reviewed-by: Daniel Zahka <daniel.zahka@gmail.com>

Thanks for the review.