linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: "Huang, Ying" <ying.huang@intel.com>
To: Ryan Roberts <ryan.roberts@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	 David Hildenbrand <david@redhat.com>,
	 Matthew Wilcox <willy@infradead.org>,
	 Gao Xiang <xiang@kernel.org>,  Yu Zhao <yuzhao@google.com>,
	 Yang Shi <shy828301@gmail.com>,  Michal Hocko <mhocko@suse.com>,
	 Kefeng Wang <wangkefeng.wang@huawei.com>,
	 <linux-kernel@vger.kernel.org>, <linux-mm@kvack.org>,
	 Tim Chen <tim.c.chen@linux.intel.com>
Subject: Re: [PATCH v2 2/2] mm: swap: Swap-out small-sized THP without splitting
Date: Fri, 20 Oct 2023 11:12:24 +0800	[thread overview]
Message-ID: <87sf66f0mf.fsf@yhuang6-desk2.ccr.corp.intel.com> (raw)
In-Reply-To: <4b351eaa-c2db-408b-9ce2-4b12e3d6b30a@arm.com> (Ryan Roberts's message of "Thu, 19 Oct 2023 15:25:39 +0100")

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

> On 19/10/2023 06:49, Huang, Ying wrote:
>> Ryan Roberts <ryan.roberts@arm.com> writes:
>> 
>>> On 18/10/2023 07:55, Huang, Ying wrote:
>>>> Ryan Roberts <ryan.roberts@arm.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.
>> 
>
> [...]
>
>>>>> +		/*
>>>>> +		 * 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.
>
> I'd like to decide how best to solve this problem before I can figure out how
> much code sharing I can do for the order-0 vs order > 0 allocators.
>
> I see a couple of potential options:
>
> 1) Manipulate nr_swap_pages to include the entries that are marked SWAP_MAP_BAD,
> so when reserving a new per-order/per-cpu cluster, subtract SWAPFILE_CLUSTER,
> and then add nr_pages for each allocation from that cluster.
>
> 2) Don't mark the entries in the reserved cluster as SWAP_MAP_BAD, which would
> allow the scanner to steal (order-0) entries. The scanner could set a flag in
> the cluster info to mark it as having been allocated from by the scanner, so the
> next attempt to allocate a high order from it would cause discarding it as the
> cpu's current cluster and trying to get a fresh cluster from the free list.
>
> While option 2 is a bit more complex, I prefer it as a solution. What do you think?

I think that this is a good choice to start with.  We may build more
optimization on top of it if necessary.

--
Best Regards,
Huang, Ying


      reply	other threads:[~2023-10-20  3:14 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-17 16:13 [PATCH v2 0/2] Swap-out small-sized THP without splitting Ryan Roberts
2023-10-17 16:13 ` [PATCH v2 1/2] mm: swap: Remove CLUSTER_FLAG_HUGE from swap_cluster_info:flags Ryan Roberts
2023-10-17 16:13 ` [PATCH v2 2/2] mm: swap: Swap-out small-sized THP without splitting Ryan Roberts
2023-10-18  6:55   ` Huang, Ying
2023-10-18 14:07     ` Ryan Roberts
2023-10-19  5:49       ` Huang, Ying
2023-10-19 14:25         ` Ryan Roberts
2023-10-20  3:12           ` Huang, Ying [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87sf66f0mf.fsf@yhuang6-desk2.ccr.corp.intel.com \
    --to=ying.huang@intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=david@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.com \
    --cc=ryan.roberts@arm.com \
    --cc=shy828301@gmail.com \
    --cc=tim.c.chen@linux.intel.com \
    --cc=wangkefeng.wang@huawei.com \
    --cc=willy@infradead.org \
    --cc=xiang@kernel.org \
    --cc=yuzhao@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).