Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <438d6f6d-2571-69d9-844e-9af9e6b4f820@intel.com>
Date:   Wed, 19 Jul 2023 10:09:53 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.11.0
Subject: Re: [RFC PATCH v2 3/3] mm: mlock: update mlock_pte_range to handle
 large folio
Content-Language: en-US
To:     Yosry Ahmed <yosryahmed@google.com>
CC:     Yu Zhao <yuzhao@google.com>, <linux-mm@kvack.org>,
        <linux-kernel@vger.kernel.org>, <akpm@linux-foundation.org>,
        <willy@infradead.org>, <david@redhat.com>, <ryan.roberts@arm.com>,
        <shy828301@gmail.com>, Hugh Dickins <hughd@google.com>
References: <20230712060144.3006358-1-fengwei.yin@intel.com>
 <20230712060144.3006358-4-fengwei.yin@intel.com>
 <CAOUHufYef--8MxFettL6fOGjVx2vyZHZQU6EEaTCoW0XBvuC8Q@mail.gmail.com>
 <CAOUHufZ6=9P_=CAOQyw0xw-3q707q-1FVV09dBNDC-hpcpj2Pg@mail.gmail.com>
 <40cbc39e-5179-c2f4-3cea-0a98395aaff1@intel.com>
 <CAOUHufZHyEvU-c2O6B6stM_QVMxc22zV4Szn52myYqjdZvptUA@mail.gmail.com>
 <16844254-7248-f557-b1eb-b8b102c877a2@intel.com>
 <CAJD7tkYAkVOE2caqEj_hTmm47Kex451prBQ1wKTRUiOwnDcwNA@mail.gmail.com>
 <b995e802-1500-6930-79d0-8cc4bfe89589@intel.com>
 <CAJD7tkZtHku-kaK02MAdgaxNzr9hQkPty=cw44R_9HdTS+Pd5w@mail.gmail.com>
 <CAJD7tkZWXdHwpW5AeKqmn6TVCXm1wmKr-2RN2baRJ7c4ciTJng@mail.gmail.com>
 <208aff10-8a32-6ab8-f03a-7f3c9d3ca0f7@intel.com>
 <CAJD7tkYT6EZMwit8C9MTftUxMmuWtn2YpZ+NSVhy0xVCYuafsg@mail.gmail.com>
From:   Yin Fengwei <fengwei.yin@intel.com>
In-Reply-To: <CAJD7tkYT6EZMwit8C9MTftUxMmuWtn2YpZ+NSVhy0xVCYuafsg@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?cFVjQ2w3WWEvQXR3WGt3dkFqN3hKV0xrZHVOTXN0TkVYVld6UDU0QVZ5Z2lE?=
 =?utf-8?B?RmV1QmJSSFRnbVFhMjdGS0c0ZkRtT2J6RTdaRTZUNEZWU2Z6UVA4bUpzUFZi?=
 =?utf-8?B?MjhMeEt2dkJxeC9HNCtSb3VmOU9qYTRJN0VPWDFubHpuZjhMOEMyTlIyR3l2?=
 =?utf-8?B?UkdEVnZwcTErdE54OEJITE8vT25QOVpjdzJxQ25GV3M3emw3aDlBcW84c1hs?=
 =?utf-8?B?N05sc2tWcnFUeDBGMFVqUTRmN0hQL1o3dWJrbVBUdFNJWERxOVhZWk1EYzV2?=
 =?utf-8?B?bmJKa3R5TzBoL0tCNkQrUzBKN2owZy9xUmJhYzFlVzNONG5qNXFSTlZlNk5t?=
 =?utf-8?B?SGgwQVY1MStNSWpLNHV3VEFVeDdXSC9CTDRBaEVkNHNhSUJhRVFIS2E1TVdC?=
 =?utf-8?B?blRPbnlSUHlnNWxkcm1VUnE2Q0lKdXlzajJHU3YvUVVDTC9zUDBCbFFqUlVL?=
 =?utf-8?B?WGhyTFF3SmxTRDRMTEgwZWtMUVNOb2paNGNlU2RGWG00SFRaLzZhOVBVSlkv?=
 =?utf-8?B?NFpOL1B6eWwrQ2g4eW5TY3lwK25Hc1Q1ckZOcndxS0YvT1RVNk4yb1piRkYw?=
 =?utf-8?B?eWlBb0RiUytVWVQ1Z0E0SGJzeVkvY0Q2WndETktrQ3l3SFJRb3JobWNBSFFi?=
 =?utf-8?B?VnhSeVRuWnlHN1lad3NHZWx3QjdzY1F2ZDVJSjZESmtyak0wcFF0Uml6VFdC?=
 =?utf-8?B?Wi8rbkx4RjNGNzd3ZXVObmFTVHMzUUdXYWpzVzZubmNnUWN2SGNDQk1Va29s?=
 =?utf-8?B?RkI0WWlYQjUyS3JOT1l4QUhicjQvSmZUTW9oT29pZ01EZnYrVUplVDYxNjJD?=
 =?utf-8?B?OXpuM0FrUDVybVBoOGFwSy80U1ViV1dGYmo5RXFWaGE3OFF5Nkl5eFNPbUx3?=
 =?utf-8?B?dHBmcVpVY0ZLY25RYTByYWtsVnJkd3JwdVhVc1E5TlUzd1cyZ2F1Q0NoNWxi?=
 =?utf-8?B?dHNWN1NBM3kwNkhRVVZIc3pKR0RqVTNHQkJocXYyb0dXYk5ZZUpMZnkzem5N?=
 =?utf-8?B?T1U0WS9OMmJSZUJ1OWcvdi9kUlU2OHJ0QXFaR09zeGdseTIrVGR4dEdXczhh?=
 =?utf-8?B?RFJOUzhkbFNJNzdkaEJQbzNaMUxobFd6UGR2MERoY0RlYjdQekhFMGdIeC9o?=
 =?utf-8?B?TU1uS3NIUUxDK0kyNjhGSWwweXhkNldNUnI4TFJ1aDRSaHdjWmdxWXJJVGNN?=
 =?utf-8?B?S1BhTkVoKzU4SG5WdU9KemlweGtDTFFETDl1WTBMT2tnZ2llemF3UHhoNElE?=
 =?utf-8?B?ajF6czdxMjBCdkxJalBzL1dwSTdJTzZOSTdha01aRldsWjlpWFdHSEJuZjNx?=
 =?utf-8?B?VTJuMExiUkRyU0VwTlVsRE5RVTJPNWlSczZkU1dxR3FPN24rbEVXWGV4dXZr?=
 =?utf-8?B?TTBiRVVsZHpiejlzaHUza00vUnlkZU9TcjZIUzJ4dHpxUFFiWUpyTnM0Y1pG?=
 =?utf-8?B?ZGRidnZicitDSVR2MHZKMVQ5cnZUbUdtMXdsVFlXYzlubW8xc0R5YlR1T3Vt?=
 =?utf-8?B?TXhDRnBKekJValNwMkFWL1g4UC9XM1FuS0hEN1VheG9yY3Jka0VjYlI2c1cx?=
 =?utf-8?B?TTNkN1czR2l2bHI5bFJRcC9IU0NONUFIMDN5N0gremRGS1ZqNGNxaEh1OEhx?=
 =?utf-8?B?RGhLQ3dNRzFhWW9GZVZLWnZZQjZRNVZaM1pnOE9nc0NtZHc5bkZlRndwZnNj?=
 =?utf-8?B?dnZBaG1FV1EwOUc1bHJEMWE5Si84V2tDQXNyQmpiTW4zUXpLMTJacEFRL0Ja?=
 =?utf-8?B?UXRHWXRZT1lESHJPVDJ6NkpHUEZ3TjhuZDBDWDJiTUtqenRFSDhlaFBRbENR?=
 =?utf-8?B?em8vT2dYRmZvb3Y3MkZhQTBlRlhRbjREbExrRTNlSFVkT1VkbnM4VnplcG12?=
 =?utf-8?B?QWhPNVpwVVZEMDQyOHBHTEdVV2NIQlhYZU91b3F5SlZrSG5aYVNsZjRTOStW?=
 =?utf-8?B?SFZMY1dRM1daaEQ4VFZiQTFhdUh6V1RVVXVmdUVUalowSmcxWnF3WGgrMTNh?=
 =?utf-8?B?UnNobGdnVXI0TUNwMzl1RmI0Tmt6TVZGQm85Z3FrOWRvWEJGL2JEc1pNN1lS?=
 =?utf-8?B?L2VYamFrNkROTTJVRHJMQm52QTFHRUpkSXM4aUtqOGtmR0NqSUZSSkxhczBr?=
 =?utf-8?B?eHlEQjRBWGlmeE9MRHBhN3pXUEkyalF4UElPdkJQV1NkVndJRDkrSXFkTE5K?=
 =?utf-8?B?NkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5ee32cd7-f1eb-437f-04fc-08db87fd3ce2
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4820.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jul 2023 02:09:52.3561
 (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: ZgcG0HKwlpnneHVA1iBgP+7TrJVUd0889ao6hUHR0EeuxH30MVqXlQzsyhPen9dtRpTDeFrUaqw4gvn1LrwK7Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5224
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 7/19/23 10:00, Yosry Ahmed wrote:
> On Tue, Jul 18, 2023 at 6:57 PM Yin Fengwei <fengwei.yin@intel.com> wrote:
>>
>>
>> On 7/19/23 09:52, Yosry Ahmed wrote:
>>> On Tue, Jul 18, 2023 at 6:32 PM Yosry Ahmed <yosryahmed@google.com> wrote:
>>>> On Tue, Jul 18, 2023 at 4:47 PM Yin Fengwei <fengwei.yin@intel.com> wrote:
>>>>>
>>>>>
>>>>> On 7/19/23 06:48, Yosry Ahmed wrote:
>>>>>> On Sun, Jul 16, 2023 at 6:58 PM Yin Fengwei <fengwei.yin@intel.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 7/17/23 08:35, Yu Zhao wrote:
>>>>>>>> On Sun, Jul 16, 2023 at 6:00 PM Yin, Fengwei <fengwei.yin@intel.com> wrote:
>>>>>>>>> On 7/15/2023 2:06 PM, Yu Zhao wrote:
>>>>>>>>>> There is a problem here that I didn't have the time to elaborate: we
>>>>>>>>>> can't mlock() a folio that is within the range but not fully mapped
>>>>>>>>>> because this folio can be on the deferred split queue. When the split
>>>>>>>>>> happens, those unmapped folios (not mapped by this vma but are mapped
>>>>>>>>>> into other vmas) will be stranded on the unevictable lru.
>>>>>>>>> This should be fine unless I missed something. During large folio split,
>>>>>>>>> the unmap_folio() will be migrate(anon)/unmap(file) folio. Folio will be
>>>>>>>>> munlocked in unmap_folio(). So the head/tail pages will be evictable always.
>>>>>>>> It's close but not entirely accurate: munlock can fail on isolated folios.
>>>>>>> Yes. The munlock just clear PG_mlocked bit but with PG_unevictable left.
>>>>>>>
>>>>>>> Could this also happen against normal 4K page? I mean when user try to munlock
>>>>>>> a normal 4K page and this 4K page is isolated. So it become unevictable page?
>>>>>> Looks like it can be possible. If cpu 1 is in __munlock_folio() and
>>>>>> cpu 2 is isolating the folio for any purpose:
>>>>>>
>>>>>> cpu1                                        cpu2
>>>>>>                                                 isolate folio
>>>>>> folio_test_clear_lru() // 0
>>>>>>                                                 putback folio // add
>>>>>> to unevictable list
>>>>>> folio_test_clear_mlocked()
>>>>> Yes. Yu showed this sequence to me in another email. I thought the putback_lru()
>>>>> could correct the none-mlocked but unevictable folio. But it doesn't because
>>>>> of this race.
>>>> (+Hugh Dickins for vis)
>>>>
>>>> Yu, I am not familiar with the split_folio() case, so I am not sure it
>>>> is the same exact race I stated above.
>>>>
>>>> Can you confirm whether or not doing folio_test_clear_mlocked() before
>>>> folio_test_clear_lru() would fix the race you are referring to? IIUC,
>>>> in this case, we make sure we clear PG_mlocked before we try to to
>>>> clear PG_lru. If we fail to clear it, then someone else have the folio
>>>> isolated after we clear PG_mlocked, so we can be sure that when they
>>>> put the folio back it will be correctly made evictable.
>>>>
>>>> Is my understanding correct?
>>> Hmm, actually this might not be enough. In folio_add_lru() we will
>>> call folio_batch_add_and_move(), which calls lru_add_fn() and *then*
>>> sets PG_lru. Since we check folio_evictable() in lru_add_fn(), the
>>> race can still happen:
>>>
>>>
>>> cpu1                              cpu2
>>>                                       folio_evictable() //false
>>> folio_test_clear_mlocked()
>>> folio_test_clear_lru() //false
>>>                                       folio_set_lru()
>>>
>>> Relying on PG_lru for synchronization might not be enough with the
>>> current code. We might need to revert 2262ace60713 ("mm/munlock:
>>> delete smp_mb() from __pagevec_lru_add_fn()").
>>>
>>> Sorry for going back and forth here, I am thinking out loud.
>>
>> Yes. Currently, the order in lru_add_fn() is not correct.
>>
>> I think we should move folio_test_clear_lru(folio) into
>>
>> lru locked range. As the lru lock here was expected to
>>
>> use for sync here. Check the comment in lru_add_fn().
> 
> Right, I am wondering if it would be better to just revert
> 2262ace60713 and rely on the memory barrier and operations ordering
> instead of the lru lock. The lru lock is heavily contended as-is, so
> avoiding using it where possible is preferable I assume.
My understanding is set_lru after add folio to lru list is correct.
Once folio_set_lru(), others can do isolation of this folio. But if
this folio is not in lru list yet, what could happen? It's not required
to hold lru lock to do isolation.

> 
>>
>>
>> Regards
>>
>> Yin, Fengwei
>>
>>
>>>
>>>> If yes, I can add this fix to my next version of the RFC series to
>>>> rework mlock_count. It would be a lot more complicated with the
>>>> current implementation (as I stated in a previous email).
>>>>
>>>>>>
>>>>>> The page would be stranded on the unevictable list in this case, no?
>>>>>> Maybe we should only try to isolate the page (clear PG_lru) after we
>>>>>> possibly clear PG_mlocked? In this case if we fail to isolate we know
>>>>>> for sure that whoever has the page isolated will observe that
>>>>>> PG_mlocked is clear and correctly make the page evictable.
>>>>>>
>>>>>> This probably would be complicated with the current implementation, as
>>>>>> we first need to decrement mlock_count to determine if we want to
>>>>>> clear PG_mlocked, and to do so we need to isolate the page as
>>>>>> mlock_count overlays page->lru. With the proposal in [1] to rework
>>>>>> mlock_count, it might be much simpler as far as I can tell. I intend
>>>>>> to refresh this proposal soon-ish.
>>>>>>
>>>>>> [1]https://lore.kernel.org/lkml/20230618065719.1363271-1-yosryahmed@google.com/
>>>>>>
>>>>>>>
>>>>>>> Regards
>>>>>>> Yin, Fengwei
>>>>>>>