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: <56c8f4f9-b54b-b0bb-250c-ec8643accfc7@intel.com>
Date:   Wed, 2 Aug 2023 20:35:15 +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
To:     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>, <yuzhao@google.com>, <david@redhat.com>,
        <shy828301@gmail.com>
References: <20230728161356.1784568-1-fengwei.yin@intel.com>
 <3bbfde16-ced1-dca8-6a3f-da893e045bc5@arm.com>
Content-Language: en-US
From:   "Yin, Fengwei" <fengwei.yin@intel.com>
In-Reply-To: <3bbfde16-ced1-dca8-6a3f-da893e045bc5@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UUxYQVUxV1pOcU5jTHBPeXQ2MjZkRjZydGowaHJiSkFCZjhrc2xLeExJRGxV?=
 =?utf-8?B?NG80M0M1NHA3ODNScGxlN0ZUWG1iUENBT2tOTUJCRXF2T2RnQjJScXhBNEd2?=
 =?utf-8?B?MGZacSt4MW1Nd3JsUDkrNHNrcXBrN0NLK2p5YTBVbzY5dms3aWZZVXRPNmk0?=
 =?utf-8?B?cWlkMzgrYmkwK0YwSXE3SzhkMHdlcDBkUjFtSGZTTmMvWk0xTTdvZFhJUWox?=
 =?utf-8?B?ZGVVaU9OdjF5dlQzaFExbG5vd2FlaUdyUy9PV3ZTWmt5ZG5KYnJPZ1RQQVd5?=
 =?utf-8?B?MVFvcWo1ZndWVDliTVgwU1pEUGJDY0d4cDBsQUx2dncyL1lZZHVSUUJVaEZL?=
 =?utf-8?B?TksrSXFhZkVTejlBTTg4UnZtcnNSbGtHQXV1Ry85a0g4TCtPSEVKYU1Ta0cr?=
 =?utf-8?B?enI3WTBmcWlQaWFqV0dMS2dJaGM5cU9abXdXM1p1TW9rcmVPMTVvR2ZVVkww?=
 =?utf-8?B?UGNSd3prdlVXU0dHeUZ6WWVsc2ZIUDllcXA3eEJzdXp4OFNwZEJaUW8xNDY3?=
 =?utf-8?B?WGJ2Ym9jYzJEZlE2MExOc0M5Y3A3TC9zOGpZY3RVelgzZkptS3Z1VVRtbmt0?=
 =?utf-8?B?TnF6UHgxcnA4Ni9qTCtBQWhyeTVBd3lFZmkxK0l1WHF1ZktzNnN4UnpyeUxt?=
 =?utf-8?B?VTI1L1UyTWdzaUltZFJtUzliK3NoZGpBandwK1pKN2NPckdYZlY2Y3BDY0xv?=
 =?utf-8?B?aGs2S0RUV09jN3N0QjlVd2hRZWtadTZvRFZpa253UVhFaG00QW1kOXhpVWlh?=
 =?utf-8?B?N0VXeUc1S3pmV0dtcjBvYytZbGUxQXZHREFyQ0QwREZZYWtTMUs2bXh1cXhD?=
 =?utf-8?B?SnlBbkJsOEJjb2thaytnZW1HUlFVSVBvOWpmb0N2cnFYK1piRWFPdmdwWFVP?=
 =?utf-8?B?V0NJOFZ2S2J5WHhpaXprTUpic0ZTYlI5N0VaMlpFTnhOT3hKaTlheVN3ZW1z?=
 =?utf-8?B?bTZMVzhNanRYUklVdHFzTmJiOWF4QmE0N0UyWnFnbEtkUlZSRHMyM1Z6K1R3?=
 =?utf-8?B?MmJ3VFQxRmVBSjN2SXJxUUFKTTRmUXFnU1hmam1FdklEVnZJOFhnYVFDV1I3?=
 =?utf-8?B?UW5PMEFhWkhkR3VsRnA3M21GdzhjTHVTeU9hRHFJNTlpWjd5WnNzY3BJRGJQ?=
 =?utf-8?B?amdwcjdxZlFoM0tRZmFxZHN0anhxK3BRdnZoYVVpU2ozb2dYMExodEdxek5T?=
 =?utf-8?B?WTYrVEpqczN3UFY1RjgvblhwTCtGRk90L3RkSTZqQ1pPcS9lZ1JLLzZGR3Mx?=
 =?utf-8?B?dlB2ZnhxOGp6dStqMk1sS1FxZ0JSaDRaVndIaGxqa1FLa25CTEhrL1orNGZK?=
 =?utf-8?B?NTNGKzhCUFVVWXVUNElBMCtQSkJCUnZuU3lmTDFHZjhST3Zmb2E4OXpzVE5N?=
 =?utf-8?B?ZHhJMDBYb0taU25PRGQraHlrVkZpZHIxUGQvcnQrQ1Zna28xL1crVmRrOWFL?=
 =?utf-8?B?N1N0bDlIQnB5S0oxd1pCMXpOSGVEQis0RkZkSzgvRml0eXdZSzJ6V2xqOGl5?=
 =?utf-8?B?Rm9WeFVyZndQa0Y1eCsxbG5xNmMxTzlRaTlFcnl4Z29wMjVhby91UW8yVGdR?=
 =?utf-8?B?d0pGWEpidnhxSGZ0czBJNXFQK1c4KzlYTFQwQnlzQ0xmUXJaREliam5tRE95?=
 =?utf-8?B?MWNRdDlONWJrRVJkNWRHQXU1YUgzMG01R2FlSEo0allSbmIzYTR5eVRtVm1U?=
 =?utf-8?B?d2dXS1haVys5NklYYnlKejB3RWttSEg5RVNSSThCZjF5OGVTeExxc3FuZHgr?=
 =?utf-8?B?aU8yK0NNbGZVb2pZbVJ5RmU1TXdsYjdFWGdrd0pIQUVJRFdUMTNvMkRleGkw?=
 =?utf-8?B?YU9WaUtrd0ZlOTJrdnJMSEg4ZWVEVnhYdFkyQ0NjeC80KzhZN05OK0d2Slh4?=
 =?utf-8?B?Z0tZV0ZiY1NOdkJFQjVWcnlNVFB5TGxGMjFuRG5RNkpEMk0zZm9heXJNV3Nz?=
 =?utf-8?B?WmZjN0w5YUlEaEliaTVoNUc1RVU0bXRpUFRBa2d4VEVMVTlIdzVHdHRhR3lU?=
 =?utf-8?B?eFpoQThYaU52ZXlYc011eFVQcTB0T0Q0OHBVeXZGN3dlUG5aYTduazhIYisy?=
 =?utf-8?B?OWVnRlpsb3VyRnRxcTYzUkt6SnkwT25hV2pGNWxtNWNscWhLYnFqYkMyTTFi?=
 =?utf-8?Q?kw61INXyfbagfHS4dclIDCgQ8?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ede8682e-5f1f-4138-bc24-08db9354f5ea
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4820.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Aug 2023 12:35:31.6946
 (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: LhkEWCIwJ8BOtQMM3csKlpRRD3hSI2nXlc3EjQR51sT2tdHgr1qmTqQsjAklHB0Bcb0oKnU3AvJAHKoR/aMRRQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB7564
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 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.

> 
> Given the aspiration for most of the memory to be large folios going forwards,
> wouldn't it be better to avoid splitting the large folio where the large folio
> is mapped entirely within the range of the madvise operation? Sorry if this has
> already been discussed and decided against - I didn't follow the RFC too
> closely. Or perhaps you plan to do this as a follow up?
Yes. We are on same page. RFC patchset did that. But there are some other opens
on the RFC. So I tried to submit this part of change which is bug fix. The other
thing left in RFC is optimization (avoid split large folio if we can).


Regards
Yin, Fengwei

> 
> Thanks,
> Ryan
>