Received-SPF: pass (google.com: domain of linux-kernel+bounces-205406-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240606165303.431215-1-flintglass@gmail.com> <CAJD7tkZ=o3AN+4Cj5UBJv6zcrjPFW5T1_53iHB2qtShRRhKhbQ@mail.gmail.com>
 <CAPpoddc5ge0myLfN8burL9fEUjc0oaB0C8Yc3_J923hd_O9u4A@mail.gmail.com>
In-Reply-To: <CAPpoddc5ge0myLfN8burL9fEUjc0oaB0C8Yc3_J923hd_O9u4A@mail.gmail.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Thu, 6 Jun 2024 21:58:11 -0700
Message-ID: <CAJD7tkZPB8SSGgaobvFtQ5aOHjBzLt9DLxYT4j8k0sSyyLdURA@mail.gmail.com>
Subject: Re: [PATCH] mm: zswap: limit number of zpools based on CPU and RAM
To: Takero Funaki <flintglass@gmail.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>, Nhat Pham <nphamcs@gmail.com>, 
	Chengming Zhou <chengming.zhou@linux.dev>, Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jun 6, 2024 at 6:01=E2=80=AFPM Takero Funaki <flintglass@gmail.com>=
 wrote:
>
> 2024=E5=B9=B46=E6=9C=887=E6=97=A5(=E9=87=91) 2:46 Yosry Ahmed <yosryahmed=
@google.com>:
>
> >
> > There are a lot of magic numbers in this patch, and it seems like it's
> > all based on theory. I don't object to making the number of zpools
> > dynamic in some way, but unless we do it in a data-driven way where we
> > understand the implications, I think the added complexity and
> > inconsistency is not justified.
> >
> > For example, 2*CPU zpools is an overkill and will cause a lot of
> > fragmentation. We use 32 zpools right now for machines with 100s of
> > CPUs. I know that you are keeping 32 as the limit, but why 2*CPUs if
> > nr_cpus <=3D 16?
> >
> > Also, the limitation based on memory size assumes that zsmalloc is the
> > only allocator used by zswap, which is unfortunately not the case.
> >
> > The current implementation using 32 zpools all the time is not
> > perfect, and I did write a patch to make it at least be min(nr_cpus,
> > 32), but it is simple and it works. Complexity should be justified.
> >
>
> Thanks for your comments.
> I agree the 2*cpu is too much. it was conservatively chosen assuming
> 1/2 contention while all cores are accessing zswap. Much smaller
> factor or non-linear scale as your comments in the main thread would
> be better.

Chengming is currently experimenting with fixing the lock contention
problem in zsmalloc by making the lock more granular. Based on the
data he finds, we may be able to just drop the multiple zpools patch
from zswap.

I'd wait for his findings before investing more into improving this.

>
> I found your patch from the main thread.
> One point I'm afraid, this hashing will fail if nr_zswap_zpools is 1
> or is not rounded to order of 2. hash_ptr crashes when bit is 0.

Yeah that patch was just for experimenting, I did not test it well.
Thanks for taking a look.