Received-SPF: pass (google.com: domain of linux-kernel+bounces-105931-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <a75ea34a-7512-4169-b987-95f11a7f3dd0@intel.com>
Date: Mon, 18 Mar 2024 18:00:19 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 5/6] mm: vmscan: Avoid split during shrink_folio_list()
To: "Huang, Ying" <ying.huang@intel.com>, Ryan Roberts <ryan.roberts@arm.com>
CC: David Hildenbrand <david@redhat.com>, "linux-mm@kvack.org"
	<linux-mm@kvack.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, Andrew Morton <akpm@linux-foundation.org>,
	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>, Barry Song
	<21cnbao@gmail.com>, Chris Li <chrisl@kernel.org>
References: <20240311150058.1122862-1-ryan.roberts@arm.com>
 <20240311150058.1122862-6-ryan.roberts@arm.com>
 <d8086108-05e7-4765-a6f5-dfb85fb44b90@redhat.com>
 <1db242d3-5ff1-4ef5-b20a-578a317fa859@arm.com>
 <caafeabd-fb94-45de-9dcc-2451ddc0ef50@redhat.com>
 <5cc147fa-492e-46c3-963d-2f6e6680dce9@arm.com>
 <878r2gwahg.fsf@yhuang6-desk2.ccr.corp.intel.com>
Content-Language: en-US
From: "Yin, Fengwei" <fengwei.yin@intel.com>
In-Reply-To: <878r2gwahg.fsf@yhuang6-desk2.ccr.corp.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
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?QXhjUnloSS8xeGZWcHhjaDlsSlBGU0paYy9LZW9BUVB2aUFPOFpsRVV6ZU9E?=
 =?utf-8?B?ZU9ESjE0S2k0N05oZWhzODdQejFjVG5ZWXc2UFNOQjhzZG1QOHVBMjJaZURt?=
 =?utf-8?B?UW1nZ09Nay9Mck5vd1NVYmR2Q2dkdVVKWVRtc1VaTWptYTRXM0Zsa2pZUDlv?=
 =?utf-8?B?bEpDT3hMRmRFSG9JMHk0RTl0MWt3UG0vUWxJNG5Vd1UyYmtZMGVjamYyVEtH?=
 =?utf-8?B?SHc4YWFmN1NjR3hTTVpjOFBqS3hxTzdsb0s4ZnpNVE1tdnErVGVkMUxhTGEx?=
 =?utf-8?B?RmJ0YW9NNFFUaFQxV1Y1OGhqL1lCVTlxT1BaMEYwK29qd3ZFd25VMUQvalJp?=
 =?utf-8?B?L3ZucnZBb3dUUjFpSHFaZWtTWDg3dFRCVGJPVENNL1BiSENGMjZiWHhuSzNo?=
 =?utf-8?B?bVZZNFZ1a1pkbnRIVmZ0bGJjQzhUREVGSEFnN256MjdnNXp6VUkvdFVTV0Qx?=
 =?utf-8?B?U2daRE5idnJDcGlsVHNocTRZVU1xZy9kVldKQ05jTFNOY1RycmJueElsZVNy?=
 =?utf-8?B?RXNmQmhTc2tLOU8wVVNFSGE1dEVJSVZXSVFVUmJXSWlISFZwSGk5TTE2YXZ2?=
 =?utf-8?B?aEFPNFZBN0Y3cXFxVGhBek1UV1Z6M0I2bnpGTTdIOHdjbG9TOExxUjFMUGU4?=
 =?utf-8?B?NEhobWV5Wmx2UGVuWnRSSkJ2b1JnT3dzMVEwMmpyQ0o1QTR1anYrbGh0UmJQ?=
 =?utf-8?B?QWZ6dUNVNjJ2WjdJVGhPQUZXS2d4Rkh6djlyeUFBRHE3Y2l3NmZ1VGU2MGtE?=
 =?utf-8?B?eHVvcjFycExhSUNuc215ZExMYWRkN0ptd1BIUitFejZKaUJBSFI1UmhKNita?=
 =?utf-8?B?cEdBWHJCcy9kcHlROFJHZjY3NzdPWU5HK045MXV3RGlVck50WW9XYTFUZzM5?=
 =?utf-8?B?TnZrRzlsT3R2b0M3amFENXF4c1FDWUsraEpnazBDakdTTkszTkZKZ2NIQkFE?=
 =?utf-8?B?MWUxeERiVGNQVm84cXBJM2g2bHRLWml2d1dxV3BJMjZETTFVU0d5bnA1bURS?=
 =?utf-8?B?R1hWQzk0Q0lHd0lWTDFWb2VlNUwvdGpSSUZjblR4bFpRV25PMDRLQnFKeXBn?=
 =?utf-8?B?T0hrd2JvbW5ySmkzMkxqVzJCUDNzWmJJQjI4c01TUUE2S3Z2b1p0eGtrR28w?=
 =?utf-8?B?RjB2SmlrNVFkUW04b2tQSzNCbXdRSUgxUXIxWkJXTVZQVXExUS91bkNiRHVU?=
 =?utf-8?B?LytnWU1PV2kwL1hpZTYrd25wM1FnNEZOOW4vbGNtMG9SaVlwMjJyOXJHNHpw?=
 =?utf-8?B?aWs1b1NRZDREalFoRkpNeHFqeENPSlMzRWdxWFE1dXZPTmpxQ0M3eGpDZGM5?=
 =?utf-8?B?d2U0L0dtdXNTMm50VVZNRVh1ZWlYRmozb2NRL1g1L3RVaUFKMGJyUFdmVkFK?=
 =?utf-8?B?UnM0V3dKQlN6aUhBY2VCeGFwbU1EaVVYbHdKZW1WWHkrSFJvdXdhVks0ZFRa?=
 =?utf-8?B?bXlERWIyUytvclNXMURBMy95azFoTmZmQW1ZSTlEZFB6dEs1OTgxcW5yelJy?=
 =?utf-8?B?NGRCMUljTnVYL0ZvRUhFZGExSGF1bk9PdlpVTC8veHd1MGhkamFHRG5idkpD?=
 =?utf-8?B?SUJ6bG00T2RPMndTMWFzYSs1VDBJMkNrUU5mYlphSGJicFhGYjdKcWJ3SVZH?=
 =?utf-8?B?OFE0UVFjZ1VWWVBOOGFKVmVmM0RmZUEweGc5dGRYbVBiNlQ1K3pyK1dqMHdN?=
 =?utf-8?B?K1kwekFnN0lLM3VHdjAycGJ4MEZhRTllWk83WEtnRWc0TFhCTGs0Tkh2WXJH?=
 =?utf-8?B?MEhRNDNROUZDdnl5WXF5WEtWVkQ4YmpCLzNsR2FQYU9oakVJTTNtbWlZYUd1?=
 =?utf-8?B?VE90Ty8vZ05UVGpMM0RHdEdnVHdQVHI4Z3pnTS82TnZjR25kQ0UrajE5NjZs?=
 =?utf-8?B?MHFEVE1qZXozb1FpSE5lN0E5UTVFVVdBQ2tUWlltVTZwb2pFWjRYVUk3YTQy?=
 =?utf-8?B?ZmE3bm5hZzlvcGVrOVd2RVZCdHdZQmpwNldRbE01RmxySExiTDAzc0FrOXcw?=
 =?utf-8?B?cFVTYVBuUGpzUVBSOW5tbFFabUc1RWVTN2ExUFRQVUx6Y3lmZDNWd1VOVVlG?=
 =?utf-8?B?bXJiajZHd0hPcXJWdTVSM1Z6ajc3aHVGcmlSL1QvVzhlVXdjcG93WDJtbXRU?=
 =?utf-8?Q?xEUew2Li6ivanTmJUIz1GYfAS?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7e3694da-91db-47b4-bf17-08dc47323bbc
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4820.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Mar 2024 10:00:25.7883
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ecT1A38vLe6pCAKPfiRiDjqsUDUnPGDJlp5qW8PN7gUdNyL2gjESNZmNcBmkEQyxc+MsbAkw3hDloRdnEuAsaQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4618
X-OriginatorOrg: intel.com


On 3/18/2024 10:16 AM, Huang, Ying wrote:
> Ryan Roberts <ryan.roberts@arm.com> writes:
> 
>> Hi Yin Fengwei,
>>
>> On 15/03/2024 11:12, David Hildenbrand wrote:
>>> On 15.03.24 11:49, Ryan Roberts wrote:
>>>> On 15/03/2024 10:43, David Hildenbrand wrote:
>>>>> On 11.03.24 16:00, Ryan Roberts wrote:
>>>>>> Now that swap supports storing all mTHP sizes, avoid splitting large
>>>>>> folios before swap-out. This benefits performance of the swap-out path
>>>>>> by eliding split_folio_to_list(), which is expensive, and also sets us
>>>>>> up for swapping in large folios in a future series.
>>>>>>
>>>>>> If the folio is partially mapped, we continue to split it since we want
>>>>>> to avoid the extra IO overhead and storage of writing out pages
>>>>>> uneccessarily.
>>>>>>
>>>>>> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
>>>>>> ---
>>>>>>     mm/vmscan.c | 9 +++++----
>>>>>>     1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>>>>> index cf7d4cf47f1a..0ebec99e04c6 100644
>>>>>> --- a/mm/vmscan.c
>>>>>> +++ b/mm/vmscan.c
>>>>>> @@ -1222,11 +1222,12 @@ static unsigned int shrink_folio_list(struct list_head
>>>>>> *folio_list,
>>>>>>                         if (!can_split_folio(folio, NULL))
>>>>>>                             goto activate_locked;
>>>>>>                         /*
>>>>>> -                     * Split folios without a PMD map right
>>>>>> -                     * away. Chances are some or all of the
>>>>>> -                     * tail pages can be freed without IO.
>>>>>> +                     * Split partially mapped folios map
>>>>>> +                     * right away. Chances are some or all
>>>>>> +                     * of the tail pages can be freed
>>>>>> +                     * without IO.
>>>>>>                          */
>>>>>> -                    if (!folio_entire_mapcount(folio) &&
>>>>>> +                    if (!list_empty(&folio->_deferred_list) &&
>>>>>>                             split_folio_to_list(folio,
>>>>>>                                     folio_list))
>>>>>>                             goto activate_locked;
>>>>>
>>>>> Not sure if we might have to annotate that with data_race().
>>>>
>>>> I asked that exact question to Matthew in another context bt didn't get a
>>>> response. There are examples of checking if the deferred list is empty with and
>>>> without data_race() in the code base. But list_empty() is implemented like this:
>>>>
>>>> static inline int list_empty(const struct list_head *head)
>>>> {
>>>>      return READ_ONCE(head->next) == head;
>>>> }
>>>>
>>>> So I assumed the READ_ONCE() makes everything safe without a lock? Perhaps not
>>>> sufficient for KCSAN?
I don't think READ_ONCE() can replace the lock.

>>>
>>> Yeah, there is only one use of data_race with that list.
>>>
>>> It was added in f3ebdf042df4 ("THP: avoid lock when check whether THP is in
>>> deferred list").
>>>
>>> Looks like that was added right in v1 of that change [1], so my best guess is
>>> that it is not actually required.
>>>
>>> If not required, likely we should just cleanup the single user.
>>>
>>> [1]
>>> https://lore.kernel.org/linux-mm/20230417075643.3287513-2-fengwei.yin@intel.com/
>>
>> Do you have any recollection of why you added the data_race() markup?
> 
> Per my understanding, this is used to mark that the code accesses
> folio->_deferred_list without lock intentionally, while
> folio->_deferred_list may be changed in parallel.  IIUC, this is what
> data_race() is used for.  Or, my understanding is wrong?
Yes. This is my understanding also.


Regards
Yin, Fengwei

> 
> --
> Best Regards,
> Huang, Ying