mm/vmalloc: fix align value calculation error

PATCH Superseded zijun_hu 2 participants 7 messages Aug 4, 2016 – Aug 10, 2016

Subsystems (3)

Memory management vmalloc Bitops API

lore

Share & tools

lore

zijun_hu

author

2016-08-04 08:02 UTC #0 lore raw

PATCH 1/1 mm/vmalloc: fix align value calculation error

zijun_hu

author

2016-08-04 08:37 UTC #1 lore raw

Quoted 34 lines · zijun_hu

Quoted 1 line
From e40d1066f61394992e0167f259001ae9d2581dc1 Mon Sep 17 00:00:00 2001
From: zijun_hu <[email protected]>
Date: Thu, 4 Aug 2016 14:22:52 +0800
Subject: [PATCH] mm/vmalloc: fix align value calculation error
it causes double align requirement for __get_vm_area_node() if parameter size is power of 2 and VM_IOREMAP is set in parameter flags
it is fixed by using order_base_2 instead of fls_long() due to lack of get_count_order() for long parameter
Signed-off-by zijun_hu <[email protected]>
---
 mm/vmalloc.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)
mm/vmalloc.c +4 -3
index 91f44e7..8b17c51 100644
@@ -1357,11 +1357,19 @@ static struct vm_struct *__get_vm_area_node(unsigned long size,
 {
 	struct vmap_area *va;
 	struct vm_struct *area;
+	int ioremap_size_order;
 
 	BUG_ON(in_interrupt());
-	if (flags & VM_IOREMAP)
-		align = 1ul << clamp_t(int, fls_long(size),
-				       PAGE_SHIFT, IOREMAP_MAX_ORDER);
+	if (flags & VM_IOREMAP) {
+		if (unlikely(size < 2))
+			ioremap_size_order = size;

mm/vmalloc.c static struct vm_struct *__get_vm_area_node(unsigned long size, · line 1375

Quoted 10 lines

mm/vmalloc.c +7 -0

+		else if (unlikely((signed long)size < 0))
+			ioremap_size_order = sizeof(size) * 8;
+		else
+			ioremap_size_order = order_base_2(size);
+		align = 1ul << clamp_t(int, ioremap_size_order, PAGE_SHIFT,
+				IOREMAP_MAX_ORDER);
+	}
 
 	size = PAGE_ALIGN(size);
 	if (unlikely(!size))

another fix approach is shown as follows

From: zijun_hu <[email protected]>
Date: Thu, 4 Aug 2016 14:22:52 +0800
Subject: [PATCH] mm/vmalloc: fix align value calculation error

it causes double align requirement for __get_vm_area_node() if parameter size is power of 2 and VM_IOREMAP is set in parameter flags

it is fixed by handling the specail case manually due to lack of get_count_order() for long parameter

Signed-off-by zijun_hu <[email protected]>

---
 mm/vmalloc.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

mm/vmalloc.c +8 -3

index 91f44e7..dbbca8a 100644
@@ -1357,11 +1357,16 @@ static struct vm_struct *__get_vm_area_node(unsigned long size,
 {
 	struct vmap_area *va;
 	struct vm_struct *area;
+	int ioremap_size_order;
 
 	BUG_ON(in_interrupt());
-	if (flags & VM_IOREMAP)
-		align = 1ul << clamp_t(int, fls_long(size),
-				       PAGE_SHIFT, IOREMAP_MAX_ORDER);
+	if (flags & VM_IOREMAP) {
+		ioremap_size_order = fls_long(size);
+		if (is_power_of_2(size) && size != 1)
+			ioremap_size_order--;
+		align = 1ul << clamp_t(int, ioremap_size_order, PAGE_SHIFT,
+				IOREMAP_MAX_ORDER);
+	}
 
 	size = PAGE_ALIGN(size);
 	if (unlikely(!size))

Signature · 7 lines

1.9.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2016-08-04 21:24 UTC #2 lore raw

Quoted 21 lines

it causes double align requirement for __get_vm_area_node() if parameter size is power of 2 and VM_IOREMAP is set in parameter flags
it is fixed by handling the specail case manually due to lack of get_count_order() for long parameter
...
mm/vmalloc.c +1 -3
@@ -1357,11 +1357,16 @@ static struct vm_struct *__get_vm_area_node(unsigned long size,
 {
 	struct vmap_area *va;
 	struct vm_struct *area;
+	int ioremap_size_order;
 
 	BUG_ON(in_interrupt());
-	if (flags & VM_IOREMAP)
-		align = 1ul << clamp_t(int, fls_long(size),
-				       PAGE_SHIFT, IOREMAP_MAX_ORDER);

mm/vmalloc.c static struct vm_struct *__get_vm_area_node(unsigned long size, · line 1372

Quoted 10 lines

mm/vmalloc.c +7 -0

+	if (flags & VM_IOREMAP) {
+		ioremap_size_order = fls_long(size);
+		if (is_power_of_2(size) && size != 1)
+			ioremap_size_order--;
+		align = 1ul << clamp_t(int, ioremap_size_order, PAGE_SHIFT,
+				IOREMAP_MAX_ORDER);
+	}
 
 	size = PAGE_ALIGN(size);
 	if (unlikely(!size))

I'm having trouble with this, and a more complete description would have helped!

As far as I can tell, the current code will decide the following:

size=0x10000: alignment=0x10000
size=0x0f000: alignment=0x8000

And your patch will change it so that

size=0x10000: alignment=0x8000
size=0x0f000: alignment=0x8000

Correct?

If so, I'm struggling to see the sense in this. Shouldn't we be changing things so that

size=0x10000: alignment=0x10000
size=0x0f000: alignment=0x10000

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

zijun_hu

author

2016-08-05 02:27 UTC #3 lore raw

Quoted 35 lines · Andrew Morton

Quoted 21 lines
it causes double align requirement for __get_vm_area_node() if parameter size is power of 2 and VM_IOREMAP is set in parameter flags
it is fixed by handling the specail case manually due to lack of get_count_order() for long parameter
...
mm/vmalloc.c +1 -3
@@ -1357,11 +1357,16 @@ static struct vm_struct *__get_vm_area_node(unsigned long size,
 {
 	struct vmap_area *va;
 	struct vm_struct *area;
+	int ioremap_size_order;
 
 	BUG_ON(in_interrupt());
-	if (flags & VM_IOREMAP)
-		align = 1ul << clamp_t(int, fls_long(size),
-				       PAGE_SHIFT, IOREMAP_MAX_ORDER);
mm/vmalloc.c static struct vm_struct *__get_vm_area_node(unsigned long size, · line 1372
Quoted 10 lines
mm/vmalloc.c +7 -0
+	if (flags & VM_IOREMAP) {
+		ioremap_size_order = fls_long(size);
+		if (is_power_of_2(size) && size != 1)
+			ioremap_size_order--;
+		align = 1ul << clamp_t(int, ioremap_size_order, PAGE_SHIFT,
+				IOREMAP_MAX_ORDER);
+	}
 
 	size = PAGE_ALIGN(size);
 	if (unlikely(!size))
I'm having trouble with this, and a more complete description would have helped!

Quoted 5 lines

As far as I can tell, the current code will decide the following:
size=0x10000: alignment=0x10000
size=0x0f000: alignment=0x8000

no, the current code doesn't achieve the above results as shown below size=0x10000 -> fls_long(0x10000)=17 -> alignment=0x20000 size=0x0f000 -> fls_long(0x0f000)=16 -> alignment=0x10000 it is wrong for power of 2 value such as size=0x10000

Quoted 7 lines

And your patch will change it so that
size=0x10000: alignment=0x8000
size=0x0f000: alignment=0x8000
Correct?

no, my patch will results in the following calculations size=0x10000: alignment=0x10000
size=0x0f000: alignment=0x10000

Quoted 7 lines

If so, I'm struggling to see the sense in this. Shouldn't we be changing things so that
size=0x10000: alignment=0x10000
size=0x0f000: alignment=0x10000
?

okay, it is the aim of my patch as explained above

Quoted 2 lines

zijun_hu

author

2016-08-05 15:48 UTC #4 lore raw

In reply to Andrew Morton

Quoted 35 lines · Andrew Morton

Quoted 21 lines
it causes double align requirement for __get_vm_area_node() if parameter size is power of 2 and VM_IOREMAP is set in parameter flags
it is fixed by handling the specail case manually due to lack of get_count_order() for long parameter
...
mm/vmalloc.c +1 -3
@@ -1357,11 +1357,16 @@ static struct vm_struct *__get_vm_area_node(unsigned long size,
 {
 	struct vmap_area *va;
 	struct vm_struct *area;
+	int ioremap_size_order;
 
 	BUG_ON(in_interrupt());
-	if (flags & VM_IOREMAP)
-		align = 1ul << clamp_t(int, fls_long(size),
-				       PAGE_SHIFT, IOREMAP_MAX_ORDER);
mm/vmalloc.c static struct vm_struct *__get_vm_area_node(unsigned long size, · line 1372
Quoted 10 lines
mm/vmalloc.c +7 -0
+	if (flags & VM_IOREMAP) {
+		ioremap_size_order = fls_long(size);
+		if (is_power_of_2(size) && size != 1)
+			ioremap_size_order--;
+		align = 1ul << clamp_t(int, ioremap_size_order, PAGE_SHIFT,
+				IOREMAP_MAX_ORDER);
+	}
 
 	size = PAGE_ALIGN(size);
 	if (unlikely(!size))
I'm having trouble with this, and a more complete description would have helped!

Quoted 5 lines

As far as I can tell, the current code will decide the following:
size=0x10000: alignment=0x10000
size=0x0f000: alignment=0x8000

Quoted 7 lines

And your patch will change it so that
size=0x10000: alignment=0x8000
size=0x0f000: alignment=0x8000
Correct?

no, my patch will results in the following calculations size=0x10000: alignment=0x10000
size=0x0f000: alignment=0x10000

Quoted 7 lines

If so, I'm struggling to see the sense in this. Shouldn't we be changing things so that
size=0x10000: alignment=0x10000
size=0x0f000: alignment=0x10000
?

okay, it is the aim of my patch as explained above i provide another solution as shown below i appreciate it since it is more canonical please help to review and apply it kindly

Andrew Morton

maintainer

2016-08-09 21:28 UTC #5 lore raw

Quoted 9 lines · On Fri, 5 Aug 2016 23:48:21 +0800 zijun_hu <[email protected]> wrote:

From: zijun_hu <[email protected]>
Date: Fri, 5 Aug 2016 22:10:07 +0800
Subject: [PATCH 1/1] mm/vmalloc: fix align value calculation error
it causes double align requirement for __get_vm_area_node() if parameter size is power of 2 and VM_IOREMAP is set in parameter flags
get_order_long() is implemented and used instead of fls_long() for fixing the bug

Makes sense. I think.

Quoted 31 lines

include/linux/bitops.h +17 -0

@@ -192,6 +192,23 @@ static inline unsigned fls_long(unsigned long l)
 }
 
 /**
+ * get_order_long - get order after rounding @l up to power of 2
+ * @l: parameter
+ *
+ * it is same as get_count_order() but long type parameter
+ * or 0 is returned if @l == 0UL
+ */
+static inline int get_order_long(unsigned long l)
+{
+	if (l == 0UL)
+		return 0;
+	else if (l & (l - 1UL))
+		return fls_long(l);
+	else
+		return fls_long(l) - 1;
+}
+
+/**
  * __ffs64 - find first set bit in a 64 bit word
  * @word: The 64 bit word
  *

mm/vmalloc.c +0 -0

index 91f44e7..7d717f3 100644

mm/vmalloc.c static struct vm_struct *__get_vm_area_node(unsigned long size, · line 1366

Quoted 8 lines

mm/vmalloc.c +1 -1

@@ -1360,7 +1360,7 @@ static struct vm_struct *__get_vm_area_node(unsigned long size,
 
 	BUG_ON(in_interrupt());
 	if (flags & VM_IOREMAP)
-		align = 1ul << clamp_t(int, fls_long(size),
+		align = 1ul << clamp_t(int, get_order_long(size),
 				       PAGE_SHIFT, IOREMAP_MAX_ORDER);
 
 	size = PAGE_ALIGN(size);

It would be better to call this get_count_order_long(), I think? To match get_count_order()?

get_count_order() is a weird name and perhaps both of these should be renamed to things which actually make sense. That's a separate issue.

include/linux/bitops.h renamed from include/linux/bitops.h~mm-vmalloc-fix-align-value-calculation-error-fix +17 ~~-17~~

@@ -75,6 +75,23 @@ static inline int get_count_order(unsign
 	return order;
 }
 
+/**
+ * get_count_order_long - get order after rounding @l up to power of 2
+ * @l: parameter
+ *
+ * The same as get_count_order() but accepts a long type parameter
+ * or 0 is returned if @l == 0UL
+ */
+static inline int get_count_order_long(unsigned long l)
+{
+	if (l == 0UL)
+		return 0;
+	else if (l & (l - 1UL))
+		return fls_long(l);
+	else
+		return fls_long(l) - 1;
+}
+
 static __always_inline unsigned long hweight_long(unsigned long w)
 {
 	return sizeof(w) == 4 ? hweight32(w) : hweight64(w);

@@ -192,23 +209,6 @@ static inline unsigned fls_long(unsigned
 }
 
 /**
- * get_order_long - get order after rounding @l up to power of 2
- * @l: parameter
- *
- * it is same as get_count_order() but long type parameter
- * or 0 is returned if @l == 0UL
- */
-static inline int get_order_long(unsigned long l)
-{
-	if (l == 0UL)
-		return 0;
-	else if (l & (l - 1UL))
-		return fls_long(l);
-	else
-		return fls_long(l) - 1;
-}
-
-/**
  * __ffs64 - find first set bit in a 64 bit word
  * @word: The 64 bit word
  *

mm/vmalloc.c renamed from mm/vmalloc.c~mm-vmalloc-fix-align-value-calculation-error-fix +1 -1

@@ -1360,7 +1360,7 @@ static struct vm_struct *__get_vm_area_n
 
 	BUG_ON(in_interrupt());
 	if (flags & VM_IOREMAP)
-		align = 1ul << clamp_t(int, get_order_long(size),
+		align = 1ul << clamp_t(int, get_count_order_long(size),
 				       PAGE_SHIFT, IOREMAP_MAX_ORDER);
 
 	size = PAGE_ALIGN(size);

zijun_hu

author

2016-08-10 05:33 UTC #6 lore raw

Quoted 52 lines · Andrew Morton

Quoted 9 lines · On Fri, 5 Aug 2016 23:48:21 +0800 zijun_hu <[email protected]> wrote:
From: zijun_hu <[email protected]>
Date: Fri, 5 Aug 2016 22:10:07 +0800
Subject: [PATCH 1/1] mm/vmalloc: fix align value calculation error
it causes double align requirement for __get_vm_area_node() if parameter size is power of 2 and VM_IOREMAP is set in parameter flags
get_order_long() is implemented and used instead of fls_long() for fixing the bug
Makes sense. I think.
Quoted 38 lines
include/linux/bitops.h +17 -0
@@ -192,6 +192,23 @@ static inline unsigned fls_long(unsigned long l)
 }
 
 /**
+ * get_order_long - get order after rounding @l up to power of 2
+ * @l: parameter
+ *
+ * it is same as get_count_order() but long type parameter
+ * or 0 is returned if @l == 0UL
+ */
+static inline int get_order_long(unsigned long l)
+{
+	if (l == 0UL)
+		return 0;
+	else if (l & (l - 1UL))
+		return fls_long(l);
+	else
+		return fls_long(l) - 1;
+}
+
+/**
  * __ffs64 - find first set bit in a 64 bit word
  * @word: The 64 bit word
  *
mm/vmalloc.c +1 -1
index 91f44e7..7d717f3 100644
@@ -1360,7 +1360,7 @@ static struct vm_struct *__get_vm_area_node(unsigned long size,
 
 	BUG_ON(in_interrupt());
 	if (flags & VM_IOREMAP)
-		align = 1ul << clamp_t(int, fls_long(size),
+		align = 1ul << clamp_t(int, get_order_long(size),
 				       PAGE_SHIFT, IOREMAP_MAX_ORDER);
 

Quoted 5 lines

mm/vmalloc.c static struct vm_struct *__get_vm_area_node(unsigned long size, · line 1366
Quoted 1 line
mm/vmalloc.c +0 -0
 	size = PAGE_ALIGN(size);
It would be better to call this get_count_order_long(), I think? To match get_count_order()?

yes, i agree with you to correct function name

i provide another patch called v2 based on your suggestion as shown below it have following correction against original patch v1
1) use name get_count_order_long() instead of get_order_long()
2) return -1 if @l == 0 to consist with get_order_long()
3) cast type to int before returning from get_count_order_long()
4) move up function parameter checking for __get_vm_area_node()
5) more commit message is offered to make issue and approach clear any comments about new patch is welcome

this new patch called patch v2 is shown below