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;
Date:   Wed, 7 Dec 2022 10:12:26 -0800
From:   Mike Kravetz <mike.kravetz@oracle.com>
To:     Muchun Song <muchun.song@linux.dev>
Cc:     Sidhartha Kumar <sidhartha.kumar@oracle.com>,
        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>
Subject: Re: [PATCH mm-unstable v5 01/10] mm: add folio dtor and order setter
 functions
Message-ID: <Y5DXivFOA+bO0IYQ@monkey>
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>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4161AF1A-9508-4DF8-B756-FEB476EB32B5@linux.dev>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?wapR3vCnI+5wTOZhkd8tt0eSvwY82xCU3wC1C6IPHfKJMctC+TvSDujws7Z9?=
 =?us-ascii?Q?PxxNTQL2h2Zjtf9Jcvg8HC0shZvGTFDh/A9w4zkngM4FBd2fZy8v+F9qLnRe?=
 =?us-ascii?Q?rH8xqffJh20wSogOLH4hqZd7yJgLfvKFvSZu+3JzxqG01nr7E4dY3mGcULMk?=
 =?us-ascii?Q?1/BqnLAh81yX77TAXg8XBPtuA7rftGkxOS5icShk8FyGY+xjanPel9XFV5+z?=
 =?us-ascii?Q?mAYqb+C9IMhjnYAXym2w+P+ttzF6vzSsAelTxmRVMjI5YXspFshdKbcA/U6c?=
 =?us-ascii?Q?pAmliZ3EEQ1cOxf78/8FyFPztWRBBJPcX4XFy08kmD5cbhybyHvQdovDyClz?=
 =?us-ascii?Q?ofY9edjcVTN9cnVvkmQXf+Kfsn+iZgWng0nblanpM9yKXE8ajNuW5CWVHXSB?=
 =?us-ascii?Q?QJwteEY7dyRcDPrCqKV25I4yUsUEzU6PdQgP8FG6V/3JN4YmQEBPSeBc3fig?=
 =?us-ascii?Q?2gJqBsPJJN9qGZu73ASdv6hHokqqxi7sHXju9AQIjLtzC/BPeozHQDy53fFy?=
 =?us-ascii?Q?tOfzkjLJPxm+s5nhcFUKCVIE77t4vok6pBC6TLlUHYpgq9oUfTjtJgQ/a+6V?=
 =?us-ascii?Q?u57Lp/D+V7m5a/HFlaqmN2/M07bIrp8GxDO3sIjUEAHZNfVUiZDF8imNQptu?=
 =?us-ascii?Q?SKFFspZfsL10DR6wlzNNVIuIPAYK1CAAkBzY+WQW5N7LgDzwwilOfwdNXieH?=
 =?us-ascii?Q?ApDwuu6EldVnes43OCuR0d0eSlLQrbnXjSyR/8IXGK5Dh0CSeNoogNdBIfX3?=
 =?us-ascii?Q?blq4u/Iwc6CgG38oVHy9uqgtZTuHfpc7BSbgJph3VjEHKW3FtjhKlMAxOy2b?=
 =?us-ascii?Q?BfG2dNwiOmNvrpKVTy5noGLZBTCD0RMNGs2UFKkGrUGxd+UsLBjt3kyZpQVv?=
 =?us-ascii?Q?/rfZ4CB4VetxlBfNspe/LVDJ5GdK+/uLeTHmHKQRsURPBRZYTOpfK1uiZB54?=
 =?us-ascii?Q?XWUFGmubA5JX0yoAIWetnexaalmDdQTpzmU3+MyXyZV3WSikFId9YKYUixCi?=
 =?us-ascii?Q?KiUuuusLSGiAfbl028F4NM/BkxFxUfT+VrdCYJs/Jj672onyKHLxcZ+kf/oF?=
 =?us-ascii?Q?WQOLMIzxhk4iswQIQVgf5+XDS5udwQN29uaPh3PF9lgy61LV8PRp4WhC2BQN?=
 =?us-ascii?Q?lK0LSOnusWxVoZn1S4GBo5T3VwNka7h8rXXAkGt99Vo0MJ1PCT+6It6QIcz2?=
 =?us-ascii?Q?XKUmtnEtvScIKy2cAQU0KPJCPujhGBaiaRMSozBvUUjF5MqwnFxtBagUR18l?=
 =?us-ascii?Q?5r1RYZXysFLGLoSEEZjzY86MFW3yCI3OE+iIC9yV6FuZ746E3fxYwFTcRR4m?=
 =?us-ascii?Q?6McDqdmigFrMZc+S0CkbnLRr9hPGrdLixE2aKtfR52p1Yy+t7Fs7wsgPDCwl?=
 =?us-ascii?Q?C/p1cNljLnqMt08+7lDyGHLVpNesymZjVnZUfzQ9+7SdC1BTxZtm3jS6IxvW?=
 =?us-ascii?Q?XqWhiTxtSiw7De2iVXnMPFMAqSxSFs1xvkgwdUoxaXY0jv/VlgNuiEylga6s?=
 =?us-ascii?Q?00C7V1AP9vc5fX9UuzWY0cVdjoLyVMxqoa9SDehvl9AWoh8RLsbA210waGP6?=
 =?us-ascii?Q?XXki3NsqBsVyO7/mXVUnFVnU5ldpQIJp/cDH+bEXyIUOmYQnYB5nU63rqsY5?=
 =?us-ascii?Q?oQ=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b3487aab-f6ab-4c5d-7fec-08dad87e9a23
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Dec 2022 18:12:30.6559
 (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: ybnbiHCTIldo6JWprkGF5qHAtiArXeImTPRSY0n4R/ncAM7e6sCI2MI8tp8izEeWC1h+D+EG6TgxTFqvXI8BrA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR10MB4134
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 mlxscore=0 spamscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2210170000 definitions=main-2212070155
X-Proofpoint-ORIG-GUID: idf7qgBJNGlmP64vYi4EnkCIvsbzhPIh
X-Proofpoint-GUID: idf7qgBJNGlmP64vYi4EnkCIvsbzhPIh
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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/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.

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.
-- 
Mike Kravetz