Re: [patch] SLQB slab allocator

Andi Kleen
2009-01-23T11:42:13Z

On Fri, Jan 23, 2009 at 12:25:55PM +0100, Nick Piggin wrote:
> > > +#ifdef CONFIG_SLQB_SYSFS
> > > + struct kobject kobj; /* For sysfs */
> > > +#endif
> > > +#ifdef CONFIG_NUMA
> > > + struct kmem_cache_node *node[MAX_NUMNODES];
> > > +#endif
> > > +#ifdef CONFIG_SMP
> > > + struct kmem_cache_cpu *cpu_slab[NR_CPUS];
> >
> > Those both really need to be dynamically allocated, otherwise
> > it wastes a lot of memory in the common case
> > (e.g. NR_CPUS==128 kernel on dual core system). And of course
> > on the proposed NR_CPUS==4096 kernels it becomes prohibitive.
> >
> > You could use alloc_percpu? There's no alloc_pernode
> > unfortunately, perhaps there should be one.
>
> cpu_slab is dynamically allocated, by just changing the size of
> the kmem_cache cache at boot time.

You'll always have at least the MAX_NUMNODES waste because
you cannot tell the compiler that the cpu_slab field has
moved.

> Probably the best way would
> be to have dynamic cpu and node allocs for them, I agree.

It's really needed.

> Any plans for an alloc_pernode?

It shouldn't be very hard to implement. Or do you ask if I'm volunteering? @)

> > > + * - investiage performance with memoryless nodes. Perhaps CPUs can be given
> > > + * a default closest home node via which it can use fastpath functions.
> >
> > FWIW that is what x86-64 always did. Perhaps you can just fix ia64 to do
> > that too and be happy.
>
> What if the node is possible but not currently online?

Nobody should allocate on it then.

> > > +/* Not all arches define cache_line_size */
> > > +#ifndef cache_line_size
> > > +#define cache_line_size() L1_CACHE_BYTES
> > > +#endif
> > > +
> >
> > They should. better fix them?
>
> git grep -l -e cache_line_size arch/ | egrep '\.h$'
>
> Only ia64, mips, powerpc, sparc, x86...

It's straight forward to that define everywhere.

>
> > > + if (unlikely(slab_poison(s)))
> > > + memset(start, POISON_INUSE, PAGE_SIZE << s->order);
> > > +
> > > + start += colour;
> >
> > One thing i was wondering. Did you try to disable the colouring and see
> > if it makes much difference on modern systems? They tend to have either
> > larger caches or higher associativity caches.
>
> I have tried, but I don't think I found a test where it made a
> statistically significant difference. It is not very costly to
> implement, though.

how about the memory usage?

also this is all so complicated already that every simplification helps.

> > > +#endif
> > > +
> > > +#ifdef CONFIG_NUMA
> > > +static struct kmem_cache kmem_node_cache;
> > > +static struct kmem_cache_cpu kmem_node_cpus[NR_CPUS];
> > > +static struct kmem_cache_node kmem_node_nodes[MAX_NUMNODES];
> > > +#endif
> >
> > That all needs fixing too of course.
>
> Hmm. I was hoping it could stay simple as it is just a static constant
> (for a given NR_CPUS) overhead.

The issue is that distro kernels typically run with NR_CPUS >>> num_possible_cpus()
And we'll see likely higher NR_CPUS (and MAX_NUMNODES) in the future,
but also still want to run the same kernels on really small systems (e.g.
Atom based) without wasting their memory.

So for anything NR_CPUS you should use per_cpu data -- that is correctly
sized automatically.

For MAX_NUMNODES we don't have anything equivalent currently, so
you would also need alloc_pernode() I guess.

Ok you can just use per cpu for them too and only use the first
entry in each node. That's cheating, but not too bad.

> I wonder if bootmem is still up here?

bootmem is finished when slab comes up.
>
> Could bite the bullet and do a multi-stage bootstap like SLUB, but I
> want to try avoiding that (but init code is also of course much less
> important than core code and total overheads).

For DEFINE_PER_CPU you don't need special allocation.

Probably want a DEFINE_PER_NODE() for this or see above.

>
> > > +static ssize_t align_show(struct kmem_cache *s, char *buf)
> > > +{
> > > + return sprintf(buf, "%d\n", s->align);
> > > +}
> > > +SLAB_ATTR_RO(align);
> > > +
> >
> > When you map back to the attribute you can use a index into a table
> > for the field, saving that many functions?
> >
> > > +STAT_ATTR(CLAIM_REMOTE_LIST, claim_remote_list);
> > > +STAT_ATTR(CLAIM_REMOTE_LIST_OBJECTS, claim_remote_list_objects);
> >
> > This really should be table driven, shouldn't it? That would give much
> > smaller code.
>
> Tables probably would help. I will keep it close to SLUB for now,
> though.

Hmm, then fix slub?

-Andi

--
ak@linux.intel.com -- Speaking for myself only.