Received-SPF: pass (google.com: domain of linux-kernel+bounces-18525-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240103003011.211382-1-v-songbaohua@oppo.com> <20240106013021.GA123449@google.com>
In-Reply-To: <20240106013021.GA123449@google.com>
From: Barry Song <21cnbao@gmail.com>
Date: Sat, 6 Jan 2024 15:38:24 +0800
Message-ID: <CAGsJ_4xp7HFuYbDp3UjMqFKSuz2HJn+5JnJdB-PP_GmucQqOpg@mail.gmail.com>
Subject: Re: [PATCH] zram: easy the allocation of zcomp_strm's buffers with 2 pages
To: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: minchan@kernel.org, axboe@kernel.dk, linux-kernel@vger.kernel.org, 
	linux-block@vger.kernel.org, Barry Song <v-songbaohua@oppo.com>, 
	Andrew Morton <akpm@linux-foundation.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Jan 6, 2024 at 9:30=E2=80=AFAM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (24/01/03 13:30), Barry Song wrote:
> > There is no need to keep zcomp_strm's buffers contiguous physically.
> > And rarely, 1-order allocation can fail while buddy is seriously
> > fragmented.
>
> Dunno. Some of these don't sound like convincing reasons, I'm afraid.
> We don't allocate compression streams all the time, we do it once
> per-CPU. And if the system is under such a terrible memory pressure

We actually do it many times actually because we free it while unplugging a=
nd
re-allocate it during hotplugging. this can happen quite often for systems =
like
Android using hotplug for power management.

> then one probably should not use zram at all, because zsmalloc needs
> pages for its pool.

In my humble opinion, 1-order allocation and 0-order allocation are differe=
nt
things, 1-order is still more difficult though it is easier than
2-order which was
a big pain causing allocation latency for tasks' kernel stacks and negative=
ly
affecting user experience. it has now been replaced by vmalloc and makes
life easier :-)

>
> I also wonder whether Android uses HW compression, in which case we
> may need to have physically contig pages. Not to mention TLB shootdowns
> that virt contig pages add to the picture.

I don't understand how HW compression and TLB shootdown are related as zRAM
is using a traditional comp API.
We are always passing a virtual address, traditional HW drivers use their o=
wn
buffers to do DMA.

int crypto_comp_compress(struct crypto_comp *comp,
const u8 *src, unsigned int slen,
u8 *dst, unsigned int *dlen);
int crypto_comp_decompress(struct crypto_comp *comp,
  const u8 *src, unsigned int slen,
  u8 *dst, unsigned int *dlen);

In new acomp API, we are passing a sg - users' buffers to drivers directly,
sg_init_one(&input, src, entry->length);
sg_init_table(&output, 1);
sg_set_page(&output, page, PAGE_SIZE, 0);
acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, dl=
en);
ret =3D crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req),
&acomp_ctx->wait);

but i agree one-nents sg might have some advantage in scompress case
after we move
to new acomp APIs if we have this patch I sent recently [patch 3/3],
https://lore.kernel.org/linux-mm/20240103095006.608744-1-21cnbao@gmail.com/

For the current zRAM code, I guess HW compression/TLB is not a concern.

>
> [..]
> > @@ -37,7 +38,7 @@ static void zcomp_strm_free(struct zcomp_strm *zstrm)
> >  {
> >       if (!IS_ERR_OR_NULL(zstrm->tfm))
> >               crypto_free_comp(zstrm->tfm);
> > -     free_pages((unsigned long)zstrm->buffer, 1);
> > +     vfree(zstrm->buffer);
> >       zstrm->tfm =3D NULL;
> >       zstrm->buffer =3D NULL;
> >  }
> > @@ -53,7 +54,7 @@ static int zcomp_strm_init(struct zcomp_strm *zstrm, =
struct zcomp *comp)
> >        * allocate 2 pages. 1 for compressed data, plus 1 extra for the
> >        * case when compressed size is larger than the original one
> >        */
> > -     zstrm->buffer =3D (void *)__get_free_pages(GFP_KERNEL | __GFP_ZER=
O, 1);
> > +     zstrm->buffer =3D vzalloc(2 * PAGE_SIZE);
> >       if (IS_ERR_OR_NULL(zstrm->tfm) || !zstrm->buffer) {
> >               zcomp_strm_free(zstrm);
> >               return -ENOMEM;

Thanks
Barry