linux-kernel - Re: [PATCH 2/4] mm/shmem, swap: avoid redundant Xarray lookup during swapin

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9314d711-6bbd-4b6b-b95b-64fb6c7dae6a@arm.com>
Date: Wed, 18 Jun 2025 12:59:36 +0530
From: Dev Jain <dev.jain@....com>
To: Kairui Song <ryncsn@...il.com>
Cc: linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
 Hugh Dickins <hughd@...gle.com>, Baolin Wang
 <baolin.wang@...ux.alibaba.com>, Matthew Wilcox <willy@...radead.org>,
 Kemeng Shi <shikemeng@...weicloud.com>, Chris Li <chrisl@...nel.org>,
 Nhat Pham <nphamcs@...il.com>, Baoquan He <bhe@...hat.com>,
 Barry Song <baohua@...nel.org>, linux-kernel@...r.kernel.org,
 Ryan Roberts <ryan.roberts@....com>
Subject: Re: [PATCH 2/4] mm/shmem, swap: avoid redundant Xarray lookup during
 swapin


On 18/06/25 12:52 pm, Kairui Song wrote:
> On Wed, Jun 18, 2025 at 3:17 PM Dev Jain <dev.jain@....com> wrote:
>>
>> On 18/06/25 12:05 am, Kairui Song wrote:
>>> From: Kairui Song <kasong@...cent.com>
>>>
>>> Currently shmem calls xa_get_order to get the swap radix entry order,
>>> requiring a full tree walk. This can be easily combined with the swap
>>> entry value checking (shmem_confirm_swap) to avoid the duplicated
>>> lookup, which should improve the performance.
>> Nice spot!
>>
>>> Signed-off-by: Kairui Song <kasong@...cent.com>
>>> ---
>>>    mm/shmem.c | 33 ++++++++++++++++++++++++---------
>>>    1 file changed, 24 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>> index 4e7ef343a29b..0ad49e57f736 100644
>>> --- a/mm/shmem.c
>>> +++ b/mm/shmem.c
>>> @@ -505,15 +505,27 @@ static int shmem_replace_entry(struct address_space *mapping,
>>>
>>>    /*
>>>     * Sometimes, before we decide whether to proceed or to fail, we must check
>>> - * that an entry was not already brought back from swap by a racing thread.
>>> + * that an entry was not already brought back or split by a racing thread.
>>>     *
>>>     * Checking folio is not enough: by the time a swapcache folio is locked, it
>>>     * might be reused, and again be swapcache, using the same swap as before.
>>> + * Returns the swap entry's order if it still presents, else returns -1.
>>>     */
>>> -static bool shmem_confirm_swap(struct address_space *mapping,
>>> -                            pgoff_t index, swp_entry_t swap)
>>> +static int shmem_swap_check_entry(struct address_space *mapping, pgoff_t index,
>>> +                               swp_entry_t swap)
>> I think the function name shmem_confirm_swap is already good enough? Anyhow the
>> changed name should at least be shmem_check_entry_is_swap.
>>
> Good, I can keep the function name unchanged or follow your
> suggestion, I thought a `confirm` function returning non-binary return

True. I will vote for keeping the name unchanged; you have already documented
the return value so it should be fine. Just can you put a new line between
"Returns the swap entry's order..." and the previous line to make it clear.

> value may look strange. I'm terrible at naming things :P