linux-kernel - Re: [PATCH v5 2/9] mm: swap: mTHP allocate swap entries from nonfull list

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: <871q2lhr4s.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date: Mon, 19 Aug 2024 16:08:03 +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>,  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 2/9] mm: swap: mTHP allocate swap entries from
 nonfull list

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

> On Wed, Aug 7, 2024 at 6:14 PM Huang, Ying <ying.huang@...el.com> wrote:
>>
>> Chris Li <chrisl@...nel.org> writes:
>>

[snip]

>> >
>> >  /*
>> > @@ -553,6 +568,19 @@ static bool scan_swap_map_try_ssd_cluster(struct swap_info_struct *si,
>> >       if (tmp == SWAP_NEXT_INVALID) {
>> >               if (!list_empty(&si->free_clusters)) {
>> >                       ci = list_first_entry(&si->free_clusters, struct swap_cluster_info, list);
>> > +                     list_del(&ci->list);
>> > +                     spin_lock(&ci->lock);
>> > +                     ci->order = order;
>> > +                     ci->flags = 0;
>> > +                     spin_unlock(&ci->lock);
>> > +                     tmp = cluster_index(si, ci) * SWAPFILE_CLUSTER;
>> > +             } else if (!list_empty(&si->nonfull_clusters[order])) {
>> > +                     ci = list_first_entry(&si->nonfull_clusters[order],
>> > +                                           struct swap_cluster_info, list);
>> > +                     list_del(&ci->list);
>> > +                     spin_lock(&ci->lock);
>> > +                     ci->flags = 0;
>> > +                     spin_unlock(&ci->lock);
>> >                       tmp = cluster_index(si, ci) * SWAPFILE_CLUSTER;
>> >               } else if (!list_empty(&si->discard_clusters)) {
>>
>> We should check discard_clusters before nonfull clusters.
>
> And the reason behind that is?
>
> I see the discard_cluster can take a long time. It will take a
> synchronous wait for the issuing the discard command. Why not just use
> the  nonfull list and return immediately. When the discard command
> finished. It will show up in the free list anyway.

I think that you are right.  We don't need to wait for discard here.

> BTW, what is your take on my  previous analysis of the current SSD
> prefer write new cluster can wear out the SSD faster?

No.  I don't agree with you on that.  However, my knowledge on SSD
wearing out algorithm is quite limited.

> I think it might be useful to provide users an option to choose to
> write a non full list first. The trade off is more friendly to SSD
> wear out than preferring to write new blocks. If you keep doing the
> swap long enough, there will be no new free cluster anyway.

It depends on workloads.  Some workloads may demonstrate better spatial
locality.

> The example I give in this email:
>
> https://lore.kernel.org/linux-mm/CACePvbXGBNC9WzzL4s2uB2UciOkV6nb4bKKkc5TBZP6QuHS_aQ@mail.gmail.com/
>
> Chris
>>
>> >                       /*
>> > @@ -967,6 +995,7 @@ static void swap_free_cluster(struct swap_info_struct *si, unsigned long idx)
>> >       ci = lock_cluster(si, offset);
>> >       memset(si->swap_map + offset, 0, SWAPFILE_CLUSTER);
>> >       ci->count = 0;
>> > +     ci->order = 0;
>> >       ci->flags = 0;
>> >       free_cluster(si, ci);
>> >       unlock_cluster(ci);
>> > @@ -2922,6 +2951,9 @@ static int setup_swap_map_and_extents(struct swap_info_struct *p,
>> >       INIT_LIST_HEAD(&p->free_clusters);
>> >       INIT_LIST_HEAD(&p->discard_clusters);
>> >
>> > +     for (i = 0; i < SWAP_NR_ORDERS; i++)
>> > +             INIT_LIST_HEAD(&p->nonfull_clusters[i]);
>> > +
>> >       for (i = 0; i < swap_header->info.nr_badpages; i++) {
>> >               unsigned int page_nr = swap_header->info.badpages[i];
>> >               if (page_nr == 0 || page_nr > swap_header->info.last_page)

--
Best Regards,
Huang, Ying