[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5fa01666-51d4-4ccb-bcd4-3b3620dd2e2b@linux.dev>
Date: Sat, 12 Oct 2024 11:33:14 +0800
From: Chengming Zhou <chengming.zhou@...ux.dev>
To: Kairui Song <ryncsn@...il.com>, Johannes Weiner <hannes@...xchg.org>
Cc: Yosry Ahmed <yosryahmed@...gle.com>, linux-mm@...ck.org,
Andrew Morton <akpm@...ux-foundation.org>, Nhat Pham <nphamcs@...il.com>,
Chris Li <chrisl@...nel.org>, Barry Song <v-songbaohua@...o.com>,
"Huang, Ying" <ying.huang@...el.com>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm/zswap: avoid touching XArray for unnecessary
invalidation
On 2024/10/12 11:04, Kairui Song wrote:
> Johannes Weiner <hannes@...xchg.org> 于 2024年10月12日周六 02:28写道:
>>
>> On Fri, Oct 11, 2024 at 10:53:31AM -0700, Yosry Ahmed wrote:
>>> On Fri, Oct 11, 2024 at 10:20 AM Kairui Song <ryncsn@...il.com> wrote:
>>>>
>>>> From: Kairui Song <kasong@...cent.com>
>>>>
>>>> zswap_invalidation simply calls xa_erase, which acquires the Xarray
>>>> lock first, then does a look up. This has a higher overhead even if
>>>> zswap is not used or the tree is empty.
>>>>
>>>> So instead, do a very lightweight xa_empty check first, if there is
>>>> nothing to erase, don't touch the lock or the tree.
>>
>> Great idea!
>>
>>> XA_STATE(xas, ..);
>>>
>>> rcu_read_lock();
>>> entry = xas_load(&xas);
>>> if (entry) {
>>> xas_lock(&xas);
>>> WARN_ON_ONCE(xas_reload(&xas) != entry);
>>> xas_store(&xas, NULL);
>>> xas_unlock(&xas);
>>> }
>>> rcu_read_unlock():
>>
>> This does the optimization more reliably, and I think we should go
>> with this version.
>
> Hi Yosry and Johannes,
>
> This is a good idea. But xa_empty is just much lighweighter, it's just
> a inlined ( == NULL ) check, so unsurprising it has better performance
> than xas_load.
>
> And surprisingly it's faster than zswap_never_enabled. So I think it
Do you have CONFIG_ZSWAP_DEFAULT_ON enabled? In your case, CPU will go
to the unlikely branch of static_key every time, which maybe the cause.
> could be doable to introduce something like zswap_may_have_swpentry as
> Yosry suggested.
>
> So how about a combined version with xas_load and xa_empty? Check
> xa_empty first as a faster path, then xas_load, then xas_store.
Yeah, I also think this combined version is better.
Thanks.
>
> Here is the benchmark result (time of swapin 2G zero pages in us):
>
> Before: 1908944 1905870 1905322 1905627 1901667
> xa_empty: 1835343 1827367 1828402 1831841 1832719
> z.._enabled: 1838428 1831162 1838205 1837287 1840980
> xas_load: 1874606 1878971 1870182 1875852 1873403
> combined: 1845309 1832919 1831904 1836455 1842570
>
> `combined` is xa_empty + xas_load.
Powered by blists - more mailing lists