lkml.org 
[lkml]   [2023]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 1/5] mm/zswap: reuse dstmem when decompress
    On Thu, Dec 14, 2023 at 6:42 AM Chengming Zhou
    <zhouchengming@bytedance.com> wrote:
    [..]
    > >>>> -
    > >>>> /* decompress */
    > >>>> - dlen = PAGE_SIZE;
    > >>>> - src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
    > >>>> + acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
    > >>>> + mutex_lock(acomp_ctx->mutex);
    > >>>>
    > >>>> + zpool = zswap_find_zpool(entry);
    > >>>> + src = zpool_map_handle(zpool, entry->handle, ZPOOL_MM_RO);
    > >>>> if (!zpool_can_sleep_mapped(zpool)) {
    > >>>> - memcpy(tmp, src, entry->length);
    > >>>> - src = tmp;
    > >>>> + memcpy(acomp_ctx->dstmem, src, entry->length);
    > >>>> + src = 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 using
    > >>> 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_ctx,
    > >> 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 = raw_smp_processor_id();
    >
    > mutex = per_cpu(zswap_mutex, cpu);
    > mutex_lock(mutex);
    >
    > dstmem = 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 = 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.

    \
     
     \ /
      Last update: 2023-12-14 19:26    [W:3.570 / U:0.056 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site