Received-SPF: pass (google.com: domain of linux-kernel+bounces-205135-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: <20240604175340.218175-1-yosryahmed@google.com> <ZmI573n9-SoK4dIg@google.com>
In-Reply-To: <ZmI573n9-SoK4dIg@google.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Thu, 6 Jun 2024 16:03:55 -0700
Message-ID: <CAJD7tkbO+ZLdhs-9BpthztZX32i8C4=QEnoiXGS7bM399nqwzg@mail.gmail.com>
Subject: Re: [PATCH] mm: zsmalloc: share slab caches for all zsmalloc zpools
To: Minchan Kim <minchan@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>, 
	Sergey Senozhatsky <senozhatsky@chromium.org>, Vlastimil Babka <vbabka@suse.cz>, 
	David Rientjes <rientjes@google.com>, Christoph Lameter <cl@linux.com>, 
	Erhard Furtner <erhard_f@mailbox.org>, linux-mm@kvack.org, linux-kernel@vger.kernel.org, 
	Yu Zhao <yuzhao@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jun 6, 2024 at 3:36=E2=80=AFPM Minchan Kim <minchan@kernel.org> wro=
te:
>
> On Tue, Jun 04, 2024 at 05:53:40PM +0000, Yosry Ahmed wrote:
> > Zswap creates multiple zpools to improve concurrency. Each zsmalloc
> > zpool creates its own 'zs_handle' and 'zspage' slab caches. Currently w=
e
> > end up with 32 slab caches of each type.
> >
> > Since each slab cache holds some free objects, we end up with a lot of
> > free objects distributed among the separate zpool caches. Slab caches
> > are designed to handle concurrent allocations by using percpu
> > structures, so having a single instance of each cache should be enough,
> > and avoids wasting more memory than needed due to fragmentation.
> >
> > Additionally, having more slab caches than needed unnecessarily slows
> > down code paths that iterate slab_caches.
> >
> > In the results reported by Eric in [1], the amount of unused slab memor=
y
> > in these caches goes down from 242808 bytes to 29216 bytes (-88%). This
> > is calculated by (num_objs - active_objs) * objsize for each 'zs_handle=
'
> > and 'zspage' cache. Although this patch did not help with the allocatio=
n
> > failure reported by Eric with zswap + zsmalloc, I think it is still
> > worth merging on its own.
> >
> > [1]https://lore.kernel.org/lkml/20240604134458.3ae4396a@yea/
>
> I doubt this is the right direction.
>
> Zsmalloc is used for various purposes, each with different object
> lifecycles. For example, swap operations relatively involve short-lived
> objects, while filesystem use cases might have longer-lived objects.
> This mix of lifecycles could lead to fragmentation with this approach.

Even in a swapfile, some objects can be short-lived and some objects
can be long-lived, and the line between swap and file systems both
becomes blurry with shmem/tmpfs. I don't think having separate caches
here is vital, but I am not generally familiar with the file system
use cases and I don't have data to prove/disprove it.

>
> I believe the original problem arose when zsmalloc reduced its lock
> granularity from the class level to a global level. And then, Zswap went
> to mitigate the issue with multiple zpools, but it's essentially another
> bandaid on top of the existing problem, IMO.

IIRC we reduced the granularity when we added writeback support to
zsmalloc, which was relatively recent. I think we have seen lock
contention with zsmalloc long before that. We have had a similar patch
internally to use multiple zpools in zswap for many years now.

+Yu Zhao

Yu has more historical context about this, I am hoping he will shed
more light about this.

>
> The correct approach would be to further reduce the zsmalloc lock
> granularity.

I definitely agree that the correct approach should be to fix the lock
contention at the source and drop zswap's usage of multiple zpools.
Nonetheless, I think this patch provides value in the meantime. The
fragmentation within the slab caches is real with zswap's use case.
OTOH, sharing a cache between swap and file system use cases leading
to fragmentation within the same slab cache is a less severe problem
in my opinion.

That being said, I don't feel strongly. If you really don't like this
patch I am fine with dropping it.