linux-kernel - Re: [PATCH 0/2] mm: swap: mTHP swap allocator base on swap cluster order

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]

Date: Thu, 30 May 2024 10:52:21 +0800
From: "Huang, Ying" <ying.huang@...el.com>
To: Chris Li <chrisl@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>,  Kairui Song
 <kasong@...cent.com>,  Ryan Roberts <ryan.roberts@....com>,
  linux-kernel@...r.kernel.org,  linux-mm@...ck.org,  Barry Song
 <baohua@...nel.org>
Subject: Re: [PATCH 0/2] mm: swap: mTHP swap allocator base on swap cluster
 order

Chris Li <chrisl@...nel.org> writes:

> Hi Ying,
>
> On Wed, May 29, 2024 at 1:57 AM Huang, Ying <ying.huang@...el.com> wrote:
>>
>> Chris Li <chrisl@...nel.org> writes:
>>
>> > I am spinning a new version for this series to address two issues
>> > found in this series:
>> >
>> > 1) Oppo discovered a bug in the following line:
>> > +               ci = si->cluster_info + tmp;
>> > Should be "tmp / SWAPFILE_CLUSTER" instead of "tmp".
>> > That is a serious bug but trivial to fix.
>> >
>> > 2) order 0 allocation currently blindly scans swap_map disregarding
>> > the cluster->order.
>>
>> IIUC, now, we only scan swap_map[] only if
>> !list_empty(&si->free_clusters) && !list_empty(&si->nonfull_clusters[order]).
>> That is, if you doesn't run low swap free space, you will not do that.
>
> You can still swap space in order 0 clusters while order 4 runs out of
> free_cluster
> or nonfull_clusters[order]. For Android that is a common case.

When we fail to allocate order 4, we will fallback to order 0.  Still
don't need to scan swap_map[].  But after looking at your below reply, I
realized that the swap space is almost full at most times in your cases.
Then, it's possible that we run into scanning swap_map[].
list_empty(&si->free_clusters) &&
list_empty(&si->nonfull_clusters[order]) will become true, if we put too
many clusters in si->percpu_cluster.  So, if we want to avoid to scan
swap_map[], we can stop add clusters in si->percpu_cluster when swap
space runs low.  And maybe take clusters out of si->percpu_cluster
sometimes.

Another issue is nonfull_cluster[order1] cannot be used for
nonfull_cluster[order2].  In definition, we should not fail order 0
allocation, we need to steal nonfull_cluster[order>0] for order 0
allocation.  This can avoid to scan swap_map[] too.  This may be not
perfect, but it is the simplest first step implementation.  You can
optimize based on it further.

And, I checked your code again.  It appears that si->percpu_cluster may
be put in si->nonfull_cluster[], then be used by another CPU.  Please
check it.

--
Best Regards,
Huang, Ying

[snip]