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: <e3044d46-3b38-dc2e-b8d2-8ec1033f85e7@intel.com>
Date:   Wed, 19 Jul 2023 10:28:41 +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>
 <438d6f6d-2571-69d9-844e-9af9e6b4f820@intel.com>
 <CAJD7tkYWH8umBFgmxPmeOkRF=pauVW=MvyyN+z17XMHN+q8JKg@mail.gmail.com>
From:   Yin Fengwei <fengwei.yin@intel.com>
In-Reply-To: <CAJD7tkYWH8umBFgmxPmeOkRF=pauVW=MvyyN+z17XMHN+q8JKg@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?QXVmZFc2akJKNEFlM0RrY2x3aGVHNCtaRVl5dXlRT1JBTkppVytXMjZYd3Jn?=
 =?utf-8?B?emQySVgyZ0lrOVJqK3YrMFNBZmlXNzhza0QvNkNSUVFmUVRXMkJGK2pNY1lM?=
 =?utf-8?B?aDlOdnpYWUIzdEMrTTF4blZoRTdHdGlvSmt6dmp5SjE2TGpkN0FKbklHTVNT?=
 =?utf-8?B?Q3FORUh6VndudUtRQit0QWpsVzJNNXNuZlArc2dZZW9SRi9OZm9ZWmxUeEFW?=
 =?utf-8?B?K3lUejFMbVhDcVlMSjdoRUxqN1NsL0ROeUhVUHprNFhvNzYrWjY4d0RvblZL?=
 =?utf-8?B?VEg3RGQ4YlF3WnlYRFd2S1ZCVHc1UVR6R3pHMHJzOXFtVnNDejVBVUVESXZI?=
 =?utf-8?B?RzBSREJESVVDMHkrbk1LbEp5YUxqeWxPM2FhbXRRUmlOb1Q2V0tTVENjMVhs?=
 =?utf-8?B?Y0ZqMEJKT3dJOGRPNVRqa2RDRmszdVBYNnNZaFZ4MEFkUjc4OEdGb3lJZ3Y3?=
 =?utf-8?B?anh0U0Z4YUtoN3hQQzh3U2RaKzRSeExoTlZURW5DMGtBWmx1UHVEYVZVeThj?=
 =?utf-8?B?NnYrMGRJbEx2RkkxUVVud3g4Yy93YWN0VDRkYmY3S2gzeDBMdUJNV1N1ZFVv?=
 =?utf-8?B?c1NRcFRMSDRLbTlmSGoxQ2NyYkFMYWQvMFpXR0EvRE15blFFOGNwTFBVTFZu?=
 =?utf-8?B?YWY2a2FKL1V4SDZadzAwNVArKzlMdjdzQStiV2xlajJhS2Q4RVNhaldPcE1n?=
 =?utf-8?B?NUVmU3h6eHBmZ0d5TnpnWjZ0Tm0wQWRkeGxOY3hiU1QxemxiQUtOSWZ2QS9H?=
 =?utf-8?B?aDZLVnA3SzJxdmFqRGJJa1ZSbDk0bzAwZ2tQbGg2ZXZWUUwxMzVXQ21XNEtH?=
 =?utf-8?B?L0lpRXhDd2psRllwbGJ2NEhaTVR3QlA3Y1AyODhISURyelZpTytDQjVGT2Mv?=
 =?utf-8?B?Z3JWZ2NGakMrVVZna0tiK3RhWGd3V3NlQ2lGbjNCcWtLWndTYkY2QU1OWm1m?=
 =?utf-8?B?T3VjWlRyZnoyYXVqa1RhRG5vdlFVSmpFcHJxaHc1clpBYkhaVGF3OGVKTDdy?=
 =?utf-8?B?QmVhS0NDb2NXMmNHUzUzaFN0N2xZcEgzUjVXaGRnVXgyZGIrQXdEMlRYd2VQ?=
 =?utf-8?B?Zjd4ajZkMnR3WHFTUkErU1ZGdWgycWI1c09Za2ZHMTFYeWVqTkJIdEtxcDhr?=
 =?utf-8?B?UFNOeURVcnJqemNhSjVqeG1QczFlZEp3STNmTWxFVTZGOCtoUkVsM3BvUmRu?=
 =?utf-8?B?M2dmdUZPTDREcTdzb2xQYndLZEpNWmZ5dCttUTJ2eTdISEw2S21FcTNqSVZR?=
 =?utf-8?B?ZVRsamZRandydDVBQUJpbzYzdnRPdUdrVlRKNldBeTJIR212TnRuaTRwaU1N?=
 =?utf-8?B?K3RtRmY5V3Q0VlNYZEp0Qm9NSzFqdjBlZ0VSRHJMa004bDJ3TGVPWmUvazRq?=
 =?utf-8?B?bW9zZmpXV214WVMxUER6blhsTzJDUzdvTzdwZUlNRDkraXVleUJaMnQ1dnpo?=
 =?utf-8?B?SHZIMTRiRHV1Vmk1LzJPenlaNXA5OFN5UjBubkIvMytkdUZRdDE2L3R2MlhR?=
 =?utf-8?B?Y3BySHNSaWFjd3FPa2Z5M2U3MTVXbkFEOVhZaVF4Ymt2TVRpMkxYNENkdWJv?=
 =?utf-8?B?M1NNQm5SK1UvWUZYNFdzamFHRVczakt0VXkxeTdUQ2FlQ2pSbFVJakpEREc2?=
 =?utf-8?B?aEVjZ2dvOFdLQmthRXYrM0FoaWJpMGkvd3d6STV5d3l3VGJhTFg5RFB6bVlH?=
 =?utf-8?B?RENsQlM0a1IyRzYzNXhjditJSmdxZWk1UmV6Y2duSkd6cGVQb3hlV2ROdStL?=
 =?utf-8?B?SWZ3VFhMNmpvT1lNN0ZwL3BUVHdBZUdOaG9xa1p2bDBiYkRFcmdTRmNqRUE3?=
 =?utf-8?B?L3RRcEhoK3JXbzIrOEdQTVduWmZxRlJTN3hKbjlkZysyM2lWZ2RLelFQS0Q4?=
 =?utf-8?B?UmpKSVZDRyttZk53Q09uK3lyRHJsUnhHb3dKdnFIUVYzTW01SVNvT2V6RVlJ?=
 =?utf-8?B?ZTI5Q0NNNkhwM2pTaVVPeWg2NzF2R3dUZFpaLzVseXZtcFFVbVB6Vjk3Qk5h?=
 =?utf-8?B?K0FwNE93dEZyVm1ENHl4UW82SllCOFRHVFhzQk0vVkZqcTZEWStBaWJzYThO?=
 =?utf-8?B?YWtTN1h3YzliRTJJOXJ3THdKMU1VV3lNd0VkL0UxUi9oQTFMMW55TS8wdXNv?=
 =?utf-8?B?KytuVTJaRnhFTjY2QmFIWFhhY3JXS2RhYVZhMFNlWndVQ1ZtOVF4TzdMeVJn?=
 =?utf-8?B?OHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: dacaff49-d753-4fd4-8d6b-08db87ffdcca
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:28:39.4446
 (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: aUewhgczVvGvD7BWP/toPS2CQHteYsj9NNBMsMW/zj6xsZN+A4zxCUSPZ/TCGabnKUvNNUByXFV7cX0OIw52kg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5094
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,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,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:22, Yosry Ahmed wrote:
> On Tue, Jul 18, 2023 at 7:10 PM Yin Fengwei <fengwei.yin@intel.com> wrote:
>>
>>
>>
>> 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.
> 
> IIUC, clearing PG_lru is an atomic lockless operation to make sure no
> one else is isolating the folio, but then you need to hold the lruvec
> lock and actually delete the folio from the lru to complete its
> isolation. This is my read of folio_isolate_lru().
> 
> Anyway, whether we rely on the lruvec lock or memory barrier +
> operation ordering doesn't make a huge difference (I think?). The code
> seemed to work with the latter before mlock_count was introduced.
> 
> If we decide to go with the latter, I can integrate the fix into the
> refresh of my mlock_count rework RFC series (as it would be dependent
> on that series). If we decide to go with the lruvec, then it can be
> done as part of this series, or separately.
Let's wait the response from Huge and Yu. :).

> 
> Thanks.
> 
>>
>>>
>>>>
>>>>
>>>> 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
>>>>>>>>>