Re: [PATCH] mm/zswap: Replace kmap_atomic() with kmap_local_page()

From: Chris Li <chrisl@kernel.org>
To: "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>
Cc: Seth Jennings <sjenning@redhat.com>,
	Dan Streetman <ddstreet@ieee.org>,
	Vitaly Wool <vitaly.wool@konsulko.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Ira Weiny <ira.weiny@intel.com>, Nhat Pham <nphamcs@gmail.com>
Subject: Re: [PATCH] mm/zswap: Replace kmap_atomic() with kmap_local_page()
Date: Mon, 27 Nov 2023 12:16:56 -0800	[thread overview]
Message-ID: <CAF8kJuMakA-UbHi4Z5uCtB+6S0vcZiULKXu6GQh+7KBHQSR6Bg@mail.gmail.com> (raw)
In-Reply-To: <20231127160058.586446-1-fabio.maria.de.francesco@linux.intel.com>

Hi Fabio,

On Mon, Nov 27, 2023 at 8:01 AM Fabio M. De Francesco
<fabio.maria.de.francesco@linux.intel.com> wrote:
>
> kmap_atomic() has been deprecated in favor of kmap_local_page().
>
> Therefore, replace kmap_atomic() with kmap_local_page() in
> zswap.c.
>
> kmap_atomic() is implemented like a kmap_local_page() which also
> disables page-faults and preemption (the latter only in !PREEMPT_RT
> kernels). The kernel virtual addresses returned by these two API are
> only valid in the context of the callers (i.e., they cannot be handed to
> other threads).
>
> With kmap_local_page() the mappings are per thread and CPU local like
> in kmap_atomic(); however, they can handle page-faults and can be called
> from any context (including interrupts). The tasks that call
> kmap_local_page() can be preempted and, when they are scheduled to run
> again, the kernel virtual addresses are restored and are still valid.

As far as I can tell, the kmap_atomic() is the same as
kmap_local_page() with the following additional code before calling to
"__kmap_local_page_prot(page, prot)", which is common between these
two functions.

        if (IS_ENABLED(CONFIG_PREEMPT_RT))
                migrate_disable();
        else
                preempt_disable();

        pagefault_disable();

From the performance perspective, kmap_local_page() does less so it
has some performance gain. I am trying to think would it have another
unwanted side effect of enabling interrupt and page fault while zswap
decompressing a page.
The decompression should not generate page fault. The interrupt
enabling might introduce extra latency, but most of the page fault was
having interrupt enabled anyway. The time spent in decompression is
relatively small compared to the whole duration of the page fault. So
the interrupt enabling during those short windows should be fine.
"Should" is the famous last word.

I am tempted to Ack on it. Let me sleep on it a before more. BTW,
thanks for the patch.

Chris