On 02/06/25 at 01:07am, Kairui Song wrote: > On Wed, Feb 5, 2025 at 5:27 PM Baoquan He <bhe@xxxxxxxxxx> 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@xxxxxxxxxx> > > --- > > 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
