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: <b995eccf-5818-84ee-560e-20c00f9936b4@oracle.com>
Date:   Wed, 7 Dec 2022 10:49:21 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH mm-unstable v5 01/10] mm: add folio dtor and order setter
 functions
Content-Language: en-US
To:     Mike Kravetz <mike.kravetz@oracle.com>,
        Muchun Song <muchun.song@linux.dev>
Cc:     linux-kernel@vger.kernel.org,
        Linux Memory Management List <linux-mm@kvack.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Muchun Song <songmuchun@bytedance.com>,
        Matthew Wilcox <willy@infradead.org>,
        Mina Almasry <almasrymina@google.com>,
        Miaohe Lin <linmiaohe@huawei.com>, hughd@google.com,
        tsahu@linux.ibm.com, jhubbard@nvidia.com,
        David Hildenbrand <david@redhat.com>
References: <20221129225039.82257-1-sidhartha.kumar@oracle.com>
 <20221129225039.82257-2-sidhartha.kumar@oracle.com>
 <BAD34D59-187B-4BB3-B139-7983A8B62648@linux.dev> <Y5ALigw0kUO/B3z2@monkey>
 <4161AF1A-9508-4DF8-B756-FEB476EB32B5@linux.dev> <Y5DXivFOA+bO0IYQ@monkey>
From:   Sidhartha Kumar <sidhartha.kumar@oracle.com>
In-Reply-To: <Y5DXivFOA+bO0IYQ@monkey>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?T0pJY2JGTmt1MVNzSkpQVmY2dUpNM0pOWFE5Q1gyaHJwMFgyZkc2Y0FseEg4?=
 =?utf-8?B?YzFXbVd4YlpFbWJEQjJ0N2FPZnBQVm51VW4zVythWmFWUmNzaWd4RHZ3R1A4?=
 =?utf-8?B?bjFGbU55emtiL0MzdXlyZ3hjV1BpWnJSMGRxbUh2bU1DYnBSWkZmQ2FONnFa?=
 =?utf-8?B?RUN5V0hwc3JVZlhyZzc1YnlCcUp2YjNFVnhMN3lPSWJxN2hFWWltR21WQ0Z6?=
 =?utf-8?B?TVY1UW1FeHBVTDhzNHA4OE5pZ1doOU1kdjhCZExsc0Z2cFloMzVzVy9GSDAz?=
 =?utf-8?B?dWhMTVlYdXlzaU9jWWswbEttRTZXNU5xQTRlRjJkK0xFRWpveG5FRkFDRW1a?=
 =?utf-8?B?UXQ1R2VsU3N2cU51NlhLUHhhMURFRkhEaTZwbjIwRTg5L2N6ZTQ5ZkZweVBi?=
 =?utf-8?B?ZGIrU3htZGVzUEhDQ24wSlVqNDUvejN0WEZ2djhjdXNaQk54OXBZYlp4OWVm?=
 =?utf-8?B?cTU2TzcyR3dxOUpQc3VLNkFaTEVrN24yRmNGSm5lVkhhclRwTnNtUjA4N2Fp?=
 =?utf-8?B?RFVxZ2ZLcmV0Q2Q5cGk0THp5akx0eDE3bnlLb0h6L3hyWVN6RjhpNUZneHVr?=
 =?utf-8?B?RDhRV0VWdTl1eXg0Q2lZSlhsRlUvK01TVk1PZUxsZUlJSGlsMXFQR29YaHgx?=
 =?utf-8?B?eEtKRmxqZ2wzMnVCWXR6L0xEQk1OYkI2a0xjOVQ0YWo0djBicjZTRXEzZkpo?=
 =?utf-8?B?TW51S0FtcHUzUGhTVjY1SHNqRDNrTGtSK0R6UDFlaC90cC9qMkorcDdRc0ps?=
 =?utf-8?B?V0JBRExHcGpCYWgxMS9vVTFoRGtoeE44ODR1dkhCYXIzdmpFamxJQkxoZWZW?=
 =?utf-8?B?amhqQmxkQ2FCNU1pNDRmV3U2dGhvRytwMEVDV1N6QnVGYWg3Z0QrME8rWHZn?=
 =?utf-8?B?ZGNVdm9ad3ZoazBkenpieEVoWjRySk5jZko4YjJSS2V1dTUwVTdLMTIyekd4?=
 =?utf-8?B?eFFKR00yY2dEQzZsMExPMUUyQmljWGcxMWRGaGgza01xQXQzZjVtb1pMdXcr?=
 =?utf-8?B?ZENiRkhIN05EMUwrV1RvcW5tcEgzZWNzUE52T3lycDdyNzlVWXFiWVRYWU9l?=
 =?utf-8?B?U3ovY0d4TmlvTFFMSTA3SWZDSDgvb0U3VmVpbVVBNWpKaWVBS1pleDI4TGYr?=
 =?utf-8?B?OTMySW1SUTNEbTJGVVhwdmR4eTVZN1hKc2NkZVFoWVlscnpuL2xYUTFkYTFR?=
 =?utf-8?B?TWphMEpvN2FqbWJabm9VcUxUNytta09BNHBTeGpBblU1UWRIeElFQUhocVNu?=
 =?utf-8?B?bUVUVDVLZktPM2l5RTdiVlh2YkVYcm9nWm1HbnJ0dEU1ZGZOWWdzeEUyWTdv?=
 =?utf-8?B?eU5RU0UzWStUQ2xwNVBPUzhTaHdDc0xmY1U0ZWFwWHk1TTZySEp2cm5YYkZB?=
 =?utf-8?B?Y1J3TEkvYVk0S2xQTU1ZM1lNYi9vSnFNY3VuanNnMEY1M2tNcUwyK3IrbUR3?=
 =?utf-8?B?NksrRDBiMlQ1VVhjc3NyYi9MdnFKMUIzbm9Pd3dNRWpwTGRDdkhQRE1zNWkr?=
 =?utf-8?B?bXArZExwSnBlb3VUTFdEc21HT3hZeE1Bdkd3L0hpd0wybjlVbEo1T0lnbzdG?=
 =?utf-8?B?ZVRLRDFsMDBLRmQwOVBXVktDb29uL1pvTGNQQkx0V1M2V0lGNWk5bHJNRzMx?=
 =?utf-8?B?MEhoTDFtWm51TVdnVksvc2VBWS9xSTBab2dEY2owd2xHK1pXRm9wNWEwa2cy?=
 =?utf-8?B?NDRaQWxPb3gwM2Z5eEovSXJ2QjRXSDBIWklDdWdzYWlXd0dqVWdiMC8wTlRa?=
 =?utf-8?B?OStDcDdYazd4Z2MzVng2T2xrdEtrdXN3WmZXdzRHR2w5cHRKSzByYW1kcHAr?=
 =?utf-8?B?V0RGQ0JMOFQvOXRJa2hPSVFxSitpYzM1bFptOFdKWnRTeSs4RWF0YmdKdjgz?=
 =?utf-8?B?b1c1elk3NlJ1K0RrV2xrcUFVUURVWU5ueGk0NUswbVBVRkVEYjJHTHMrdGtK?=
 =?utf-8?B?U1l1WUJXY1Ivd1F4MlptS0wzamhKM2p0Y1NKMFhrb0lvQWNkSCsvNFJBSHVz?=
 =?utf-8?B?VGRLbTIwQlpUZmdiRTNvQkprWUF1NGhLM0xVL3FrWHJ6NGYzU29ZVVg2SnlM?=
 =?utf-8?B?Z0piL2RDWktVeHVBeFZRSFk1akRRWnJjb1liMFVhNytkRVIzZG9KQ0F5WURJ?=
 =?utf-8?B?U2F5ZWZrai9yODNaVVEwUFdxcm1YZzZjcnBXc0lHWFVBcVVTQy9abnBySEpC?=
 =?utf-8?B?b0lHMGZjWXh3WHVGNFVxTFBnWjlUeGpxbEx6VVVOVVNkY0pYZUNRekhVbm9V?=
 =?utf-8?B?MmZUMHBDb0ZyM3dtSndTZlB1UWR3PT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ebfb9738-28d5-4c86-21bd-08dad883c3e7
X-MS-Exchange-CrossTenant-AuthSource: CH0PR10MB5113.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Dec 2022 18:49:26.6616
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Gg5YlOeYTmJMpcmCGKLudp0Bk+MbCIGAdX9Gh1Em5pVHI8vM79026TanCGfEpJz4pOQVbK+ewNFy7OmonoQNZzZclSueYEvVYhfhOUCHNEQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR10MB7334
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-12-07_09,2022-12-07_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 bulkscore=0
 suspectscore=0 phishscore=0 malwarescore=0 spamscore=0 mlxscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2210170000 definitions=main-2212070158
X-Proofpoint-ORIG-GUID: ONURiwlv8fA4BdHWLOVzlM3dQZORuZkn
X-Proofpoint-GUID: ONURiwlv8fA4BdHWLOVzlM3dQZORuZkn
X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE 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 12/7/22 10:12 AM, Mike Kravetz wrote:
> On 12/07/22 12:11, Muchun Song wrote:
>>
>>
>>> On Dec 7, 2022, at 11:42, Mike Kravetz <mike.kravetz@oracle.com> wrote:
>>>
>>> On 12/07/22 11:34, Muchun Song wrote:
>>>>
>>>>
>>>>> On Nov 30, 2022, at 06:50, Sidhartha Kumar <sidhartha.kumar@oracle.com> wrote:
>>>>>
>>>>> Add folio equivalents for set_compound_order() and set_compound_page_dtor().
>>>>>
>>>>> Also remove extra new-lines introduced by mm/hugetlb: convert
>>>>> move_hugetlb_state() to folios and mm/hugetlb_cgroup: convert
>>>>> hugetlb_cgroup_uncharge_page() to folios.
>>>>>
>>>>> Suggested-by: Mike Kravetz <mike.kravetz@oracle.com>
>>>>> Suggested-by: Muchun Song <songmuchun@bytedance.com>
>>>>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>>>>> ---
>>>>> include/linux/mm.h | 16 ++++++++++++++++
>>>>> mm/hugetlb.c       |  4 +---
>>>>> 2 files changed, 17 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>>> index a48c5ad16a5e..2bdef8a5298a 100644
>>>>> --- a/include/linux/mm.h
>>>>> +++ b/include/linux/mm.h
>>>>> @@ -972,6 +972,13 @@ static inline void set_compound_page_dtor(struct page *page,
>>>>> page[1].compound_dtor = compound_dtor;
>>>>> }
>>>>>
>>>>> +static inline void folio_set_compound_dtor(struct folio *folio,
>>>>> + enum compound_dtor_id compound_dtor)
>>>>> +{
>>>>> + VM_BUG_ON_FOLIO(compound_dtor >= NR_COMPOUND_DTORS, folio);
>>>>> + folio->_folio_dtor = compound_dtor;
>>>>> +}
>>>>> +
>>>>> void destroy_large_folio(struct folio *folio);
>>>>>
>>>>> static inline int head_compound_pincount(struct page *head)
>>>>> @@ -987,6 +994,15 @@ static inline void set_compound_order(struct page *page, unsigned int order)
>>>>> #endif
>>>>> }
>>>>>
>>>>> +static inline void folio_set_compound_order(struct folio *folio,
>>>>> + unsigned int order)
>>>>> +{
>>>>> + folio->_folio_order = order;
>>>>> +#ifdef CONFIG_64BIT
>>>>> + folio->_folio_nr_pages = order ? 1U << order : 0;
>>>>
>>>> It seems that you think the user could pass 0 to order. However,
>>>> ->_folio_nr_pages and ->_folio_order fields are invalid for order-0 pages.
>>>> You should not touch it. So this should be:
>>>>
>>>> static inline void folio_set_compound_order(struct folio *folio,
>>>>      unsigned int order)
>>>> {
>>>> 	if (!folio_test_large(folio))
>>>> 		return;
>>>>
>>>> 	folio->_folio_order = order;
>>>> #ifdef CONFIG_64BIT
>>>> 	folio->_folio_nr_pages = 1U << order;
>>>> #endif
>>>> }
>>>
>>> I believe this was changed to accommodate the code in
>>> __destroy_compound_gigantic_page().  It is used in a subsequent patch.
>>> Here is the v6.0 version of the routine.
>>
>> Thanks for your clarification.
>>
>>>
>>> static void __destroy_compound_gigantic_page(struct page *page,
>>> unsigned int order, bool demote)
>>> {
>>> 	int i;
>>> 	int nr_pages = 1 << order;
>>> 	struct page *p = page + 1;
>>>
>>> 	atomic_set(compound_mapcount_ptr(page), 0);
>>> 	atomic_set(compound_pincount_ptr(page), 0);
>>>
>>> 	for (i = 1; i < nr_pages; i++, p = mem_map_next(p, page, i)) {
>>> 		p->mapping = NULL;
>>> 		clear_compound_head(p);
>>> 		if (!demote)
>>> 			set_page_refcounted(p);
>>> 	}
>>>
>>> 	set_compound_order(page, 0);
>>> #ifdef CONFIG_64BIT
>>> 	page[1].compound_nr = 0;
>>> #endif
>>> 	__ClearPageHead(page);
>>> }
>>>
>>>
>>> Might have been better to change this set_compound_order call to
>>> folio_set_compound_order in this patch.
>>>
>>
>> Agree. It has confused me a lot. I suggest changing the code to the
>> followings. The folio_test_large() check is still to avoid unexpected
>> users for OOB.
>>
>> static inline void folio_set_compound_order(struct folio *folio,
>> 					    unsigned int order)
>> {
>> 	VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
>> 	// or
>> 	// if (!folio_test_large(folio))
>> 	// 	return;
>>
>> 	folio->_folio_order = order;
>> #ifdef CONFIG_64BIT
>> 	folio->_folio_nr_pages = order ? 1U << order : 0;
>> #endif
>> }
> 
> I think the VM_BUG_ON_FOLIO is appropriate as it would at least flag
> data corruption.
> 
As Mike pointed out, my intention with supporting the 0 case was to 
cleanup the __destroy_compound_gigantic_page code by moving the ifdef 
CONFIG_64BIT lines to folio_set_compound_order(). I'll add the 
VM_BUG_ON_FOLIO line as well as a comment to make it clear it is not 
normally supported.

> Thinking about this some more, it seems that hugetlb is the only caller
> that abuses folio_set_compound_order (and previously set_compound_order)
> by passing in a zero order.  Since it is unlikely that anyone knows of
> this abuse, it might be good to add a comment to the routine to note
> why it handles the zero case.  This might help prevent changes which
> would potentially break hugetlb.

+/*
+ * _folio_nr_pages and _folio_order are invalid for
+ * order-zero pages. An exception is hugetlb, which passes
+ * in a zero order in __destroy_compound_gigantic_page().
+ */
  static inline void folio_set_compound_order(struct folio *folio,
                 unsigned int order)
  {
+       VM_BUG_ON_FOLIO(!folio_test_large(folio), folio);
+
         folio->_folio_order = order;
  #ifdef CONFIG_64BIT
         folio->_folio_nr_pages = order ? 1U << order : 0;

Does this comment work?