From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 17/18] drm/i915: Use a slab for object allocation Date: Mon, 5 Nov 2012 18:10:05 +0000 Message-ID: <20121105181005.7640c441@bwidawsk.net> References: <1350666204-8101-1-git-send-email-chris@chris-wilson.co.uk> <1350666204-8101-17-git-send-email-chris@chris-wilson.co.uk> <20121105180753.59c6aebd@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from shiva.chad-versace.us (209-20-75-48.static.cloud-ips.com [209.20.75.48]) by gabe.freedesktop.org (Postfix) with ESMTP id 2152D9E9E2 for ; Mon, 5 Nov 2012 10:09:45 -0800 (PST) In-Reply-To: <20121105180753.59c6aebd@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Ben Widawsky Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Mon, 5 Nov 2012 18:07:53 +0000 Ben Widawsky wrote: > On Fri, 19 Oct 2012 18:03:23 +0100 > Chris Wilson 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 > > Reviewed-by: Jesse Barnes > > To sum up: > patches 5-11 are Reviewed-by: Ben Widawsky oops, 11 is not reviewed-by me. 11 and 12 are meh IMO. > patch 13 is Reviewed-by: Ben Widawsky > patch 14-17 is Acked-by: Ben Widawsky > > patches 1-17 are Tested-by: Ben Widawsky > > > --- > > drivers/gpu/drm/i915/i915_dma.c | 3 +++ > > drivers/gpu/drm/i915/i915_drv.h | 4 ++++ > > drivers/gpu/drm/i915/i915_gem.c | 28 +++++++++++++++++++++++----- > > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 5 ++--- > > drivers/gpu/drm/i915/i915_gem_stolen.c | 4 ++-- > > 5 files changed, 34 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > index 14271aa..69969be 100644 > > --- a/drivers/gpu/drm/i915/i915_dma.c > > +++ b/drivers/gpu/drm/i915/i915_dma.c > > @@ -1776,6 +1776,9 @@ int i915_driver_unload(struct drm_device *dev) > > > > destroy_workqueue(dev_priv->wq); > > > > + if (dev_priv->slab) > > + kmem_cache_destroy(dev_priv->slab); > > + > > pci_dev_put(dev_priv->bridge_dev); > > kfree(dev->dev_private); > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 5a0e0cd..5084b29 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -398,6 +398,7 @@ struct intel_gmbus { > > > > typedef struct drm_i915_private { > > struct drm_device *dev; > > + struct kmem_cache *slab; > > > > const struct intel_device_info *info; > > > > @@ -1341,12 +1342,15 @@ int i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data, > > int i915_gem_wait_ioctl(struct drm_device *dev, void *data, > > struct drm_file *file_priv); > > void i915_gem_load(struct drm_device *dev); > > +void *i915_gem_object_alloc(struct drm_device *dev); > > +void i915_gem_object_free(struct drm_i915_gem_object *obj); > > int i915_gem_init_object(struct drm_gem_object *obj); > > void i915_gem_object_init(struct drm_i915_gem_object *obj, > > const struct drm_i915_gem_object_ops *ops); > > struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > > size_t size); > > void i915_gem_free_object(struct drm_gem_object *obj); > > + > > int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj, > > uint32_t alignment, > > bool map_and_fenceable, > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 0349899..8183c0f 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -193,6 +193,18 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data, > > return 0; > > } > > > > +void *i915_gem_object_alloc(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + return kmem_cache_alloc(dev_priv->slab, GFP_KERNEL | __GFP_ZERO); > > +} > > + > > +void i915_gem_object_free(struct drm_i915_gem_object *obj) > > +{ > > + struct drm_i915_private *dev_priv = obj->base.dev->dev_private; > > + kmem_cache_free(dev_priv->slab, obj); > > +} > > + > > static int > > i915_gem_create(struct drm_file *file, > > struct drm_device *dev, > > @@ -216,7 +228,7 @@ i915_gem_create(struct drm_file *file, > > if (ret) { > > drm_gem_object_release(&obj->base); > > i915_gem_info_remove_obj(dev->dev_private, obj->base.size); > > - kfree(obj); > > + i915_gem_object_free(obj); > > return ret; > > } > > > > @@ -3780,12 +3792,12 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev, > > struct address_space *mapping; > > u32 mask; > > > > - obj = kzalloc(sizeof(*obj), GFP_KERNEL); > > + obj = i915_gem_object_alloc(dev); > > if (obj == NULL) > > return NULL; > > > > if (drm_gem_object_init(dev, &obj->base, size) != 0) { > > - kfree(obj); > > + i915_gem_object_free(obj); > > return NULL; > > } > > > > @@ -3868,7 +3880,7 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj) > > i915_gem_info_remove_obj(dev_priv, obj->base.size); > > > > kfree(obj->bit_17); > > - kfree(obj); > > + i915_gem_object_free(obj); > > } > > > > int > > @@ -4246,8 +4258,14 @@ init_ring_lists(struct intel_ring_buffer *ring) > > void > > i915_gem_load(struct drm_device *dev) > > { > > - int i; > > drm_i915_private_t *dev_priv = dev->dev_private; > > + int i; > > + > > + dev_priv->slab = > > + kmem_cache_create("i915_gem_object", > > + sizeof(struct drm_i915_gem_object), 0, > > + SLAB_HWCACHE_ALIGN, > > + NULL); > > > > INIT_LIST_HEAD(&dev_priv->mm.active_list); > > INIT_LIST_HEAD(&dev_priv->mm.inactive_list); > > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > > index ca3497e..f307e31 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > > @@ -276,8 +276,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, > > if (IS_ERR(attach)) > > return ERR_CAST(attach); > > > > - > > - obj = kzalloc(sizeof(*obj), GFP_KERNEL); > > + obj = i915_gem_object_alloc(dev); > > if (obj == NULL) { > > ret = -ENOMEM; > > goto fail_detach; > > @@ -285,7 +284,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, > > > > ret = drm_gem_private_object_init(dev, &obj->base, dma_buf->size); > > if (ret) { > > - kfree(obj); > > + i915_gem_object_free(obj); > > goto fail_detach; > > } > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > > index 86c4af4..d7cfa71 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > > @@ -261,7 +261,7 @@ _i915_gem_object_create_stolen(struct drm_device *dev, > > { > > struct drm_i915_gem_object *obj; > > > > - obj = kzalloc(sizeof(*obj), GFP_KERNEL); > > + obj = i915_gem_object_alloc(dev); > > if (obj == NULL) > > return NULL; > > > > @@ -286,7 +286,7 @@ _i915_gem_object_create_stolen(struct drm_device *dev, > > return obj; > > > > cleanup: > > - kfree(obj); > > + i915_gem_object_free(obj); > > return NULL; > > } > > > > > -- Ben Widawsky, Intel Open Source Technology Center