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: <CACePvbX1EWQk03YcC47s7+vn40kEFb_3wp3D_GmJV-8Fn2j+=Q@mail.gmail.com>
Date: Fri, 16 Aug 2024 00:36:50 -0700
From: Chris Li <chrisl@...nel.org>
To: "Huang, Ying" <ying.huang@...el.com>
Cc: Kairui Song <ryncsn@...il.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 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.

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.

I do want to clean this up. While we are at it, because the cluster
can only belong to one list at a time. We can use a list indicator as
integer rather than bits mask. If we give the discard list the same
treatment, that should remove the special condition to add a cluster
to another list as well.

Chris

> >       ci->flags = CLUSTER_FLAG_FREE;
> >       ci->order = 0;
> >  }
> > @@ -474,7 +477,6 @@ static void swap_do_scheduled_discard(struct swap_info_struct *si)
> >                               SWAPFILE_CLUSTER);
> >
> >               spin_lock(&si->lock);
> > -
> >               spin_lock(&ci->lock);
> >               __free_cluster(si, ci);
> >               memset(si->swap_map + idx * SWAPFILE_CLUSTER,
> > @@ -666,7 +668,7 @@ static void cluster_alloc_range(struct swap_info_struct *si, struct swap_cluster
> >               if (ci->flags & CLUSTER_FLAG_FRAG)
> >                       si->frag_cluster_nr[ci->order]--;
> >               list_move_tail(&ci->list, &si->full_clusters);
> > -             ci->flags = 0;
> > +             ci->flags = CLUSTER_FLAG_FULL;
> >       }
> >  }
>
> --
> Best Regards,
> Huang, Ying