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

["Huang, Ying" <ying.huang@intel.com>]

Chris Li <chrisl@kernel.org> writes:

> On Wed, Aug 7, 2024 at 6:14 PM Huang, Ying <ying.huang@intel.com> wrote:
>>
>> Chris Li <chrisl@kernel.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