Received-SPF: pass (google.com: domain of linux-kernel+bounces-3431-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <c53eb450-ebce-4ac3-91cd-59d5ae44d702@bytedance.com>
Date: Mon, 18 Dec 2023 19:26:47 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 6/6] mm/zswap: directly use percpu mutex and buffer in
 load/store
Content-Language: en-US
To: Yosry Ahmed <yosryahmed@google.com>
Cc: Seth Jennings <sjenning@redhat.com>, Dan Streetman <ddstreet@ieee.org>,
 Chris Li <chriscli@google.com>, Nhat Pham <nphamcs@gmail.com>,
 Vitaly Wool <vitaly.wool@konsulko.com>,
 Andrew Morton <akpm@linux-foundation.org>,
 Johannes Weiner <hannes@cmpxchg.org>, linux-kernel@vger.kernel.org,
 linux-mm@kvack.org, Chris Li <chrisl@kernel.org>
References: <20231213-zswap-dstmem-v2-0-daa5d9ae41a7@bytedance.com>
 <20231213-zswap-dstmem-v2-6-daa5d9ae41a7@bytedance.com>
 <CAJD7tkZSrd-R-vuVqh29fgZ6bmo3EJxdb2Eahm0Ozg5hkPEjRg@mail.gmail.com>
From: Chengming Zhou <zhouchengming@bytedance.com>
In-Reply-To: <CAJD7tkZSrd-R-vuVqh29fgZ6bmo3EJxdb2Eahm0Ozg5hkPEjRg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 2023/12/18 17:37, Yosry Ahmed wrote:
> On Mon, Dec 18, 2023 at 12:22 AM Chengming Zhou
> <zhouchengming@bytedance.com> wrote:
>>
>> Since the introduce of reusing the dstmem in the load path, it seems
>> confusing that we are now using acomp_ctx->dstmem and acomp_ctx->mutex
>> now for purposes other than what the naming suggests.
>>
>> Yosry suggested 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.
>>
>> So this patch remove dstmem and mutex from acomp_ctx, and rename the
>> zswap_dstmem to zswap_buffer, using the percpu mutex and buffer on
>> the load and store paths. And refactor out __zswap_store() to only
>> include the compress & store, since I found zswap_store() is too long.
> 
> I am not sure refactoring out __zswap_store() is useful to be honest,
> but I am not objecting to it, it mirrors __zswap_load() in a sense.

Yes, it mirrors __zswap_load() and only includes compress and store.
And it makes easy for me to only concentrate on __zswap_store/load()
when renaming the percpu buffers and mutex. But if anyone has objection,
I can drop it.

> However, if you want to do so, please do it in a separate patch from
> renaming the percpu buffers and mutex. This will make reviewing easier
> (and make my Suggested-by correctly scoped).

Right, will do.

> 
> Also, any reason why raw_smp_processor_id() is used here instead of
> smp_processor_id()?
> 

Here we don't need the CPU id stable, since we only need to pick one
CPU and use the mutex to serialize.

And from the comments below in <include/linux/smp.h>, WARN would happen
if we use smp_processor_id() here without other helpers.

 * The CPU id is stable when:
 *
 *  - IRQs are disabled;
 *  - preemption is disabled;
 *  - the task is CPU affine.

 * When CONFIG_DEBUG_PREEMPT; we verify these assumption and WARN
 * when smp_processor_id() is used when the CPU id is not stable.

Thanks!