Re: [PATCH v2 1/2] mm: swap: swap cluster switch to double link list

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

Chris Li <chrisl@kernel.org> writes:

> On Tue, Jun 18, 2024 at 12:56 AM Huang, Ying <ying.huang@intel.com> wrote:
>>
>> Chris Li <chrisl@kernel.org> writes:
>>
>> > On Sun, Jun 16, 2024 at 11:21 PM Huang, Ying <ying.huang@intel.com> wrote:
>> >>
>> >> Hi, Chris,
>> >>
>> >> Chris Li <chrisl@kernel.org> writes:

[snip]

>> >> > diff --git a/include/linux/swap.h b/include/linux/swap.h
>> >> > index 3df75d62a835..cd9154a3e934 100644
>> >> > --- a/include/linux/swap.h
>> >> > +++ b/include/linux/swap.h
>> >> > @@ -242,23 +242,22 @@ enum {
>> >> >   * space with SWAPFILE_CLUSTER pages long and naturally aligns in disk. All
>> >> >   * free clusters are organized into a list. We fetch an entry from the list to
>> >> >   * get a free cluster.
>> >> > - *
>> >> > - * The data field stores next cluster if the cluster is free or cluster usage
>> >> > - * counter otherwise. The flags field determines if a cluster is free. This is
>> >> > - * protected by swap_info_struct.lock.
>> >> >   */
>> >> >  struct swap_cluster_info {
>> >> >       spinlock_t lock;        /*
>> >> > -                              * Protect swap_cluster_info fields
>> >> > -                              * and swap_info_struct->swap_map
>> >> > +                              * Protect swap_cluster_info count and state
>> >>
>> >> Protect swap_cluster_info fields except 'list' ?
>> >
>> > I change it to protect the swap_cluster_info bitfields in the second patch.
>>
>> Although I still prefer my version, I will not insist on that.
>
> Sure, I actually don't have a strong preference about that. It is just comments.
>
>>
>> >>
>> >> > +                              * field and swap_info_struct->swap_map
>> >> >                                * elements correspond to the swap
>> >> >                                * cluster
>> >> >                                */
>> >> > -     unsigned int data:24;
>> >> > -     unsigned int flags:8;
>> >> > +     unsigned int count:12;
>> >> > +     unsigned int state:3;
>> >>
>> >> I still prefer normal data type over bit fields.  How about
>> >>
>> >>         u16 usage;
>> >>         u8  state;
>> >
>> > I don't mind the "count" rename to "usage". That is probably a better
>> > name. However I have another patch intended to add more bit fields in
>> > the cluster info struct. The second patch adds "order" and the later
>> > patch will add more. That is why I choose bitfield to be more condense
>> > with bits.
>>
>> We still have space for another "u8" for "order".  It appears trivial to
>> change it to bit fields when necessary in the future.
>
> We can, I don't see it necessary to change from bit field to u8 and
> back to bit field in the future. It is more of a personal preference
> issue.

I have to say that I don't think that it's just a personal preference.
IMO, if it's unnecessary, we shouldn't use bit fields.  You cannot
guarantee that your future changes will be merged in its current state.
So, I still think that it's better to avoid bit fields for now.

>> >>
>> >> And, how about use 'usage' instead of 'count'?  Personally I think that
>> >> it is more clear.  But I don't have strong opinions on this.
>> >>
>> >> > +     struct list_head list;  /* Protected by swap_info_struct->lock */
>> >> >  };
>> >> > -#define CLUSTER_FLAG_FREE 1 /* This cluster is free */
>> >> > -#define CLUSTER_FLAG_NEXT_NULL 2 /* This cluster has no next cluster */
>> >> > +
>> >> > +#define CLUSTER_STATE_FREE   1 /* This cluster is free */
>> >>

[snip]

>> >> >  /*
>> >> > @@ -481,21 +371,22 @@ static void __free_cluster(struct swap_info_struct *si, unsigned long idx)
>> >> >  */
>> >> >  static void swap_do_scheduled_discard(struct swap_info_struct *si)
>> >> >  {
>> >> > -     struct swap_cluster_info *info, *ci;
>> >> > +     struct swap_cluster_info *ci;
>> >> >       unsigned int idx;
>> >> >
>> >> > -     info = si->cluster_info;
>> >> > -
>> >> > -     while (!cluster_list_empty(&si->discard_clusters)) {
>> >> > -             idx = cluster_list_del_first(&si->discard_clusters, info);
>> >> > +     while (!list_empty(&si->discard_clusters)) {
>> >> > +             ci = list_first_entry(&si->discard_clusters, struct swap_cluster_info, list);
>> >> > +             list_del(&ci->list);
>> >> > +             idx = ci - si->cluster_info;
>> >> >               spin_unlock(&si->lock);
>> >> >
>> >> >               discard_swap_cluster(si, idx * SWAPFILE_CLUSTER,
>> >> >                               SWAPFILE_CLUSTER);
>> >> >
>> >> >               spin_lock(&si->lock);
>> >> > -             ci = lock_cluster(si, idx * SWAPFILE_CLUSTER);
>> >> > -             __free_cluster(si, idx);
>> >> > +
>> >> > +             spin_lock(&ci->lock);
>> >>
>> >> Personally, I still prefer to use lock_cluster(), which is more readable
>> >> and matches unlock_cluster() below.
>> >
>> > lock_cluster() uses an index which is not matching unlock_cluster()
>> > which is using a pointer to cluster.
>>
>> lock_cluster()/unlock_cluster() are pair and fit original design
>> well.  They use different parameter because swap cluster is optional.
>>
>> > When you get the cluster from the list, you have a cluster pointer. I
>> > feel it is unnecessary to convert to index then back convert to
>> > cluster pointer inside lock_cluster(). I actually feel using indexes
>> > to refer to the cluster is error prone because we also have offset.
>>
>> I don't think so, it's common to use swap offset.
>
> Swap offset is not an issue, it is all over the place. The cluster
> index(offset/512) is the one I try to avoid.
> I have made some mistakes myself on offset vs index.

Yes.  That's not good, but it's hard to be avoided too.  Can we make the
variable name more consistent?  index: cluster index, offset: swap
offset.

And, in fact, swap offset is the parameter of lock_cluster() instead of
cluster index.

>> >
>> >>
>> >> > +             __free_cluster(si, ci);
>> >> >               memset(si->swap_map + idx * SWAPFILE_CLUSTER,
>> >> >                               0, SWAPFILE_CLUSTER);
>> >> >               unlock_cluster(ci);
>> >> > @@ -521,20 +412,19 @@ static void swap_users_ref_free(struct percpu_ref *ref)
>> >> >       complete(&si->comp);
>> >> >  }
>> >> >

[snip]

>> >> > @@ -611,10 +497,10 @@ scan_swap_map_ssd_cluster_conflict(struct swap_info_struct *si,
>> >> >  {
>> >> >       struct percpu_cluster *percpu_cluster;
>> >> >       bool conflict;
>> >> > -
>> >>
>> >> Usually we use one blank line after local variable declaration.
>> > Ack.
>> >
>> >>
>> >> > +     struct swap_cluster_info *first = list_first_entry(&si->free_clusters, struct swap_cluster_info, list);
>> >> >       offset /= SWAPFILE_CLUSTER;
>> >> > -     conflict = !cluster_list_empty(&si->free_clusters) &&
>> >> > -             offset != cluster_list_first(&si->free_clusters) &&
>> >> > +     conflict = !list_empty(&si->free_clusters) &&
>> >> > +             offset !=  first - si->cluster_info &&
>> >> >               cluster_is_free(&si->cluster_info[offset]);
>> >> >
>> >> >       if (!conflict)
>> >> > @@ -655,10 +541,14 @@ static bool scan_swap_map_try_ssd_cluster(struct swap_info_struct *si,
>> >> >       cluster = this_cpu_ptr(si->percpu_cluster);
>> >> >       tmp = cluster->next[order];
>> >> >       if (tmp == SWAP_NEXT_INVALID) {
>> >> > -             if (!cluster_list_empty(&si->free_clusters)) {
>> >> > -                     tmp = cluster_next(&si->free_clusters.head) *
>> >> > -                                     SWAPFILE_CLUSTER;
>> >> > -             } else if (!cluster_list_empty(&si->discard_clusters)) {
>> >> > +             if (!list_empty(&si->free_clusters)) {
>> >> > +                     ci = list_first_entry(&si->free_clusters, struct swap_cluster_info, list);
>> >> > +                     list_del(&ci->list);
>> >>
>> >> The free cluster is deleted from si->free_clusters now.  But later you
>> >> will call scan_swap_map_ssd_cluster_conflict() and may abandon the
>> >> cluster.  And in alloc_cluster() later, it may be deleted again.
>> >
>> > Yes, that is a bug. Thanks for catching that.
>> >
>> >>
>> >> > +                     spin_lock(&ci->lock);
>> >> > +                     ci->state = CLUSTER_STATE_PER_CPU;
>> >>
>> >> Need to change ci->state when move a cluster off the percpu_cluster.
>> >
>> > In the next patch. This patch does not use the off state yet.
>>
>> But that is confusing to use wrong state name, the really meaning is
>> something like CLUSTER_STATE_NON_FREE.  But as I suggested above, we can
>
> It can be FREE and on the per cpu pointer as well. That is the tricky part.
> It can happen on the current code as well.

cluster_set_count_flag(0, 0) is called in alloc_cluster().  So, it's not
an issue in current code.  If you need more, that shouldn't be done in
this patch.

>> keep swap_cluster_info.flags and CLUSTER_FLAG_FREE in this patch.
>
> Maybe. Will consider that.
>
>>
>> >>
>> >> > +                     spin_unlock(&ci->lock);
>> >> > +                     tmp = (ci - si->cluster_info) * SWAPFILE_CLUSTER;
>> >> > +             } else if (!list_empty(&si->discard_clusters)) {
>> >> >                       /*
>> >> >                        * we don't have free cluster but have some clusters in
>> >> >                        * discarding, do discard now and reclaim them, then
>> >> > @@ -1062,8 +952,8 @@ 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);
>> >> > -     cluster_set_count_flag(ci, 0, 0);
>> >> > -     free_cluster(si, idx);
>> >> > +     ci->count = 0;
>> >> > +     free_cluster(si, ci);
>> >> >       unlock_cluster(ci);
>> >> >       swap_range_free(si, offset, SWAPFILE_CLUSTER);
>> >> >  }

[snip]

--
Best Regards,
Huang, Ying