diff mbox

[5/6] drm,i915: Introduce drm_malloc_gfp()

Message ID 1459861057-25931-6-git-send-email-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson April 5, 2016, 12:57 p.m. UTC
I have instances where I want to use drm_malloc_ab() but with a custom
gfp mask. And with those, where I want a temporary allocation, I want to
try a high-order kmalloc() before using a vmalloc().

So refactor my usage into drm_malloc_gfp().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: dri-devel@lists.freedesktop.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Acked-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/i915/i915_gem.c            |  4 +---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  8 +++-----
 drivers/gpu/drm/i915/i915_gem_gtt.c        |  5 +++--
 drivers/gpu/drm/i915/i915_gem_userptr.c    | 15 ++++-----------
 include/drm/drm_mem_util.h                 | 19 +++++++++++++++++++
 5 files changed, 30 insertions(+), 21 deletions(-)

Comments

Chris Wilson April 5, 2016, 1:05 p.m. UTC | #1
On Tue, Apr 05, 2016 at 01:57:36PM +0100, Chris Wilson wrote:
> I have instances where I want to use drm_malloc_ab() but with a custom
> gfp mask. And with those, where I want a temporary allocation, I want to
> try a high-order kmalloc() before using a vmalloc().
> 
> So refactor my usage into drm_malloc_gfp().
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Acked-by: Dave Airlie <airlied@redhat.com>

> +static __inline__ void *drm_malloc_gfp(size_t nmemb, size_t size, gfp_t gfp)
> +{
> +	if (size != 0 && nmemb > SIZE_MAX / size)
> +		return NULL;

I know Dave G. has some fancy code to detect when the size parameter is
not constant, but one thing I noticed was that gcc would uninline this
function and we would lose the constant folding. Is there anything we
can do to convince gcc to avoid a div here (other than pure macro)?
-Chris
Tvrtko Ursulin April 6, 2016, 9:40 a.m. UTC | #2
On 05/04/16 14:05, Chris Wilson wrote:
> On Tue, Apr 05, 2016 at 01:57:36PM +0100, Chris Wilson wrote:
>> I have instances where I want to use drm_malloc_ab() but with a custom
>> gfp mask. And with those, where I want a temporary allocation, I want to
>> try a high-order kmalloc() before using a vmalloc().
>>
>> So refactor my usage into drm_malloc_gfp().
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Acked-by: Dave Airlie <airlied@redhat.com>
>
>> +static __inline__ void *drm_malloc_gfp(size_t nmemb, size_t size, gfp_t gfp)
>> +{
>> +	if (size != 0 && nmemb > SIZE_MAX / size)
>> +		return NULL;
>
> I know Dave G. has some fancy code to detect when the size parameter is
> not constant, but one thing I noticed was that gcc would uninline this
> function and we would lose the constant folding. Is there anything we
> can do to convince gcc to avoid a div here (other than pure macro)?

Don't know, apart from maybe _always_inline if it is not considered too big.

But I wanted to ask, why it is interesting to allow size == 0 ? Why not:

	if (size == 0 || nmemb > SIZE_MAX / size)
		return NULL;

?

Regards,

Tvrtko
Chris Wilson April 6, 2016, 9:47 a.m. UTC | #3
On Wed, Apr 06, 2016 at 10:40:19AM +0100, Tvrtko Ursulin wrote:
> 
> On 05/04/16 14:05, Chris Wilson wrote:
> >On Tue, Apr 05, 2016 at 01:57:36PM +0100, Chris Wilson wrote:
> >>I have instances where I want to use drm_malloc_ab() but with a custom
> >>gfp mask. And with those, where I want a temporary allocation, I want to
> >>try a high-order kmalloc() before using a vmalloc().
> >>
> >>So refactor my usage into drm_malloc_gfp().
> >>
> >>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>Cc: dri-devel@lists.freedesktop.org
> >>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>Acked-by: Dave Airlie <airlied@redhat.com>
> >
> >>+static __inline__ void *drm_malloc_gfp(size_t nmemb, size_t size, gfp_t gfp)
> >>+{
> >>+	if (size != 0 && nmemb > SIZE_MAX / size)
> >>+		return NULL;
> >
> >I know Dave G. has some fancy code to detect when the size parameter is
> >not constant, but one thing I noticed was that gcc would uninline this
> >function and we would lose the constant folding. Is there anything we
> >can do to convince gcc to avoid a div here (other than pure macro)?
> 
> Don't know, apart from maybe _always_inline if it is not considered too big.
> 
> But I wanted to ask, why it is interesting to allow size == 0 ? Why not:
> 
> 	if (size == 0 || nmemb > SIZE_MAX / size)
> 		return NULL;
> 
> ?

Cargo-culting. I guess the only thought was to avoid the div-by-zero and
to fallthrough to returning kmalloc(0) for equivalent behaviour.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index be4cf13343d5..985f067c1f0e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2421,9 +2421,7 @@  void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj)
 		int n;
 
 		n = obj->base.size >> PAGE_SHIFT;
-		pages = kmalloc(n*sizeof(*pages), GFP_TEMPORARY | __GFP_NOWARN);
-		if (pages == NULL)
-			pages = drm_malloc_ab(n, sizeof(*pages));
+		pages = drm_malloc_gfp(n, sizeof(*pages), GFP_TEMPORARY);
 		if (pages != NULL) {
 			n = 0;
 			for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0ee61fd014df..6ee4f00f620c 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1783,11 +1783,9 @@  i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count,
-			     GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
-	if (exec2_list == NULL)
-		exec2_list = drm_malloc_ab(sizeof(*exec2_list),
-					   args->buffer_count);
+	exec2_list = drm_malloc_gfp(args->buffer_count,
+				    sizeof(*exec2_list),
+				    GFP_TEMPORARY);
 	if (exec2_list == NULL) {
 		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
 			  args->buffer_count);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ae9cb2735767..18f2bd7caad5 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3413,8 +3413,9 @@  intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info,
 	int ret = -ENOMEM;
 
 	/* Allocate a temporary list of source pages for random access. */
-	page_addr_list = drm_malloc_ab(obj->base.size / PAGE_SIZE,
-				       sizeof(dma_addr_t));
+	page_addr_list = drm_malloc_gfp(obj->base.size / PAGE_SIZE,
+					sizeof(dma_addr_t),
+					GFP_TEMPORARY);
 	if (!page_addr_list)
 		return ERR_PTR(ret);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 291a9393493d..67883ebf9504 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -494,10 +494,7 @@  __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	ret = -ENOMEM;
 	pinned = 0;
 
-	pvec = kmalloc(npages*sizeof(struct page *),
-		       GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
-	if (pvec == NULL)
-		pvec = drm_malloc_ab(npages, sizeof(struct page *));
+	pvec = drm_malloc_gfp(npages, sizeof(struct page *), GFP_TEMPORARY);
 	if (pvec != NULL) {
 		struct mm_struct *mm = obj->userptr.mm->mm;
 
@@ -634,14 +631,10 @@  i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 	pvec = NULL;
 	pinned = 0;
 	if (obj->userptr.mm->mm == current->mm) {
-		pvec = kmalloc(num_pages*sizeof(struct page *),
-			       GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
+		pvec = drm_malloc_gfp(num_pages, sizeof(struct page *), GFP_TEMPORARY);
 		if (pvec == NULL) {
-			pvec = drm_malloc_ab(num_pages, sizeof(struct page *));
-			if (pvec == NULL) {
-				__i915_gem_userptr_set_active(obj, false);
-				return -ENOMEM;
-			}
+			__i915_gem_userptr_set_active(obj, false);
+			return -ENOMEM;
 		}
 
 		pinned = __get_user_pages_fast(obj->userptr.ptr, num_pages,
diff --git a/include/drm/drm_mem_util.h b/include/drm/drm_mem_util.h
index e42495ad8136..741ce75a72b4 100644
--- a/include/drm/drm_mem_util.h
+++ b/include/drm/drm_mem_util.h
@@ -54,6 +54,25 @@  static __inline__ void *drm_malloc_ab(size_t nmemb, size_t size)
 			 GFP_KERNEL | __GFP_HIGHMEM, PAGE_KERNEL);
 }
 
+static __inline__ void *drm_malloc_gfp(size_t nmemb, size_t size, gfp_t gfp)
+{
+	if (size != 0 && nmemb > SIZE_MAX / size)
+		return NULL;
+
+	if (size * nmemb <= PAGE_SIZE)
+	    return kmalloc(nmemb * size, gfp);
+
+	if (gfp & __GFP_RECLAIMABLE) {
+		void *ptr = kmalloc(nmemb * size,
+				    gfp | __GFP_NOWARN | __GFP_NORETRY);
+		if (ptr)
+			return ptr;
+	}
+
+	return __vmalloc(size * nmemb,
+			 gfp | __GFP_HIGHMEM, PAGE_KERNEL);
+}
+
 static __inline void drm_free_large(void *ptr)
 {
 	kvfree(ptr);