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 60F4EC433EF for ; Tue, 12 Oct 2021 03:31:01 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id BC1EE60F9B for ; Tue, 12 Oct 2021 03:31:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org BC1EE60F9B 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 327A76B006C; Mon, 11 Oct 2021 23:31:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2D6C9900002; Mon, 11 Oct 2021 23:31:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1C5616B0073; Mon, 11 Oct 2021 23:31:00 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0084.hostedemail.com [216.40.44.84]) by kanga.kvack.org (Postfix) with ESMTP id 098326B006C for ; Mon, 11 Oct 2021 23:31:00 -0400 (EDT) Received: from smtpin31.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id A51F61801A81F for ; Tue, 12 Oct 2021 03:30:59 +0000 (UTC) X-FDA: 78686359038.31.3B68307 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf15.hostedemail.com (Postfix) with ESMTP id 4D7DBD001F2B for ; Tue, 12 Oct 2021 03:30:59 +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=/FlDkhlCJDEgJlklXpMfkioHOPQMknur5g/o0jJp21s=; b=TsCc83P4aOe38y8iRLhHz4ytEu 7EjbzAke2mrPCzzoHhoxCK68rYFA++nP8RzUAj7CRRwCEQan1UccaRN0A8os0g5sro1mbMM0jNhAD X+ORvSlpGpj03gRFf63gxU9Gu6EVwvr+uVgNMJT+enc6EVORsZAhT3ihbCEw25T64RK2FmdcJfoeL Xet9g1YZsuAy651uznIrVVaurGmupqsqmYJ5yblSUybMjpi3SiZbmqv5rnKNNmjD9M0q0Xsu4zemP +TIcgKAf1p892sTxsUUORwyn7fPE7D2up28VHZVjUoSQZ3Nxhu4RzTSCPvAvegmRPbDg+iBmqMmlg +qogmXgQ==; Received: from willy by casper.infradead.org with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1ma8UX-006BMm-0e; Tue, 12 Oct 2021 03:30:20 +0000 Date: Tue, 12 Oct 2021 04:30:12 +0100 From: Matthew Wilcox To: Johannes Weiner Cc: linux-mm@kvack.org Subject: Re: [PATCH 00/62] Separate struct slab from struct page Message-ID: References: <20211004134650.4031813-1-willy@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 4D7DBD001F2B X-Stat-Signature: 9xbs9t8337buytdoy4rksrg55se431e6 Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=TsCc83P4; dmarc=none; spf=none (imf15.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: 1634009459-397026 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 Mon, Oct 11, 2021 at 04:07:14PM -0400, Johannes Weiner wrote: > This looks great to me. It's a huge step in disentangling struct page, > and it's already showing very cool downstream effects in somewhat > unexpected places like the memory cgroup controller. Thanks! > > I'm not entirely happy with slab_test_cache() for a predicate name. > > I actually liked SlabAllocation() better, but then I remembered that we're > > trying to get away from InterCapping, and somehow slab_test_allocation() > > didn't feel right either. > > I touched on this in the reply to the memcg patch, but I wonder if it > would be better to not have this test against struct slab at all. > > It's basically a type test that means slab_is_slab(), and its > existence implies that if you see 'struct slab' somewhere, you don't > know for sure - without having more context - whether it's been vetted > or not. This makes 'struct slab' and option/maybe type that needs a > method of self-identification. Right now it can use the PG_slab bit in > the flags field shared with the page, but if it were separately > allocated some day, that would occupy dedicated memory. > > And describing memory, that's struct page's role: to identify what's > sitting behind a random pfn or an address. "struct page should be a > pointer and a type tag", or something like that ;-) > > If instead of this: > > slab = virt_to_slab(p); > if (!slab_test_cache(slab)) { > page = (struct page *)slab; > do_page_things(page); > } else { > ... > } > > you wrote it like this: > > page = virt_to_page(p); > if (PageSlab(page)) { /* page->type */ > slab = page_slab(page); /* page->pointer */ > do_slab_things(slab); > } else { > do_bypass_things(page); > } > > it would keep the ambiguity and type resolution in the domain of the > page, and it would make struct slab a strong type that doesn't need to > self-identify. ... yeah. I'm absolutely on board with the goal of moving to each struct page being very small and having essentially a single discriminated pointer to its descriptor. eg all 2^N pages allocated to a slab would have a bit pattern of 0110 in the bottom 4 bits. So PageSlab would still make sense, and calling page_slab() on something that wasn't tagged with 0110 would cause some kind of error. But until we get to that goal, it's hard to write code that's going to work in either scenario. We do need to keep the PG_slab bit to distinguish slab pages from other kinds of memory. And we don't want to be unnecessarily calling compound_head() (although I'm willing to do that for the sake of better looking code). What if we replicated the PG_slab bit across all the tail pages today? So we could call PageSlab() on any page and instead of it doing a compound_head() lookup, it just looked at the tail->flags. Then page_slab() does the actual compound_head() lookup. I think it should be fairly cheap to set the extra bits at allocation because we just set tail->compound_head, so the lines should still be in cache. (PageSlab is marked as PF_NO_TAIL, so it calls compound_head() on every access).