Received-SPF: pass (google.com: domain of linux-kernel+bounces-101047-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <24dc6251-8582-790f-bbd3-465deed946f5@oppo.com>
Date: Wed, 13 Mar 2024 10:21:42 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [RFC PATCH v3 3/5] mm: swap: make should_try_to_free_swap()
 support large-folio
To: Ryan Roberts <ryan.roberts@arm.com>, Barry Song <21cnbao@gmail.com>,
 akpm@linux-foundation.org, linux-mm@kvack.org
Cc: chengming.zhou@linux.dev, chrisl@kernel.org, david@redhat.com,
 hannes@cmpxchg.org, kasong@tencent.com,
 linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
 mhocko@suse.com, nphamcs@gmail.com, shy828301@gmail.com,
 steven.price@arm.com, surenb@google.com, wangkefeng.wang@huawei.com,
 willy@infradead.org, xiang@kernel.org, ying.huang@intel.com,
 yosryahmed@google.com, yuzhao@google.com, Barry Song <v-songbaohua@oppo.com>
References: <20240304081348.197341-1-21cnbao@gmail.com>
 <20240304081348.197341-4-21cnbao@gmail.com>
 <e73c12ff-5234-44d5-a2b3-99cdc61a9c37@arm.com>
From: Chuanhua Han <hanchuanhua@oppo.com>
In-Reply-To: <e73c12ff-5234-44d5-a2b3-99cdc61a9c37@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?dmlTcGYwOFIwc01xazNwb096cXZYZkJ2emVzSEc1WjQ3K29Mb2E3V0lJeXhP?=
 =?utf-8?B?UE4ydmY1aFBqZjJpMkx5RC9jWXJ4SXRsWnlndzlxTXc5bDFvRk9YY010cWpL?=
 =?utf-8?B?UERkNWV0Mk9PSjQ0UkRUSU9XMmxsY1BEWU5oSnEyUXR0aUpIaTh2Sy9tNUxs?=
 =?utf-8?B?dHJTMkNaRlhiaDNKMjh4cGF1dmFtNnoyZUpyWnlPaTh4R2NoNHZPNE5rdHRM?=
 =?utf-8?B?VVB3WlU1ZmhWNndjT0VTUFhNQ3o3SFk4K2ZrTFBBaWpkYUVOVSt0UE5heThV?=
 =?utf-8?B?RTNuTDBpNlNoWllqZEFraEFtRHduaWdBVXQwU1N2RTZsSU9NNWZ1UGhFS3FC?=
 =?utf-8?B?Vno5NlJONVNUMmxxS1NMTEc1ZHNROFJXTHpLUEZTQ2x1Z3owNTdENmUwU3pF?=
 =?utf-8?B?RjA4anpXcHJ3aDRaU1pTSmZLcXNWeHBITDNFRkpWdUpGNFJNUzZRZ05PZDNt?=
 =?utf-8?B?dDBTaVgyWGcrcDBWdHpZMjVNcFRVN2wxdnpvN3hoWHZuZ3lEKyt6cGJNVEI0?=
 =?utf-8?B?T1BjY3ptMGptQlord1hpSVNuRzN3Q1RJcU85dHJKTEhyOC9zbGdLYk1TbjJT?=
 =?utf-8?B?Z2xXTXNoTDZSd2ZhY1VRKzFvaklKRXBLR1pjVTJtd1puOWJtSVMwWHdWS2Np?=
 =?utf-8?B?T2c1Zys3STN5RndQcWhvbHRyYnpyTnpqUFY5MEVUMHhWRk1GOS9aUWhOVkNz?=
 =?utf-8?B?bllORjRlQ3BKSTRPd0xhZ2NKeVRMNlh5UExKTmhDMndPTnIrbTlpUWkxOElG?=
 =?utf-8?B?S0dlMHVHeElHZXlBMVZJOVJwVHdxRVorbld1NGZ3Vi84VVhNb2ZLb2RGQUNQ?=
 =?utf-8?B?SHpLRENTaThGM3hFalQ0VkZWRDNpZW5Cc1ZuVFhHRldRcHg3dmJ1bmVsWG5H?=
 =?utf-8?B?amRjM3ZZT0h6ZkJ5OGtqZWdiVUIvbVlhTWtySGJkdkJjZU5jVGFOTjhNYlhz?=
 =?utf-8?B?cG5DNUVRaDN3VjlDbE5ENHNGZkhOSjNOdVVwdTdxSmV1aURZKzMyeXErUlpx?=
 =?utf-8?B?NzMrdm5IQ1BWSW9uUGhZNTcyYU12K2VRM2hJVGpad0RCbnA3YnNSUG51QVVG?=
 =?utf-8?B?S2tzYTliQVJtU1EvRW4yeDh1STBBeHd5a0dpdjc1aFJXYTUrck9LeTI4Lzdo?=
 =?utf-8?B?em10N2d4bmdBSldlV3UxQUV6WHE2VkU3RzJaMFdjNEFFTy9mNHZtU3puaTdu?=
 =?utf-8?B?Z1BsR1lWTU1nc3R4K01vSlFnd2ZmZlNyd1FmTTcySFlzVmJHc2xzbnpGeW9i?=
 =?utf-8?B?cm02SlREdmpHWlNKMmNWdlMxSklvaFpQUXdmNjFaMEE2NitlUEFRdTdvLy90?=
 =?utf-8?B?d2xCbHh6RGNWNzJVcEhNLy9qWExXMmVvTFZPRE5SWUJhQXkyMHhKNlBGTldm?=
 =?utf-8?B?MmpSbEpucW45bStZem1LMmc3Vks3QU8zaXRXenU3bFVkSWVENjVERlhoQWgr?=
 =?utf-8?B?Y1I1RlZVR1pOTXRJMTZhV1FwWXE4aTlrRDg0MTlhK0xJZEpJNlg4M1BPZ055?=
 =?utf-8?B?SGpNOVdGbWkyem9hajJrc3I0aHRWUmlGZ0hnZENCdzZOeGNpMjhDbkhISGU5?=
 =?utf-8?B?VUpRdXpLRXkvRVZWSXo5elJ1R0daVVVWSFpqNnp3SFhiQ3dQWVlqU0JSVUVv?=
 =?utf-8?B?STlDMlJpdnNTMHdrOUZjT000UzJiQVZvVWk5dDlxNWk0MENleHF0YkRVTGd3?=
 =?utf-8?B?SHArMTVjM2Jsc29NeElEdWRXSEdZczJKSENBeUVDd3d5NTZ6WG16UnVyTWxr?=
 =?utf-8?B?RHlmQlRrN2pNMFJIL0diVEpXc29rOWpwM3FUdHhIWHg4VjMwRkhoL0E0V2pO?=
 =?utf-8?B?TlROYS9KZTlNMEhPaTAxNkRnSnJlZ0J1Tis4R1NnQzBBMUFFZ3ZkSFo4dmh1?=
 =?utf-8?B?bng3NytZbmtNYzVqc2dIdVdDajZ3Ni9tNXByK3RpUFhLRVZqVUJjVFVCS3pS?=
 =?utf-8?B?YkZsaWtSejJ4dGR3S1J3ckpNbzdaTW9kdFo3Z2lOaEkrV0FRYkdEM0EzSzl6?=
 =?utf-8?B?YytuVE9ia2FvTTZ1WCtkVzZ3WERyV0llMWw3M0F6RGJmSUV6VHlHWHl3OTJw?=
 =?utf-8?B?UlNaOWVWVjVsd21nVzRZOVYzZDJTSEt2d3RLODc0cmhEbXZ1bVFOb3ZpWFgy?=
 =?utf-8?B?VXRKTFFhNHNLS2JwQStXZ1pTRkJDazZ0WituVWxsM2JOczBSRU9zeUpRZU9K?=
 =?utf-8?B?UXc9PQ==?=
X-OriginatorOrg: oppo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6b548685-16f1-427f-9b66-08dc4304559b
X-MS-Exchange-CrossTenant-AuthSource: TYZPR02MB7988.apcprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Mar 2024 02:21:47.7028
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f1905eb1-c353-41c5-9516-62b4a54b5ee6
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WepdixPbOqA1MWeonBIFx3Klz6sYfZiTEIp7haz4PC60o0IVBEqUD2HrLZOnh+UwUCp4sLVz0UnayNFxixpC1w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SEZPR02MB5614

hi, Ryan Roberts

在 2024/3/12 20:34, Ryan Roberts 写道:
> On 04/03/2024 08:13, Barry Song wrote:
>> From: Chuanhua Han <hanchuanhua@oppo.com>
>>
>> should_try_to_free_swap() works with an assumption that swap-in is always done
>> at normal page granularity, aka, folio_nr_pages = 1. To support large folio
>> swap-in, this patch removes the assumption.
>>
>> Signed-off-by: Chuanhua Han <hanchuanhua@oppo.com>
>> Co-developed-by: Barry Song <v-songbaohua@oppo.com>
>> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
>> Acked-by: Chris Li <chrisl@kernel.org>
>> ---
>>  mm/memory.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index abd4f33d62c9..e0d34d705e07 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3837,7 +3837,7 @@ static inline bool should_try_to_free_swap(struct folio *folio,
>>  	 * reference only in case it's likely that we'll be the exlusive user.
>>  	 */
>>  	return (fault_flags & FAULT_FLAG_WRITE) && !folio_test_ksm(folio) &&
>> -		folio_ref_count(folio) == 2;
>> +		folio_ref_count(folio) == (1 + folio_nr_pages(folio));
> I don't think this is correct; one reference has just been added to the folio in
> do_swap_page(), either by getting from swapcache (swap_cache_get_folio()) or by
> allocating. If it came from the swapcache, it could be a large folio, because we
> swapped out a large folio and never removed it from swapcache. But in that case,
> others may have partially mapped it, so the refcount could legitimately equal
> the number of pages while still not being exclusively mapped.
>
> I'm guessing this logic is trying to estimate when we are likely exclusive so
> that we remove from swapcache (release ref) and can then reuse rather than CoW
> the folio? The main CoW path currently CoWs page-by-page even for large folios,
> and with Barry's recent patch, even the last page gets copied. So not sure what
> this change is really trying to achieve?
>
First, if it is a large folio in the swap cache, then its refcont is at
least folio_nr_pages(folio) :  


For example, in add_to_swap_cache path:

int add_to_swap_cache(struct folio *folio, swp_entry_t entry,
                        gfp_t gfp, void **shadowp)
{
        struct address_space *address_space = swap_address_space(entry);
        pgoff_t idx = swp_offset(entry);
        XA_STATE_ORDER(xas, &address_space->i_pages, idx,
folio_order(folio));
        unsigned long i, nr = folio_nr_pages(folio); <---
        void *old;
        ...
        folio_ref_add(folio, nr); <---
        folio_set_swapcache(folio);
        ...
}


  *

    Then in the do_swap_page path:

  * if (should_try_to_free_swap(folio, vma, vmf->flags))
            folio_free_swap(folio);
  *

  * It also indicates that only folio in the swap cache will call
    folio_free_swap
  * to delete it from the swap cache, So I feel like this patch is
    necessary!? ????

>>  }
>>  
>>  static vm_fault_t pte_marker_clear(struct vm_fault *vmf)

Thanks,

Chuanhua