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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E9C63C433F5 for ; Tue, 12 Oct 2021 14:14:05 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 736AF6108F for ; Tue, 12 Oct 2021 14:14:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 736AF6108F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id BA0326B0072; Tue, 12 Oct 2021 10:14:04 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B4BD0900002; Tue, 12 Oct 2021 10:14:04 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A13CD6B0074; Tue, 12 Oct 2021 10:14:04 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0184.hostedemail.com [216.40.44.184]) by kanga.kvack.org (Postfix) with ESMTP id 8FFAD6B0072 for ; Tue, 12 Oct 2021 10:14:04 -0400 (EDT) Received: from smtpin01.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 40D4432067 for ; Tue, 12 Oct 2021 14:14:04 +0000 (UTC) X-FDA: 78687979608.01.BFF6E98 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf08.hostedemail.com (Postfix) with ESMTP id D8D4630000B3 for ; Tue, 12 Oct 2021 14:14:02 +0000 (UTC) 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=vzbqDoMrQI4dDIP2elTM9U+IQRRS2xYzGbi6D+BZ5I8=; b=D6yu48bksI2WOZoY553k0LfYID eCvp7w9Kx5X/6LRTXJBGzO/0kyvRm4fAgu5/eYimpPlnwz9+E29zgtAi5drQQeQLKVzbEWGuIa6d8 bV2pNER81qoS7jkhdbt3Wlv/FoVNt0fsJ4psOUjL+WqFS+vRLO3t0o6rvODQKI0EWL32MD5i02wPT aV6UdU81pgDwMVgjQ44cV2xCGUock6xyeLUGyvH3DczUetCzSCTWRomIA2il1kCA1qDisuKWTpsBP q+NV75K3VIFwpIyxUjuJxq/xLzU+bhiq62Y7PjPoNZrTy72iG6cX3zDiQ456CMrNfQWZ1PecR+n78 Vj3a6IAw==; Received: from willy by casper.infradead.org with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1maIWi-006ZUU-VK; Tue, 12 Oct 2021 14:13:21 +0000 Date: Tue, 12 Oct 2021 15:13:08 +0100 From: Matthew Wilcox To: David Hildenbrand Cc: linux-mm@kvack.org Subject: Re: [PATCH 03/62] mm: Split slab into its own type Message-ID: References: <20211004134650.4031813-1-willy@infradead.org> <20211004134650.4031813-4-willy@infradead.org> <02a055cd-19d6-6e1d-59bb-e9e5f9f1da5b@redhat.com> <425cd66f-2040-4278-6149-69a329a82f79@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <425cd66f-2040-4278-6149-69a329a82f79@redhat.com> X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: D8D4630000B3 X-Stat-Signature: pzpt3418ny3picf9yd5n8tz17mi891p4 Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=D6yu48bk; dmarc=none; spf=none (imf08.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org X-HE-Tag: 1634048042-317128 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 Tue, Oct 12, 2021 at 09:25:02AM +0200, David Hildenbrand wrote: > > > What I can see is that we want (and must right now for generic > > > infrastructure) keep some members of the the struct page" (e.g., flags, > > > _refcount) at the very same place, because generic infrastructure relies on > > > them. > > > > > > Maybe that has already been discussed somewhere deep down in folio mail > > > threads, but I would have expected that we keep struct-page generic inside > > > struct-page and only have inside "struct slab" what's special for "struct > > > slab". > > > > > > I would have thought that we want something like this (but absolutely not > > > this): > > > > > > struct page_header { > > > unsigned long flags; > > > } > > > > > > struct page_footer { > > > atomic_t _refcount; > > > #ifdef CONFIG_MEMCG > > > unsigned long memcg_data; > > > #endif > > > } > > > > > > struct page { > > > struct page_header header; > > > uint8_t reserved[$DO_THE_MATH] > > > struct page_footer footer; > > > }; > > > > The problem with this definition is the number of places which refer > > to page->flags and must now be churned to page->header.flags. > > _refcount is rather better encapsulated, and I'm not entirely sure > > how much we'd have to do for memcg_data. Maybe that was what you meant > > by "this but absolutely not this"? I don't quite understand what that > > was supposed to mean. > > Exactly what you mentioned above (changing all callers) is what I didn't > want :) > > I was thinking of a way to have these "fixed fields" be defined only once, > and ideally, perform any access to these fields via the "struct page" > instead of via the "struct slab". > > Like, when wanting to set a page flag with a slab, access them > > ((struct page *)slab)->flags > > instead of using slab->flags. Essentially not duplicating these fields and > accessing them via the "refined" page types, but only putting a "reserved" > placeholder in. That would mean that we wouldn't need patch #1 and #2, > because we wouldn't be passing around pgflags (using whatever fancy type we > will decide on), all such accesses would continue going via the "struct > page" -- because that's where these common fields actually reside in right > now at places we cannot simply change -- because common infrastructure (PFN > walkers, ...) heavily relyies on the flags, the refcount and even the > mapcount (page types) being set accordingly. OK, I think I see what you want: struct slab { struct page_header _1; ... slab specific stuff ... struct page_footer _2; }; and then cast struct slab to struct page inside slab_nid(), slab_address(), slab_pgdat(), slab_order() and so on. To a certain extent, I think that's just an implementation detail; the important part is the use of struct slab, and then calling slab_nid() instead of page_nid(). A wrinkle here is that slab does use some of the bits in page->flags for its own purposes, eg PG_active becomes PG_pfmemalloc for PageSlab pages. One of the things I did in the folio patches that I'm not too fond of now is: struct folio { union { struct { ... }; struct page page; }; }; so that I could do &folio->page instead of casting to struct page. But maybe both of these approaches are just bad ideas, and I should do: static inline void slab_clear_pfmemalloc(struct slab *slab) { PageClearActive(slab_page(slab)); } instead of the current: clear_bit(PG_pfmemalloc, &slab->flags);