Received-SPF: pass (google.com: domain of linux-kernel+bounces-213919-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240612124750.2220726-1-usamaarif642@gmail.com>
 <20240612124750.2220726-2-usamaarif642@gmail.com> <ZmoBf6RPJzC2RaqM@google.com>
 <85804484-9973-41a1-a05d-000833285f39@gmail.com> <CAJD7tkYBxN4uAHLacAx=m2+B9zPidz0V5pGP030yvNYLTnk=VQ@mail.gmail.com>
 <0572d8b1-3b17-45a8-bf75-f66e19216d38@gmail.com>
In-Reply-To: <0572d8b1-3b17-45a8-bf75-f66e19216d38@gmail.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Thu, 13 Jun 2024 12:26:42 -0700
Message-ID: <CAJD7tkYGFsYbbbHp3+MMHTuxNcG_Z+i-5TCo3wieVArcra5wmA@mail.gmail.com>
Subject: Re: [PATCH v4 1/2] mm: store zero pages to be swapped out in a bitmap
To: Usama Arif <usamaarif642@gmail.com>
Cc: akpm@linux-foundation.org, hannes@cmpxchg.org, shakeel.butt@linux.dev, 
	david@redhat.com, ying.huang@intel.com, hughd@google.com, willy@infradead.org, 
	nphamcs@gmail.com, chengming.zhou@linux.dev, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org, kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"

[..]
> >>>> @@ -498,6 +507,8 @@ static void swap_do_scheduled_discard(struct swap_info_struct *si)
> >>>>               __free_cluster(si, idx);
> >>>>               memset(si->swap_map + idx * SWAPFILE_CLUSTER,
> >>>>                               0, SWAPFILE_CLUSTER);
> >>>> +            for (i = 0; i < SWAPFILE_CLUSTER; i++)
> >>>> +                    clear_bit(idx * SWAPFILE_CLUSTER + i, si->zeromap);
> >>> Same here. I didn't look into the specific code paths, but shouldn't the
> >>> cluster be unused (and hence its zeromap bits already cleared?).
> >>>
> >> I think this one is needed (or atleast very good to have). There are 2
> >> paths:
> >>
> >> 1) swap_cluster_schedule_discard (clears zeromap) -> swap_discard_work
> >> -> swap_do_scheduled_discard (clears zeromap)
> >>
> >> Path 1 doesnt need it as swap_cluster_schedule_discard already clears it.
> >>
> >> 2) scan_swap_map_slots -> scan_swap_map_try_ssd_cluster ->
> >> swap_do_scheduled_discard (clears zeromap)
> >>
> >> Path 2 might need it as zeromap isnt cleared earlier I believe
> >> (eventhough I think it might already be 0).
> > Aren't the clusters in the discard list free by definition? It seems
> > like we add a cluster there from swap_cluster_schedule_discard(),
> > which we establish above that it gets called on a free cluster, right?
>
> You mean for path 2? Its not from swap_cluster_schedule_discard. The
> whole call path is
>
> get_swap_pages -> scan_swap_map_slots -> scan_swap_map_try_ssd_cluster
> -> swap_do_scheduled_discard. Nowhere up until swap_do_scheduled_discard
> was the zeromap cleared, which is why I think we should add it here.

swap_do_scheduled_discard() iterates over clusters from
si->discard_clusters. Clusters are added to that list from
swap_cluster_schedule_discard().

IOW, swap_cluster_schedule_discard() schedules freed clusters to be
discarded, and swap_do_scheduled_discard() later does the actual
discarding, whether it's through si->discard_work scheduled by
swap_cluster_schedule_discard(), or when looking for a free cluster
through scan_swap_map_try_ssd_cluster().

Did I miss anything?