On Thu, Dec 28, 2023 at 9:43 AM Barry Song <21cnbao@xxxxxxxxx> wrote:
>
> On Thu, Dec 28, 2023 at 7:26 AM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
> >
> > On Wed, Dec 27, 2023 at 3:10 AM Barry Song <21cnbao@xxxxxxxxx> wrote:
> > >
> > > On Wed, Dec 27, 2023 at 5:16 PM Chengming Zhou
> > > <zhouchengming@xxxxxxxxxxxxx> wrote:
> > > >
> > > > Thanks for your explanation! Maybe it's best for us to return to 2 pages
> > > > if no other people's comments. And this really need more documentation :-)
> >
> > Fine by me. Hmm we're basically wasting one extra page per CPU (since
> > these buffers are per-CPU), correct? That's not ideal, but not *too*
> > bad for now I suppose...
> >
> > >
> > > I agree. we need some doc.
> > >
> > > besides, i actually think we can skip zswap frontend if
> > > over-compression is really
> > > happening.
> >
> > IIUC, zsmalloc already checked that - and most people are (or should
> > be) using zsmalloc for zswap anyway. I wouldn't be opposed to adding
> > an added layer of protection on the zswap side, but not super high
> > priority I'd say.
>
> Thanks for this info. I guess you mean the below ?
> unsigned long zs_malloc(struct zs_pool *pool, size_t size, gfp_t gfp)
> {
> ...
>
> if (unlikely(!size || size > ZS_MAX_ALLOC_SIZE))
> return (unsigned long)ERR_PTR(-EINVAL);
BTW, do you think zsmalloc is worth a patch to change the ret from
EINVAL to ENOSPC?
This seems more sensible and matches the behaviour of zswap, and zbud, z3fold.
ret = zpool_malloc(zpool, dlen, gfp, &handle);
if (ret == -ENOSPC) {
zswap_reject_compress_poor++;
goto put_dstmem;
}
if (ret) {
zswap_reject_alloc_fail++;
goto put_dstmem;
}
buf = z
>
> }
>
> i find zbud also has similar code:
> static int zbud_alloc(struct zbud_pool *pool, size_t size, gfp_t gfp,
> unsigned long *handle)
> {
> int chunks, i, freechunks;
> struct zbud_header *zhdr = NULL;
> enum buddy bud;
> struct page *page;
>
> if (!size || (gfp & __GFP_HIGHMEM))
> return -EINVAL;
> if (size > PAGE_SIZE - ZHDR_SIZE_ALIGNED - CHUNK_SIZE)
> return -ENOSPC;
>
> and z3fold,
>
> static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
> unsigned long *handle)
> {
> int chunks = size_to_chunks(size);
> struct z3fold_header *zhdr = NULL;
> struct page *page = NULL;
> enum buddy bud;
> bool can_sleep = gfpflags_allow_blocking(gfp);
>
> if (!size || (gfp & __GFP_HIGHMEM))
> return -EINVAL;
>
> if (size > PAGE_SIZE)
> return -ENOSPC;
>
>
> Thus, I agree that another layer to check size in zswap isn't necessary now.
Thanks
Barry
