linux-kernel - Re: [PATCH v4 1/2] mm: store zero pages to be swapped out in a bitmap

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: <8b155ba8-ff53-4202-b2eb-afe73db77d7e@gmail.com>
Date: Thu, 13 Jun 2024 20:38:20 +0100
From: Usama Arif <usamaarif642@...il.com>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: akpm@...ux-foundation.org, hannes@...xchg.org, shakeel.butt@...ux.dev,
 david@...hat.com, ying.huang@...el.com, hughd@...gle.com,
 willy@...radead.org, nphamcs@...il.com, chengming.zhou@...ux.dev,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org, kernel-team@...a.com
Subject: Re: [PATCH v4 1/2] mm: store zero pages to be swapped out in a bitmap


On 13/06/2024 20:26, Yosry Ahmed wrote:
> [..]
>>>>>> @@ -498,6 +507,8 @@ static void swap_do_scheduled_discard(struct swap_info_struct *si)
>>>>>>                __free_cluster(si, idx);
>>>>>>                memset(si->swap_map + idx * SWAPFILE_CLUSTER,
>>>>>>                                0, SWAPFILE_CLUSTER);
>>>>>> +            for (i = 0; i < SWAPFILE_CLUSTER; i++)
>>>>>> +                    clear_bit(idx * SWAPFILE_CLUSTER + i, si->zeromap);
>>>>> Same here. I didn't look into the specific code paths, but shouldn't the
>>>>> cluster be unused (and hence its zeromap bits already cleared?).
>>>>>
>>>> I think this one is needed (or atleast very good to have). There are 2
>>>> paths:
>>>>
>>>> 1) swap_cluster_schedule_discard (clears zeromap) -> swap_discard_work
>>>> -> swap_do_scheduled_discard (clears zeromap)
>>>>
>>>> Path 1 doesnt need it as swap_cluster_schedule_discard already clears it.
>>>>
>>>> 2) scan_swap_map_slots -> scan_swap_map_try_ssd_cluster ->
>>>> swap_do_scheduled_discard (clears zeromap)
>>>>
>>>> Path 2 might need it as zeromap isnt cleared earlier I believe
>>>> (eventhough I think it might already be 0).
>>> Aren't the clusters in the discard list free by definition? It seems
>>> like we add a cluster there from swap_cluster_schedule_discard(),
>>> which we establish above that it gets called on a free cluster, right?
>> You mean for path 2? Its not from swap_cluster_schedule_discard. The
>> whole call path is
>>
>> get_swap_pages -> scan_swap_map_slots -> scan_swap_map_try_ssd_cluster
>> -> swap_do_scheduled_discard. Nowhere up until swap_do_scheduled_discard
>> was the zeromap cleared, which is why I think we should add it here.
> swap_do_scheduled_discard() iterates over clusters from
> si->discard_clusters. Clusters are added to that list from
> swap_cluster_schedule_discard().
>
> IOW, swap_cluster_schedule_discard() schedules freed clusters to be
> discarded, and swap_do_scheduled_discard() later does the actual
> discarding, whether it's through si->discard_work scheduled by
> swap_cluster_schedule_discard(), or when looking for a free cluster
> through scan_swap_map_try_ssd_cluster().
>
> Did I miss anything?

Ah ok, and the schedule_discard in free_cluster wont be called scheduled 
before swap_range_free. Will only keep the one in swap_range_free. Thanks!