Received-SPF: pass (google.com: domain of linux-kernel+bounces-264-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <cd587281-093d-4eca-8eb6-81d589573e32@oracle.com>
Date: Thu, 14 Dec 2023 15:56:34 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [External] : 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>
From: Jianfeng Wang <jianfeng.w.wang@oracle.com>
In-Reply-To: <ZXuJKAInbgtuSU9b@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?TjFhRWNvSS9EQ2FnY241L3hFZjJ1ZkkyYit5ZEVoYXkvQytZQkQ3NVZEUnND?=
 =?utf-8?B?TWFkM0hrbkFUM0F0ZHhZM1h1MFFESlRac2dZaUZwcWNRTi9rZVVNb2haejQv?=
 =?utf-8?B?WGJxdlBSbnprYjF0ZlArZmFWUWloZU5NRGZYZFpiSWJkS2c1NElmdkJkVzNk?=
 =?utf-8?B?d2dlcFAvdzA2KzBqMzluY0VhWjBIeW8zb0dCZWIvSkhlOTRtVzNpMVY5dkx6?=
 =?utf-8?B?ci8zaVcrMXFiWUp2aktnWnJlcFlnaGxnNytQQktVSXRhclIvUnZvQzJzeU5P?=
 =?utf-8?B?K2l6RFZqK1pVRlJmNWNrK3grbjN5Y0p3djd1ZG1LdUhiL05PNlNoN2lueHVw?=
 =?utf-8?B?dkhoSjBVTnlaSHpoZ2lwRk11Y3RBSWFvN0FwdE5FeTFzM1Y4dUJGVXJGQVky?=
 =?utf-8?B?czczWHpmRXV2cU9ITGFTWUUwT250MEdrTWI4NVptbXZzN1UyU2RGMlh6alVh?=
 =?utf-8?B?U2dWWWwyRVNFWWxvajFlWkVQSFpOdUdMWnBzQi84UGdhVXprZmloTWl3d21R?=
 =?utf-8?B?djl0M3cxa3BnUHRPV3I2TjkvYVQwSEdFbThHSXkvOGdaRm9HVU1XQkNJM0Iw?=
 =?utf-8?B?SEt1YXNOaU9EZkwrQkg3ZWk0c1QxY1NxSC8vUkF4NVBVTnR4UE5OaTBXSmlK?=
 =?utf-8?B?MTAzSFZqdDlTUFJ1bzVOQlNVSVpTRWc4RG1kK3ZIRzFhUzFCQU1VVGcxbEZ0?=
 =?utf-8?B?cll6YmVZUmdud2lIQkxXeHl4NGZFNkIydEdRWVlxeHZLVjlHMkZQc2ZNc1Fx?=
 =?utf-8?B?V3c5RU1ma2FEWlkvMkRRQTgxRlppRlJQTi80b3JKWnBVY0tSWk5YbzAxVE0y?=
 =?utf-8?B?bFdMZ29uYWphaC93S0dXNDd0U29yd21mOGYzUk9nZ2FxbzZNcFg5cnVSZjZ5?=
 =?utf-8?B?czU2emllK00xL3lMaitOU0JScnpLUUM3MTVYekUvL294T3JsWk5ZK0F4bzVH?=
 =?utf-8?B?MktWQzRncDlhY3F0WnZ0TUFybnNJRVVXU2sxZEFnWS82alZZdEtvbGRodWcz?=
 =?utf-8?B?WGZmRTJrNi9xY2F5WFdSSXJiNXpYQlFrNmx5b1RLTCt6UEpsMTM4MFU5bFRx?=
 =?utf-8?B?SDF1VWQ0T0Exa1FuNTJrUmtsRThQTld4akQwSDR0cnJDRmQ5T3Fya2xFd1cz?=
 =?utf-8?B?QnpRd1M0NE1JYXRVaDVhbVk5RTBQdGp6dVF4eWxCS2JUUFd0VUpaTURhS2Jo?=
 =?utf-8?B?c3JSQ1ZHUkFaRFNPSUI0SDNlKzFkNlNjOHlkcDhBRFJRc3daWmh6VU1rMWF6?=
 =?utf-8?B?S3hkQTlzOUpuckZRWDF2dTFPSm95WWVDdDk4QmNTUXAxbzhMdVZsSnRGMkdu?=
 =?utf-8?B?aWtHYk1uZ2ZDOHZFSGlLd2NFOEtvc1pyeHgrckVkTTRXSE1pNXU2MjNmRGJ5?=
 =?utf-8?B?SVdPa3RuYWRjdGlsWEc1TS96cTRoQXNtdmgwU3BVbGV1djVnWmI0ei85dHd3?=
 =?utf-8?B?cENVUWpYTHhOOVQxWEpiYWFvNGhkcGxmeTFDQ2tNK2Q1Rm5teXMxSEM0RHBv?=
 =?utf-8?B?N1Bsa0F2MklKUWlwUmpyWUtxTlpIUTRDTWRIckRWbWVJRnJ0U29zRE42ME5m?=
 =?utf-8?B?czMzUk41R01yVHBISnBPcU1wbDQyODlNdTRoSzgvRUZ1Y1NabkNOMnhyd2gr?=
 =?utf-8?B?UEFmOTZ0NzJBNmRsU1EwM1BjL3A1dkVhaFNIeWRRUWlLRThPejZ2aHdaaEZI?=
 =?utf-8?B?YUhCNlVGa2tPQnFrMnorZmQ3SFAwNmx2cS9JZ0hSY2thdFNob080MGdCYVFu?=
 =?utf-8?B?UnZCbGNPd3RJeWloQzR1NTM4dldRb3F0RzBjVGVjWndMYjFvM29jNnk1S0lQ?=
 =?utf-8?B?b1F5Rk14bG5TUVZBV2x2WFRLTjFoakdmK0dGVlJOMUQrVlhOemhFRkNtNC8z?=
 =?utf-8?B?NjNmRnpkTXZINmJKWjlpQ3hyYVAyYVV2ZzFvYVJHVXh4aUlwTE5scjVZYkEr?=
 =?utf-8?B?bHl1a1VvYkMxVFNlKzZhdkNJQWFCNUFVR2RlVktBQ0plYThwenBtTHo2WlFI?=
 =?utf-8?B?R1lKKzBTWGVmWTF4UDZzTCtNczZaVzFWR0NEM2RpdkhLcEpjSTRxek56ZVNu?=
 =?utf-8?B?cENJQlZaa01yc0hzVisyWVpkbm1zeGV5Wk9ic2dhdHZsRkNZOFRsODJrdjUz?=
 =?utf-8?B?M3RJT2xHM3ZTVUk1Nmo4a3lOcWFOU0NqYmdYQnRrZzFTWmlCNVFkVHNkZk45?=
 =?utf-8?B?MWc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	jEG+dZaLpiA/9ONu6bupMZP4AxnViy5o5BUkkCr51SDrzVUcbPgylHOwjAMxLhyKEXxObrkgO/CyRumU+9geP5f8qBKHY0ZIINi1CXgTaT/WKyFmoeHTov6OtPWybNkS8Z4ZZvLhlYb201rlxG51e68zLrEwaXlNHIGGOtUUxUXhCym28L36CJfDEwA09K/8ZoZnZp4hLO0FgBcY00k5Ml0CZHImsaYWnKX6TvG1PWfZsq86E15+STdyOk1WavX0MHZRWeCZqeRaLsPgqrVaj+Fj/DCrqdK/qhK/pg5fgxxzQtThYc0LevmVSeA2sO7RwWf1sdM8wE45WWwVFyFCRJhOEBvHEAE+0mu9TDueZO0L2WrpPtz2x1xCPGsBtPLWUIlXYowLcSdVsAtJRubKUYsBb+kW9hmKFjPPVwK9d7Ha88S0euQlbcUMIVHDtel7d/54SEgA04ckMu0f/GWJ7IpobRhjkI1BNojfhYVNcBRwPzngTrtPOwtz/vWrSfWY/RMqjNjmlzBcCiRZ9Klct1T0zOlYR2nXNBx69q74tXE/78R1OAREE+Bu3fdGcormOXGhPvQeQzrkuXJt4gFrHHXVT2LBfM8H1TiKUHWmg9YiJFE6lijpFAbR0Svo94C+ToR/bm4eUC7UIkJB38X+DF6VTyMU/lzu8GC6S2oekDId4yri1M6z/0tmwVOhJL4u6kIxEcmFpMePa9TlJPecRcaGmffsoSXZtHJWnuMY+o4GBN2ucFSqCebgAgzX78gAGY1cEjGMMFq/7DQ/EYAZJUo0KFgOPIwknudbHTzW4EKdA/7Y4XGMCTMbrV2mfOELIqPNnmsTMRxeuZf66qD/aMhCmPXsiMST22e33tv2BiIuzo9oBj1lvzgFtSaWZ/mo7LCtZvbDK9HP6ellxGDTY3S9SSQgzn6Fyup8uODapRA=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6927d088-acab-42dc-751c-08dbfd004e98
X-MS-Exchange-CrossTenant-AuthSource: BYAPR10MB3558.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Dec 2023 23:56:36.7228
 (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: 1TdK7TcKdTaN6zWgt1vDaY/GsbfseVM/PHiXLWxidYROo8st/6fqoESoShUXakmxYAD/+zRdXOQK0ZC7I9Wzjh5d7StQq18C7MMyWgpnxjk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR10MB5875
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_15,2023-12-14_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 malwarescore=0 spamscore=0
 suspectscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2312140167
X-Proofpoint-ORIG-GUID: RCoxFebgXiWNCyA-cG53PbUB0R5cIJQ_
X-Proofpoint-GUID: RCoxFebgXiWNCyA-cG53PbUB0R5cIJQ_

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.

>> Signed-off-by: Jianfeng Wang <jianfeng.w.wang@oracle.com>
>> ---
>>  mm/mmap.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 1971bfffcc03..da0308eef435 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -2330,7 +2330,10 @@ static void unmap_region(struct mm_struct *mm, struct ma_state *mas,
>>  	struct mmu_gather tlb;
>>  	unsigned long mt_start = mas->index;
>>  
>> +	/* Defer lru_add_drain() to tlb_finish_mmu() for the ifndef case. */
>> +#ifdef CONFIG_MMU_GATHER_NO_GATHER
>>  	lru_add_drain();
>> +#endif
>>  	tlb_gather_mmu(&tlb, mm);
>>  	update_hiwater_rss(mm);
>>  	unmap_vmas(&tlb, mas, vma, start, end, tree_end, mm_wr_locked);
>> @@ -3300,7 +3303,10 @@ void exit_mmap(struct mm_struct *mm)
>>  		return;
>>  	}
>>  
>> +	/* Defer lru_add_drain() to tlb_finish_mmu() for the ifndef case. */
>> +#ifdef CONFIG_MMU_GATHER_NO_GATHER
>>  	lru_add_drain();
>> +#endif
>>  	flush_cache_mm(mm);
>>  	tlb_gather_mmu_fullmm(&tlb, mm);
>>  	/* update_hiwater_rss(mm) here? but nobody should be looking */
>> -- 
>> 2.42.1
>>
>>