Re: slab-alignment-rework.patch in -mc

[Andrea Arcangeli <andrea@suse.de>]

On Mon, Apr 19, 2004 at 06:42:36PM +0200, Manfred Spraul wrote:
> Then pass "4" as the align parameter to kmem_cache_create. That's the 
> main point of the patch: it's now possible to explicitely specify the 
> requested alignment. 32 for the 3rd level page tables, the optimal 
> number for the pte_chains, etc.
> 
> >No best-guess must
> >be made automatically by the slab code, rounding it to 16 bytes.
> >
> If you pass 0 as align to kmem_cache_create, then it's rounded to L2 
> size. It's questionable if that's really the best thing - on 
> uniprocessor, 16-byte might result is better performance - there is no 
> risk of false sharing.

my point is that 0 as align must not round to l2 size or you break
stuff. you likely have broken the vma alignment to make the most obvious
example because I don't think it's a l1 cache multiple.

Removing the HW_ALIGN bitflag isn't nearly enough to avoid breaking
stuff after your changes to the kmem_cache_create API, to avoid breaking
stuff you need to change _all_ other kmem_cache_create and replace 0
with 4 if they didn't have the HW_ALIGN parameter, you didn't do that
and things breaks, not just for small objects, but for the ones bigger
than the cachesize too.

If you fixup all other kmem_cache_create callers to pass 4 instead of 0
that could be fine with me, but I still think this removal of the
HW_ALIGN bitflag is completely worthless, you still have a branch
checking !align, so you don't seem to gain anything and you break stuff.

Note: I really appreciate the runtime evaluation of the cachesize, those
parts are fine, changing offset to "align" is also more than welcome,
only the removal of HW_ALIGN bitflag w/o s/0/4/ where HW_ALIGN was
missing is bogus and breaks stuff.