linux-kernel - Re: [PATCH v5 0/9] 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]

Message-ID: <CAMgjq7Aztur8WQ=rMWiLBhgRU5_6TGXnLQQ7-RM73AK5jJxEzw@mail.gmail.com>
Date: Sun, 18 Aug 2024 01:47:46 +0800
From: Kairui Song <ryncsn@...il.com>
To: Chris Li <chrisl@...nel.org>
Cc: "Huang, Ying" <ying.huang@...el.com>, David Hildenbrand <david@...hat.com>, 
	Andrew Morton <akpm@...ux-foundation.org>, Hugh Dickins <hughd@...gle.com>, 
	Ryan Roberts <ryan.roberts@....com>, Kalesh Singh <kaleshsingh@...gle.com>, 
	linux-kernel@...r.kernel.org, linux-mm@...ck.org, 
	Barry Song <baohua@...nel.org>
Subject: Re: [PATCH v5 0/9] mm: swap: mTHP swap allocator base on swap cluster order

On Fri, Aug 16, 2024 at 3:37 PM Chris Li <chrisl@...nel.org> wrote:
>
> On Thu, Aug 8, 2024 at 1:40 AM Huang, Ying <ying.huang@...el.com> wrote:
> >
> > Kairui Song <ryncsn@...il.com> writes:
> >
> > [snip]
> >
> > > --- a/mm/swapfile.c
> > > +++ b/mm/swapfile.c
> > > @@ -450,7 +450,10 @@ static void __free_cluster(struct swap_info_struct *si, struct swap_cluster_info
> > >       lockdep_assert_held(&si->lock);
> > >       lockdep_assert_held(&ci->lock);
> > >
> > > -     list_move_tail(&ci->list, &si->free_clusters);
> > > +     if (ci->flags)
> > > +             list_move_tail(&ci->list, &si->free_clusters);
> > > +     else
> > > +             list_add_tail(&ci->list, &si->free_clusters);
> >
> > If we use list_del_init() to delete the cluster, we can always use
> > list_move_tail()?  If so, the logic can be simplified.
>
> Thanks for the suggestion.

Hi All, thanks for the review and discussion.

> I feel that list_del_init() generates more instruction than necessary.
> It is my bad that I leave the discard list without not a list flag bit
> for it.

Right, list_del_init is a little bit more noisy than list_del indeed.

But considering after this patch, all non-discard clusters are always
a on list (free/nonfull/full) already, and a cluster will be dangling
only when being removed from the discard list (when doing discard
work, it need to unlock si->lock, so the cluster have to be hidden
from other racers).

I think it's good to use list_del_init when deleting from the discard
list in this patch, then list_move_tail can always be used when
changing the list of a cluster.
Discard should be a much less common operation so this should
be OK.