Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1643018 > unrolled thread

[PATCH 1/2] drm: replace drm_[cm]alloc* by kvmalloc alternatives

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] drm: replace drm_[cm]alloc* by kvmalloc alternatives Michal Hocko <mhocko@kernel.org> - 2017-05-17 09:00 +0200
    Re: [PATCH 1/2] drm: replace drm_[cm]alloc* by kvmalloc alternatives Chris Wilson <chris@chris-wilson.co.uk> - 2017-05-17 09:50 +0200
      Re: [PATCH 1/2] drm: replace drm_[cm]alloc* by kvmalloc alternatives Michal Hocko <mhocko@kernel.org> - 2017-05-17 11:10 +0200
        Re: [PATCH 1/2] drm: replace drm_[cm]alloc* by kvmalloc alternatives Chris Wilson <chris@chris-wilson.co.uk> - 2017-05-17 11:20 +0200
          Re: [PATCH 1/2] drm: replace drm_[cm]alloc* by kvmalloc alternatives Michal Hocko <mhocko@kernel.org> - 2017-05-17 11:30 +0200

#1643018 — [PATCH 1/2] drm: replace drm_[cm]alloc* by kvmalloc alternatives

From: Michal Hocko <mhocko@suse.com>

drm_[cm]alloc* has grown their own kvmalloc 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 but it is the same
thing as kvmalloc_array in principle.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---

Hi,
I didn't add Reviewed-by from Chris because the original patch [1]
didn't change drm_calloc_large which I have missed in that posting.
This patch is the same otherwwise.

[1] http://lkml.kernel.org/r/20170516090606.5891-1-mhocko@kernel.org

 include/drm/drm_mem_util.h | 31 +++----------------------------
 1 file changed, 3 insertions(+), 28 deletions(-)

diff --git a/include/drm/drm_mem_util.h b/include/drm/drm_mem_util.h
index d0f6cf2e5324..a1ddf55fda67 100644
--- a/include/drm/drm_mem_util.h
+++ b/include/drm/drm_mem_util.h
@@ -31,43 +31,18 @@
 
 static __inline__ void *drm_calloc_large(size_t nmemb, size_t size)
 {
-	if (size != 0 && nmemb > SIZE_MAX / size)
-		return NULL;
-
-	if (size * nmemb <= PAGE_SIZE)
-	    return kcalloc(nmemb, size, GFP_KERNEL);
-
-	return vzalloc(size * nmemb);
+	return kvmalloc_array(nmemb, size, GFP_KERNEL | __GFP_ZERO);
 }
 
 /* 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

[toc] | [next] | [standalone]

#1643050

On Wed, May 17, 2017 at 08:55:08AM +0200, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> drm_[cm]alloc* has grown their own kvmalloc 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 but it is the same
> thing as kvmalloc_array in principle.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Just a little surprised that calloc_large users still exist.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

One more feature request from mm, can we have the 
	if (size != 0 && n > SIZE_MAX / size)
check exported by itself. It is used by both kvmalloc_array and
kmalloc_array, and in my ioctls I have it open-coded as well to
differentiate between the -EINVAL (for bogus user values) and 
genuine -ENOMEM.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[toc] | [prev] | [next] | [standalone]

#1643157

On Wed 17-05-17 08:38:09, Chris Wilson wrote:
> On Wed, May 17, 2017 at 08:55:08AM +0200, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > drm_[cm]alloc* has grown their own kvmalloc 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 but it is the same
> > thing as kvmalloc_array in principle.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> Just a little surprised that calloc_large users still exist.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks!

> One more feature request from mm, can we have the 
> 	if (size != 0 && n > SIZE_MAX / size)
> check exported by itself.

What do you exactly mean by exporting? Something like the following?
I haven't compile tested it outside of mm with different config options.
Sticking alloc_array_check into mm_types.h is kind of gross but I do not
have a great idea where to put it. A new header doesn't seem nice.
---
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7cb17c6b97de..f908b14ffc4c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -534,7 +534,7 @@ static inline void *kvzalloc(size_t size, gfp_t flags)
 
 static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags)
 {
-	if (size != 0 && n > SIZE_MAX / size)
+	if (!alloc_array_check(n, size))
 		return NULL;
 
 	return kvmalloc(n * size, flags);
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 45cdb27791a3..d7154b43a0d1 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -601,4 +601,10 @@ typedef struct {
 	unsigned long val;
 } swp_entry_t;
 
+static inline bool alloc_array_check(size_t n, size_t size)
+{
+	if (size != 0 && n > SIZE_MAX / size)
+		return false;
+	return true;
+}
 #endif /* _LINUX_MM_TYPES_H */
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 3c37a8c51921..e936ca7c55a1 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -602,7 +602,7 @@ int memcg_update_all_caches(int num_memcgs);
  */
 static inline void *kmalloc_array(size_t n, size_t size, gfp_t flags)
 {
-	if (size != 0 && n > SIZE_MAX / size)
+	if (!alloc_array_check(n, size))
 		return NULL;
 	if (__builtin_constant_p(n) && __builtin_constant_p(size))
 		return kmalloc(n * size, flags);

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1643167

On Wed, May 17, 2017 at 11:03:50AM +0200, Michal Hocko wrote:
> On Wed 17-05-17 08:38:09, Chris Wilson wrote:
> > On Wed, May 17, 2017 at 08:55:08AM +0200, Michal Hocko wrote:
> > > From: Michal Hocko <mhocko@suse.com>
> > > 
> > > drm_[cm]alloc* has grown their own kvmalloc 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 but it is the same
> > > thing as kvmalloc_array in principle.
> > > 
> > > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > 
> > Just a little surprised that calloc_large users still exist.
> > 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Thanks!
> 
> > One more feature request from mm, can we have the 
> > 	if (size != 0 && n > SIZE_MAX / size)
> > check exported by itself.
> 
> What do you exactly mean by exporting?

Just make available to others so that little things like choice between
SIZE_MAX and ULONG_MAX are consistent and actually reflect the right
limit (as dictated by kmalloc/kvmalloc/vmalloc...).

> Something like the following?
> I haven't compile tested it outside of mm with different config options.
> Sticking alloc_array_check into mm_types.h is kind of gross but I do not
> have a great idea where to put it. A new header doesn't seem nice.
> ---
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7cb17c6b97de..f908b14ffc4c 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -534,7 +534,7 @@ static inline void *kvzalloc(size_t size, gfp_t flags)
>  
>  static inline void *kvmalloc_array(size_t n, size_t size, gfp_t flags)
>  {
> -	if (size != 0 && n > SIZE_MAX / size)
> +	if (!alloc_array_check(n, size))
>  		return NULL;
>  
>  	return kvmalloc(n * size, flags);
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 45cdb27791a3..d7154b43a0d1 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -601,4 +601,10 @@ typedef struct {
>  	unsigned long val;
>  } swp_entry_t;
>  
> +static inline bool alloc_array_check(size_t n, size_t size)
> +{
> +	if (size != 0 && n > SIZE_MAX / size)
> +		return false;
> +	return true;

Just return size == 0 || n <= SIZE_MAX /size ?

Whether or not size being 0 makes for a sane user is another question.
The guideline is that size is the known constant from sizeof() or
whatever and n is the variable number to allocate.

But yes, that inline is what I want :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[toc] | [prev] | [next] | [standalone]

#1643172

On Wed 17-05-17 10:12:41, Chris Wilson wrote:
> On Wed, May 17, 2017 at 11:03:50AM +0200, Michal Hocko wrote:
[...]
> > +static inline bool alloc_array_check(size_t n, size_t size)
> > +{
> > +	if (size != 0 && n > SIZE_MAX / size)
> > +		return false;
> > +	return true;
> 
> Just return size == 0 || n <= SIZE_MAX /size ?
> 
> Whether or not size being 0 makes for a sane user is another question.
> The guideline is that size is the known constant from sizeof() or
> whatever and n is the variable number to allocate.
> 
> But yes, that inline is what I want :)

I will think about this. Maybe it will help to simplify/unify some other
users. Do you have any pointers to save me some grepping...?

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web