From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 37367C001B2 for ; Thu, 8 Dec 2022 19:33:09 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B7C428E000E; Thu, 8 Dec 2022 14:33:08 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B2B878E0001; Thu, 8 Dec 2022 14:33:08 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9F3428E000E; Thu, 8 Dec 2022 14:33:08 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 9075B8E0001 for ; Thu, 8 Dec 2022 14:33:08 -0500 (EST) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 57808120174 for ; Thu, 8 Dec 2022 19:33:08 +0000 (UTC) X-FDA: 80220137256.09.36DF1D7 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf24.hostedemail.com (Postfix) with ESMTP id 56A88180023 for ; Thu, 8 Dec 2022 19:33:06 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b="S50po2I/"; spf=none (imf24.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1670527986; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=O5Q6FwLPWf/ojE4bErGewZWkACboNhaOT79XbgwFx/M=; b=VNso9WSgEy3qSbAk0OrjtaD9uYkWgQOS/Lv8fD039RL7EJIq4T96/Ig27KV2YistNUQYJM wgwPrjkYp+xUWWXalRXlMiaQuNWBhW5YGNy9zLFhQa8w2slVegBNGgfMxPaQAwAffdSfkv sC+vJsCTeoIzyETp4xSt/9lLLDD1YjM= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b="S50po2I/"; spf=none (imf24.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1670527986; a=rsa-sha256; cv=none; b=ADQaqAVPvIK/q/GlfTBx+sDIyo9cPiCeT+5B0Lf/rU8HwIDIbsEkLzzL046y8A+rQdv9Lf FUYzZH4gjmC3BwwXSuiHWtQz0EcH5o2mg2l+qgV91Dyre9jDo3p5Kgb6hsDZx3OtTVGVL+ sS+bW3QybbjbY4JnNohqH+Oz9/BSeOU= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Transfer-Encoding: Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Sender:Reply-To:Content-ID:Content-Description; bh=O5Q6FwLPWf/ojE4bErGewZWkACboNhaOT79XbgwFx/M=; b=S50po2I/E6Oec6KqCzMeN8/b8H sovQ29kkwnDLIDABM1CAzrokg1hirN+B4O0d1XsY9aFCZppvKUwZP4CXJ2eCtYL1i7D0+/wcbKj9h d+5YEt4PxdOr4fIyGvm/zgwelfRRFj9gnKAIxVVnrEWH0EU5SlDfbG5aadyXKR0ek4o4ZFa+qg0E3 bVcNkHofAbi5eqUO1W4jdv2cWBC8Eu9YnRhGs5Y5obaHWGckt3+1fdm3OFWozizeE+PaVIWNQbX+/ 1OCNZUg2c33pbjzM3i51WlDtgrlIB2nCnU5G7Djv6RxScYO9vNy/ei/ZvCMMsN0lEy1UoQyTbGpEQ 1EdR9KKA==; Received: from willy by casper.infradead.org with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1p3Mdm-007Fbh-3z; Thu, 08 Dec 2022 19:33:06 +0000 Date: Thu, 8 Dec 2022 19:33:06 +0000 From: Matthew Wilcox To: Sidhartha Kumar Cc: John Hubbard , linux-kernel@vger.kernel.org, linux-mm@kvack.org, akpm@linux-foundation.org, songmuchun@bytedance.com, mike.kravetz@oracle.com, tsahu@linux.ibm.com, david@redhat.com Subject: Re: [PATCH mm-unstable] mm: clarify folio_set_compound_order() zero support Message-ID: References: <20221207223731.32784-1-sidhartha.kumar@oracle.com> <92965844-c430-8b8e-d9f1-705d7578bceb@nvidia.com> <0187f9c2-e80a-9cde-68bc-c9bdbd96b6fe@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <0187f9c2-e80a-9cde-68bc-c9bdbd96b6fe@oracle.com> X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 56A88180023 X-Stat-Signature: 5ah3gx4taur3tmcibta3samgjye6kwy6 X-Rspam-User: X-HE-Tag: 1670527986-965363 X-HE-Meta: U2FsdGVkX1+qOoC4NvqbOoTC6zMf+Y2rd0sQT7Bo3y0hq18uqGvQrnlUJ9qhQ2nv0f07QiOdDVqhvohQibFFhgfio1a5OqGsCydZ7c55nBQ+LtZQwAUbYE5sTeXh7KLpGok3i820clkFzwGYiHiRKbvESn8cX44oVsWIF2atSg2Ilrrk5vT/Qg+PARxSY69mik29vCtxn/5EUtb3eKfFCzZpZZV4cvYX+R4/T3fhXlGJYgjWNuO+376E3CFhNV/9eFWnKBdDunxPuCTE7jXyxfI78y6IwjgRD6JzrPE58qRaFDZTQApeXbRw3LtDTYqRh5Q0xoTe8Rac7M1AqNYeD0UCc2nqvXP8lHnGb0t/xdjWy9N5hb3GevlkhIsSzwnKvLVRDvCITmGUt0W9yJmcy7I4zFQmdY72Dkl88ShiI4M/ZHU8k1Pxij+xXuMGIWDGf/GFmELeTxstEtPykcUjBuwkTmCtwNtGTB7cSefSqhp0USA252Yy/AHicHvEln3Y7WinN5ZbjRTlHl4iYNgoc/uigjV9x74bSFGQzY5+vLfTasM8JSSnRnmX41HCpuBRxBNDZN0D8rfFXGaD4KtGI6m1bc/nCsnws/hKyHPdSu5GwBUF70iM/sLX30rogQ7QUGEnRP0YnkhNSdCvz5uh2+nqJt4t1klZlVv++iV8+S7XFDtvrZMnefKc0IoXOAEGk1VgIfL3EoccKrRakE1MVRA+Db2hVsT3pS4zIHMzIkQYdSWK/9BX3LegbY2PcTMVZTOzaDX7ORtgJgVqAJ3/CfBPJEp2jBM0Pq/XG0wa6530B17g0MSiAGWpWe60eiGXfIeOjlaHCVEMu/2Ux5sIpRb+1fphAQxHHfSKorP5j3MoUHdxH7Ov12ZC8MdRtBPEcZCKRFiSxvDHMUZiIUZGrg== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Dec 08, 2022 at 10:06:07AM -0800, Sidhartha Kumar wrote: > 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. None of the above! Whatever we're calling this function *it does not belong* in mm.h. Anything outside the MM calling it is going to be a disaster -- can you imagine what will happen if a filesystem or device driver is handed a folio and decides "Oh, I'll just change the size of this folio"? It is an attractive nuisance and should be confined to mm/internal.h *at best*. Equally, we *must not have* separate folio_set_order() and folio_set_nr_pages(). These are the same thing! They must be kept in sync! If we are to have a folio_set_order() instead of open-coding it, then it should also update nr_pages. So, given that this is now an internal-to-mm, if not internal-to-hugetlb function, I see no reason that it should not handle the case of 0. I haven't studied what hugetlb_dissolve does, or why it can't use the standard split_folio(), but I'm sure there's a good reason.