Received-SPF: pass (google.com: domain of linux-kernel+bounces-42106-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: <20240103003011.211382-1-v-songbaohua@oppo.com>
 <20240106013021.GA123449@google.com> <CAGsJ_4xp7HFuYbDp3UjMqFKSuz2HJn+5JnJdB-PP_GmucQqOpg@mail.gmail.com>
 <20240115023457.GA1504420@google.com>
In-Reply-To: <20240115023457.GA1504420@google.com>
From: Barry Song <21cnbao@gmail.com>
Date: Mon, 29 Jan 2024 10:46:38 +0800
Message-ID: <CAGsJ_4x6e7=tWVKfauJaGMRmJ3tz7GymKosYcHrxoxEnAHyX6g@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 Mon, Jan 15, 2024 at 10:35=E2=80=AFAM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (24/01/06 15:38), Barry Song wrote:
> > 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 unpluggi=
ng and
> > re-allocate it during hotplugging. this can happen quite often for syst=
ems like
> > Android using hotplug for power management.
>
> Okay, makes sense.
> Do you see these problems in real life? I don't recall any reports.

i don't have problems with the current zram which supports normal pages onl=
y.

but  in our out-of-tree code, we have enhanced zram/zsmalloc to support lar=
ge
folios compression/decompression, which will make zram work much better
with large anon folios/mTHP things on which Ryan Roberts is working on.

I mean, a large folio with for example 16 normal pages can be saved as
one object in zram.
In millions of phones, we have deployed this approach and seen huge improve=
ment
on compression ratio and cpu consumption decrease. in that case, we
need a larger
per-cpu buffer, and have seen frequent failure on allocation. that
inspired me to send
this patch in advance.

>
> > > 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 dif=
ferent
> > 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 nega=
tively
> > affecting user experience. it has now been replaced by vmalloc and make=
s
> > life easier :-)
>
> Sure.
>
> > > I also wonder whether Android uses HW compression, in which case we
> > > may need to have physically contig pages. Not to mention TLB shootdow=
ns
> > > 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.
>
> Oh, those are not related. TLB shootdowns are what now will be added to
> all compressions/decompressions, so it's sort of extra cost.

i am sorry i still don't understand where the tlb shootdowns come
from. we don't unmap
this per-cpu buffers during compression and decompression, do we ?

am i missing something?

> HW compression (which android may be doing?) is another story.
>
> Did you run any perf tests on zram w/ and w/o the patch?
>
> > We are always passing a virtual address, traditional HW drivers use the=
ir own
> > 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 direc=
tly,
> > 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=
, dlen);
> > 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
>
> Right.
>
> > 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/
>
> Nice.

Thanks
Barry