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: <25faaf8f-6eb6-5c8f-de5e-31aedca61d34@intel.com>
Date:   Sat, 29 Jul 2023 00:14:47 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.13.0
Subject: Re: [RFC PATCH v2 2/4] madvise: Use notify-able API to clear and
 flush page table entries
Content-Language: en-US
To:     Yu Zhao <yuzhao@google.com>
CC:     <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>,
        <akpm@linux-foundation.org>, <minchan@kernel.org>,
        <willy@infradead.org>, <david@redhat.com>, <ryan.roberts@arm.com>,
        <shy828301@gmail.com>
References: <20230721094043.2506691-1-fengwei.yin@intel.com>
 <20230721094043.2506691-3-fengwei.yin@intel.com>
 <CAOUHufY2tdO0JNTiY=RzHitR7CB1cM5kA=7bd6nbCUW6KM_OVA@mail.gmail.com>
 <05bc90b6-4954-b945-f0d8-373f565c1248@intel.com>
 <CAOUHufYJE40wcT4HTYFJ_7X5=my3OPbMyMBt+QNZdByuL6j58Q@mail.gmail.com>
 <feb58221-e481-3d71-8707-6ffe90158b66@intel.com>
 <CAOUHufa9rFS-VjbCRG6KGjb4YKOZioH=dLdTyFLWqEFePoL+wQ@mail.gmail.com>
 <0843fb4d-ab0b-2766-281c-ef32b6031dd7@intel.com>
 <CAOUHufaZCCVr1C19tZH=+wmWN7pPoJMLuivr=e90Akj29X1evw@mail.gmail.com>
From:   "Yin, Fengwei" <fengwei.yin@intel.com>
In-Reply-To: <CAOUHufaZCCVr1C19tZH=+wmWN7pPoJMLuivr=e90Akj29X1evw@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?OHU1N0VpSGNoMWRUc0pack9zUGkrT2x2Snpzdmd4VHhsRVhtTDBPYTEzUlI2?=
 =?utf-8?B?bDV1NWZVVVlOWng2SGtjRENPUFpTRlFLWU81MjhRZDJqTFFwMVZKbi9VbmJi?=
 =?utf-8?B?eEdCaFBRdExrWVBHYVVrM2ZpUmhGMXg4UDNacEIxNFJSckFzQTMya1oxU09L?=
 =?utf-8?B?anpxYktybHVFV3V2aGdHdi9DM1ZWK0xsRlQ1M0pPYzNZd1F5UTlBUEV5a1dG?=
 =?utf-8?B?c1hmUElZek5tRDJPVFR1Qyt0Qlg1dFpHN0JzeExjYXJ0bCsrVXlVTzhXYzhT?=
 =?utf-8?B?azB5aFN2QnNkM0hiMU80QW91UC8vQmVOS3FHY25wNnNwTmY5UjNlZEcya1FV?=
 =?utf-8?B?Y1l6eFZMR1QvTmVJaFo2d1dpS2F1QUdIUFIySTFlL2RaU0FQYXk3cElSc2o4?=
 =?utf-8?B?SUg2RkJNc1RCc0lmT1MzTlBlY3Rja1pDaHkzQ1M0TnlhWDlSUnpjU3E1YXh5?=
 =?utf-8?B?WnAzRW42Mm9Yc3o4MWFZbGk4K2FoZmZTbXppTjVmYmNOR0NKeVdmQ0Q1S2lj?=
 =?utf-8?B?eFZpM0xWd1ZvZHVzdndTdHlLQi9QYk1odHljUzFlM3FwUW9LalFEbGxoWkdr?=
 =?utf-8?B?ZXdYYUVhV0xVSzFpbjdwOGpwZGZnWk9jOHpPbHlkV0lCVzF6VVJEUnY0NHo4?=
 =?utf-8?B?bVJ3UGJKVGQ2VXBhUjlpSXZtOUl3WGJSOEVqVEpWd005elVrT1dBOEJURlBZ?=
 =?utf-8?B?Uzl4dk1OaVpmMFBVNjBEQkVBNnFvbXZNNW1FRHBYWk4vSmR0R09rWTdpRWVW?=
 =?utf-8?B?WWZ1Yi9SSG1nVHBNcWlOOThremZTUjAxL1lNZlNZZXZDYnVvVTFMOXBYemRT?=
 =?utf-8?B?cHlGaVk3R2Nuam16VDhRTFJpK3NhK0NJK0xTNG9KU0hxSUhKcU9uOEdaN0hL?=
 =?utf-8?B?ZHhtL2dqWStibFdFRTdCUVkra0ZPWis1VUUvN3laZG9VZWtwZy9LY2V0WnBV?=
 =?utf-8?B?dG9WSVYzUCszVDVDVEI5Z2g2eGMzb29mMHlvNHQrWmR2WVZJMm8yT3NYd2VV?=
 =?utf-8?B?TmZrbTVTNC9QcFJwQUdtODRNUGd4NXdxVkVXRzBuN0hHSnhlT09mSlEwRnpH?=
 =?utf-8?B?dkRTbjFjU1RNakJKRmUzb2FrYTA5QU00dXRFUUppUHJhVjFqaDhvL1JSWXpj?=
 =?utf-8?B?QkE5MjZST1MxemQvOXB1OUNEc0JHMEhTSGRXeDBLeEdjRm15THdLSGlwcFBW?=
 =?utf-8?B?bGdPazdzUVJrVUtMUDlPWUpHeFl5RlVrVjV0enVWMSs4dkllT3ZFRlVhblQ0?=
 =?utf-8?B?cWZqTUR3WUR2YXRoMm9KUW0wV3VjUVZScEpXSVlRTDFoY3FVMlArcmxzM0Vy?=
 =?utf-8?B?Mk5FcVdLaXJFYVVnQ3c4ejRWdCtybnhQd3FuMVVmSjNncjIxYUNqMkZ3YWow?=
 =?utf-8?B?QjBEL1RlMGluU3pIWU5SMU11dzZGRFkyKzFUWTVZVForTlpyYlFZTXhqSUVE?=
 =?utf-8?B?RlpYOC80RU9xY0lIajdPN1Q4S2wrZ1MzTllOSDl4NVg5TzlNdjRPOGp0TEpq?=
 =?utf-8?B?Qmg1b0NiVUd5OFhUd3RqampJTlM5OTVmc1R1bmVzKzJ3ZDl6bXJjK253dGtI?=
 =?utf-8?B?NWdhZEtNdGZrenlYRTVtWWVmN1FKdjFPQUhLYUx1Zy94TzU2d3E3Yi82bjUy?=
 =?utf-8?B?aERsT2cwNkt4WVg5Rk1weHZLNjRGUGh5TCswcFRFUFp4YjlqUHZrL3dUM0J4?=
 =?utf-8?B?VWFPcGZ0S2ZmK1d0VlpQL3FyQTRXOHFYbXJaUHlXMDdKTjhKZlp3QXk3ZVpx?=
 =?utf-8?B?M05zRnFnNXZwVTAxWnhLLzdJRFpDMFRNTzJqbFhBZzA0MStjMUV5enRvc3Fn?=
 =?utf-8?B?WXR2citCeEhIQy9mR3krd05Ob2psQzVxYnI1cm9kODdmdFdOZ2t5SXZVU0Vx?=
 =?utf-8?B?YWVDQUJwTFo0eXFubEhGaW0yUndXSlpHUG1Ka0hSend1TkpPQlF0MWlDYTF0?=
 =?utf-8?B?ZTBaUFFFYmp1Qml4VEU4SUd0QWJGZExVbzZWQjM0U2ZTZ2owbTFXVDdJRFZr?=
 =?utf-8?B?TE5xdTdGYUdBNC9CQUhMQ2VrZWQyNGliRzVxK0p1aWxOM3hKVU5IVGhzVzdJ?=
 =?utf-8?B?R0M3eVZyaFZodlozY3JWVmE0UmR3UVFLckpEekJOTHIwVGhUMVNoYjR3Smwx?=
 =?utf-8?B?ejk3Q29ZMFVCRXJvT3pDVXdqTDBiS2NwUDh2czk5TTcvQjRxN2lhTEFXb3Zr?=
 =?utf-8?B?VEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9f89d293-14ee-447e-0025-08db8f85c94e
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4820.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Jul 2023 16:14:57.8358
 (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: fZsXEXGZoftv5LyoLN5KU1TinjyaIVgy0oQ8ATPwGVN1dN7GeolMyj7SBCfYyN2VbI/vJaMX1AKJGn2Efmr/oQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CYYPR11MB8407
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/27/2023 11:28 AM, Yu Zhao wrote:
> On Wed, Jul 26, 2023 at 12:21 AM Yin Fengwei <fengwei.yin@intel.com> wrote:
>>
>>
>> On 7/26/23 13:40, Yu Zhao wrote:
>>> On Tue, Jul 25, 2023 at 10:44 PM Yin Fengwei <fengwei.yin@intel.com> wrote:
>>>>
>>>>
>>>> On 7/26/23 11:26, Yu Zhao wrote:
>>>>> On Tue, Jul 25, 2023 at 8:49 PM Yin Fengwei <fengwei.yin@intel.com> wrote:
>>>>>>
>>>>>>
>>>>>> On 7/25/23 13:55, Yu Zhao wrote:
>>>>>>> On Fri, Jul 21, 2023 at 3:41 AM Yin Fengwei <fengwei.yin@intel.com> wrote:
>>>>>>>>
>>>>>>>> Currently, in function madvise_cold_or_pageout_pte_range(), the
>>>>>>>> young bit of pte/pmd is cleared notify subscripter.
>>>>>>>>
>>>>>>>> Using notify-able API to make sure the subscripter is signaled about
>>>>>>>> the young bit clearing.
>>>>>>>>
>>>>>>>> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
>>>>>>>> ---
>>>>>>>>  mm/madvise.c | 18 ++----------------
>>>>>>>>  1 file changed, 2 insertions(+), 16 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/mm/madvise.c b/mm/madvise.c
>>>>>>>> index f12933ebcc24..b236e201a738 100644
>>>>>>>> --- a/mm/madvise.c
>>>>>>>> +++ b/mm/madvise.c
>>>>>>>> @@ -403,14 +403,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
>>>>>>>>                         return 0;
>>>>>>>>                 }
>>>>>>>>
>>>>>>>> -               if (pmd_young(orig_pmd)) {
>>>>>>>> -                       pmdp_invalidate(vma, addr, pmd);
>>>>>>>> -                       orig_pmd = pmd_mkold(orig_pmd);
>>>>>>>> -
>>>>>>>> -                       set_pmd_at(mm, addr, pmd, orig_pmd);
>>>>>>>> -                       tlb_remove_pmd_tlb_entry(tlb, pmd, addr);
>>>>>>>> -               }
>>>>>>>> -
>>>>>>>> +               pmdp_clear_flush_young_notify(vma, addr, pmd);
>>>>>>>>                 folio_clear_referenced(folio);
>>>>>>>>                 folio_test_clear_young(folio);
>>>>>>>>                 if (folio_test_active(folio))
>>>>>>>> @@ -496,14 +489,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
>>>>>>>>
>>>>>>>>                 VM_BUG_ON_FOLIO(folio_test_large(folio), folio);
>>>>>>>>
>>>>>>>> -               if (pte_young(ptent)) {
>>>>>>>> -                       ptent = ptep_get_and_clear_full(mm, addr, pte,
>>>>>>>> -                                                       tlb->fullmm);
>>>>>>>> -                       ptent = pte_mkold(ptent);
>>>>>>>> -                       set_pte_at(mm, addr, pte, ptent);
>>>>>>>> -                       tlb_remove_tlb_entry(tlb, pte, addr);
>>>>>>>> -               }
>>>>>>>> -
>>>>>>>> +               ptep_clear_flush_young_notify(vma, addr, pte);
>>>>>>>
>>>>>>> These two places are tricky.
>>>>>>>
>>>>>>> I agree there is a problem here, i.e., we are not consulting the mmu
>>>>>>> notifier. In fact, we do pageout on VMs on ChromeOS, and it's been a
>>>>>>> known problem to me for a while (not a high priority one).
>>>>>>>
>>>>>>> tlb_remove_tlb_entry() is batched flush, ptep_clear_flush_young() is
>>>>>>> not. But, on x86, we might see a performance improvement since
>>>>>>> ptep_clear_flush_young() doesn't flush TLB at all. On ARM, there might
>>>>>>> be regressions though.
>>>>>>>
>>>>>>> I'd go with ptep_clear_young_notify(), but IIRC, Minchan mentioned he
>>>>>>> prefers flush. So I'll let him chime in.
>>>>>> I am OK with either way even no flush way here is more efficient for
>>>>>> arm64. Let's wait for Minchan's comment.
>>>>>
>>>>> Yes, and I don't think there would be any "negative" consequences
>>>>> without tlb flushes when clearing the A-bit.
>>>>>
>>>>>>> If we do end up with ptep_clear_young_notify(), please remove
>>>>>>> mmu_gather -- it should have been done in this patch.
>>>>>>
>>>>>> I suppose "remove mmu_gather" means to trigger flush tlb operation in
>>>>>> batched way to make sure no stale data in TLB for long time on arm64
>>>>>> platform.
>>>>>
>>>>> In madvise_cold_or_pageout_pte_range(), we only need struct
>>>>> mmu_gather *tlb because of tlb_remove_pmd_tlb_entry(), i.e., flushing
>>>>> tlb after clearing the A-bit. There is no correction, e.g., potential
>>>>> data corruption, involved there.
>>>>
>>>> From https://lore.kernel.org/lkml/20181029105515.GD14127@arm.com/,
>>>> the reason that arm64 didn't drop whole flush tlb in ptep_clear_flush_young()
>>>> is to prevent the stale data in TLB. I suppose there is no correction issue
>>>> there also.
>>>>
>>>> So why keep stale data in TLB in madvise_cold_or_pageout_pte_range() is fine?
>>>
>>> Sorry, I'm not sure I understand your question here.
>> Oh. Sorry for the confusion. I will explain my understanding and
>> question in detail.
>>
>>>
>>> In this patch, you removed tlb_remove_tlb_entry(), so we don't need
>>> struct mmu_gather *tlb any more.
>> Yes. You are right.
>>
>>>
>>> If you are asking why I prefer ptep_clear_young_notify() (no flush),
>>> which also doesn't need tlb_remove_tlb_entry(), then the answer is
>>> that the TLB size doesn't scale like DRAM does: the gap has been
>>> growing exponentially. So there is no way TLB can hold stale entries
>>> long enough to cause a measurable effect on the A-bit. This isn't a
>>> conjecture -- it's been proven conversely: we encountered bugs (almost
>>> every year) caused by missing TLB flushes and resulting in data
>>> corruption. They were never easy to reproduce, meaning stale entries
>>> never stayed long in TLB.
>>
>> when I read https://lore.kernel.org/lkml/20181029105515.GD14127@arm.com/,
>>
>> my understanding is that arm64 still keep something in ptep_clear_flush_young.
>> The reason is finishing TLB flush by next context-switch to make sure no
>> stale entries in TLB cross next context switch.
>>
>> Should we still keep same behavior (no stable entries in TLB cross next
>> context switch) for madvise_cold_or_pageout_pte_range()?
>>
>> So two versions work (I assume we should keep same behavior) for me:
>>   1. using xxx_flush_xxx() version. but with possible regression on arm64.
>>   2. using none flush version. But do batched TLB flush.
> 
> I see. I don't think we need to flush at all, i.e., the no flush
> version *without* batched TLB flush. So far nobody can actually prove
> that flushing TLB while clearing the A-bit is beneficial, not even in
> theory :)

I will just send the fix for folio_mapcount() (with your reviewed-by) as
it's bug fix and it's better to be merged standalone.

The other three patches need more time for discussion.

Regards
Yin, Fengwei