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@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
>
> 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.
