Message-ID: <87mswfuppr.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date:   Thu, 19 Oct 2023 13:49:04 +0800
From:   "Huang, Ying" <ying.huang@...el.com>
To:     Ryan Roberts <ryan.roberts@....com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>,
        Matthew Wilcox <willy@...radead.org>,
        Gao Xiang <xiang@...nel.org>, Yu Zhao <yuzhao@...gle.com>,
        Yang Shi <shy828301@...il.com>, Michal Hocko <mhocko@...e.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>,
        <linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>,
        Tim Chen <tim.c.chen@...ux.intel.com>
Subject: Re: [PATCH v2 2/2] mm: swap: Swap-out small-sized THP without
 splitting

Ryan Roberts <ryan.roberts@....com> writes:

> On 18/10/2023 07:55, Huang, Ying wrote:
>> Ryan Roberts <ryan.roberts@....com> writes:
>>

[snip]

>>> diff --git a/include/linux/swap.h b/include/linux/swap.h
>>> index a073366a227c..35cbbe6509a9 100644
>>> --- a/include/linux/swap.h
>>> +++ b/include/linux/swap.h
>>> @@ -268,6 +268,12 @@ struct swap_cluster_info {
>>>  struct percpu_cluster {
>>>  	struct swap_cluster_info index; /* Current cluster index */
>>>  	unsigned int next; /* Likely next allocation offset */
>>> +	unsigned int large_next[];	/*
>>> +					 * next free offset within current
>>> +					 * allocation cluster for large folios,
>>> +					 * or UINT_MAX if no current cluster.
>>> +					 * Index is (order - 1).
>>> +					 */
>>>  };
>>>
>>>  struct swap_cluster_list {
>>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>>> index b83ad77e04c0..625964e53c22 100644
>>> --- a/mm/swapfile.c
>>> +++ b/mm/swapfile.c
>>> @@ -987,35 +987,70 @@ static int scan_swap_map_slots(struct swap_info_struct *si,
>>>  	return n_ret;
>>>  }
>>>
>>> -static int swap_alloc_cluster(struct swap_info_struct *si, swp_entry_t *slot)
>>> +static int swap_alloc_large(struct swap_info_struct *si, swp_entry_t *slot,
>>> +			    unsigned int nr_pages)
>> 
>> This looks hacky.  IMO, we should put the allocation logic inside
>> percpu_cluster framework.  If percpu_cluster framework doesn't work for
>> you, just refactor it firstly.
>
> I'm not sure I really understand what you are suggesting - could you elaborate?
> What "framework"? I only see a per-cpu data structure and
> scan_swap_map_try_ssd_cluster(), which is very much geared towards order-0
> allocations.

I suggest to share as much code as possible between order-0 and order >
0 swap entry allocation.  I think that we can make
scan_swap_map_try_ssd_cluster() works for order > 0 swap entry allocation.

> Are you suggesting you want to allocate large entries (> order-0) from the same
> cluster that is used for small (order-0) entries? The problem with this approach
> is that there may not be enough space left in the current cluster for the large
> entry that you are trying to allocate. Then you would need to take a new cluster
> from the free list and presumably leave the previous cluster with unused entries
> (which will now only be accessible by scanning). That unused space could be
> considerable.
>
> That's why I am currently reserving a "current cluster" per order; that way, all
> allocations are the same order, they are all naturally aligned and there is no
> waste.

I am fine to use one swap cluster per order per CPU.  I just think that
we should share code.

> Perhaps I could implement "stealing" between cpus so that a cpu trying to
> allocate a specific order, but which doesn't have a current cluster for that
> order and the free list is empty, could allocate from another cpu's current
> cluster? I don't think it's a good idea to mix different orders in the same
> cluster though.

I think we can start from a simple solution, that is, just fall back to
split the large folio.  Then, we can optimize it step by step.

> I guess if really low, I could remove a current cluster from a cpu and allow it
> to be scanned for order-0 allocations at least?

Better to have same behavior between order- and order > 0.  Perhaps
begin with the current solution (allow swap entries in per-CPU cluster
to be stolen).  Then we can optimize based on this.

Not directly related to this patchset.  Maybe we can replace
swap_slots_cache with per-CPU cluster in the future.  This will reduce
the code complexity.

> Any opinions gratefully received!

Thanks!

>> 
>>>  {
>>> +	int order_idx;
>>>  	unsigned long idx;
>>>  	struct swap_cluster_info *ci;
>>> +	struct percpu_cluster *cluster;
>>>  	unsigned long offset;
>>>
>>>  	/*
>>>  	 * Should not even be attempting cluster allocations when huge
>>>  	 * page swap is disabled.  Warn and fail the allocation.
>>>  	 */
>>> -	if (!IS_ENABLED(CONFIG_THP_SWAP)) {
>>> +	if (!IS_ENABLED(CONFIG_THP_SWAP) ||
>>> +	    nr_pages < 4 || nr_pages > SWAPFILE_CLUSTER ||
>>> +	    !is_power_of_2(nr_pages)) {
>>>  		VM_WARN_ON_ONCE(1);
>>>  		return 0;
>>>  	}
>>>
>>> -	if (cluster_list_empty(&si->free_clusters))
>>> +	/*
>>> +	 * Not using clusters so unable to allocate large entries.
>>> +	 */
>>> +	if (!si->cluster_info)
>>>  		return 0;
>>>
>>> -	idx = cluster_list_first(&si->free_clusters);
>>> -	offset = idx * SWAPFILE_CLUSTER;
>>> -	ci = lock_cluster(si, offset);
>>> -	alloc_cluster(si, idx);
>>> -	cluster_set_count(ci, SWAPFILE_CLUSTER);
>>> +	order_idx = ilog2(nr_pages) - 2;
>>> +	cluster = this_cpu_ptr(si->percpu_cluster);
>>> +	offset = cluster->large_next[order_idx];
>>> +
>>> +	if (offset == UINT_MAX) {
>>> +		if (cluster_list_empty(&si->free_clusters))
>>> +			return 0;
>>> +
>>> +		idx = cluster_list_first(&si->free_clusters);
>>> +		offset = idx * SWAPFILE_CLUSTER;
>>>
>>> -	memset(si->swap_map + offset, SWAP_HAS_CACHE, SWAPFILE_CLUSTER);
>>> +		ci = lock_cluster(si, offset);
>>> +		alloc_cluster(si, idx);
>>> +		cluster_set_count(ci, SWAPFILE_CLUSTER);
>>> +
>>> +		/*
>>> +		 * If scan_swap_map_slots() can't find a free cluster, it will
>>> +		 * check si->swap_map directly. To make sure this standby
>>> +		 * cluster isn't taken by scan_swap_map_slots(), mark the swap
>>> +		 * entries bad (occupied). (same approach as discard).
>>> +		 */
>>> +		memset(si->swap_map + offset + nr_pages, SWAP_MAP_BAD,
>>> +			SWAPFILE_CLUSTER - nr_pages);
>> 
>> There's an issue with this solution.  If the free space of swap device
>> runs low, it's possible that
>> 
>> - some cluster are put in the percpu_cluster of some CPUs
>>   the swap entries there are marked as used
>> 
>> - no free swap entries elsewhere
>> 
>> - nr_swap_pages isn't 0
>> 
>> So, we will still scan LRU, but swap allocation fails, although there's
>> still free swap space.
>
> Ahh yes, good spot.
>
>> 
>> I think that we should follow the method we used for the original
>> percpu_cluster.  That is, if all free swap entries are in
>> percpu_cluster, we will start to allocate from percpu_cluster.
>
> As i suggested above, I think I could do this by removing a cpu's current
> cluster for a given order from the percpu_cluster and making it generally
> available for scanning. Does that work for you?

replied above.

>> 
>>> +	} else {
>>> +		idx = offset / SWAPFILE_CLUSTER;
>>> +		ci = lock_cluster(si, offset);
>>> +	}
>>> +
>>> +	memset(si->swap_map + offset, SWAP_HAS_CACHE, nr_pages);
>>>  	unlock_cluster(ci);
>>> -	swap_range_alloc(si, offset, SWAPFILE_CLUSTER);
>>> +	swap_range_alloc(si, offset, nr_pages);
>>>  	*slot = swp_entry(si->type, offset);
>>>
>>> +	offset += nr_pages;
>>> +	if (idx != offset / SWAPFILE_CLUSTER)
>>> +		offset = UINT_MAX;
>>> +	cluster->large_next[order_idx] = offset;
>>> +
>>>  	return 1;
>>>  }
>>>
>> 
>> [snip]

--
Best Regards,
Huang, Ying

Powered by blists - more mailing lists