Since ci->lock has been taken when isolating cluster from
si->free_clusters or taking si->percpu_cluster->next[order],
it's unnecessary to scan and check the cluster range availability
if i'ts empty cluster, and this can accelerate the huge page
swapping.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/swapfile.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 9c9a4ec6d4c6..61efde853eea 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -729,6 +729,9 @@ static bool cluster_scan_range(struct swap_info_struct *si,
 	unsigned long offset, end = start + nr_pages;
 	unsigned char *map = si->swap_map;
 
+	if (cluster_is_empty(ci))
+		return true;
+
 	for (offset = start; offset < end; offset++) {
 		switch (READ_ONCE(map[offset])) {
 		case 0:
-- 
2.41.0

On Wed, Feb 5, 2025 at 5:27 PM Baoquan He <bhe@redhat.com> wrote:
>
> Since ci->lock has been taken when isolating cluster from
> si->free_clusters or taking si->percpu_cluster->next[order],
> it's unnecessary to scan and check the cluster range availability
> if i'ts empty cluster, and this can accelerate the huge page
> swapping.
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  mm/swapfile.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 9c9a4ec6d4c6..61efde853eea 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -729,6 +729,9 @@ static bool cluster_scan_range(struct swap_info_struct *si,
>         unsigned long offset, end = start + nr_pages;
>         unsigned char *map = si->swap_map;
>
> +       if (cluster_is_empty(ci))
> +               return true;
> +

Hi Baoquan,

Thanks for the series.

Most commits are looking great, but this one is a bit questionable.
cluster_scan_range is only called by alloc_swap_scan_cluster, and it
already checks if the cluster has enough empty slots to use, so this
might be redundant.

It is possible that cluster_scan_range sees an empty cluster if the
cluster lock was dropped for reclaiming HAS_CACHE, but the chance
should be extremely low, that this might be a negative optimization.

On 02/06/25 at 01:07am, Kairui Song wrote:
> On Wed, Feb 5, 2025 at 5:27 PM Baoquan He <bhe@redhat.com> wrote:
> >
> > Since ci->lock has been taken when isolating cluster from
> > si->free_clusters or taking si->percpu_cluster->next[order],
> > it's unnecessary to scan and check the cluster range availability
> > if i'ts empty cluster, and this can accelerate the huge page
> > swapping.
> >
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  mm/swapfile.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 9c9a4ec6d4c6..61efde853eea 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -729,6 +729,9 @@ static bool cluster_scan_range(struct swap_info_struct *si,
> >         unsigned long offset, end = start + nr_pages;
> >         unsigned char *map = si->swap_map;
> >
> > +       if (cluster_is_empty(ci))
> > +               return true;
> > +
> 
> Hi Baoquan,
> 
> Thanks for the series.

Thanks for your reviewing.

> 
> Most commits are looking great, but this one is a bit questionable.
> cluster_scan_range is only called by alloc_swap_scan_cluster, and it
> already checks if the cluster has enough empty slots to use, so this
> might be redundant.

Hmm, maybe no. Assume we want to allocate 2M space on system with 4K
page size. Even if a empty cluster is taken into consideration,
cluster_scan_range() will loop 512 times to check if each slot is
available. That for sure is not necessary in the case, while the added
empty cluster checking is very cheap.

> 
> It is possible that cluster_scan_range sees an empty cluster if the
> cluster lock was dropped for reclaiming HAS_CACHE, but the chance
> should be extremely low, that this might be a negative optimization.

It may be not like that. If it's empty cluster, the added checking will
return directly. Then 'need_reclaim' is kept false, there's no chance to
drop cluster lock to do reclaiming for HAS_CACHE. Means for empty
cluster scanning, the ci->lock is kept held. Not sure if I missed
anything.

Thanks
Baoquan

On Thu, Feb 6, 2025 at 9:41 AM Baoquan He <bhe@redhat.com> wrote:
>
> On 02/06/25 at 01:07am, Kairui Song wrote:
> > On Wed, Feb 5, 2025 at 5:27 PM Baoquan He <bhe@redhat.com> wrote:
> > >
> > > Since ci->lock has been taken when isolating cluster from
> > > si->free_clusters or taking si->percpu_cluster->next[order],
> > > it's unnecessary to scan and check the cluster range availability
> > > if i'ts empty cluster, and this can accelerate the huge page
> > > swapping.
> > >
> > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > > ---
> > >  mm/swapfile.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > > index 9c9a4ec6d4c6..61efde853eea 100644
> > > --- a/mm/swapfile.c
> > > +++ b/mm/swapfile.c
> > > @@ -729,6 +729,9 @@ static bool cluster_scan_range(struct swap_info_struct *si,
> > >         unsigned long offset, end = start + nr_pages;
> > >         unsigned char *map = si->swap_map;
> > >
> > > +       if (cluster_is_empty(ci))
> > > +               return true;
> > > +
> >
> > Hi Baoquan,
> >
> > Thanks for the series.
>
> Thanks for your reviewing.
>
> >
> > Most commits are looking great, but this one is a bit questionable.
> > cluster_scan_range is only called by alloc_swap_scan_cluster, and it
> > already checks if the cluster has enough empty slots to use, so this
> > might be redundant.
>
> Hmm, maybe no. Assume we want to allocate 2M space on system with 4K
> page size. Even if a empty cluster is taken into consideration,
> cluster_scan_range() will loop 512 times to check if each slot is
> available. That for sure is not necessary in the case, while the added
> empty cluster checking is very cheap.
>
> >
> > It is possible that cluster_scan_range sees an empty cluster if the
> > cluster lock was dropped for reclaiming HAS_CACHE, but the chance
> > should be extremely low, that this might be a negative optimization.
>
> It may be not like that. If it's empty cluster, the added checking will
> return directly. Then 'need_reclaim' is kept false, there's no chance to
> drop cluster lock to do reclaiming for HAS_CACHE. Means for empty
> cluster scanning, the ci->lock is kept held. Not sure if I missed
> anything.

Ah, right, sorry, I just understood your code wrongly. This makes
sense to me now.

>
> Thanks
> Baoquan
>