On Fri, Aug 16, 2024 at 3:37 PM Chris Li <chrisl@xxxxxxxxxx> wrote: > > On Thu, Aug 8, 2024 at 1:40 AM Huang, Ying <ying.huang@xxxxxxxxx> wrote: > > > > Kairui Song <ryncsn@xxxxxxxxx> 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.
