Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 13 Oct 2021 18:55:46 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Johannes Weiner <hannes@cmpxchg.org>
Cc:     linux-mm@kvack.org, Kent Overstreet <kent.overstreet@gmail.com>,
        "Kirill A. Shutemov" <kirill@shutemov.name>,
        Vlastimil Babka <vbabka@suse.cz>,
        Michal Hocko <mhocko@suse.com>, Roman Gushchin <guro@fb.com>,
        linux-kernel@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH 00/11] PageSlab: eliminate unnecessary compound_head()
 calls
Message-ID: <YWcdoktn30ofnsPO@casper.infradead.org>
References: <20211012180148.1669685-1-hannes@cmpxchg.org>
 <YWXgrhRDIxcoBhA1@casper.infradead.org>
 <YWXwXINogE0Qb0Ip@cmpxchg.org>
 <YWZQNj+axlIQrD5C@casper.infradead.org>
 <YWbj6+myCposUKk1@cmpxchg.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YWbj6+myCposUKk1@cmpxchg.org>
Precedence: bulk

On Wed, Oct 13, 2021 at 09:49:31AM -0400, Johannes Weiner wrote:
> On Wed, Oct 13, 2021 at 04:19:18AM +0100, Matthew Wilcox wrote:
> > For today, testing PageSlab on the tail page helps the test proceed
> > in parallel with the action.  Looking at slub's kfree() for an example:
> > 
> >         page = virt_to_head_page(x);
> >         if (unlikely(!PageSlab(page))) {
> >                 free_nonslab_page(page, object);
> >                 return;
> >         }
> >         slab_free(page->slab_cache, page, object, NULL, 1, _RET_IP_);
> > 
> > Your proposal is certainly an improvement (since gcc doesn't know
> > that compound_head(compound_head(x)) == compound_head(x)), but I
> > think checking on the tail page is even better:
> > 
> > 	page = virt_to_page(x);
> > 	if (unlikely(!PageSlab(page))) {
> > 		free_nonslab_page(compound_head(page), object);
> > 		return;
> > 	}
> > 	slab = page_slab(page);
> > 	slab_free(slab->slab_cache, slab, object, NULL, 1, _RET_IP_);
> > 
> > The compound_head() parts can proceed in parallel with the check of
> > PageSlab().
> >
> > As far as the cost of setting PageSlab, those cachelines are already
> > dirty because we set compound_head on each of those pages already
> > (or in the case of freeing, we're about to clear compound_head on
> > each of those pages).
> 
> ... but this is not. I think the performance gains from this would
> have to be significant to justify complicating page flags further.

My argument isn't really "this is more efficient", because I think
the performance gains are pretty minimal.  More that I would like to
be able to write code in the style which we'll want to use when we're
using dynamically allocated memory descriptors.  It's all just code,
and we can change it at any time, but better to change it to something
that continues to work well in the future.

I don't think we end up with "virt_to_head_page()" in a dynamically
allocated memory descriptor world.  The head page contains no different
information from the tail pages, and indeed the tail pages don't know
that they're tail pages, or where the head page is.  Or maybe they're
all tail pages.

I could see a world where we had a virt_to_memdesc() which returned
a generic memory descriptor that could be cast to a struct slab if
the flags within that memdesc said it was a slab.  But I think it works
out better to tag the memory descriptor pointer with a discriminator
that defines what the pointer is.  Plus it saves a page flag.

Maybe that's the best way to approach it -- how would you want to write
this part of kfree() when memory descriptors are dynamically allocated?