Re: [PATCH v5] net/mlx5: Reclaim max 50K pages at once

[Leon Romanovsky <leon@kernel.org>]

On Wed, Jun 26, 2024 at 04:19:17AM +0800, Zhu Yanjun wrote:
> 在 2024/6/25 13:00, Anand Khoje 写道:
> > 
> > On 6/25/24 02:11, Jesse Brandeburg wrote:
> > > On 6/24/2024 8:33 AM, Anand Khoje wrote:
> > > 
> > > > --- 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
> > > Can you please explain why this is negative? There doesn't seem to be
> > > any reason mentioned in the commit message or code.
> > > 
> > > At the very least it's super confusing to have a MAX be negative, and at
> > > worst it's a bug. I don't have any other context on this code besides
> > > this patch, so an explanation would be helpful.
> > > 
> > > 
> > > 
> > Hi Jesse,
> > 
> > The way Mellanox ConnectX5 driver handles 'release of allocated pages
> > from HCA' or 'allocation of pages to HCA', is by sending an event to the
> > host. This event will have number of pages in it. If the number is
> > positive, that indicates HCA is requesting that number of pages to be
> > allocated. And if that number is negative, it is the HCA indicating that
> > that number of pages can be reclaimed by the host.
> > 
> > In this patch we are restricting the maximum number of pages that can be
> > reclaimed to be 50000 (effectively this would be -50000 as it is
> > reclaim). This limit is based on the capability of the firmware as it
> > cannot release more than 50000 back to the host in one go.
> > 
> > I hope that explains.
> 
> To be honest, I am also obvious why this MACRO is defined as a negative
> number. From the above, I can understand why. I think, perhaps many people
> also wonder why it is defined as a negative. IMO, it is better that you put
> the above explanations into the source code as comments.
> When users check the source code, from the comments, users will know why it
> is defined as a negative number.

I see no problem with adding a comment to the code, but I think that it
won't help anyone. The whole reclaim/give page logic inside the mlx5
driver is written with the assumption that the number of pages is
negative for reclaim and positive for give.

Thanks

> 
> Thanks a lot.
> Zhu Yanjun
> 
> > 
> > Thanks,
> > 
> > Anand
> > 
> 
>