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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 26 Jun 2024 08:34:37 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Zhu Yanjun <yanjun.zhu@...ux.dev>
Cc: Anand Khoje <anand.a.khoje@...cle.com>,
	Jesse Brandeburg <jesse.brandeburg@...el.com>,
	linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, saeedm@...lanox.com, tariqt@...dia.com,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	davem@...emloft.net
Subject: Re: [PATCH v5] net/mlx5: Reclaim max 50K pages at once

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
> > 
> 
>