Re: [PATCH 17/18] drm/i915: Use a slab for object allocation

[Ben Widawsky <ben@bwidawsk.net>]

On Mon, 05 Nov 2012 20:57:05 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Mon, 5 Nov 2012 17:49:35 +0000, Ben Widawsky <ben@bwidawsk.net> wrote:
> > On Fri, 19 Oct 2012 18:03:23 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 
> > > The primary purpose of this was to debug some use-after-free memory
> > > corruption that was causing an OOPS inside drm/i915. As it turned out
> > > the corruption was being caused elsewhere and i915.ko as a major user of
> > > many objects was being hit hardest.
> > > 
> > > Indeed as we do frequent the generic kmalloc caches, dedicating one to
> > > ourselves (or at least naming one for us depending upon the core) aids
> > > debugging our own slab usage.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> > So I previously acked this patch, but you dropped that, so now you get
> > more words.
> > 
> > Why not go all the way and move all allocations we do in i915 to this
> > slab (adding a flag with whether or not to zero the memory first)? While
> > I agree in terms of space, nothing takes up as much as objects, I think
> > it would be nice to do this so we can totally track what our driver is
> > doing. In fact I would suggest this is something which core drm should
> > provide to all of the drivers (we already use drm_alloc calls in some
> > places).
> 
> I thought the slab cache was a fixed size, so presumably you mean
> introduce a slab per object we allocate? Of which the only other
> interesting one is the requests. And we don't tend to have many requests
> pending at any one time (outside of exceptional error cases), unlike the
> thousands of bo that tend to be allocated. So is it worth dedicating a
> separate slab to the few but frequently allocated requests?
> -Chris
> 

I guess I wasn't really thinking about the details. I knew in the back
of my mind that the slab cache was a fixed size, but was thinking we
could have a generically large enough object for most of our other
things (I think they're mostly small except for dev_priv). But, now
thinking about it a bit, I guess that wastes the memory we're trying to
scrutinize, so it's probably silly.

In any case, this made me review kmem_cache_create a bit, so we can bump
this patch to:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center