Re: [PATCH v3 07/14] slub: Remove page->counters

[Matthew Wilcox <willy@infradead.org>]

On Thu, Apr 19, 2018 at 03:42:37PM +0200, Vlastimil Babka wrote:
> On 04/18/2018 08:49 PM, Matthew Wilcox wrote:
> > From: Matthew Wilcox <mawilcox@microsoft.com>
> > 
> > Use page->private instead, now that these two fields are in the same
> > location.  Include a compile-time assert that the fields don't get out
> > of sync.
> > 
> > Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> 
> Why not retain a small union of "counters" and inuse/objects/frozens
> within the SLUB's sub-structure? IMHO it would be more obvious and
> reduce churn?

Could do.  Same issues with five layers of indentation though.
If there's consensus that that's a better way to go, then I'll redo
the series to look that way.

There is a way to reduce the indentation ... but we'd have to compile
with -fms-extensions (or -fplan9-extensions, but that wasn't added until
gcc 4.6, whereas -fms-extensions was added back in the egcs days).

-fms-extensions lets you do this:

struct a { int b; int c; };
struct d { struct a; int e; };
int init(struct d *);

int main(void)
{
	struct d d;
	init(&d);
	return d.b + d.c + d.e;
}

so we could then:

struct slub_counters {
	union {
		unsigned long counters;
		struct {
			unsigned inuse:16;
			unsigned objects:15;
			unsigned frozen:1;
		};
	};
};

struct page {
	union {
		struct {
			union {
				void *s_mem;
				struct slub_counters;

Given my employer, a request to add -fms-extensions to the Makefile
might be regarded with a certain amount of suspicion ;-)

> > @@ -358,17 +359,10 @@ static __always_inline void slab_unlock(struct page *page)
> >  
> >  static inline void set_page_slub_counters(struct page *page, unsigned long counters_new)
> >  {
> > -	struct page tmp;
> > -	tmp.counters = counters_new;
> > -	/*
> > -	 * page->counters can cover frozen/inuse/objects as well
> > -	 * as page->_refcount.  If we assign to ->counters directly
> > -	 * we run the risk of losing updates to page->_refcount, so
> > -	 * be careful and only assign to the fields we need.
> > -	 */
> > -	page->frozen  = tmp.frozen;
> > -	page->inuse   = tmp.inuse;
> > -	page->objects = tmp.objects;
> 
> BTW was this ever safe to begin with? IIRC bitfields are frowned upon as
> a potential RMW. Or is there still at least guarantee the RMW happens
> only within the 32bit struct and not the whole 64bit word, which used to
> include also _refcount?

I prefer not to think about it.  Indeed, I don't think that doing
page->tmp = tmp; where both are 32-bit quantities is guaranteed to not
do an RMW.