Received-SPF: pass (google.com: domain of linux-kernel+bounces-42085-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <9af163e3-fa45-4fe0-a95d-28096aa9909d@vivo.com>
Date: Mon, 29 Jan 2024 10:21:36 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mm:vmscan: shrink skip folios in the exiting task
To: Matthew Wilcox <willy@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
 linux-kernel@vger.kernel.org, opensource.kernel@vivo.com
References: <20240124124308.461-1-justinjiang@vivo.com>
 <ZbEOrim8tjMXB5os@casper.infradead.org>
 <726d018c-01d0-48b3-870b-c63fb06eb0d0@vivo.com>
 <ZbX1uEbrN-lwnnaW@casper.infradead.org>
From: zhiguojiang <justinjiang@vivo.com>
In-Reply-To: <ZbX1uEbrN-lwnnaW@casper.infradead.org>
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?R2s4Rk5rRTN6dVVtanFJYTBIUHIyeFFzOGFEOGtOWHg1K1hhQjM5R2lSdmRZ?=
 =?utf-8?B?Ty8yZUZpVlpOZGhSOXliTzF0VE9zU29CWUo2NXQ5czRBVzE3YVVsdi9SSDFY?=
 =?utf-8?B?OVAvMCtuTmNxa3JMWlNtb3pTTVdxdDJoZEIxR0tDQ0JsaFVSNHVzQ0NtdHlr?=
 =?utf-8?B?aGhHZHlGTm9jSGdCaU5nb0RTY21nMFkyUWNmUGxKeFdXTk43Z3BGR1BQYmVw?=
 =?utf-8?B?VmVoWFBiR0cyQ0E2RmNBV3R0RXVEQTZMRkdwbnhZeDdiUVhpY3BwbDdUb3FD?=
 =?utf-8?B?M0R5bWJPUk5rM1VIbEtlajZPTmVQY2lnbno1MlE5RUJYYVRsSUU5TGNLakF3?=
 =?utf-8?B?YWZMTkFERHRNR3BqVGhNSVZmTG5oN0piNit1L1BKTUoyZWVXVEZQWEtnV2Yv?=
 =?utf-8?B?eE9KOUg5SEdFTzNGMTdBWmZyWFhlTEorTmRoREU0enFWbUdGOFpFVkdMdlg3?=
 =?utf-8?B?SzBjUXJNejMyOXZZd0p6WFFjVHUvSFYySnZkcm9YMUtQc1RraVpTT0szWmRH?=
 =?utf-8?B?M3M4ZmtvZGxXU3NwdzdZamhyd3UwTEFnMzJWUW5UdXRNRGViY1JVNGs1L2Nq?=
 =?utf-8?B?eG1YYzROSlpZVDdzdW52Ynp0akZDWlVsYUpjOU9PVmdqSjY4YzNqRmVsWWZ3?=
 =?utf-8?B?RUNaVjdwR25kYldmZlIyRUNRdGt2NGpwcFVyQ1UrajhFYkxBM0RPVlNqWUVh?=
 =?utf-8?B?bVBxL3phQklPS3dEVjNXRXBDbVpHOVZZTWJHYmxpSnJQeXRWZ0JPVlh3VWc5?=
 =?utf-8?B?ZGZHRnZTSTJNL0I1KzF4czUxTGVNckhaNmtXRnBTYzZGRGZwWUJMekZtWGhK?=
 =?utf-8?B?WnhCdmZwalQ3UGNSbTR0eHJqYTBjMEg5dVFyNlFWNGp1WUFSTDZ0Z3I3V1ZN?=
 =?utf-8?B?dWNMdW91Z0V5L01YM3FvSlJUTFNBbTlYMm5oeVl3WkttbzExZ3lLZVFoVDlT?=
 =?utf-8?B?VGNCTjhFSzZLTXVwaDdKbDlWVE94dU5rRWpKVjM2WkZtS2NJYng5dm1DOGh1?=
 =?utf-8?B?QU9PcnVMbUpiYno3ZWcwU21FdFd0aHhJTFJ2RlZJZ3Rub1VhQXNJZ05kcUNL?=
 =?utf-8?B?blpFTFg4N3hIYkdZdSs3Sk5abVJCQmFoQ3dvemllYXp3UEZyYXVzc3VTS1p5?=
 =?utf-8?B?djZ4SCtZbFF3RCtIKytoR3JjVmV6ZUg3bmpmQk9iaHp1MWxvRHdLOEd3RFhR?=
 =?utf-8?B?bWJNZmhheStvMTFXQ09vVEVwdjlxZzB0L2FCa04wNlRvRnBGRlJRSy9VU3dC?=
 =?utf-8?B?MGx4am5lRmdIeHVPMHdFU05rWWpQenhScmZwTE8wUTlvNG9aeVZ5L0NWSEl6?=
 =?utf-8?B?NGEvS3IzbXEvbzc0emFSUHQ3Tk1tSVFtTm51NjhxL0V2R3ZOUzlyY3ZNcEJy?=
 =?utf-8?B?MkR5NWdla293U0x2V2JhSlhzQVMxY3RsU1RFeFA3clpETkxNVVBzNy95Sk44?=
 =?utf-8?B?bWpvS2NKUGVMVXpJWTBnMHdYV3U1THhZcVh4NDFwS2h1cUk0U2NWbVJFVXNW?=
 =?utf-8?B?R2RrYW1qckNnc1FUZ0ZpclVOMzIyNVNtOVBHWk4wZXorQUJQL3BOeEJ0SzEv?=
 =?utf-8?B?bXpjRnk2a0Y3Qmx0TlJnazJjdzNnU2s0b0V4RnpSNU9KSDdJeHJIUXBCUllv?=
 =?utf-8?B?OXN1TDhPVXhhalhyM0hCbDlPMkk4cUFYTTdrb3ZVRDVzcHYvV011NGVQTlI4?=
 =?utf-8?B?MC96WnFIRjJSR0ovd29FRExYMzdtM2p5OFZ0cWZINTNNL0xKVi9HRDdYd1hY?=
 =?utf-8?B?N3pIeUlLT3pnMTE2VDhOUytIRUpFSkZtWjhmTk5EMDdpQzZKOWNjUCthS3R6?=
 =?utf-8?B?MTZLeitBNEJ3aHhta3ZWN0xscWdMT3NHc2MxbnFJV1dOSVl2NkRwNlhiNDh4?=
 =?utf-8?B?eEg0RFk5VnZacHphd0JobXRjTWgyNXlqREdRTmI3QnNyenZPb0dNb01jSWJJ?=
 =?utf-8?B?aTQ2NmtlRzlBNTBSSU4wQVMyK2hPeDVZVk9jSDNBeXFlc0lJZXNSRVhHMStU?=
 =?utf-8?B?cU9CVlRiYUxCWGk3VHZCTmllSVRuR0FiYVg1TEhaYXI0OVdMNHZodlFrWmo3?=
 =?utf-8?B?NGtSWTNJOFMvNmpPenlNUUs2TkxyaUd5bDBsUVpXWWNMd3lKdGEyUDA4and6?=
 =?utf-8?Q?0mWz/vp3mmW86KBWUMtCKDtJ1?=
X-OriginatorOrg: vivo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b06294e6-f7a9-4121-c117-08dc20710645
X-MS-Exchange-CrossTenant-AuthSource: JH0PR06MB6849.apcprd06.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jan 2024 02:21:38.9024
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 923e42dc-48d5-4cbe-b582-1a797a6412ed
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: jdB9FT0OS7BxBxBlaQUdRDvl7Wp5aRw/mZt2QccC0glsvofdGtQnWHVO2DrUwSpOE83qE1+mEvfCw5E71dUUbw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: KL1PR06MB7430


在 2024/1/28 14:35, Matthew Wilcox 写道:
> On Thu, Jan 25, 2024 at 09:34:53AM +0800, zhiguojiang wrote:
>>>> In the scenarios of the low memory system and mutil backed-applications,
>>>> the time-consuming problem caused by shrinking the exiting task's folios
>>>> will be more severe.
>>> What testing have you done of this patch?  How often does it happen?
>>> Are there particular workloads that benefit from this?  (I'm not sure
>>> what "mutil backed-applications" are?)
>> 1 Yes, this patch has been tested.
>>
>> 2 When the exiting tasks and shrink_inactive_list occur at the same time,
>>     the folios which shrink_inactive_list reclaims may be the exiting tasks's
>> folios
>>     in lruvecs. And when system is low memory, it more likely to occur,
>> because
>>     more backend applidatuions will be killed.
>>     The shrink_inactive_list reclaims the exiting tasks's folios in lruvecs
>> and
>>     transforms the exiting tasks's anon folios into swap memory, which leads
>>     to the increasing load of the current exiting tasks.
> Ah, we're talking about an OOM scenario.  OK, that makes some sense.
> You should have mentioned that in the patch description.
Hi,

1 Ok, I will update a more comprehensive description in next version.

2 I think this issue can occur not only in OOM scenario, but also in
   normal task exit scenario. So:

1) if (unlikely(!atomic_read(&vma->vm_mm->mm_users))) represents
   the scenario where the task exits normally.

2) if(test_bit(MMF_OOM_SKIP, &vma->vm_mm->flags)) represents the
    OOM Reaper scenario.

3 MMF_OOM_SKIP can only represent OOM scenario and cannot represent
   normal task exit scenario, as when MMF_OOM_SKIP is set in normal
   task exit scenario, the memory folios of the task have already been
   released. And the shrink_inactive_list should recognize these lru folios
   in exiting task before this exiting task releases its memory folios.

     tlb_gather_mmu_fullmm(&tlb, mm);
     /* update_hiwater_rss(mm) here? but nobody should be looking */
     /* Use ULONG_MAX here to ensure all VMAs in the mm are unmapped */
     unmap_vmas(&tlb, &mas, vma, 0, ULONG_MAX, ULONG_MAX, false);
     mmap_read_unlock(mm);

     /*
      * Set MMF_OOM_SKIP to hide this task from the oom killer/reaper
      * because the memory has been already freed.
      */
     set_bit(MMF_OOM_SKIP, &mm->flags);
>> 3 This patch can alleviate the load of the tasks exiting process. This patch
>>     can make that the exiting tasks release its anon folios faster instead of
>>     releasing its swap memory from its anon folios swap-in in
>> shrink_inactive_list.
>>
>> 4 "mutil backed-applications" means that there are a large number of
>>      the backend applications in the system.
>>
>> Thanks
>>> And I do mean specifically of this patch, because to my eyes it
>>> shouldn't even compile.
>> Has been tested.
> That's odd.  I thought GCC used to complain about
>
> 	long x = 0x100000000;
>
> but it does the right thing.  Except on 32-bit where it'll say
> "warning: integer constant out of range".
>
> In any case, the number you chose is not going to work on 32-bit systems
> or on ARM or x86.  It conflicts with protection keys on x86 and MTE on
> ARM.
You're right, I overlooked the situation with the 32-bit system.
> We can do it without any new magic numbers.  I'm not sure this is a
> great approach, but this should work:
>
> +++ b/mm/rmap.c
> @@ -840,6 +840,12 @@ static bool folio_referenced_one(struct folio *folio,
>          int referenced = 0;
>          unsigned long start = address, ptes = 0;
>
> +       /* Skip this folio if it's mapped by an exiting task */
> +       if (test_bit(MMF_OOM_SKIP, &vma->vm_mm->flags)) {
> +               pra->referenced = -1;
> +               return false;
> +       }
> +
>          while (page_vma_mapped_walk(&pvmw)) {
>                  address = pvmw.address;
>
I agree with your point of view. I think this is currently the best 
solution,
   but I think it also needs to be added with:
if (unlikely(!atomic_read(&vma->vm_mm->mm_users)))

Please help review it again.

Best Regards.