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: <cf3f6439-ac2b-53ae-6b55-5ff0b8003fb1@oracle.com>
Date:   Wed, 14 Dec 2022 12:35:23 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH mm-unstable] mm: move folio_set_compound_order() to
 mm/internal.h
Content-Language: en-US
To:     John Hubbard <jhubbard@nvidia.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
Cc:     akpm@linux-foundation.org, songmuchun@bytedance.com,
        mike.kravetz@oracle.com, willy@infradead.org
References: <20221213212053.106058-1-sidhartha.kumar@oracle.com>
 <f149df8f-b3ae-b9ce-eb4c-4f684cba0fe8@nvidia.com>
From:   Sidhartha Kumar <sidhartha.kumar@oracle.com>
In-Reply-To: <f149df8f-b3ae-b9ce-eb4c-4f684cba0fe8@nvidia.com>
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?ZGJMODRvL2dhQ3F4alhCZXhqSDNoN1MxeUJ2Q294djlzd1hpZVkwaDlsdFF0?=
 =?utf-8?B?YXVleTdOY2lUZy9sS05WZFBmMEZWdGw2TThaOEZJNXpMZ25mT1FqQjdRdnpz?=
 =?utf-8?B?cGg2a1JNRW5pWlEyaWI4VlR6cHltY2s1NzdWREsrUmZWa2h2b3ZRNU1yQ1hK?=
 =?utf-8?B?MnN1VWxxTDBzU2tPK1U3WWcrVXI5OXRPMU1uY1NoeThCdU1TK21IMnJFSVRz?=
 =?utf-8?B?Y2lQY1pKQnJOSjlIamhrT0x0WWV6dzlFS2JaZlk2WkZYRTFsVEs5VStONUFY?=
 =?utf-8?B?NTBOUm9lb3NGeWNlc0YvY05oOTRXT0svc2xYK3ZXbXFuSkJrOXdydllMclcy?=
 =?utf-8?B?V3RlNGJEUmdUK2x5VE93eEVhN2VqeExJemEvNmZUNUlCcTZTNGxKUGRSZFZG?=
 =?utf-8?B?WnpyWCswYUxCcmJGZEUzOUFBL0o2c0NCbTd3bnNuNUZsQUNTU0tkNThRMy9F?=
 =?utf-8?B?RUk2eE9laGlNNi9YZ2NiQ29mbkZBeE9BWm5QUDBLcVExVSt0aXpCQkdBU3R0?=
 =?utf-8?B?L0h1a2J4c1VFTjJLaDFEUERRSzdNS2o5eHllNS9IVVJaN2xCSjNGKzZTQmxI?=
 =?utf-8?B?TEZxaGlkU1o0Tm9WU2dqRldsWElPOGFobmhmb1plSmJYVVZRM2hXQWxmZWlB?=
 =?utf-8?B?WXhCYk1QOTVzQkY4OXdKWkNwM3RDckZxUTh4bnk4b3p4M3hCSldBbmdiaDVY?=
 =?utf-8?B?ZW9GNjN4L2lJejRvQXY1MnhqVFlFMkhudmJhOFUyMDh3YXFpZEhiYVVIZGhq?=
 =?utf-8?B?b0NRV1prM2Q0YUxuYjlDajZpZTE0L2xUd0tTWmlZWlZiZjE5VGxIQXJFeHJS?=
 =?utf-8?B?VGprSUR6Y1NUTGZRUUJuUnozWGx3WldxcUNnUkJJRm1VWnQ1R2RmdEpqeEg1?=
 =?utf-8?B?eTY5YUFFb2hoMXBsWGNRNm1SUlNaV2djQTU4NC9seTJxeEczRjRhYlFFRldl?=
 =?utf-8?B?OHhMaTZGbGJJM1YxenpxZjdKWlEwY2ZrZGJITGh4eDBaSGVQN2VuVW9hTU1O?=
 =?utf-8?B?Zi9JZXAzQlFDNzgyZU1QQm9zam01NDYxMVVRQ2FCOHcwWDlsb3psWGc5QVhy?=
 =?utf-8?B?THlDd1h3dGt4NCtaWnEzYnI4OHdIdjA2aGovWVlMMVgxdGFRa3NuMjkyQ3c1?=
 =?utf-8?B?dGRsNmdXZHlhSmJubGl3eGJOTWw3akRnKyt2YnhTWWFmQmc0S2lNMGdENWxM?=
 =?utf-8?B?UGdMUVl5T085ZmJSb0tHK2RQL3IvdmhnUXNrU1RIK0ZmNCtSNmJEdWZPdEhl?=
 =?utf-8?B?ejV5cWdCNy9yNFY0QUxGc0orR2tBd1hDTXJoVHJSUzM1Rm1FNndsU2MvVUx5?=
 =?utf-8?B?ZzBaSmREYVJQcUNJei9NZE41eWJqZWdvQjRrZXB0bHFBOWxmekJkeWU1RDNE?=
 =?utf-8?B?WVp2WTlkampBb2w4MUdGbSt0VzlQd0pOeUEvcWFNcGJHYnpxMVI0aE5iYXpq?=
 =?utf-8?B?L3I2dE9aM211anh0UDk1UjRZZk5XM2YzdXEzV2JUMHZTd2NlRzNGUitTN2VU?=
 =?utf-8?B?WkgxZHd2Rjd5SkhpUjZpMXZMOEhUSkFYQVB2RktScU5uK1NjbG5SUVh6emxm?=
 =?utf-8?B?VWJseG9nMnFTUDlCOUlJaTdJWWdscVd4MUtubFprNVBEMGtURk5VVEd2NzR5?=
 =?utf-8?B?VjFpMnlrV2FIYWNNOHNGbExZMHRyU1VBMHF6cFU1VEY3T1pwbGs5dlp1bVgz?=
 =?utf-8?B?eE01OGVxY1JlQzdoMnpXdDBwWjk5czIwZ2p5STZnbnNXRW1SbStTc1VwTXdL?=
 =?utf-8?B?OWpBbUtZTDR0TXJqRmVaTVRDZ3I3d2w3NDUvT1RpK09mYStCQmk4L1RNT0lr?=
 =?utf-8?B?NlpZUmZMWnJ4S05jRFVnaGlUZzRkd3UvVTIwb1JCVnRqdlNoaTJPYzZkUFZl?=
 =?utf-8?B?c1NkdXNTYkkzUWxnU1d5UmN1WTR5ZDRGbkN4WnRmeTBCdWxLeVV4VE5KWlhD?=
 =?utf-8?B?MVFEeU9sMHU4QjljTVRwMXRhY3hqQ1hqTWs5R1ByUS9ZYXVxcVZiWWJFVHJ3?=
 =?utf-8?B?dmpmYlpocldURjZ6emtsd2lkQThndXQzL1VYMDRRanhoMzdCUEFYN3ZDcStY?=
 =?utf-8?B?Ulp4QnBvOXN6S1l6M016eDNxQWJrMHR6Vkw1cHBBK0RVbEw5YysyalZyRHJk?=
 =?utf-8?B?c3JMcDlyKzBLRmYwb25HcDdseEF1VGM4N3oxbWgrZmgyL3ZtM2M5UFk3d2pr?=
 =?utf-8?B?WUE9PQ==?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 40e54d1e-cf2d-496d-35a5-08dade12bb7a
X-MS-Exchange-CrossTenant-AuthSource: CH0PR10MB5113.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Dec 2022 20:35:26.4347
 (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: OedS8jH9Y27Up5i1Iw3s4shSJvBxRGCWMuGcOjEi/l1cPwdmsBQLYSf6ueljEKZV1WbKaTl5RzpesNQRjdNVaVczu+9/oU6bAA9TVA52lSA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB4835
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-14_11,2022-12-14_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 malwarescore=0 phishscore=0
 mlxlogscore=999 suspectscore=0 spamscore=0 adultscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2212140168
X-Proofpoint-GUID: cjaVCa2hAhJu2H05NNPKHqeYxrdDKu88
X-Proofpoint-ORIG-GUID: cjaVCa2hAhJu2H05NNPKHqeYxrdDKu88
X-Spam-Status: No, score=-2.8 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/14/22 12:43 AM, John Hubbard wrote:
> On 12/13/22 13:20, Sidhartha Kumar wrote:
>> folio_set_compound_order() is moved to an mm-internal location so external
>> folio users cannot misuse this function. Change the name of the function
>> to folio_set_order() and use WARN_ON_ONCE() rather than BUG_ON. Also,
>> handle the case if a non-large folio is passed and add clarifying comments
>> to the function.
>>
>> Link: https://lore.kernel.org/lkml/20221207223731.32784-1-sidhartha.kumar@oracle.com/T/
>> Fixes: 9fd330582b2f ("mm: add folio dtor and order setter functions")
>>
>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>> Suggested-by: Mike Kravetz <mike.kravetz@oracle.com>
>> Suggested-by: Muchun Song <songmuchun@bytedance.com>
>> Suggested-by: Matthew Wilcox <willy@infradead.org>
>> Suggested-by: John Hubbard <jhubbard@nvidia.com>
>> ---
>>   include/linux/mm.h | 16 ----------------
>>   mm/hugetlb.c       |  6 +++---
>>   mm/internal.h      | 21 +++++++++++++++++++++
>>   3 files changed, 24 insertions(+), 19 deletions(-)
> 
> I think this looks good. One small question below.
> 
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 7dc376052d40..300d92d2b49d 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1019,22 +1019,6 @@ static inline void set_compound_order(struct page *page, unsigned int order)
>>   #endif
>>   }
>>   
>> -/*
>> - * folio_set_compound_order is generally passed a non-zero order to
>> - * initialize a large folio.  However, hugetlb code abuses this by
>> - * passing in zero when 'dissolving' a large folio.
>> - */
>> -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;
>> -#endif
>> -}
>> -
>>   /* Returns the number of pages in this potentially compound page. */
>>   static inline unsigned long compound_nr(struct page *page)
>>   {
>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>> index 7cdbcc22587b..810e840bb4f1 100644
>> --- a/mm/hugetlb.c
>> +++ b/mm/hugetlb.c
>> @@ -1344,7 +1344,7 @@ static void __destroy_compound_gigantic_folio(struct folio *folio,
>>   			set_page_refcounted(p);
>>   	}
>>   
>> -	folio_set_compound_order(folio, 0);
>> +	folio_set_order(folio, 0);
>>   	__folio_clear_head(folio);
>>   }
>>   
>> @@ -1808,7 +1808,7 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
>>   	__folio_clear_reserved(folio);
>>   	__folio_set_head(folio);
>>   	/* we rely on prep_new_hugetlb_folio to set the destructor */
>> -	folio_set_compound_order(folio, order);
>> +	folio_set_order(folio, order);
>>   	for (i = 0; i < nr_pages; i++) {
>>   		p = folio_page(folio, i);
>>   
>> @@ -1872,7 +1872,7 @@ static bool __prep_compound_gigantic_folio(struct folio *folio,
>>   		p = folio_page(folio, j);
>>   		__ClearPageReserved(p);
>>   	}
>> -	folio_set_compound_order(folio, 0);
>> +	folio_set_order(folio, 0);
>>   	__folio_clear_head(folio);
>>   	return false;
>>   }
>> diff --git a/mm/internal.h b/mm/internal.h
>> index bcf75a8b032d..829b6a60ceb7 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -378,6 +378,27 @@ extern void *memmap_alloc(phys_addr_t size, phys_addr_t align,
>>   int split_free_page(struct page *free_page,
>>   			unsigned int order, unsigned long split_pfn_offset);
>>   
>> +/*
>> + * This will have no effect, other than possibly generating a warning, if the
>> + * caller passes in a non-large folio.
>> + */
>> +static inline void folio_set_order(struct folio *folio, unsigned int order)
>> +{
>> +	if (!folio_test_large(folio)) {
>> +		WARN_ON_ONCE(order);
>> +		return;
>> +	}
> 
> Would it be better to do this (below)? I'm not sure of the value of
> warning on "order"--it's a little odd and unexplained and doesn't really
> do anything more helpful than simply warning about what why the code is
> failing, which is really about !large, rather than order. Unless I'm
> missing something?
> 
> 	if (WARN_ON_ONCE(!folio_test_large(folio)))
> 		return;
>I agree that warning this way is clearer, I will change in a v2.

> Sorry to drive you crazy over nits. This is the last one from me. :)
> 
No worries, I appreciate the feedback.

Thanks,
Sidhartha Kumar

> thanks,