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: <0187f9c2-e80a-9cde-68bc-c9bdbd96b6fe@oracle.com>
Date:   Thu, 8 Dec 2022 10:06:07 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [PATCH mm-unstable] mm: clarify folio_set_compound_order() zero
 support
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, tsahu@linux.ibm.com,
        david@redhat.com
References: <20221207223731.32784-1-sidhartha.kumar@oracle.com>
 <92965844-c430-8b8e-d9f1-705d7578bceb@nvidia.com>
 <ec8f46ca-9ea6-4567-2038-22f6d3000ed5@oracle.com>
 <d17530ad-8e12-8069-d619-a2d72fe80e15@nvidia.com>
From:   Sidhartha Kumar <sidhartha.kumar@oracle.com>
In-Reply-To: <d17530ad-8e12-8069-d619-a2d72fe80e15@nvidia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UHBaczdXbXJEREc1YlA3UmRvOExlQndJVHltLytnbjFVZm00MjFHUDVYTVZ2?=
 =?utf-8?B?YkRVYWkwaVZaQ1oyb3E5TVZtQ3REM09Fc3EzdHRuYTVXYjdnKzFVUXIrMXRk?=
 =?utf-8?B?VlJSZVVTZ0lXK3lPdHM4TkFMM1MrMDNxT3VoVlhReFUwWUpIMEhBcGdzZm5Y?=
 =?utf-8?B?S1J0SmV6SjlWdk9XMmwrWFIvY2taRm5DQnVnUnBRVDRVc0Q3V0dzMFhON1Rr?=
 =?utf-8?B?cU1nelIzLzl6NGVnS1A1NXM3QnlVeGJsQmxMbTF3VmxPcFIzMDBzNWhQSUpz?=
 =?utf-8?B?OEh4YlZla3FiS0tWN3dFRmpXY3RGOGlTOTFaSWN1R1B6SmRzOXZ1a3pqQmkx?=
 =?utf-8?B?ZU5rNE43SUUyam1WRmpUWER2LzNqdnRBeE9WU0hVMkdXMHZ1WVZNeHY1a1pL?=
 =?utf-8?B?U3VXRmdYY1ZrVjFrRUZXSzd0Q3ZrZjZheEpjdjJMcFdSNytJYU01S3VBUXp3?=
 =?utf-8?B?MGU1L3NBV1RuZHQ2aGVQeHpJYzBZSmxxeEN3K3FDSnF4VU5EZG5sRUhlZnJB?=
 =?utf-8?B?V2VPSnczYm0rdUJXcjFoVWFFTzAzLzV6cjh1Q21NeGxjRklNV0VTMk1DM1E1?=
 =?utf-8?B?OHRzQjl4cU91UE5RRDZPdVhKY05VenhIUUNPNlB3WlpQZ1F3cTNzZEdFYjBv?=
 =?utf-8?B?UG5ORWV2dXZkbTZNbEJqYkJvcGZGa3Q4SlhBbHdyRHFNeUE0MmRUdW9DM3BX?=
 =?utf-8?B?UFJXWFQ3RXpzT0RpV0hET1dqdjFSR3dYTi9sZGptZGN0dFF6RVoxaWRwNHpr?=
 =?utf-8?B?ZS9xa1ZnZ1BHNm8zZVIvZDEydHlWdVVDdUQ3dDRscG9oQXk1VG1qMXdYc2hC?=
 =?utf-8?B?OXEvaWduODAxMzRlV01RbXZOQnNKVjZaTDArZWJuQy9WY3FnVWFodU9IWks2?=
 =?utf-8?B?dkJNRkl3WVdUck1HcXRVcFp4UTNSRVB6Tm1RRVJGMm1rcU1ZVnRsTGNIUkFB?=
 =?utf-8?B?aW9JL08rc044T1FDUGYzV1NqTE1MamFVL3U5UGJCazYzM1FkRG9TQTdxOHh3?=
 =?utf-8?B?aExkSE5IbDZzZTJxZ3lkWjdZdjNzbml1L0RtMnJGckpUdGc1Rmx1RHJRbHBU?=
 =?utf-8?B?VHhHLzA1L0JKTElNS1lLeUxvUktZT3o4L1RmMGREeFBlRFl0VXVyaFg5WUh4?=
 =?utf-8?B?UDEzRGpubXljdXJhWHMwZERxN1Rpc2k4bFZHdmpzSmpWL0lOWlplaHNWTG43?=
 =?utf-8?B?V1JYeUJhNmMvMzRJeXZjMTBFeVRnN2ZTMWZOVUpkaUd2d2wrY09Sd2RaSE5M?=
 =?utf-8?B?L296Ty9mbG50K3o3UEl4KytoVno5MnJ0YnJMMy9iQkdGYzdXMnVyYVVkN2RC?=
 =?utf-8?B?Q0hNSHNMbE90aWVvMWhOOXJOUDZVL1NYRmhDUlR1cExBZVNzTzJQYUM0UGhI?=
 =?utf-8?B?VEI0V1pNUk1YcU1YaVFsMmNnT0lxd0FZdGxUcittNTFLSExOSk9TcDgzQUNa?=
 =?utf-8?B?NzhPaVU1NzNvckRGRm9EekNNTnppT0J6SU41TEFnMnNYTExKMHFpa3NTbkV0?=
 =?utf-8?B?U1NQeEJETnVscU9ma3VYUUY5RWtnblVyMWp6V1RhNWpzdGNpZUo0SHZrS2hX?=
 =?utf-8?B?dUFuODBIUU94eHkvcHlNZktkQzNFQzR0QjMwWEluaGplcEI1OTh1R2wyay9a?=
 =?utf-8?B?RHF0SnRqeVFzaHdoZG5GRXdpR2UyUEY3ODRuNU1sZmRqQUROYWU1U2dFUTdh?=
 =?utf-8?B?UHZ6YnlFNTlGaWpsUkQ5M2VqMHJOMCt0eWhzVFhCbmp2TDVONU1KQitGR3RL?=
 =?utf-8?B?SmVvTHNpUG1JOEJwU0RhRzU5K20wZDBEWUU1aUx6b085VmJ5ZDhOQnh1K2lM?=
 =?utf-8?B?Y3QrY3RZSlRNdjR6T01ZQlhQMUdQNmw2TGtKYnlkRk9ia21PTmtXUnRWeU5V?=
 =?utf-8?B?RTM4YkVjNmxhckVnVDNEUCtHcndSTDd6S1V3MkpLbm1YWHd5ZDcveUZmUE82?=
 =?utf-8?B?eWd1T094Q0pBWWF5TTBhOWI3ME44aGROcnNGc0dBYUdJS1Y0RGtqYmJzaStI?=
 =?utf-8?B?T1JJWkRIQnFLTjZ2MXpPcjJhOVlzRDdXYUNySWxXRTdGbUNFVlFWakRZN0NK?=
 =?utf-8?B?YUNmZW9nYzJabnArVzdpUmYxS3BnaHhsL2lxc1pVRm1lNVQ2ejlmbktSUlVa?=
 =?utf-8?B?Vkg2dGpKTFVzQVdmRU9QWFgreXM4N3d1bnVKU1BRck94OE15U2tMU1U3Vkxq?=
 =?utf-8?B?bnl3dms0M1FhaE9weGR1dnhXZ081VVFjbTc1K2NMNFVFa3FJbXhVZFNnY1pn?=
 =?utf-8?B?YnZ5eXFJRi82VnR0eDQxY1FWWGpRPT0=?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8443c0ee-b1ad-4193-2960-08dad946e4b9
X-MS-Exchange-CrossTenant-AuthSource: CH0PR10MB5113.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2022 18:06:13.6824
 (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: jXLyul5ql4XKYBFZ1M9truGV2zqtgocfnMXC474vF7UNd/SPI1fRi8+uaAQ8xbzhmfEk4fh92ZBAZUqO6m5AxvWqTfah7acq6G7LG4nmOk0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR10MB4136
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-08_11,2022-12-08_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 suspectscore=0
 mlxscore=0 spamscore=0 adultscore=0 bulkscore=0 phishscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2210170000 definitions=main-2212080152
X-Proofpoint-GUID: zRZhzWvattaM6IpwMSYFsQd9Ee_JIa7A
X-Proofpoint-ORIG-GUID: zRZhzWvattaM6IpwMSYFsQd9Ee_JIa7A
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 6:27 PM, John Hubbard wrote:
> On 12/7/22 17:42, Sidhartha Kumar wrote:
>>> Wouldn't it be better to instead just create a new function for that
>>> case, such as:
>>>
>>>      dissolve_large_folio()
>>>
>>
>> Prior to the folio conversion, the helper function 
>> __destroy_compound_gigantic_page() did:
>>
>>      set_compound_order(page, 0);
>> #ifdef CONFIG_64BIT
>>      page[1].compound_nr = 0;
>> #endif
>>
>> as part of dissolving the page. My goal for this patch was to create a 
>> function that would encapsulate that segment of code with a single 
>> call of folio_set_compound_order(folio, 0). set_compound_order() does 
>> not set compound_nr to 0 when 0 is passed in to the order argument so 
>> explicitly setting it is required. I don't think a separate 
>> dissolve_large_folio() function for the hugetlb case is needed as 
>> __destroy_compound_gigantic_folio() is pretty concise as it is.
>>
> 
> Instead of "this is abusing function X()" comments, we should prefer
> well-named functions that do something understandable. And you can get
> that by noticing that folio_set_compound_order() collapses down to
> nearly nothing in the special "order 0" case. So just inline that code
> directly into __destroy_compound_gigantic_folio(), taking a moment to
> fill in and consolidate the CONFIG_64BIT missing parts in mm.h.
> 
> And now you can get rid of this cruft and "abuse" comment, and instead
> just end up with two simple lines of code that are crystal clear--as
> they should be, in a "__destroy" function. Like this:
> 
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 105878936485..cf227ed00945 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1754,6 +1754,7 @@ static inline void set_page_links(struct page 
> *page, enum zone_type zone,
>   #endif
>   }
> 
> +#ifdef CONFIG_64BIT
>   /**
>    * folio_nr_pages - The number of pages in the folio.
>    * @folio: The folio.
> @@ -1764,13 +1765,32 @@ static inline long folio_nr_pages(struct folio 
> *folio)
>   {
>       if (!folio_test_large(folio))
>           return 1;
> -#ifdef CONFIG_64BIT
>       return folio->_folio_nr_pages;
> +}
> +
> +static inline void folio_set_nr_pages(struct folio *folio, long nr_pages)
> +{
> +    folio->_folio_nr_pages = nr_pages;
> +}
>   #else
> +/**
> + * folio_nr_pages - The number of pages in the folio.
> + * @folio: The folio.
> + *
> + * Return: A positive power of two.
> + */
> +static inline long folio_nr_pages(struct folio *folio)
> +{
> +    if (!folio_test_large(folio))
> +        return 1;
>       return 1L << folio->_folio_order;
> -#endif
>   }
> 
> +static inline void folio_set_nr_pages(struct folio *folio, long nr_pages)
> +{
> +}
> +#endif
> +
>   /**
>    * folio_next - Move to the next physical folio.
>    * @folio: The folio we're currently operating on.
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index e3500c087893..b507a98063e6 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1344,7 +1344,8 @@ static void 
> __destroy_compound_gigantic_folio(struct folio *folio,
>               set_page_refcounted(p);
>       }
> 
> -    folio_set_compound_order(folio, 0);
> +    folio->_folio_order = 0;
> +    folio_set_nr_pages(folio, 0);
>       __folio_clear_head(folio);
>   }
> 
> 
> Yes? 

This works for me, I will take this approach along with Muchun's 
feedback about a wrapper function so as not to touch _folio_order 
directly and send out a new version.

One question I have is if I should then get rid of 
folio_set_compound_order() as hugetlb is the only compound page user 
I've converted to folios so far and its use can be replaced by the 
suggested folio_set_nr_pages() and folio_set_order().

Hugetlb also has one has one call to folio_set_compound_order() with a 
non-zero order, should I replace this with a call to folio_set_order() 
and folio_set_nr_pages() as well, or keep folio_set_compound_order() and 
remove zero order support and the comment. Please let me know which 
approach you would prefer.

Thanks,
Sidhartha Kumar


> thanks,