| Message ID | 1459861057-25931-6-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 --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);