Re: [PATCH] drm: use kvmalloc_array for drm_malloc*

[Michal Hocko]

On Tue 16-05-17 10:31:19, Chris Wilson wrote:
> On Tue, May 16, 2017 at 11:06:06AM +0200, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > drm_malloc* has grown their own kmalloc with vmalloc fallback
> > implementations. MM has grown kvmalloc* helpers in the meantime. Let's
> > use those because it a) reduces the code and b) MM has a better idea
> > how to implement fallbacks (e.g. do not vmalloc before kmalloc is tried
> > with __GFP_NORETRY).
> 
> Better? The same idea. The only difference I was reluctant to hand out
> large pages for long lived objects. If that's the wisdom of the core mm,
> so be it.

vmalloc tends to fragment physical memory more os it is preferable to
try the physically contiguous request first and only fall back to
vmalloc if the first attempt would be too costly or it fails.

> Reviewed-by: Chris Wilson 

thanks!
-- 
Michal Hocko
SUSE Labs
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: use kvmalloc_array for drm_malloc*

[Michal Hocko]

On Tue 16-05-17 12:09:08, Chris Wilson wrote:
> On Tue, May 16, 2017 at 12:53:52PM +0200, Michal Hocko wrote:
> > On Tue 16-05-17 10:31:19, Chris Wilson wrote:
> > > On Tue, May 16, 2017 at 11:06:06AM +0200, Michal Hocko wrote:
> > > > From: Michal Hocko 
> > > > 
> > > > drm_malloc* has grown their own kmalloc with vmalloc fallback
> > > > implementations. MM has grown kvmalloc* helpers in the meantime. Let's
> > > > use those because it a) reduces the code and b) MM has a better idea
> > > > how to implement fallbacks (e.g. do not vmalloc before kmalloc is tried
> > > > with __GFP_NORETRY).
> > > 
> > > Better? The same idea. The only difference I was reluctant to hand out
> > > large pages for long lived objects. If that's the wisdom of the core mm,
> > > so be it.
> > 
> > vmalloc tends to fragment physical memory more os it is preferable to
> > try the physically contiguous request first and only fall back to
> > vmalloc if the first attempt would be too costly or it fails.
> 
> Not relevant for the changelog in this patch, but it would be nice to
> have that written in kvmalloc() as to why the scatterring of 4k vmapped
> pages prevents defragmentation when compared to allocating large pages.

Well, it is not as much about defragmentation because both vmapped and
kmalloc allocations are very likely to be unmovable (at least
currently). Theoretically there shouldn't be a problem to make vmapped
pages movable as the ptes can be modified but this is not implemented...
The problem is that vmapped pages are more likely to break up more
larger order blocks. kmalloc will naturally break a single larger block.

> I have vague recollections of seeing the conversation, but a summary as
> to the reason why kvmalloc prefers large pages will be good for future
> reference.

Does the following sound better to you?

diff --git a/mm/util.c b/mm/util.c
index 464df3489903..87499f8119f2 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -357,7 +357,10 @@ void *kvmalloc_node(size_t size, gfp_t flags, int node)
WARN_ON_ONCE((flags & GFP_KERNEL) != GFP_KERNEL);
 
/*
-* Make sure that larger requests are not too disruptive - no OOM
+* We want to attempt a large physically contiguous block first because
+* it is less likely to fragment multiple larger blocks and therefore
+* contribute to a long term fragmentation less than vmalloc fallback.
+* However make sure that larger requests are not too disruptive - no 
OOM
 * killer and no allocation failure warnings as we have a fallback
 */
if (size > PAGE_SIZE) {

-- 
Michal Hocko
SUSE Labs
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: use kvmalloc_array for drm_malloc*

[Michal Hocko]

On Tue 16-05-17 11:22:30, Daniel Vetter wrote:
> On Tue, May 16, 2017 at 11:06:06AM +0200, Michal Hocko wrote:
> > From: Michal Hocko 
> > 
> > drm_malloc* has grown their own kmalloc with vmalloc fallback
> > implementations. MM has grown kvmalloc* helpers in the meantime. Let's
> > use those because it a) reduces the code and b) MM has a better idea
> > how to implement fallbacks (e.g. do not vmalloc before kmalloc is tried
> > with __GFP_NORETRY).
> > 
> > Signed-off-by: Michal Hocko 
> 
> Shouldn't we go one step further and just remove these wrappers, maybe
> with cocci?

my cocci sucks...

> Especially drm_malloc_gfp is surpremely pointless after this
> patch (and drm_malloc_ab probably not that useful either).

So what about the following instead? It passes allyesconfig compilation.
---
From 6325ff674d9800e5648b9a86fdd36c9d5de9692f Mon Sep 17 00:00:00 2001
From: Michal Hocko 
Date: Tue, 16 May 2017 11:00:47 +0200
Subject: [PATCH] drm: replace drm_[cm]alloc* by kvmalloc alternatives

drm_[cm]alloc* has grown their own kmalloc with vmalloc fallback
implementations. MM has grown kvmalloc* helpers in the meantime. Let's
use those because it a) reduces the code and b) MM has a better idea
how to implement fallbacks (e.g. do not vmalloc before kmalloc is tried
with __GFP_NORETRY).

drm_calloc_large needs to get __GFP_ZERO explicitly.

drm_free_large can be replaced by kvfree directly.

Signed-off-by: Michal Hocko 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c| 16 ++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 19 +++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c |  7 +-
 drivers/gpu/drm/drm_gem.c  |  6 +-
 drivers/gpu/drm/etnaviv/etnaviv_gem.c  | 12 ++--
 drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c|  4 +-
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c   | 12 ++--
 drivers/gpu/drm/exynos/exynos_drm_gem.c| 11 +--
 drivers/gpu/drm/i915/i915_debugfs.c|  4 +-
 drivers/gpu/drm/i915/i915_gem.c|  4 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 34 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c|  6 +-
 drivers/gpu/drm/i915/i915_gem_userptr.c|  8 +--
 drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c | 12 ++--
 drivers/gpu/drm/msm/msm_gem.c  | 10 +--
 drivers/gpu/drm/radeon/radeon_cs.c | 11 +--
 drivers/gpu/drm/radeon/radeon_gem.c|  2 +-
 drivers/gpu/drm/radeon/radeon_ring.c   |  4 +-
 drivers/gpu/drm/radeon/radeon_vm.c |  4 +-
 drivers/gpu/drm/ttm/ttm_tt.c   | 13 ++--
 drivers/gpu/drm/udl/udl_dmabuf.c   |  2 +-
 drivers/gpu/drm/udl/udl_gem.c  |  2 +-
 drivers/gpu/drm/vc4/vc4_gem.c  | 15 +++--
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 27 
 include/drm/drmP.h |  1 -
 include/drm/drm_mem_util.h | 78 --
 26 files changed, 126 insertions(+), 198 deletions(-)
 delete mode 100644 include/drm/drm_mem_util.h

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
index a6649874e6ce..9f0247cdda5e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
@@ -96,7 +96,7 @@ static int amdgpu_bo_list_set(struct amdgpu_device *adev,
int r;
unsigned long total_size = 0;
 
-   array = drm_malloc_ab(num_entries, sizeof(struct amdgpu_bo_list_entry));
+   array = kvmalloc_array(num_entries, sizeof(struct 
amdgpu_bo_list_entry), GFP_KERNEL);
if (!array)
return -ENOMEM;
memset(array, 0, num_entries * sizeof(struct amdgpu_bo_list_entry));
@@ -148,7 +148,7 @@ static int amdgpu_bo_list_set(struct amdgpu_device *adev,
for (i = 0; i < list->num_entries; ++i)
amdgpu_bo_unref(>array[i].robj);
 
-   drm_free_large(list->array);
+   kvfree(list->array);
 
list->gds_obj = gds_obj;
list->gws_obj = gws_obj;
@@ -163,7 +163,7 @@ static int amdgpu_bo_list_set(struct amdgpu_device *adev,
 error_free:
while (i--)
amdgpu_bo_unref([i].robj);
-   drm_free_large(array);
+   kvfree(array);
return r;
 }
 
@@ -224,7 +224,7 @@ void amdgpu_bo_list_free(struct amdgpu_bo_list *list)
amdgpu_bo_unref(>array[i].robj);
 
mutex_destroy(>lock);
-   drm_free_large(list->array);
+   kvfree(list->array);
kfree(list);
 }
 
@@ -244,8 +244,8 @@ int amdgpu_bo_list_ioctl(struct drm_device *dev, void *data,
 
int r;
 
-   info = drm_malloc_ab(args->in.bo_number,
-sizeof(struct drm_amdgpu_bo_list_entry));
+   info = kvmalloc_array(args->in.bo_number,
+

Re: [PATCH] drm: use kvmalloc_array for drm_malloc*

[Michal Hocko]

On Wed 17-05-17 08:59:44, Chris Wilson wrote:
> On Wed, May 17, 2017 at 09:44:53AM +0200, Michal Hocko wrote:
> > On Tue 16-05-17 12:09:08, Chris Wilson wrote:
> > > On Tue, May 16, 2017 at 12:53:52PM +0200, Michal Hocko wrote:
> > > > On Tue 16-05-17 10:31:19, Chris Wilson wrote:
> > > > > On Tue, May 16, 2017 at 11:06:06AM +0200, Michal Hocko wrote:
> > > > > > From: Michal Hocko 
> > > > > > 
> > > > > > drm_malloc* has grown their own kmalloc with vmalloc fallback
> > > > > > implementations. MM has grown kvmalloc* helpers in the meantime. 
> > > > > > Let's
> > > > > > use those because it a) reduces the code and b) MM has a better idea
> > > > > > how to implement fallbacks (e.g. do not vmalloc before kmalloc is 
> > > > > > tried
> > > > > > with __GFP_NORETRY).
> > > > > 
> > > > > Better? The same idea. The only difference I was reluctant to hand out
> > > > > large pages for long lived objects. If that's the wisdom of the core 
> > > > > mm,
> > > > > so be it.
> > > > 
> > > > vmalloc tends to fragment physical memory more os it is preferable to
> > > > try the physically contiguous request first and only fall back to
> > > > vmalloc if the first attempt would be too costly or it fails.
> > > 
> > > Not relevant for the changelog in this patch, but it would be nice to
> > > have that written in kvmalloc() as to why the scatterring of 4k vmapped
> > > pages prevents defragmentation when compared to allocating large pages.
> > 
> > Well, it is not as much about defragmentation because both vmapped and
> > kmalloc allocations are very likely to be unmovable (at least
> > currently). Theoretically there shouldn't be a problem to make vmapped
> > pages movable as the ptes can be modified but this is not implemented...
> > The problem is that vmapped pages are more likely to break up more
> > larger order blocks. kmalloc will naturally break a single larger block.
> > 
> > > I have vague recollections of seeing the conversation, but a summary as
> > > to the reason why kvmalloc prefers large pages will be good for future
> > > reference.
> > 
> > Does the following sound better to you?
> > 
> > diff --git a/mm/util.c b/mm/util.c
> > index 464df3489903..87499f8119f2 100644
> > --- a/mm/util.c
> > +++ b/mm/util.c
> > @@ -357,7 +357,10 @@ void *kvmalloc_node(size_t size, gfp_t flags, int node)
> > WARN_ON_ONCE((flags & GFP_KERNEL) != GFP_KERNEL);
> >  
> > /*
> > -* Make sure that larger requests are not too disruptive - no OOM
> > +* We want to attempt a large physically contiguous block first because
> > +* it is less likely to fragment multiple larger blocks and therefore
> > +* contribute to a long term fragmentation less than vmalloc fallback.
> > +* However make sure that larger requests are not too disruptive - no 
> > OOM
> >  * killer and no allocation failure warnings as we have a fallback
> >  */
> 
> Hmm, shouldn't we also teach vmalloc to allocate large chunks where
> possible - even mixing huge and normal pages? As well as avoiding pinning
> the pages and allowing migration.

Yes that would be possible and my vague recollection is that somebody
was working on something like that. Do not have any references, though.

> That comment is helping me to understand why the decison is made to
> favour kmalloc over vmalloc, thanks.

OK, I've sent this clarification to Andrew.

-- 
Michal Hocko
SUSE Labs
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: use kvmalloc_array for drm_malloc*

[Michal Hocko]

On Tue 16-05-17 15:08:56, Daniel Vetter wrote:
> On Tue, May 16, 2017 at 11:52:55AM +0200, Michal Hocko wrote:
> > On Tue 16-05-17 11:22:30, Daniel Vetter wrote:
> > > On Tue, May 16, 2017 at 11:06:06AM +0200, Michal Hocko wrote:
> > > > From: Michal Hocko 
> > > > 
> > > > drm_malloc* has grown their own kmalloc with vmalloc fallback
> > > > implementations. MM has grown kvmalloc* helpers in the meantime. Let's
> > > > use those because it a) reduces the code and b) MM has a better idea
> > > > how to implement fallbacks (e.g. do not vmalloc before kmalloc is tried
> > > > with __GFP_NORETRY).
> > > > 
> > > > Signed-off-by: Michal Hocko 
> > > 
> > > Shouldn't we go one step further and just remove these wrappers, maybe
> > > with cocci?
> > 
> > my cocci sucks...
> > 
> > > Especially drm_malloc_gfp is surpremely pointless after this
> > > patch (and drm_malloc_ab probably not that useful either).
> > 
> > So what about the following instead? It passes allyesconfig compilation.
> 
> Yeah, looks good, but perhaps rebased onto your first patch. That way we
> split the functional change from the refactor (not the first time innocent
> looking changes in i915 gem code resulted in surprises).

OK, I will split it.

> Your patch also seems to need some stuff from -rc1, and atm drm-misc is
> still pre-rc1, so I'll pull both patches in once that's sorted (I can do
> the rebase myself, since it's rather trivial). But pls remind me in case
> it falls through the cracks and isn't in linux-next by end of this week
> :-)

I have based it on top of the current linux next (next-20170516). Let me
know if other tree is more appropriate.
-- 
Michal Hocko
SUSE Labs
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: use kvmalloc_array for drm_malloc*

[Chris Wilson]

On Wed, May 17, 2017 at 09:44:53AM +0200, Michal Hocko wrote:
> On Tue 16-05-17 12:09:08, Chris Wilson wrote:
> > On Tue, May 16, 2017 at 12:53:52PM +0200, Michal Hocko wrote:
> > > On Tue 16-05-17 10:31:19, Chris Wilson wrote:
> > > > On Tue, May 16, 2017 at 11:06:06AM +0200, Michal Hocko wrote:
> > > > > From: Michal Hocko 
> > > > > 
> > > > > drm_malloc* has grown their own kmalloc with vmalloc fallback
> > > > > implementations. MM has grown kvmalloc* helpers in the meantime. Let's
> > > > > use those because it a) reduces the code and b) MM has a better idea
> > > > > how to implement fallbacks (e.g. do not vmalloc before kmalloc is 
> > > > > tried
> > > > > with __GFP_NORETRY).
> > > > 
> > > > Better? The same idea. The only difference I was reluctant to hand out
> > > > large pages for long lived objects. If that's the wisdom of the core mm,
> > > > so be it.
> > > 
> > > vmalloc tends to fragment physical memory more os it is preferable to
> > > try the physically contiguous request first and only fall back to
> > > vmalloc if the first attempt would be too costly or it fails.
> > 
> > Not relevant for the changelog in this patch, but it would be nice to
> > have that written in kvmalloc() as to why the scatterring of 4k vmapped
> > pages prevents defragmentation when compared to allocating large pages.
> 
> Well, it is not as much about defragmentation because both vmapped and
> kmalloc allocations are very likely to be unmovable (at least
> currently). Theoretically there shouldn't be a problem to make vmapped
> pages movable as the ptes can be modified but this is not implemented...
> The problem is that vmapped pages are more likely to break up more
> larger order blocks. kmalloc will naturally break a single larger block.
> 
> > I have vague recollections of seeing the conversation, but a summary as
> > to the reason why kvmalloc prefers large pages will be good for future
> > reference.
> 
> Does the following sound better to you?
> 
> diff --git a/mm/util.c b/mm/util.c
> index 464df3489903..87499f8119f2 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -357,7 +357,10 @@ void *kvmalloc_node(size_t size, gfp_t flags, int node)
>   WARN_ON_ONCE((flags & GFP_KERNEL) != GFP_KERNEL);
>  
>   /*
> -  * Make sure that larger requests are not too disruptive - no OOM
> +  * We want to attempt a large physically contiguous block first because
> +  * it is less likely to fragment multiple larger blocks and therefore
> +  * contribute to a long term fragmentation less than vmalloc fallback.
> +  * However make sure that larger requests are not too disruptive - no 
> OOM
>* killer and no allocation failure warnings as we have a fallback
>*/

Hmm, shouldn't we also teach vmalloc to allocate large chunks where
possible - even mixing huge and normal pages? As well as avoiding pinning
the pages and allowing migration.

That comment is helping me to understand why the decison is made to
favour kmalloc over vmalloc, thanks.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: use kvmalloc_array for drm_malloc*

[Daniel Vetter]

On Tue, May 16, 2017 at 11:52:55AM +0200, Michal Hocko wrote:
> On Tue 16-05-17 11:22:30, Daniel Vetter wrote:
> > On Tue, May 16, 2017 at 11:06:06AM +0200, Michal Hocko wrote:
> > > From: Michal Hocko 
> > > 
> > > drm_malloc* has grown their own kmalloc with vmalloc fallback
> > > implementations. MM has grown kvmalloc* helpers in the meantime. Let's
> > > use those because it a) reduces the code and b) MM has a better idea
> > > how to implement fallbacks (e.g. do not vmalloc before kmalloc is tried
> > > with __GFP_NORETRY).
> > > 
> > > Signed-off-by: Michal Hocko 
> > 
> > Shouldn't we go one step further and just remove these wrappers, maybe
> > with cocci?
> 
> my cocci sucks...
> 
> > Especially drm_malloc_gfp is surpremely pointless after this
> > patch (and drm_malloc_ab probably not that useful either).
> 
> So what about the following instead? It passes allyesconfig compilation.

Yeah, looks good, but perhaps rebased onto your first patch. That way we
split the functional change from the refactor (not the first time innocent
looking changes in i915 gem code resulted in surprises).

Your patch also seems to need some stuff from -rc1, and atm drm-misc is
still pre-rc1, so I'll pull both patches in once that's sorted (I can do
the rebase myself, since it's rather trivial). But pls remind me in case
it falls through the cracks and isn't in linux-next by end of this week
:-)

Thanks, Daniel

> ---
> From 6325ff674d9800e5648b9a86fdd36c9d5de9692f Mon Sep 17 00:00:00 2001
> From: Michal Hocko 
> Date: Tue, 16 May 2017 11:00:47 +0200
> Subject: [PATCH] drm: replace drm_[cm]alloc* by kvmalloc alternatives
> 
> drm_[cm]alloc* has grown their own kmalloc with vmalloc fallback
> implementations. MM has grown kvmalloc* helpers in the meantime. Let's
> use those because it a) reduces the code and b) MM has a better idea
> how to implement fallbacks (e.g. do not vmalloc before kmalloc is tried
> with __GFP_NORETRY).
> 
> drm_calloc_large needs to get __GFP_ZERO explicitly.
> 
> drm_free_large can be replaced by kvfree directly.
> 
> Signed-off-by: Michal Hocko 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c| 16 ++---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 19 +++---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c |  7 +-
>  drivers/gpu/drm/drm_gem.c  |  6 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c  | 12 ++--
>  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c|  4 +-
>  drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c   | 12 ++--
>  drivers/gpu/drm/exynos/exynos_drm_gem.c| 11 +--
>  drivers/gpu/drm/i915/i915_debugfs.c|  4 +-
>  drivers/gpu/drm/i915/i915_gem.c|  4 +-
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 34 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c|  6 +-
>  drivers/gpu/drm/i915/i915_gem_userptr.c|  8 +--
>  drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c | 12 ++--
>  drivers/gpu/drm/msm/msm_gem.c  | 10 +--
>  drivers/gpu/drm/radeon/radeon_cs.c | 11 +--
>  drivers/gpu/drm/radeon/radeon_gem.c|  2 +-
>  drivers/gpu/drm/radeon/radeon_ring.c   |  4 +-
>  drivers/gpu/drm/radeon/radeon_vm.c |  4 +-
>  drivers/gpu/drm/ttm/ttm_tt.c   | 13 ++--
>  drivers/gpu/drm/udl/udl_dmabuf.c   |  2 +-
>  drivers/gpu/drm/udl/udl_gem.c  |  2 +-
>  drivers/gpu/drm/vc4/vc4_gem.c  | 15 +++--
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 27 
>  include/drm/drmP.h |  1 -
>  include/drm/drm_mem_util.h | 78 
> --
>  26 files changed, 126 insertions(+), 198 deletions(-)
>  delete mode 100644 include/drm/drm_mem_util.h
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> index a6649874e6ce..9f0247cdda5e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> @@ -96,7 +96,7 @@ static int amdgpu_bo_list_set(struct amdgpu_device *adev,
>   int r;
>   unsigned long total_size = 0;
>  
> - array = drm_malloc_ab(num_entries, sizeof(struct amdgpu_bo_list_entry));
> + array = kvmalloc_array(num_entries, sizeof(struct 
> amdgpu_bo_list_entry), GFP_KERNEL);
>   if (!array)
>   return -ENOMEM;
>   memset(array, 0, num_entries * sizeof(struct amdgpu_bo_list_entry));
> @@ -148,7 +148,7 @@ static int amdgpu_bo_list_set(struct amdgpu_device *adev,
>   for (i = 0; i < list->num_entries; ++i)
>   amdgpu_bo_unref(>array[i].robj);
>  
> - drm_free_large(list->array);
> + kvfree(list->array);
>  
>   list->gds_obj = gds_obj;
>   list->gws_obj = gws_obj;
> @@ -163,7 +163,7 @@ static int 

Re: [PATCH] drm: use kvmalloc_array for drm_malloc*

[Chris Wilson]

On Tue, May 16, 2017 at 12:53:52PM +0200, Michal Hocko wrote:
> On Tue 16-05-17 10:31:19, Chris Wilson wrote:
> > On Tue, May 16, 2017 at 11:06:06AM +0200, Michal Hocko wrote:
> > > From: Michal Hocko 
> > > 
> > > drm_malloc* has grown their own kmalloc with vmalloc fallback
> > > implementations. MM has grown kvmalloc* helpers in the meantime. Let's
> > > use those because it a) reduces the code and b) MM has a better idea
> > > how to implement fallbacks (e.g. do not vmalloc before kmalloc is tried
> > > with __GFP_NORETRY).
> > 
> > Better? The same idea. The only difference I was reluctant to hand out
> > large pages for long lived objects. If that's the wisdom of the core mm,
> > so be it.
> 
> vmalloc tends to fragment physical memory more os it is preferable to
> try the physically contiguous request first and only fall back to
> vmalloc if the first attempt would be too costly or it fails.

Not relevant for the changelog in this patch, but it would be nice to
have that written in kvmalloc() as to why the scatterring of 4k vmapped
pages prevents defragmentation when compared to allocating large pages.
I have vague recollections of seeing the conversation, but a summary as
to the reason why kvmalloc prefers large pages will be good for future
reference.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: use kvmalloc_array for drm_malloc*

[Chris Wilson]

On Tue, May 16, 2017 at 11:06:06AM +0200, Michal Hocko wrote:
> From: Michal Hocko 
> 
> drm_malloc* has grown their own kmalloc with vmalloc fallback
> implementations. MM has grown kvmalloc* helpers in the meantime. Let's
> use those because it a) reduces the code and b) MM has a better idea
> how to implement fallbacks (e.g. do not vmalloc before kmalloc is tried
> with __GFP_NORETRY).

Better? The same idea. The only difference I was reluctant to hand out
large pages for long lived objects. If that's the wisdom of the core mm,
so be it.
Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: use kvmalloc_array for drm_malloc*

[Daniel Vetter]

On Tue, May 16, 2017 at 11:06:06AM +0200, Michal Hocko wrote:
> From: Michal Hocko 
> 
> drm_malloc* has grown their own kmalloc with vmalloc fallback
> implementations. MM has grown kvmalloc* helpers in the meantime. Let's
> use those because it a) reduces the code and b) MM has a better idea
> how to implement fallbacks (e.g. do not vmalloc before kmalloc is tried
> with __GFP_NORETRY).
> 
> Signed-off-by: Michal Hocko 

Shouldn't we go one step further and just remove these wrappers, maybe
with cocci? Especially drm_malloc_gfp is surpremely pointless after this
patch (and drm_malloc_ab probably not that useful either).
-Daniel
> ---
>  include/drm/drm_mem_util.h | 23 ++-
>  1 file changed, 2 insertions(+), 21 deletions(-)
> 
> diff --git a/include/drm/drm_mem_util.h b/include/drm/drm_mem_util.h
> index d0f6cf2e5324..b461e4e4e6db 100644
> --- a/include/drm/drm_mem_util.h
> +++ b/include/drm/drm_mem_util.h
> @@ -43,31 +43,12 @@ static __inline__ void *drm_calloc_large(size_t nmemb, 
> size_t size)
>  /* Modeled after cairo's malloc_ab, it's like calloc but without the 
> zeroing. */
>  static __inline__ void *drm_malloc_ab(size_t nmemb, size_t size)
>  {
> - if (size != 0 && nmemb > SIZE_MAX / size)
> - return NULL;
> -
> - if (size * nmemb <= PAGE_SIZE)
> - return kmalloc(nmemb * size, GFP_KERNEL);
> -
> - return vmalloc(size * nmemb);
> + return kvmalloc_array(nmemb, size, GFP_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, PAGE_KERNEL);
> + return kvmalloc_array(nmemb, size, gfp);
>  }
>  
>  static __inline void drm_free_large(void *ptr)
> -- 
> 2.11.0
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel