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

[Saeed Mahameed <saeed@kernel.org>]

On 02 Sep 21:00, 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. 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.
>
>Fix this by making DEFAULT_FRAG_SIZE always equal to half of the page
>size.
>
>Signed-off-by: Mingrui Cui <mingruic@outlook.com>
CC:  Dragos Tatulea <dtatulea@nvidia.com>

Dragos is on a mission to improve page_size support in mlx5.

Dragos, please look into this, I am not sure making  DEFAULT_FRAG_SIZE
dependant on PAGE_SIZE is the correct way to go,
see mlx5e_build_rq_frags_info()

I believe we don't do page flipping for > 4k pages, but I might be wrong,
anyway the code also should throw a warn_on: 

/* The last fragment of WQE with index 2*N may share the page with the
  * first fragment of WQE with index 2*N+1 in certain cases. If WQE
  * 2*N+1
  * is not completed yet, WQE 2*N must not be allocated, as it's
  * responsible for allocating a new page.
  */
if (frag_size_max == PAGE_SIZE) {
	/* 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);
	/* 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.
	 * When a page is shared, WQE bulk size is 2, otherwise
	 * just 1.
	 */
	info->wqe_index_mask = info->num_frags % 2;
}

Looking at the above makes me think that this patch is correct, but a more
careful look is needed to be taken, a Fixes tag is also required and target
'net' branch.

Thanks,
Saeed.

>---
> drivers/net/ethernet/mellanox/mlx5/core/en/params.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/params.c b/drivers/net/ethernet/mellanox/mlx5/core/en/params.c
>index 3cca06a74cf9..d96a3cbea23c 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)
>
> static int mlx5e_build_rq_frags_info(struct mlx5_core_dev *mdev,
> 				     struct mlx5e_params *params,
>-- 
>2.43.0
>
>