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 D9797C001DE for ; Thu, 10 Aug 2023 21:43:58 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 011DA6B0071; Thu, 10 Aug 2023 17:43:58 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F045C6B0072; Thu, 10 Aug 2023 17:43:57 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DF46B6B0074; Thu, 10 Aug 2023 17:43:57 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id D091A6B0071 for ; Thu, 10 Aug 2023 17:43:57 -0400 (EDT) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 49B9AB1A8E for ; Thu, 10 Aug 2023 21:43:57 +0000 (UTC) X-FDA: 81109522914.06.90033D5 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf20.hostedemail.com (Postfix) with ESMTP id 761741C000B for ; Thu, 10 Aug 2023 21:43:53 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b="RgqS/6bT"; dmarc=none; spf=none (imf20.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1691703835; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=cfv6u3KXjLTD2Hyi3b7y8e+BLY07iCzMi/8rXJR4Pck=; b=T5rpPkJ7o0o/3FWQOg5+njBl5LqQJYPTPHL/cDYodY8o0/eXTq4RGaLmRovYPHaqlCGTIv leWsEpHlXsbYuZ3ofvnNTbZHGUQ0ZqwkfGDdes/tWNIwjbPn8hodJnA757EJdGd4BlQjdp p3Uof9qEpzR473VfxqqRsPbWG/bJGUI= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b="RgqS/6bT"; dmarc=none; spf=none (imf20.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1691703835; a=rsa-sha256; cv=none; b=hXyGfNPkDG6D9xk5+VfdwGn+7PFLXMs3TfEUrG/phvaImnv0Ip8YURE8/bIlBXaBQkDZO5 F3QHpT6ZIx9s0uM6oK5oO0SD+g2z4OHZnxpWHrUBrtwZ+Z8lL3732TJZuUo7C9tkOlis88 VqyBJ3Xk8yFO2Zo/vibSNMMPN3nW7/M= DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=cfv6u3KXjLTD2Hyi3b7y8e+BLY07iCzMi/8rXJR4Pck=; b=RgqS/6bTSStFoDDUNWeTP328tm shlFBhKxpBFHIATWUpOvy7PgueAx2rnvy6JCFgeCQtNKq0gWo7jWh6q51miPUODvG6rwhu4XMiYKi EkV7wq22eprurTZJf/nf0VfcqJzzI3L13O1a4nU/OLeXcvTDvh1ztgEoBSwRfPiLtuYwMvlvFdHnm jYd1xQ7R0izBtwhTwmluWPobNCKq8Hgi5fr0vmwMwDE271TP98IKfaGxaVkEouazgT7ZzNHzWAK9P b6sje6dGQm0p9s00ZYY8wAkCAWE+TaCGy0M1lBiQWEhsnCGiAJHNHKz5kIDGgbG7sPmqGsjNnfytH FnZgXtSQ==; Received: from willy by casper.infradead.org with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1qUDRY-00EspT-AN; Thu, 10 Aug 2023 21:43:44 +0000 Date: Thu, 10 Aug 2023 22:43:44 +0100 From: Matthew Wilcox To: Peter Xu Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Yu Zhao , Mike Kravetz , Yang Shi , Andrew Morton , "Kirill A . Shutemov" , Hugh Dickins , David Hildenbrand , Ryan Roberts Subject: Re: [PATCH RFC] mm: Properly document tail pages for compound pages Message-ID: References: <20230810204944.53471-1-peterx@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230810204944.53471-1-peterx@redhat.com> X-Rspamd-Queue-Id: 761741C000B X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: i7px9jkzqt1xtn8sxjpatktnnrwbehgt X-HE-Tag: 1691703833-557379 X-HE-Meta: U2FsdGVkX1952fp2nG3ZIVM6kNYW2mMk+u1inrsAPA8vN01+8DzzF1I/XO8X9339GzSzQjkphA92yWBOYPa42G3LFqSJ/9wZBH1k4ptoEkecolmjOF8LCZ7wB+J5t26jI7jUBwRUjAq8Nw9LS9cohMh/ByPuPKYWxDrpNk0Q9oR3QtzVBwdwODZ8fMQGHqawDvtcXbg7MRGKX2t9xcWlA7/FF5vPf+WpNlnsYid6lPPq8odhzpXaf8ScPyZA00WB9Ewf4LV/iqutGpf72uKIU1SEK5WuPTAUNTF1fPus0gxtW8am0pKfpnl/a30SeZHdebwiTON0S4xlfJPHidCHyDubAHzy2iET9dPSKUJlkvdyqIsd/OftcL8z+QWKea0ck9HSnYmGQv62ec6y4lBHyS7Rb5UJHlRc7F2O4rTr2Lj6tweWoqsHpZlQRYkCbxc6jQHnGdQTywC4M2EhTr+/Lk54m2stB1Bm6FU3DsSPDL+ADrpoRBYsQsJcY0JKGJcDgza+qFVORHWAiZ5gko2GJrhYgV6ELhfsevXOIb3hwfyBSjdj3msXjgnPcZlbtw+25X6SbxzlrGrEv0NfXbTo3BZF/bnUuExS57TO6Y1uRCe7QQy8KscJHiHLTEnghrgHVo21BDrqFSiUMHb+zId6d5DxhVehddgulPYhWStegg1z6/+G62oljFG1Tx3nc54/RWbd3EikBpVO71c52c7M/c4JbJXS1Sb+5QHEZY8vYJeIVNi4RPFbjjQ4Hkie7CRv7RwoR0Kbv4j3LUvWsCWrTJN3lgwUEWaYv3NkTf4xC0hi2IPL5dLsYZxwsWIbdtyU7H9KvypzS9GXwwFRA4mMC6smmHqYLy27aqsW7eALiwRs8Ud4VfwHX7YdUSG7PosdOGD+Tl1yUG/b299yMDD+TIwrlhFcx5EgcV0/eMf2st4Aprakxr/OqVtas2ZspsZcmWLXWRdg4u/LHGBiCbt VMhNVoxI XvIi/9vm7swSp+Rp+XWRbgWoyIvRCcMHZjFt/AFik+5ZL5gySpX121yqfmMrDrD5PohxTcfkegkNJw0yUIWr7UICQdYfRXo+n4ZrOEH3YLem1znsoi9BT3ThxNRSiLeKTw0Qpk4h3ISFw9O0= 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, Aug 10, 2023 at 04:49:44PM -0400, Peter Xu wrote: > Tail page struct reuse is over-comlicated. Not only because we have > implicit uses of tail page fields (mapcounts, or private for thp swap > support, etc., that we _may_ still use in the page structs, but not obvious > the relationship between that and the folio definitions), but also because > we have 32/64 bits layouts for struct page so it's unclear what we can use > and what we cannot when trying to find a new spot in folio struct. I do not like this patch. > We also have tricks like page->mapping, where we can reuse only the tail > page 1/2 but nothing more than tail page 2. It is all mostly hidden, until > someone starts to read into a VM_BUG_ON_PAGE() of __split_huge_page_tail(). We can change those BUG_ON if we want to reuse mapping in more tail pages. Ask! > Let's document it clearly on what we can use and what we can't, with 100% > explanations on each of them. Hopefully this will make: The explanations are still very page centric. I do not like the style of them, nor how you explain them. > One pitfall is I'll need to split part of the tail page 1 definition into > 32/64 bits differently, that introduced some duplications on the fields. > But hopefully that's worthwhile as it makes everything crystal clear. Not > to mention that "pitfall" also brings a benefit that we can actually define > fields in different order for 32/64 bits when we want. No. This is going to ruin kernel-doc. > + /* > + * Some of the tail page fields (out of 8 WORDs for either 32/64 There's your first mistake; struct page is not necessarily 8 WORDs. You've got 7 words for sure, then on 32-bit you have 8 because atomic_t is word-sized. memcg_data might be the 9th word, virtual could be the tenth, two awful kmsan intrusions could bring it to twelve, and last_cpupid could bring it to thirteen. Sure, it's 8 words on x86-64 with CONFIG_MEMCG enabled. But that's just your system. > + * bits archs) may not be reused by the folio object because > + * they're already been used by the page struct: > + * > + * |-------+---------------| > + * | Index | Field | > + * |-------+---------------| > + * | 0 | flag | > + * | 1 | compound_head | > + * | 2 | N/A [0] | > + * | 3 | mapping [1] | > + * | 4 | N/A [0] | > + * | 5 | private [2] | > + * | 6 | mapcount | > + * | 7 | N/A [0] | This is wrong. You mustn't reuse refcount. refcount must remain 0 on all tail pages. And you can't use anything after refcount, because it's all optional on various configurations. > + * |-------+---------------| > + * > + * [0] "N/A" marks fields that are available to leverage for the > + * large folio. N/A is a bad way to say this. "Free" or "Available" would be better. > + * [1] "mapping" field is only used for sanity check, see > + * TAIL_MAPPING. Still valid to use for tail pages 1/2. > + * (for that, see __split_huge_page_tail()). No, definitely wrong to document this. > + * [2] "private" field is used when THP_SWAP is on (disabled on 32 > + * bits, or on hugetlb folios) . Ugh, this needs to be fixed, not documented. If you really must document it, at least say that this needs to be fixed. > + */ > union { > struct { > + /* WORD 0-1: not valid to reuse */ ... so now you're repeating all the information you provided above? > unsigned long _flags_1; > unsigned long _head_1; > - /* public: */ ... did you check kernel-doc after removing this? > + /* WORD 2 */ > unsigned char _folio_dtor; > unsigned char _folio_order; > + unsigned char _holes_1[2]; No. If you need to search for holes, use pahole. > +#ifdef CONFIG_64BIT > atomic_t _entire_mapcount; > + /* WORD 3 */ > atomic_t _nr_pages_mapped; > atomic_t _pincount; > -#ifdef CONFIG_64BIT > + /* WORD 4 */ > unsigned int _folio_nr_pages; > + unsigned int _reserved_1_1; > + /* WORD 5-6: not valid to reuse */ > + unsigned long _used_1_2[2]; > + /* WORD 7 */ > + unsigned long _reserved_1_2; > +#else > + /* WORD 3 */ > + atomic_t _entire_mapcount; > + /* WORD 4 */ > + atomic_t _nr_pages_mapped; > + /* WORD 5: only valid for 32bits */ > + atomic_t _pincount; > + /* WORD 6: not valid to reuse */ > + unsigned long _used_1_2; > + /* WORD 7 */ > + unsigned long _reserved_1; > #endif > - /* private: the union with struct page is transitional */ > }; > + /* private: the union with struct page is transitional */ You don't understand why I did it like this. Again, you have to build the kernel-doc and you'll see why the private: and public: markers are where they are. There was even a thread on it, a year or two ago, where I outlined the various tradeoffs between complexity of the output and the complexity of the input.