Re: [PATCH 23/24] drm/i915: Use a slab for object allocation

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue,  4 Sep 2012 21:03:15 +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>
> ---
>  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 2c09900..f2e3439 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1760,6 +1760,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 f19a4f2..ec8c0fc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -391,6 +391,7 @@ struct intel_gmbus {
>  
>  typedef struct drm_i915_private {
>  	struct drm_device *dev;
> +	struct kmem_cache *slab;
>  
>  	const struct intel_device_info *info;
>  
> @@ -1316,12 +1317,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 2c04ea4..a32d3eb 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;
>  	}
>  
> @@ -3770,12 +3782,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;
>  	}
>  
> @@ -3858,7 +3870,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
> @@ -4236,8 +4248,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 d91f6eb..fc9228a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -255,7 +255,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;
>  
> @@ -280,7 +280,7 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
>  	return obj;
>  
>  cleanup:
> -	kfree(obj);
> +	i915_gem_object_free(obj);
>  	return NULL;
>  }
>  

Nice.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center