Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
MIME-Version: 1.0
References: <20231213-zswap-dstmem-v1-0-896763369d04@bytedance.com>
 <20231213-zswap-dstmem-v1-1-896763369d04@bytedance.com> <CAJD7tkbicqevYuJbTUZBQUo2n_k-HKTixG5TbXGtdbtFJgFmfg@mail.gmail.com>
 <8f80fe3e-a8c7-463d-896b-99575c362839@bytedance.com> <CAJD7tkbS_3eEDbfo-w1Z+tuY44YKMYnAqaL0684Y6nh2EmHq+Q@mail.gmail.com>
 <dc84f965-9174-4ce7-aedf-f68d8e8003b4@bytedance.com>
In-Reply-To: <dc84f965-9174-4ce7-aedf-f68d8e8003b4@bytedance.com>
From:   Yosry Ahmed <yosryahmed@google.com>
Date:   Thu, 14 Dec 2023 10:24:36 -0800
Message-ID: <CAJD7tkYdisUg+wphANLFcBSLB13jH+NxqFdc-EjKBq0H-=S00Q@mail.gmail.com>
Subject: Re: [PATCH 1/5] mm/zswap: reuse dstmem when decompress
To:     Chengming Zhou <zhouchengming@bytedance.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Nhat Pham <nphamcs@gmail.com>, Chris Li <chriscli@google.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Seth Jennings <sjenning@redhat.com>,
        Dan Streetman <ddstreet@ieee.org>,
        Vitaly Wool <vitaly.wool@konsulko.com>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Dec 14, 2023 at 6:42=E2=80=AFAM Chengming Zhou
<zhouchengming@bytedance.com> wrote:
[..]
> >>>> -
> >>>>         /* decompress */
> >>>> -       dlen =3D PAGE_SIZE;
> >>>> -       src =3D zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
> >>>> +       acomp_ctx =3D raw_cpu_ptr(entry->pool->acomp_ctx);
> >>>> +       mutex_lock(acomp_ctx->mutex);
> >>>>
> >>>> +       zpool =3D zswap_find_zpool(entry);
> >>>> +       src =3D zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
> >>>>         if (!zpool_can_sleep_mapped(zpool)) {
> >>>> -               memcpy(tmp, src, entry->length);
> >>>> -               src =3D tmp;
> >>>> +               memcpy(acomp_ctx->dstmem, src, entry->length);
> >>>> +               src =3D acomp_ctx->dstmem;
> >>>
> >>> I don't like that we are now using acomp_ctx->dstmem and
> >>> acomp_ctx->mutex now for purposes other than what the naming suggests=
.
> >>
> >> The "mutex" name is coherent, "dstmem" depends on how we use IMHO.
> >> Change to just "mem"? Or do you have a better name to replace?
> >>
> >>>
> >>> How about removing these two fields from acomp_ctx, and directly usin=
g
> >>> zswap_dstmem and zswap_mutex in both the load and store paths, rename
> >>> them, and add proper comments above their definitions that they are
> >>> for generic percpu buffering on the load and store paths?
> >>
> >> Yes, they are percpu memory and lock, but they are used by per acomp_c=
tx,
> >> and the cpu maybe changing in the middle, so maybe better to keep them=
.
> >
> > I don't mean to remove completely. Keep them as (for example)
> > zswap_mem and zswap_mutex global percpu variables, and not have
> > pointers in acomp_ctx to them. Instead of using acomp_ctx->dstmem
> > today, we directly use the global zswap_mem (same for the mutex).
> >
> > This makes it clear that the buffers are not owned or exclusively used
> > by the acomp_ctx. WDYT?
>
> Does this look good to you?
>
> ```
> int cpu =3D raw_smp_processor_id();
>
> mutex =3D per_cpu(zswap_mutex, cpu);
> mutex_lock(mutex);
>
> dstmem =3D per_cpu(zswap_dstmem, cpu);

Renaming to zswap_buffer or zswap_mem would be better I think, but
yeah what I had in mind is having zswap_mutex and
zswap_[dstmem/mem/buffer] be generic percpu buffers that are used by
store and load paths for different purposes, not directly linked to
acomp_ctx.

> acomp_ctx =3D per_cpu_ptr(pool->acomp_ctx, cpu);
>
> /* compress or decompress */
> ```
>
> Another way I just think of is to make acomp_ctx own its lock and buffer,
> and we could delete these percpu zswap_mutex and zswap_dstmem instead.

You mean have two separate set of percpu buffers for zswap load &
stores paths? This is probably unnecessary.