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: <959095f6-8574-f5fc-812c-b0b9b9a3c101@intel.com>
Date:   Fri, 4 Aug 2023 15:36:05 +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: [PATCH 0/2] don't use mapcount() to check large folio sharing
Content-Language: en-US
To:     David Hildenbrand <david@redhat.com>,
        "Yin, Fengwei" <fengwei.yin@intel.com>, Yu Zhao <yuzhao@google.com>
CC:     Ryan Roberts <ryan.roberts@arm.com>, <linux-mm@kvack.org>,
        <linux-kernel@vger.kernel.org>, <stable@vger.kernel.org>,
        <akpm@linux-foundation.org>, <willy@infradead.org>,
        <vishal.moola@gmail.com>, <wangkefeng.wang@huawei.com>,
        <minchan@kernel.org>, <shy828301@gmail.com>
References: <20230728161356.1784568-1-fengwei.yin@intel.com>
 <3bbfde16-ced1-dca8-6a3f-da893e045bc5@arm.com>
 <56c8f4f9-b54b-b0bb-250c-ec8643accfc7@intel.com>
 <3541d2de-5cf8-2f84-8153-277e2bfc0101@arm.com>
 <5f98748a-97ca-6426-1e24-a5675da75381@intel.com>
 <a590da86-0c42-7d46-d320-c661a59a46c1@arm.com>
 <837ba176-c97f-f81b-c044-eb6aa3d88bb7@intel.com>
 <CAOUHufY9EQ70Pn-n2zVa9=Gm3-WHxxphp7VHia4qv9x2domdbg@mail.gmail.com>
 <40d49276-fae9-e538-61cf-64bb79233bc3@intel.com>
 <CAOUHufbcAJWUoVuCYtaDZKdcw+JPWVV0EiB=JcDvz1Jt_Au2Tw@mail.gmail.com>
 <45457815-66c4-029f-42f9-6c377e4eb1e3@intel.com>
 <75996f6b-63fe-4878-c19d-bf35ee2ad20b@redhat.com>
From:   "Yin, Fengwei" <fengwei.yin@intel.com>
In-Reply-To: <75996f6b-63fe-4878-c19d-bf35ee2ad20b@redhat.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?NHNSaEI3MlhTT1lsQXBZN1QzRnd3T3ZBWGRDOXNqVnh0VWwzYU1KdzdldWhw?=
 =?utf-8?B?TGFvKzdBd25mMFlRYW1NWGtnbXpVc3lRdnZXS1NpWkNNM3dsS0k0WnJoUlA1?=
 =?utf-8?B?T0tsRy9HSTRPV3RvZ2xWY1ErUmFIZkozVDBaVnUxMkRtVUdBa3JoK0k4em1p?=
 =?utf-8?B?WnQyc1B2ZUVDc0FQNlJ5ZDQvL3daUnhEZmszQWZCTVp2Q0ZZTFNObmxJSi9m?=
 =?utf-8?B?SllIRUJuRE81bDl0N001Q0grNXQ5c3IzNlpZSUV5Uk83ZDlmUGhlSkx1OG5N?=
 =?utf-8?B?RkVUbk0yU3pFY25FZGlkV3BMc1NZdThyLzRBRE54UjBkM2JnZXRqbEI4UUFE?=
 =?utf-8?B?SXFQNWJlY2x2b2NQZmpjVXdRVzhQK05VSTZsTGhNbHN0UGxtZEUxRW13bTRo?=
 =?utf-8?B?MC83NGdxQXJDeEgwVDFmQk5nTSs2Qm5hTXU1OWRua2RxTjF3bmNpZlc3Y1l2?=
 =?utf-8?B?eGVNT08xSFZOYjhyaTZXMTlHYTl3dm11NCt4aGxnOWhhOFVGSU9ISjRvSzU4?=
 =?utf-8?B?dmF0TTZ1c29JU28yT0VvVkh4WFlkNVRlRUpVYkV0Sis2THdheDQrbGRhVUNa?=
 =?utf-8?B?WExqb1YyaVRjZzdmc29WY001aFFZdDNYRDM1a0pMRTdheCt4UkJDZTdiYUEv?=
 =?utf-8?B?YTdxYnR1UG5JT1loVWdQQWhWeExxNk55NVVwbHRSeUNWdFg3R3MzQVV3UStr?=
 =?utf-8?B?eFB0cHM3aE10WVhMd0RkTkpOaGx1SWVyZlRVQzVuVFV1dVBXSHJGMmR1OGpG?=
 =?utf-8?B?aU5BYjBMSVI2ZUtGWlhiWldXR3BuVFlERkljNW5iSmFLeDNPY3ZjT2RIemlo?=
 =?utf-8?B?Y0g5VDM5V2dvYWdzSG9FVEwyaUJWbHpkVVQ4YVNSMC9OZGpNV1VSQy9SempF?=
 =?utf-8?B?cnI2d21QNnpjaUpiOEZyelFLVXQwSmVGQ3Zyc3RkNVdCQ1FJSzJKdUJJNE1x?=
 =?utf-8?B?SDdST3Y0dFNJMmtjc0dJUk40cU01WEhOWVdBZ0FZd1ZLU2pmQmlaQ1RDL2RI?=
 =?utf-8?B?aS9EQUxDclBXNURYMW1nczRub3l1OXZmMWVEVjZ6WkIvV0daZWozc0RuZlMy?=
 =?utf-8?B?cFR4U3E2bUlFQjdMRnZkendaS1grVnB1T3pkelhsUE1mVzBOWC9qYjcxZmUr?=
 =?utf-8?B?K0xHMi83cExTbFEwMnQ4eGhXOEo4VVZueGVCb2llTzRwOEJXTXkzdXBIb2tr?=
 =?utf-8?B?YjI5bmh5VGc4ZFBtNkZ3TnA4WWdqVDcxZ09NeXJJTFJzY3pwZTZwS1k4Q3dG?=
 =?utf-8?B?aGh4R213Vm5HRGRjcXhtL2hJazhsWWJxamt0Vko5UjQ0YnNwaWorM3dBV243?=
 =?utf-8?B?L0ZtbXFjR2ZLV0pHaFFwd3F3eWt3bUNiWis2aXJtM0JMYjl4RS8xRVRtRmQ4?=
 =?utf-8?B?VDc4cURwYUlLaEQyazFYdmxXd2RoOEhEL1ZTZjlqSHhVYkFRQ1RTS2c3QU5X?=
 =?utf-8?B?QWNBVURNTjlNSmNCYWYrUUU3MS9vdHZSdEhWTVdVZ1haWEw2TGNySjZSRUpB?=
 =?utf-8?B?YjdWaDZ1NzlIcW9CeUdMVWc1V1dwQ1NBL2NtOFFJY1R1a0RkaTl0NGI4WVEr?=
 =?utf-8?B?aEVIRytEellKUnE5Zkt3RkltcUlFNmR3aGRJT0JZWmE4VGcvZlB2RHdKSGxv?=
 =?utf-8?B?M3dDajFDRlJIM0J5UzhRN3hiNHE1aFlsN3poQ1VMNzFJaUpOM2NtK3NhMjYy?=
 =?utf-8?B?Q2JlRitMWmMwOEZKY2R3T0tBRTZYb0tPWlhGQ09iVnl5Wk05U21Ka1JxcnJm?=
 =?utf-8?B?RTNsTm5QRm1jQTFlOU1XQmlQUDZieGgvRUh1c082SmtPZXU2ZnZlVEN1bytI?=
 =?utf-8?B?RjZqb2kycEpqekMwUjU1R0p5NEp0Tnk2VnNMcXRvRG8zelloMk9nalJVdzE4?=
 =?utf-8?B?eUNRS3VaN3JWZWpYV0RzWGsvRUVhbkEyQitsUVU0SjBkNUtRc1djVlFPbTMr?=
 =?utf-8?B?TktiQnVWM09lby9JU2xva0lKYkxkS29WT3V3a2JTOTh3OTNOMGJWbGxXd3g1?=
 =?utf-8?B?M2hYaFRTTlpMNDlDME9GYzg5cDREb2Q1N3B6alcwTHdsRnNrVThNMVBBbU1U?=
 =?utf-8?B?M1ViUUZnK1NUOGxIbC9NSUFyNG5zQTZ5SmlqNGJVZmVFR1ZmVlY3M3hIU0tI?=
 =?utf-8?B?dGI1TldLSkVMOWhidFg2UjFlOEVuV3JKeW03cW5TNTBiNlNEbjFuWDFGR2Jl?=
 =?utf-8?B?T0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8c702604-db59-48d7-1519-08db94bd7bee
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4820.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Aug 2023 07:36:15.4725
 (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: KwiJILftbVmVRrWfKGbBQVm0HQbhpVfOINIcuGnHccXlkLpDGjztcss44sOJFnm/SoRu5CTgHvV/NhmLci5E6Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB6842
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,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

Hi David,

On 8/4/2023 3:31 PM, David Hildenbrand wrote:
> On 04.08.23 02:17, Yin, Fengwei wrote:
>>
>>
>> On 8/4/2023 7:38 AM, Yu Zhao wrote:
>>> On Thu, Aug 3, 2023 at 5:27 PM Yin, Fengwei <fengwei.yin@intel.com> wrote:
>>>>
>>>>
>>>>
>>>> On 8/4/2023 4:46 AM, Yu Zhao wrote:
>>>>> On Wed, Aug 2, 2023 at 6:56 AM Yin, Fengwei <fengwei.yin@intel.com> wrote:
>>>>>>
>>>>>> "
>>>>>>
>>>>>> On 8/2/2023 8:49 PM, Ryan Roberts wrote:
>>>>>>> On 02/08/2023 13:42, Yin, Fengwei wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 8/2/2023 8:40 PM, Ryan Roberts wrote:
>>>>>>>>> On 02/08/2023 13:35, Yin, Fengwei wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 8/2/2023 6:27 PM, Ryan Roberts wrote:
>>>>>>>>>>> On 28/07/2023 17:13, Yin Fengwei wrote:
>>>>>>>>>>>> In madvise_cold_or_pageout_pte_range() and madvise_free_pte_range(),
>>>>>>>>>>>> folio_mapcount() is used to check whether the folio is shared. But it's
>>>>>>>>>>>> not correct as folio_mapcount() returns total mapcount of large folio.
>>>>>>>>>>>>
>>>>>>>>>>>> Use folio_estimated_sharers() here as the estimated number is enough.
>>>>>>>>>>>>
>>>>>>>>>>>> Yin Fengwei (2):
>>>>>>>>>>>>    madvise: don't use mapcount() against large folio for sharing check
>>>>>>>>>>>>    madvise: don't use mapcount() against large folio for sharing check
>>>>>>>>>>>>
>>>>>>>>>>>>   mm/huge_memory.c | 2 +-
>>>>>>>>>>>>   mm/madvise.c     | 6 +++---
>>>>>>>>>>>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> As a set of fixes, I agree this is definitely an improvement, so:
>>>>>>>>>>>
>>>>>>>>>>> Reviewed-By: Ryan Roberts
>>>>>>>>>> Thanks.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> But I have a couple of comments around further improvements;
>>>>>>>>>>>
>>>>>>>>>>> Once we have the scheme that David is working on to be able to provide precise
>>>>>>>>>>> exclusive vs shared info, we will probably want to move to that. Although that
>>>>>>>>>>> scheme will need access to the mm_struct of a process known to be mapping the
>>>>>>>>>>> folio. We have that info, but its not passed to folio_estimated_sharers() so we
>>>>>>>>>>> can't just reimplement folio_estimated_sharers() - we will need to rework these
>>>>>>>>>>> call sites again.
>>>>>>>>>> Yes. This could be extra work. Maybe should delay till David's work is done.
>>>>>>>>>
>>>>>>>>> What you have is definitely an improvement over what was there before. And is
>>>>>>>>> probably the best we can do without David's scheme. So I wouldn't delay this.
>>>>>>>>> Just pointing out that we will be able to make it even better later on (if
>>>>>>>>> David's stuff goes in).
>>>>>>>> Yes. I agree that we should wait for David's work ready and do fix based on that.
>>>>>>>
>>>>>>> I was suggesting the opposite - not waiting. Then we can do separate improvement
>>>>>>> later.
>>>>>> Let's wait for David's work ready.
>>>>>
>>>>> Waiting is fine as long as we don't miss the next merge window -- we
>>>>> don't want these two bugs to get into another release. Also I think we
>>>>> should cc stable, since as David mentioned, they have been causing
>>>>> selftest failures.
>>>>
>>>> Stable was CCed.
>>>
>>> Need to add the "Cc: stable@vger.kernel.org" tag:
>>> Documentation/process/stable-kernel-rules.rst
>> OK. Thanks for clarification. I totally mis-understanded this. :).
>>
>> I'd like to wait for answer from Andrew whether these patches are suitable
>> for stable (I suppose you think so) branch.
> 
> Note that the COW test does not fail -- it skips -- but the behavir changed:
> 
> $ ./cow
> # [INFO] detected THP size: 2048 KiB
> # [INFO] detected hugetlb page size: 2048 KiB
> # [INFO] detected hugetlb page size: 1048576 KiB
> # [INFO] huge zeropage is enabled
> TAP version 13
> 1..190
> # [INFO] Anonymous memory tests in private mappings
> # [RUN] Basic COW after fork() ... with base page
> ok 1 No leak from parent into child
> # [RUN] Basic COW after fork() ... with swapped out base page
> ok 2 No leak from parent into child
> # [RUN] Basic COW after fork() ... with THP
> ok 3 No leak from parent into child
> # [RUN] Basic COW after fork() ... with swapped-out THP
> ok 4 No leak from parent into child
> # [RUN] Basic COW after fork() ... with PTE-mapped THP
> ok 5 No leak from parent into child
> # [RUN] Basic COW after fork() ... with swapped-out, PTE-mapped THP
> ok 6 # SKIP MADV_PAGEOUT did not work, is swap enabled?
> # [RUN] Basic COW after fork() ... with single PTE of THP
> ok 7 No leak from parent into child
> # [RUN] Basic COW after fork() ... with single PTE of swapped-out THP
> ok 8 No leak from parent into child
> # [RUN] Basic COW after fork() ... with partially mremap()'ed THP
> ok 9 No leak from parent into child
> # [RUN] Basic COW after fork() ... with partially shared THP
> ok 10 No leak from parent into child
> ...
> 
> Observe how patch #6 skips because the MADV_PAGEOUT was not effective (which might have happened due to other reasons as well, thus no failure).
> 
> The code that broke it is
> 
> commit 07e8c82b5eff8ef34b74210eacb8d9c4a2886b82
> Author: Vishal Moola (Oracle) <vishal.moola@gmail.com>
> Date:   Wed Dec 21 10:08:46 2022 -0800
> 
>     madvise: convert madvise_cold_or_pageout_pte_range() to use folios
>         This change removes a number of calls to compound_head(), and saves
>     1729 bytes of kernel text.
>         Link: https://lkml.kernel.org/r/20221221180848.20774-3-vishal.moola@gmail.com
>     Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
>     Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>     Cc: SeongJae Park <sj@kernel.org>
>     Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> 
> 
> Ever since v6.3.
> 
> The simplest way to fix it would be to revert the page_mapcount() -> folio_mapcount(),
> conversion.
> 
> 
> Probably all that is information worth having in the patch description.
Thanks a lot for checking this. I will try this patchset to see whether
it can restore the behavior (I suppose so from your broken commit info
but want to confirm).

Regards
Yin, Fengwei