Re: [PATCH RFC] mm/zswap: reference the pool by index to shrink struct zswap_entry

[Nhat Pham]

On Mon, Jul 27, 2026 at 6:17 PM Jianyue Wu <wujianyue000@xxxxxxxxx> wrote:
>
> On Tue, Jul 28, 2026 at 1:07 AM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
>
> Thanks Yosry and Nhat for the review.
>
> > Technically we are introducing a new limit on the number of pools, so
> > let's mention that.
>
> That's good, can simplify a lot, will do in v2.
>
> > Did you try to benchmark it? I doubt the indirection would actually
> > show up as most use cases should have a single pool.
>
> I ran zswap e2e benchmark comparing baseline vs this RFC and did not
> observe a measurable performance regression.
> BTW, it seems zswap crypto-sg zstd and lzo are slower than zcomp.
> Throughput in MB/s:
>  alg      | acomp store | zcomp store | acomp load | zcomp load
> ----------+-------------+-------------+------------+-----------
>  lzo      |        1140 |        1710 |       1070 |       1070
>  lzo-rle  |        1490 |        1320 |       2030 |       2070
>  lz4      |        1520 |        1600 |       1710 |       1870
>  lz4hc    |         640 |         680 |       1770 |       1850
>  zstd     |         740 |        1190 |        980 |       1720
>  deflate  |         180 |         180 |        665 |        660
> But that's another topic.

What the hell. Do you know why that's the case? That's almost double
the the throughput with zcomp???

>
> > No need to document the function name here, most probably will be
> > stale at some point.
>
> Ack.
>
> > This seems like the only user of zswap_pool_by_index(), just open-code
> > the xa_load() here.
> >
> > Let's add a NULL check here to avoid blowing up if something goes wrong.
> >
> > We need a NULL check here as well.
>
> Ack on all of these.
>
> > Maybe just add a fixed array and call it a day? :) Seems like we can
> > probably get away with, say a fixed array 16 elements.
> >
> > If we want to be really conservative we can use an array of 256
> > elements indexed by a u8. That's 1KB of memory, probably mostly wasted.
>
> I prefer dropping the xarray and go with 16 entries indexed by a u8,
> keeping runtime compressor switching, which can increase when needed.

Let's go with that. We can be defensive and just block users when they
try to add the 17th zswap pool.

Look, you don't need 17 of them :) If you're switching to the 17th
compressor there is likely a bug in your userspace config/scripts :)

>
> Agreed, the list looks like unneeded complexity, will have a try to drop it.
> Looks obj_cgroup pointer can also be dropped on !MEMCG, but zswap usually use
> in server with cgroup, maybe not worth pursuing.

Yeah you can leave that for now :)

>
> Best regards,
> Jianyue