Received-SPF: pass (google.com: domain of linux-kernel+bounces-213883-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 13 Jun 2024 22:03:09 +0300
From: Leon Romanovsky <leon@kernel.org>
To: Anand Khoje <anand.a.khoje@oracle.com>
Cc: linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org, rama.nichanamatlu@oracle.com,
	manjunath.b.patil@oracle.com
Subject: Re: [PATCH v2] RDMA/mlx5 : Reclaim max 50K pages at once
Message-ID: <20240613190309.GI4966@unreal>
References: <20240613121252.93315-1-anand.a.khoje@oracle.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240613121252.93315-1-anand.a.khoje@oracle.com>

On Thu, Jun 13, 2024 at 05:42:52PM +0530, Anand Khoje wrote:
> In non FLR context, at times CX-5 requests release of ~8 million FW pages.
> This needs humongous number of cmd mailboxes, which to be released once
> the pages are reclaimed. Release of humongous number of cmd mailboxes is
> consuming cpu time running into many seconds. Which with non preemptible
> kernels is leading to critical process starving on that cpu’s RQ.
> To alleviate this, this change restricts the total number of pages
> a worker will try to reclaim maximum 50K pages in one go.
> The limit 50K is aligned with the current firmware capacity/limit of
> releasing 50K pages at once per MLX5_CMD_OP_MANAGE_PAGES + MLX5_PAGES_TAKE
> device command.
> 
> Our tests have shown significant benefit of this change in terms of
> time consumed by dma_pool_free().
> During a test where an event was raised by HCA
> to release 1.3 Million pages, following observations were made:
> 
> - Without this change:
> Number of mailbox messages allocated was around 20K, to accommodate
> the DMA addresses of 1.3 million pages.
> The average time spent by dma_pool_free() to free the DMA pool is between
> 16 usec to 32 usec.
>            value  ------------- Distribution ------------- count
>              256 |                                         0
>              512 |@                                        287
>             1024 |@@@                                      1332
>             2048 |@                                        656
>             4096 |@@@@@                                    2599
>             8192 |@@@@@@@@@@                               4755
>            16384 |@@@@@@@@@@@@@@@                          7545
>            32768 |@@@@@                                    2501
>            65536 |                                         0
> 
> - With this change:
> Number of mailbox messages allocated was around 800; this was to
> accommodate DMA addresses of only 50K pages.
> The average time spent by dma_pool_free() to free the DMA pool in this case
> lies between 1 usec to 2 usec.
>            value  ------------- Distribution ------------- count
>              256 |                                         0
>              512 |@@@@@@@@@@@@@@@@@@                       346
>             1024 |@@@@@@@@@@@@@@@@@@@@@@                   435
>             2048 |                                         0
>             4096 |                                         0
>             8192 |                                         1
>            16384 |                                         0
> 
> Signed-off-by: Anand Khoje <anand.a.khoje@oracle.com>
> ---
> Changes in v2:
>  - In v1, CPUs were yielded if more than 2 msec are spent in
>    mlx5_free_cmd_msg(). The approach to limit the time spent is changed
>    in this version.
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> index 1b38397..b1cf97d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> @@ -482,12 +482,16 @@ static int reclaim_pages(struct mlx5_core_dev *dev, u32 func_id, int npages,
>  	return err;
>  }
>  
> +#define MAX_RECLAIM_NPAGES -50000
>  static void pages_work_handler(struct work_struct *work)
>  {
>  	struct mlx5_pages_req *req = container_of(work, struct mlx5_pages_req, work);
>  	struct mlx5_core_dev *dev = req->dev;
>  	int err = 0;
>  
> +	if (req->npages < MAX_RECLAIM_NPAGES)
> +		req->npages = MAX_RECLAIM_NPAGES;

I like this change more than previous variant with yield.
Regarding the patch:
1. Please limit the number of pages in req_pages_handler() and not int pages_work_handler().
2. Patch title should be "net/mlx5: Reclaim max 50K pages at once" and not "RDMA...".
3. You should run get_maintainer.pl script to find the right maintainers and add them to the TO or CC list.

And I still think that you will get better performance by parallelizing the reclaim process.

Thanks

> +
>  	if (req->release_all)
>  		release_all_pages(dev, req->func_id, req->ec_function);
>  	else if (req->npages < 0)
> -- 
> 1.8.3.1
> 
>