On Fri, Jun 28, 2024 at 03:13:45PM +0000, David Laight wrote:
> From: Leon Romanovsky
> > Sent: 24 June 2024 10:58
> >
> > On Wed, Jun 19, 2024 at 06:58:27PM +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@xxxxxxxxxx>
> > > Reviewed-by: Leon Romanovsky <leonro@xxxxxxxxxx>
> > > ---
> > > Changes in v4:
> > > - Fixed a nit in patch subject.
> > > ---
> > > drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c | 7 ++++++-
> > > 1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> > > index dcf58ef..06eee3a 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> > > @@ -608,6 +608,7 @@ enum {
> > > RELEASE_ALL_PAGES_MASK = 0x4000,
> > > };
> > >
> > > +#define MAX_RECLAIM_NPAGES -50000
>
> It would be traditional to enclose a negative value in ().
> (Although only 30+ year old compilers would generate unexpected code for
> foo-MAX_RECLAIM_NPAGES
> and you have to go back into the 1970s for
> foo=MAX_RECLAIM_NPAGES
> to be a problem.)
>
> > > static int req_pages_handler(struct notifier_block *nb,
> > > unsigned long type, void *data)
> > > {
> > > @@ -639,9 +640,13 @@ static int req_pages_handler(struct notifier_block *nb,
> > >
> > > req->dev = dev;
> > > req->func_id = func_id;
> > > - req->npages = npages;
> > > req->ec_function = ec_function;
> > > req->release_all = release_all;
> > > + if (npages < MAX_RECLAIM_NPAGES)
> > > + req->npages = MAX_RECLAIM_NPAGES;
> > > + else
> > > + req->npages = npages;
> > > +
> >
> > BTW, this can be written as:
> > req->npages = max_t(s32, npages, MAX_RECLAIM_NPAGES);
>
> That shouldn't need all the (s32) casts.
#define doesn't have a type, so it is better to be explicit here.
> (I don't think it even needed them before I relaxed the type check.)
>
> David
>
> >
> > Thanks
> >
> > > INIT_WORK(&req->work, pages_work_handler);
> > > queue_work(dev->priv.pg_wq, &req->work);
> > > return NOTIFY_OK;
> > > --
> > > 1.8.3.1
> > >
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
