On Thu, Feb 6, 2025 at 9:41 AM Baoquan He <bhe@xxxxxxxxxx> wrote: > > 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. Ah, right, sorry, I just understood your code wrongly. This makes sense to me now. > > Thanks > Baoquan >
