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: <2fa4d5ca-b486-b246-e4a8-531e322e86e9@intel.com>
Date:   Fri, 4 Aug 2023 16:11:43 +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>, 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?MnB3ZDFuZ1MvalR6NjhvK0Q1ZXI4WVhheFhHbDNkTHo5QVhXb3VISVBhVjF2?=
 =?utf-8?B?WTBZdEhCVTd6bmFQRU1WR1VuUDFpRlAvdmJyNmRpVVkvVk1Ic3MrbEhlU2lh?=
 =?utf-8?B?Q2NCcHZQdGI3eENrekdsd2pjL1lBRHJpLzBXbjBDWXBuVlVweDRJLzRMcGtW?=
 =?utf-8?B?Y3FHQTZOL043Rm9mdUhGRTN3NUtaZGVESytDS3NkbkNYdmZHM09DVTZlVnJz?=
 =?utf-8?B?VG9HT09pM2hMLzZXVDg5Q1RCWUg2cndOS0kxZHRYVDRTd2FyTjBua3hCMkky?=
 =?utf-8?B?YjBDMEo0VGxJbUhheXA3SFpEa1EvbVBYS1c0K3FDRURCQUFkdjlHQ2xtVEJT?=
 =?utf-8?B?RzAvdHBPMDE1M3NDcXRDY3RDYTJ5SnV4YlA5OEZxVC9ubTEzKzQxWkorL0NZ?=
 =?utf-8?B?OWlQdEJ6a0I2S250ZFJkR1hNSTY4YlJTRU4yRjJsdFdxNDlJeHgrcVdGMU9i?=
 =?utf-8?B?Qk1Lekl0c3g3K1dvNUQxKzVMSmgrWGE0dGtkNFNvaVlLb0RzZVljRkFSVCtY?=
 =?utf-8?B?UjFJQUlXUG5mUmZtbDNuV0p4aENQMmlmY3pHelh1ZUZYcnpaMHIyOWtYZEFn?=
 =?utf-8?B?UUNzTXF2aWVFWlV3dkJhTmR0eGI1Z3ZRTjVxY01JZEZSbUpvMDF1SFI5Ly9s?=
 =?utf-8?B?M09zYnBCTXVIWjVLYldkcUJ3NnhwMGZVcG9qSlRDOUJ5elhvZVExY1ZqeXYy?=
 =?utf-8?B?dXJpTTFqZmwyaUoyTnFlQlQ3R0hWUEZqdnZzdU1SR1dEaWtZNWt3Ly9xUEx6?=
 =?utf-8?B?VEVqTUhvWGJUY25Pc1ZydWFaUnh3MW5GUVFZRzROZERWaXRocWg5blhNRWdr?=
 =?utf-8?B?TzZudVdSSklYOW10S25qSzE4b0FtMS9BbldjZXNIbjhoeWJoaHBBZy9IL1cr?=
 =?utf-8?B?dU1XQ3VxS1NiVnhNZzMyYTFaMzhpQVhORnRiSFBzK2lKbFIvazl2a2dmMjIy?=
 =?utf-8?B?Vk5XeENvd0Evb1FtNjVXaGthbkduc2JzUzU5NTlDc3NKeWVYZll5OFBxTXl3?=
 =?utf-8?B?NmVxVGZudThSRnJRVGxmbDV6RVdpNyt1bjhuRmxWTXNrV2VQbDRMN0ZUbzhl?=
 =?utf-8?B?TWtRbVZITEloeVcwdHFMTmU1S1NVNlhJMVpBWXppMjk5Ump2TjdPRXVvRS9z?=
 =?utf-8?B?NGdVT1k1ejVHbUZLTlZVWWhaRWZUOEh5c2s1NndSUVg2aEIydWdvSlZNaDZn?=
 =?utf-8?B?MWQxbUxpaEx0Y2FiOU5YN0l5R2xFWSszTmU0dHpDanI2bTBYNlhTVEYycjVR?=
 =?utf-8?B?WVMySTFLTXlKSmFBWkM1eHJwb0w1ckxzTW5TT3VXUnk5ZEhyRmFlRytiMzBk?=
 =?utf-8?B?UVNxdjg1VE8wbitic2FkYm1JVWQ0VTNGUVJiYWtNRzR1VXVaUGZUWUROcVJz?=
 =?utf-8?B?SFpaOURpa2g2YWNMMjdYbHp5eUlSU3BpRnVWRmU5UG5ld3JDMHpiZWZkZWs5?=
 =?utf-8?B?UHArNjlkOUpHODJ1Yy9Tc1EycUJXREdTR1h3UDM3M0RQbEdBL2ErMklPQTdX?=
 =?utf-8?B?QTFIRi9KeUJlODloUHhXS2hSdk5xRnZzSlJKNVNzUzI2ODlEWjl2eS9uTEhF?=
 =?utf-8?B?SVI1MkRGWWpzMHFUN3hMcEZNTjR0QkQ2OFFJNTBMMFdQbktFTDhTYXMyRm9h?=
 =?utf-8?B?d1hVWnZrbDY4VHRQeXpVaWd2WncwdVBXY3k4ckxIbVVpZ3g3cDlQSHBkOHBj?=
 =?utf-8?B?MHJDMW1RNHdDN0c3SkV5MVQzTEQzSkRVNXBaK2s0emkxYkloNEdUckFFVFhr?=
 =?utf-8?B?b1NIMUp6dHpIdGE4OXBId0UrYWxZWTMrUmJocGNydGMveVN4ajErUm55RjVy?=
 =?utf-8?B?bVg5K3RpRWhJOGpoSE4xYUtIdmhVeHVFNXA0QTl1QktjUFZDbll5VW10Y2Nw?=
 =?utf-8?B?cXpoSzFEd3dFWk05cnMxeEsxRE1samVwK0N5em8zaVVKWTZ5cWlRZEZteHpC?=
 =?utf-8?B?RGZWd3oweDZJRXlrNE9JSUQwOGJ0NEJxMVJoYS8vZG5oUzN1UGhRSEN5OE9B?=
 =?utf-8?B?aURWQzc3VWRyei9qR2tJWm9IWFh5VHhJd2ZOcHZXUUc5VEIwalh2VWQyQk9x?=
 =?utf-8?B?WXltL2QvVm0vWGRad1dSZHE1bi9sd0VaaVhIMGhXR0t4ak5mT2tNeEs3TDBy?=
 =?utf-8?B?bXF4ZXpYbndmZFNqbTRBM21VVVBTN3N1MGFwU0Vrc2ZjN3VZL1NOeCtnaTlq?=
 =?utf-8?B?UWc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fa1ccd67-9a1b-4aa4-18b6-08db94c27641
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4820.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Aug 2023 08:11:53.2953
 (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: 2F6an1DiJ2Li5J05IqlyZ8UfJXM34inTnZyHrNp/Lzlo5RSE2SaImPLgl9kx7lE78VBIdFG9IM+NFTKqshSOFg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA2PR11MB5193
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,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 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.
Confirmed this patchset also can make swapped-out, PTE-mapped THP related tests from
skip to pass.


Regards
Yin, Fengwei 

> 
> 
> Probably all that is information worth having in the patch description.
>