Re: [PATCH net v2] net/mlx5e: Make DEFAULT_FRAG_SIZE relative to page size

[Jacob Keller <jacob.e.keller@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5914 bytes --]



On 9/30/2025 4:33 AM, Mingrui Cui wrote:
> When page size is 4K, DEFAULT_FRAG_SIZE of 2048 ensures that with 3
> fragments per WQE, odd-indexed WQEs always share the same page with
> their subsequent WQE, while WQEs consisting of 4 fragments does not.
> However, this relationship does not hold for page sizes larger than 8K.
> In this case, wqe_index_mask cannot guarantee that newly allocated WQEs
> won't share the same page with old WQEs.
> 
> If the last WQE in a bulk processed by mlx5e_post_rx_wqes() shares a
> page with its subsequent WQE, allocating a page for that WQE will
> overwrite mlx5e_frag_page, preventing the original page from being
> recycled. When the next WQE is processed, the newly allocated page will
> be immediately recycled. In the next round, if these two WQEs are
> handled in the same bulk, page_pool_defrag_page() will be called again
> on the page, causing pp_frag_count to become negative[1].
> 
> Moreover, this can also lead to memory corruption, as the page may have
> already been returned to the page pool and re-allocated to another WQE.
> And since skb_shared_info is stored at the end of the first fragment,
> its frags->bv_page pointer can be overwritten, leading to an invalid
> memory access when processing the skb[2].
> 
> For example, on 8K page size systems (e.g. DEC Alpha) with a ConnectX-4
> Lx MT27710 (MCX4121A-ACA_Ax) NIC setting MTU to 7657 or higher, heavy
> network loads (e.g. iperf) will first trigger a series of WARNINGs[1]
> and eventually crash[2].
> 
> Fix this by making DEFAULT_FRAG_SIZE always equal to half of the page
> size.
> 
> [1]
> WARNING: CPU: 9 PID: 0 at include/net/page_pool/helpers.h:130
> mlx5e_page_release_fragmented.isra.0+0xdc/0xf0 [mlx5_core]
> CPU: 9 PID: 0 Comm: swapper/9 Tainted: G        W          6.6.0
>  walk_stackframe+0x0/0x190
>  show_stack+0x70/0x94
>  dump_stack_lvl+0x98/0xd8
>  dump_stack+0x2c/0x48
>  __warn+0x1c8/0x220
>  warn_slowpath_fmt+0x20c/0x230
>  mlx5e_page_release_fragmented.isra.0+0xdc/0xf0 [mlx5_core]
>  mlx5e_free_rx_wqes+0xcc/0x120 [mlx5_core]
>  mlx5e_post_rx_wqes+0x1f4/0x4e0 [mlx5_core]
>  mlx5e_napi_poll+0x1c0/0x8d0 [mlx5_core]
>  __napi_poll+0x58/0x2e0
>  net_rx_action+0x1a8/0x340
>  __do_softirq+0x2b8/0x480
>  [...]
> 
> [2]
> Unable to handle kernel paging request at virtual address 393837363534333a
> Oops [#1]
> CPU: 72 PID: 0 Comm: swapper/72 Tainted: G        W          6.6.0
> Trace:
>  walk_stackframe+0x0/0x190
>  show_stack+0x70/0x94
>  die+0x1d4/0x350
>  do_page_fault+0x630/0x690
>  entMM+0x120/0x130
>  napi_pp_put_page+0x30/0x160
>  skb_release_data+0x164/0x250
>  kfree_skb_list_reason+0xd0/0x2f0
>  skb_release_data+0x1f0/0x250
>  napi_consume_skb+0xa0/0x220
>  net_rx_action+0x158/0x340
>  __do_softirq+0x2b8/0x480
>  irq_exit+0xd4/0x120
>  do_entInt+0x164/0x520
>  entInt+0x114/0x120
>  [...]
> 
> Fixes: 069d11465a80 ("net/mlx5e: RX, Enhance legacy Receive Queue memory scheme")
> Signed-off-by: Mingrui Cui <mingruic@outlook.com>
> ---
> Changes in v2:
>   - Add Fixes tag and more details to commit message.
>   - Target 'net' branch.
>   - Remove the obsolete WARN_ON() and update related comments.
> Link to v1: https://lore.kernel.org/all/MN6PR16MB5450CAF432AE40B2AFA58F61B706A@MN6PR16MB5450.namprd16.prod.outlook.com/
> 
>  .../net/ethernet/mellanox/mlx5/core/en/params.c   | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/params.c b/drivers/net/ethernet/mellanox/mlx5/core/en/params.c
> index 3cca06a74cf9..00b44da23e00 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/params.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/params.c
> @@ -666,7 +666,7 @@ static void mlx5e_rx_compute_wqe_bulk_params(struct mlx5e_params *params,
>  	info->refill_unit = DIV_ROUND_UP(info->wqe_bulk, split_factor);
>  }
>  
> -#define DEFAULT_FRAG_SIZE (2048)
> +#define DEFAULT_FRAG_SIZE (PAGE_SIZE / 2)
>  

What else does changing this DEFAULT_FRAG_SIZE affect? Presumably we're
allocating larger fragments now for 8K or larger page sizes.

>  static int mlx5e_build_rq_frags_info(struct mlx5_core_dev *mdev,
>  				     struct mlx5e_params *params,
> @@ -756,18 +756,13 @@ static int mlx5e_build_rq_frags_info(struct mlx5_core_dev *mdev,
>  		/* No WQE can start in the middle of a page. */
>  		info->wqe_index_mask = 0;
>  	} else {
> -		/* PAGE_SIZEs starting from 8192 don't use 2K-sized fragments,
> -		 * because there would be more than MLX5E_MAX_RX_FRAGS of them.
> -		 */
> -		WARN_ON(PAGE_SIZE != 2 * DEFAULT_FRAG_SIZE);
> -

So previously we would warn, but now we just fix the DEFAULT_FRAG_SIZE
so this warning is redundant.. Ok.

>  		/* Odd number of fragments allows to pack the last fragment of
>  		 * the previous WQE and the first fragment of the next WQE into
>  		 * the same page.
> -		 * As long as DEFAULT_FRAG_SIZE is 2048, and MLX5E_MAX_RX_FRAGS
> -		 * is 4, the last fragment can be bigger than the rest only if
> -		 * it's the fourth one, so WQEs consisting of 3 fragments will
> -		 * always share a page.
> +		 * As long as DEFAULT_FRAG_SIZE is (PAGE_SIZE / 2), and
> +		 * MLX5E_MAX_RX_FRAGS is 4, the last fragment can be bigger than
> +		 * the rest only if it's the fourth one, so WQEs consisting of 3
> +		 * fragments will always share a page.
>  		 * When a page is shared, WQE bulk size is 2, otherwise just 1.
>  		 */
>  		info->wqe_index_mask = info->num_frags % 2;

Would it be possible to fix the other logic so that it works for a
DEFAULT_FRAG_SIZE of 2k on 8K pages? I guess if there's no negative to
increasing the frag size then this fix makes sense since it is simple.

I guess the noted fixed commit limits to 4 fragments, but it make some
assumptions that were wrong for 8K pages?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]