On Sun, Feb 09, 2025 at 12:17:07PM +0200, Tariq Toukan wrote:
> From: William Tu <witu@xxxxxxxxxx>
>
> For the ECPF and representors, reduce the max MPWRQ size from 256KB (18)
> to 128KB (17). This prepares the later patch for saving representor
> memory.
>
> With Striding RQ, there is a minimum of 4 MPWQEs. So with 128KB of max
> MPWRQ size, the minimal memory is 4 * 128KB = 512KB. When creating page
> pool, consider 1500 mtu, the minimal page pool size will be 512KB/4KB =
> 128 pages = 256 rx ring entries (2 entries per page).
>
> Before this patch, setting RX ringsize (ethtool -G rx) to 256 causes
> driver to allocate page pool size more than it needs due to max MPWRQ
> is 256KB (18). Ex: 4 * 256KB = 1MB, 1MB/4KB = 256 pages, but actually
> 128 pages is good enough. Reducing the max MPWRQ to 128KB fixes the
> limitation.
>
> Signed-off-by: William Tu <witu@xxxxxxxxxx>
> Signed-off-by: Tariq Toukan <tariqt@xxxxxxxxxx>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en.h | 2 --
> .../net/ethernet/mellanox/mlx5/core/en/params.c | 15 +++++++++++----
> 2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> index 979fc56205e1..534fdd27c8de 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> @@ -95,8 +95,6 @@ struct page_pool;
> #define MLX5_MPWRQ_DEF_LOG_STRIDE_SZ(mdev) \
> MLX5_MPWRQ_LOG_STRIDE_SZ(mdev, order_base_2(MLX5E_RX_MAX_HEAD))
>
> -#define MLX5_MPWRQ_MAX_LOG_WQE_SZ 18
> -
> /* Keep in sync with mlx5e_mpwrq_log_wqe_sz.
> * These are theoretical maximums, which can be further restricted by
> * capabilities. These values are used for static resource allocations and
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/params.c b/drivers/net/ethernet/mellanox/mlx5/core/en/params.c
> index 64b62ed17b07..e37d4c202bba 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/params.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/params.c
> @@ -10,6 +10,9 @@
> #include <net/page_pool/types.h>
> #include <net/xdp_sock_drv.h>
>
> +#define MLX5_MPWRQ_MAX_LOG_WQE_SZ 18
> +#define MLX5_REP_MPWRQ_MAX_LOG_WQE_SZ 17
> +
> static u8 mlx5e_mpwrq_min_page_shift(struct mlx5_core_dev *mdev)
> {
> u8 min_page_shift = MLX5_CAP_GEN_2(mdev, log_min_mkey_entity_size);
> @@ -103,18 +106,22 @@ u8 mlx5e_mpwrq_log_wqe_sz(struct mlx5_core_dev *mdev, u8 page_shift,
> enum mlx5e_mpwrq_umr_mode umr_mode)
> {
> u8 umr_entry_size = mlx5e_mpwrq_umr_entry_size(umr_mode);
> - u8 max_pages_per_wqe, max_log_mpwqe_size;
> + u8 max_pages_per_wqe, max_log_wqe_size_calc;
> + u8 max_log_wqe_size_cap;
> u16 max_wqe_size;
>
> /* Keep in sync with MLX5_MPWRQ_MAX_PAGES_PER_WQE. */
> max_wqe_size = mlx5e_get_max_sq_aligned_wqebbs(mdev) * MLX5_SEND_WQE_BB;
> max_pages_per_wqe = ALIGN_DOWN(max_wqe_size - sizeof(struct mlx5e_umr_wqe),
> MLX5_UMR_FLEX_ALIGNMENT) / umr_entry_size;
> - max_log_mpwqe_size = ilog2(max_pages_per_wqe) + page_shift;
> + max_log_wqe_size_calc = ilog2(max_pages_per_wqe) + page_shift;
> +
> + WARN_ON_ONCE(max_log_wqe_size_calc < MLX5E_ORDER2_MAX_PACKET_MTU);
>
> - WARN_ON_ONCE(max_log_mpwqe_size < MLX5E_ORDER2_MAX_PACKET_MTU);
> + max_log_wqe_size_cap = mlx5_core_is_ecpf(mdev) ?
> + MLX5_REP_MPWRQ_MAX_LOG_WQE_SZ : MLX5_MPWRQ_MAX_LOG_WQE_SZ;
>
> - return min_t(u8, max_log_mpwqe_size, MLX5_MPWRQ_MAX_LOG_WQE_SZ);
> + return min_t(u8, max_log_wqe_size_calc, max_log_wqe_size_cap);
Changing the variable name looks like uneccessary complication, as it is
still used for the same purpouse.
I remember there were some patches to devlink for supporting changing
the representor parameters. Is it sth different or you decided to only
change the default value to fix the memory problem? (sorry, maybe I miss
the devlink series).
Anyway, looks fine, thanks:
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@xxxxxxxxxxxxxxx>
> }
>
> u8 mlx5e_mpwrq_pages_per_wqe(struct mlx5_core_dev *mdev, u8 page_shift,
> --
> 2.45.0
