Received-SPF: pass (google.com: domain of linux-kernel+bounces-418-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: <e62c67ac-c541-4a98-a71b-3fc7da41c293@oracle.com>
Date: Thu, 14 Dec 2023 19:57:37 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] mm: remove redundant lru_add_drain() prior to
 unmapping pages
Content-Language: en-US
To: Matthew Wilcox <willy@infradead.org>
Cc: akpm@linux-foundation.org, tim.c.chen@linux.intel.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
References: <20231214222717.50277-1-jianfeng.w.wang@oracle.com>
 <ZXuJKAInbgtuSU9b@casper.infradead.org>
 <9792a7f5-62cd-45e1-b7d6-406592cc566d@oracle.com>
 <ZXvCrfKfgwRwZTLA@casper.infradead.org>
From: Jianfeng Wang <jianfeng.w.wang@oracle.com>
In-Reply-To: <ZXvCrfKfgwRwZTLA@casper.infradead.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?MTRxYWF5c0lZZ1oxK3RxZ0w3ZjFBWnBSQjB4TWU0WlJ4ZE1zVXRvN0g0QUM1?=
 =?utf-8?B?Sk01VlltTVVId1BqTnRYWVkwdXhOcUxPcDMvYjJxZmlWWHNVdGd5Z2xOck54?=
 =?utf-8?B?Q1lJS3RUczRqbzcrdXMxMUFsWnJ0VkExYTl4cUY1ODVrc3hNNE82aHBVNzJ5?=
 =?utf-8?B?MXJhaHlxOWJ4S0RZR1BVT0JXY0Q2ZjdQYmhHR24vOXFnQXp0ZFBuVHdCeXl2?=
 =?utf-8?B?eXhralpnRDdhM2NEbUhMeElESER1MlFHT0ZaSGM5aC92SDhIMmVUaitRczhv?=
 =?utf-8?B?QWZhdGkyWHZuekg2c29FS2ZsVVByd3lQNzkzeXAvc1BPc2tWVTNwUEdtdm1X?=
 =?utf-8?B?cmFnM2pUV3BsbCtpRm43b3hiWnNpU3JXM0NiNXdGRksxd2FLdDQzRHcrT3M0?=
 =?utf-8?B?RmloekUxNGlnNGNoN1ExY1h1Y2d5ZFJBdDlnWEF1SjhFUjd5VDk3cGZjZVNw?=
 =?utf-8?B?KzBjVGIyaU1PUzhjT1cyajdwYjdEV3RNZFJsUGI4QzE1T2J2alN0STFoeG1W?=
 =?utf-8?B?MStXQjNpWE90aDhhM1NNMzhUbm9Uc043a2JKOUlnQnI0UGZqa3hMLzI5Slpn?=
 =?utf-8?B?d1JXYzg3VCswMW5KMHBZOXpxNDlYZTd0L29YUFJWMVI3ZWZwZktFZ0dNYVYr?=
 =?utf-8?B?a0IwQkhaMEJidXltKzAwekhncm1HZzVqTWkwaDRsT3FIN0RzZ0dybmFRK1lY?=
 =?utf-8?B?b21MSzJ6c1NONTFqWFZubWVNd2U4VGJROTRGQ3dRQjRYRzZoWmhEUFZyd0ZD?=
 =?utf-8?B?ZTJtOUd1YW4xRDVKVlRWa1lmT1dYczRzekdVQ1VMV2dKWTZCUWxvSmJxWEtl?=
 =?utf-8?B?NmxrVUlpNjV4bk11MklOVldDNGlvU0ZBVk1GQS9LSisyY2RvRTdIeFNYbE1J?=
 =?utf-8?B?b2dkeGlwNEZJc0N4T3VnZkcvWHN3UEFjU2JNUFE0cXppWUtBdFdTQ2JDT1dF?=
 =?utf-8?B?WkZaOVdrNFlOTlQ4dDBIQWpkQTdjRmE0N2Z3THVtcUtMUkF3bitmZnVOTjFL?=
 =?utf-8?B?UGljbGZuWE50d0F6Y2I4NXZoUmpRbzY5cHhWVC9rQU5hWDFaSElQTGZhYlNp?=
 =?utf-8?B?RXBHSmRpQjNPcFB0ZXpWcDZSV3hmTExnakxCNFdtREhhdmdZUTNFT0pFN2cv?=
 =?utf-8?B?cStoalRHVGt0R2hVM1crcC9hZHpHZHZseFR3SzJWSlIrcE05U0JpSmttOVRa?=
 =?utf-8?B?ZEZBaEZEVmF0b214MFRaTnpiUDNoclM5RmEwZ1JBeHdXUWZnNHN3YTBkSDVR?=
 =?utf-8?B?MU1MT0hGdFBVYVdXdkh1UnhXSFo0OURsUlltNm10ZzZ5eEI0ME1kd3JBWWxx?=
 =?utf-8?B?R1BNbDZqNFBGVlFUYnR3NEVRbjVsS1lzMXk4cG94QlFuQjFidzJoa2dNWWpk?=
 =?utf-8?B?czgwdy9KRnRZREZhbHgxTDVzWUxOSGdJdE1YS0NKOTI4b0NiMWhDUkUrMGFi?=
 =?utf-8?B?eG1tSmJFSEY5Nlp6SzlyRmxsQlJGNnBKOXpJcU9hSnVxQllCaDR3aEFwRVd4?=
 =?utf-8?B?SytWUzlyQ0hTT0pIdUE4SFdrSnpOekd3VG5pTmVIMDBkcXczUzVQaStOVFp2?=
 =?utf-8?B?U3BVcE4zTWUwclVZRFJhTytmUkdiZkJlbE82bXFTUFpnMEw3TzFUbEJTU3hx?=
 =?utf-8?B?aFZsb2IzZmJTd3ZZS2FQWitHakJMVWlEZUN4Q3RMWXNKV0ZjWUFhODRGMFBY?=
 =?utf-8?B?bFZJb1V2ZnhTenoyZkZKK3Nuai9yUjkvLzd3UGp3VVFxeStxVjZoL3paRTNL?=
 =?utf-8?B?MWFUMDZQYlQrNkRHT1VxRUxUOGRseDEzc0pHNDY0ck9MOUtjWGRyQ1NLMlg3?=
 =?utf-8?B?U1B3UUhOd0hoemdlVzVvQzJwbFk0cjRlUC8rd0NCb3YxWmhHeVF6NVRnZXhO?=
 =?utf-8?B?UWs1aDhuaFVPb0Nocm1FQ3ZRTWF5R1ExMVVwbVVFdllHNm1TSWhTWHREZlR6?=
 =?utf-8?B?TXVUUjB4eEtyazcyRFVCMDliT3NJaC9RaGpwbkRtOTNMT09aTmx4bTY2VmNC?=
 =?utf-8?B?cldaSkFHNlFjMkJRbUl1YWpDWWhLdzlGNFJ2N29NbzV1WVZMUHdkcldUVllo?=
 =?utf-8?B?RUJPbllDd3NLb0FzSjU5WWxvWWVEUWtDSG03SWpvOEFOZkF1UHl0RUREK0ov?=
 =?utf-8?B?dGRuOXhOcGRuL0VVQjNnWXU2cmV3UGJFLzZmTkZUeTZkTEZpR3ozUDdXb2RC?=
 =?utf-8?B?V1E9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	shDiO18rXOR7cdi+GdG/QBsrlmx5UJGM3Jcsusw/r3YJOYcrSeuOyf2cDxuXX8d7Werrxl8+yLiahwcg1iMUbSZ/8X91xKEz1Xm4c5eRfBPIYFGY9QFNmI/ti63L78jfFOxcturoh5TwlluTkjV7XQTdlcn2PgkFiy4DsSN0QuF70qzvR/71UXGOvsoJtWIlB43FDft3TL9Wzpno3zkntcrwsTDLfKjp567jA+/P+mkBHXlVs/aBeWFlk8Y8k1+zfGMwR94dgTBNSgRfEsFQGGcVXT/3pxgPovs2Uo4ia+nscBriIno0H3HLA78Wz9cLuaX59hUIjIRoWDiOdoaKXAo3J4i/dCrZfxOg3i3BUmInTnpFRphdVIXrdqmRsDDB1/SOGwbXVRkJmEIGV/O8t2WcRODDHDC/edMTCDMVEeGW9yuZ1bXzAqQb0jlviRKouOZKfBkiyRghl9bQvCVkbP0yxxNoj85/WbJGRnr0uF3bJ1t/6kTF4DYRGUDldTsptQr6KsfhDfvXgWUvHjR/HV9ZUPAwNTSeWzutCrzqZSl76QZgdbqXOfkQcfF7keJjFmimztOy6ghyGVqn88MUmPKlibMJQEsU7mbZ+drdzl8=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b934e3ab-96c0-44e1-d20f-08dbfd21fe0e
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB3558.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Dec 2023 03:57:44.4523
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: oBijigxQfvriV5WPlGE+xPDxurn0LuSVKScviaUJRMlxAYFqUmT4MOddFvCPG7S75pdT0syYYXgCtGPTkI3mopRcKZ7QIOzu/AmLkCHd+xU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR10MB6683
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.997,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-12-14_17,2023-12-14_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxscore=0 suspectscore=0
 phishscore=0 adultscore=0 mlxlogscore=999 bulkscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2312150027
X-Proofpoint-ORIG-GUID: 8Gb3NR1FURo-TsW0C4yCnfHnQ3YVlEbu
X-Proofpoint-GUID: 8Gb3NR1FURo-TsW0C4yCnfHnQ3YVlEbu

On 12/14/23 7:06 PM, Matthew Wilcox wrote:
> On Thu, Dec 14, 2023 at 03:59:00PM -0800, Jianfeng Wang wrote:
>> On 12/14/23 3:00 PM, Matthew Wilcox wrote:
>>> On Thu, Dec 14, 2023 at 02:27:17PM -0800, Jianfeng Wang wrote:
>>>> When unmapping VMA pages, pages will be gathered in batch and released by
>>>> tlb_finish_mmu() if CONFIG_MMU_GATHER_NO_GATHER is not set. The function
>>>> tlb_finish_mmu() is responsible for calling free_pages_and_swap_cache(),
>>>> which calls lru_add_drain() to drain cached pages in folio_batch before
>>>> releasing gathered pages. Thus, it is redundant to call lru_add_drain()
>>>> before gathering pages, if CONFIG_MMU_GATHER_NO_GATHER is not set.
>>>>
>>>> Remove lru_add_drain() prior to gathering and unmapping pages in
>>>> exit_mmap() and unmap_region() if CONFIG_MMU_GATHER_NO_GATHER is not set.
>>>>
>>>> Note that the page unmapping process in oom_killer (e.g., in
>>>> __oom_reap_task_mm()) also uses tlb_finish_mmu() and does not have
>>>> redundant lru_add_drain(). So, this commit makes the code more consistent.
>>>
>>> Shouldn't we put this in __tlb_gather_mmu() which already has the
>>> CONFIG_MMU_GATHER_NO_GATHER ifdefs?  That would presuambly help with, eg
>>> zap_page_range_single() too.
>>>
>>
>> Thanks. It makes sense to me.
>> This commit is motivated by a workload that use mmap/unmap heavily.
>> While the mmu_gather feature is also used by hugetlb, madvise, mprotect,
>> etc., thus I prefer to have another standalone commit (following this one)
>> that moves lru_add_drain() to __tlb_gather_mmu() to unify these cases for
>> not making redundant lru_add_drain() calls when using mmu_gather.
> 
> That's not normally the approach we take.

Okay, understood. Thanks for pointing it out.
Let me send a new patch that aligns with your suggestion.