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: <cd64f4f5-e77b-d51d-637e-51cf8dba8277@intel.com>
Date:   Sun, 23 Jul 2023 20:26:49 +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 1/4] madvise: not use mapcount() against large
 folio for sharing check
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-2-fengwei.yin@intel.com>
 <CAOUHufY0AX_wyBCZ3dNMfTg7C1ZBOZHpYSgG=uOwbeeemyFX6Q@mail.gmail.com>
Content-Language: en-US
From:   "Yin, Fengwei" <fengwei.yin@intel.com>
In-Reply-To: <CAOUHufY0AX_wyBCZ3dNMfTg7C1ZBOZHpYSgG=uOwbeeemyFX6Q@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?Nzg2Q3h3L2hhckpuYWczelYvSTJKcWg0VnB6VVh3K0N2VU8rR253TmV4Wm1j?=
 =?utf-8?B?RGpnbFkybFhmL0UxNDJoZFlOKzFFenh5bW5rK2FUUWVxamsxSlRQRnpzcmZz?=
 =?utf-8?B?T2dKYW9obklvSGFSYkpPK3RqOStZcW9oWTk3Y0pvYkRDdFJFOE4xNjg1UCta?=
 =?utf-8?B?N0g5eHU0V2YvSEgyaENjdEk4Z29sTzFyZW1rRmxWcXBTSDdGV2c4d3FFdmVm?=
 =?utf-8?B?R09GZjJWdDlMWW9NMUNIeGQ2MTV1cVNVNHkzbzV5RGEyKzVnUHZ1WTk5WXEr?=
 =?utf-8?B?SFhYSnJyclQzamZpMkNRcXREQXFFM05BVmpVd0FJbXFWWnlJOGN2bnpsRk9H?=
 =?utf-8?B?REQwdmpVazVBbTBFMFV4dksxdFc3U3Y5N1k2eFdEVkF6dndBbTFwczJnQjFR?=
 =?utf-8?B?Z0JsV25kbVprZm55aDNVRTZDd0JkY2JSeW01ZTlzRVBkdkd4L3N2WWc0NjNz?=
 =?utf-8?B?T0h0bTMrNjBRNGlvM3hjQWV6bkdxTExlcElZcWMvb0o0V01Jc1IzRENvNHBI?=
 =?utf-8?B?cWV0bHdTcG5kQXpxTjVRZDJQanVPOW1VcDQwVHZHR29laTI1bm1tMHBLaU1S?=
 =?utf-8?B?QStIZWdqWVZnVjdMek1LZ1hVQmRJemhZUmhZclh1cjF0KzVOUHNyUWZhLzh4?=
 =?utf-8?B?ZnB1VGExK0ZWMVUxL042dXZ2ZGF5Q3RyWVpuQmhHL2cxME5nVHpGMW1NZSto?=
 =?utf-8?B?Z1ZVenBRdG9VNkVoRUtIc2JvM2ZpY0FZamgrQmszYVBUYnZiL2Y3cWFWODNJ?=
 =?utf-8?B?aXhWMmIzbEt1NHVCMGl6MTVwTTFNSUZxSUI4OUNFM0tnVk13N0o3R3dhYWtj?=
 =?utf-8?B?anY1UEoyZjR2V3FIeDZqaWNoRUlmR2JnazQ5RUh4a3JKb3R3UUVMYzhpS000?=
 =?utf-8?B?cG0rMTNEanJMZ3VnYWRIV2lDdmd6MGxPT3J5SW11OEJhVEtVRkZUOGtBR0xS?=
 =?utf-8?B?Q0ZVWUpoYmRXNnJQWVIzL2NMVlRxdGVqaXA4Sm5sUVEyanF2bGtvYVRWWEpr?=
 =?utf-8?B?MzZVUDBoNFVrVVNPQU9XcHBqNlh2SmZwY2g4aTFGaEZHaERveGRhQ1VMQmhj?=
 =?utf-8?B?cElOS2NhSlF2bWsrQkJROWtGSmZMQ3dUb0lUdHUyMXVkWDJCM1Q3K2J6SUVv?=
 =?utf-8?B?bkJleVc1Z3Z0bGJTU0Rlem15K05SZnF1QU1iR1plQWJFcVBRY08yZDhBUEVX?=
 =?utf-8?B?T3hIWGNDMVRvUU9icExJQTNUUWVrS05QZ2FCTUtGUGdQdXBRMzZ4V0pCQktw?=
 =?utf-8?B?eW1ZN2dIU0lONDJrY21JUDh3RFJPN2RRYk1DWTRDUy9ERlZYUVllS0xGUmxK?=
 =?utf-8?B?VXRqNzVFQ3JJMkNiQkkyWXpsSmxDaGlaMW5CaEZFNy9kQ2FxSWxqdXhDNE9L?=
 =?utf-8?B?S0Q1MUhSbnFWa0x3V0hUNVNuQnlWMlh6RkdSaEpBTEtuNWk3SVl2OVkxSm5q?=
 =?utf-8?B?bVhBTEtGNkVLaUFiSHF2Sm9qNGE3VHBGVlp3dXRGakNibmRDQ0ZUcERMdGl0?=
 =?utf-8?B?SlN1cHVNdXEzWDFaMUNXNTlzejcrZnRxYmpZRFBUQlgyL0J5Q1JjRGh2aUZo?=
 =?utf-8?B?VGc5NVMrdGVWbFowSjZGblkzWlNEaExQMm40RUhkcEhCTmphV1c5a2RST0xG?=
 =?utf-8?B?bDFESTRjNDRYNUhWaGwvekxsbVZqdzh2VHZSeG93UDcwMGR1VFFhS2xhaU14?=
 =?utf-8?B?U3lycnJCVEE3dEFDYVBOcm9kUHVxd2sveXJoemNma2hlM09UOU11SVBhb1Qr?=
 =?utf-8?B?bnVVdFdGR1V2N3kxdkZMYkRIUTdOakZnK1BvQ3VrS3YxN0NWaXh0bkUvbXJZ?=
 =?utf-8?B?V0JSZmNVRlhEU0cyR1ozZlVqb0JpMlgwYWtvbUpFNU4xNTdSRDMrdVl1RENa?=
 =?utf-8?B?VGZWczk5d05kdlVqa0xoclRUdzZsY0N5VCtxdXVXZnd6WGJsanlPUkFQL1lz?=
 =?utf-8?B?dTBvVXd1d0FMNGZZSUhrSU5YUUpzaklNbjdRVVhlcmlOYkkxemlRcGQzeHEz?=
 =?utf-8?B?UmIrMEFnN0VrRlc1YjRtVzA0UXVqZzNKbStOS0dWMWo3OENXdWU4ak5YYWVO?=
 =?utf-8?B?UENSdVZ5djRPZkVoeTBPWDJyRDNFQlFteXUydUpSQjQxWktlbVljYWdXdkN4?=
 =?utf-8?Q?DPasA/xC4W0SgmVf159exRh9e?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9117e471-a6ae-432c-e456-08db8b781b67
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4820.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jul 2023 12:26:57.9010
 (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: upUNbOqwTar3mqdocH68hU5YkZlg7pqUpKCQ1YtgeLwuvc15pgrqlGwnQDqXSJNdUJAi+JmlwmguCEvFhNq30g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4964
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE 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/22/2023 2:57 AM, Yu Zhao wrote:
> On Fri, Jul 21, 2023 at 3:41 AM Yin Fengwei <fengwei.yin@intel.com> wrote:
>>
>> The commit
>> 07e8c82b5eff ("madvise: convert madvise_cold_or_pageout_pte_range() to
>> use folios") replaced the page_mapcount() with folio_mapcount() to
>> check whether the folio is shared by other mapping.
>>
>> But it's not correct for large folio. folio_mapcount() returns the
>> total mapcount of large folio which is not suitable to detect whether
>> the folio is shared.
>>
>> Use folio_estimated_sharers() which returns a estimated number of
>> shares. That means it's not 100% correct. But it should be OK for
>> madvise case here.
>>
>> Signed-off-by: Yin Fengwei <fengwei.yin@intel.com>
> 
> Fixes:
> Cc: stable
OK

> 
>> @@ -383,7 +383,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
>>                 folio = pfn_folio(pmd_pfn(orig_pmd));
>>
>>                 /* Do not interfere with other mappings of this folio */
>> -               if (folio_mapcount(folio) != 1)
>> +               if (folio_estimated_sharers(folio) != 1)
> 
> Strictly speaking, this isn't a bug. But it may be ok to include it in
> the same patch.
OK. I will drop the change for pmd.

> 
>>                         goto huge_unlock;
>>
>>                 if (pageout_anon_only_filter && !folio_test_anon(folio))
>> @@ -459,7 +459,7 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
>>                 if (folio_test_large(folio)) {
>>                         int err;
>>
>> -                       if (folio_mapcount(folio) != 1)
>> +                       if (folio_estimated_sharers(folio) != 1)
>>                                 break;
>>                         if (pageout_anon_only_filter && !folio_test_anon(folio))
>>                                 break;
>> @@ -682,7 +682,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr,
> 
> What about madvise_free_huge_pmd()? Should it be changed as well so
> that it's consistent with the first change? Either change both or neither.
> 
>>                 if (folio_test_large(folio)) {
>>                         int err;
>>
>> -                       if (folio_mapcount(folio) != 1)
>> +                       if (folio_estimated_sharers(folio) != 1)
> 
> This is another bug fix and should be in a separate patch.
OK. Will split to two patches.

> 
>>                                 break;
>>                         if (!folio_trylock(folio))
>>                                 break;
> 
> Please send two separate fixes, and then:
> 
> Reviewed-by: Yu Zhao <yuzhao@google.com>
Thanks a lot. I will drop the mapcount() change for pmd and sent to patches
for madvise_cold_or_pageout_pte_range() and madvise_free_pte_range().


Regards
Yin, Fengwei