lkml.org 
[lkml]   [2016]   [Aug]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] mm/vmalloc: fix align value calculation error
From
Date
On 08/10/2016 05:28 AM, Andrew Morton wrote:
> On Fri, 5 Aug 2016 23:48:21 +0800 zijun_hu <zijun_hu@zoho.com> wrote:
>
>> From: zijun_hu <zijun_hu@htc.com>
>> 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.
>
>> --- a/include/linux/bitops.h
>> +++ b/include/linux/bitops.h
>> @@ -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
>> *
>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>> index 91f44e7..7d717f3 100644
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -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()?
>
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

From 868d3c100f41e16136eed50e47bbfb03d4a16d25 Mon Sep 17 00:00:00 2001
From: zijun_hu <zijun_hu@htc.com>
Date: Wed, 10 Aug 2016 12:13:41 +0800
Subject: [PATCH v2 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, for example
size=0x10000 -> fls_long(0x10000)=17 -> align=0x20000

get_count_order_long() is implemented and used instead of fls_long() for
fixing the bug, for example
size=0x10000 -> get_count_order_long(0x10000)=16 -> align=0x10000

Signed-off-by: zijun_hu <zijun_hu@htc.com>
---
include/linux/bitops.h | 16 ++++++++++++++++
mm/vmalloc.c | 8 ++++----
2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 299e76b..93a07d1 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -192,6 +192,22 @@ static inline unsigned fls_long(unsigned long l)
}

/**
+ * get_count_order_long - get order after rounding @l up to power of 2
+ * @l: parameter
+ *
+ * it is same as get_count_order() but with long type parameter
+ */
+static inline int get_count_order_long(unsigned long l)
+{
+ if (l == 0UL)
+ return -1;
+ else if (l & (l - 1UL))
+ return (int)fls_long(l);
+ else
+ return (int)fls_long(l) - 1;
+}
+
+/**
* __ffs64 - find first set bit in a 64 bit word
* @word: The 64 bit word
*
diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 91f44e7..80660a0 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1359,14 +1359,14 @@ static struct vm_struct *__get_vm_area_node(unsigned long size,
struct vm_struct *area;

BUG_ON(in_interrupt());
- if (flags & VM_IOREMAP)
- align = 1ul << clamp_t(int, fls_long(size),
- PAGE_SHIFT, IOREMAP_MAX_ORDER);
-
size = PAGE_ALIGN(size);
if (unlikely(!size))
return NULL;

+ if (flags & VM_IOREMAP)
+ align = 1ul << clamp_t(int, get_count_order_long(size),
+ PAGE_SHIFT, IOREMAP_MAX_ORDER);
+
area = kzalloc_node(sizeof(*area), gfp_mask & GFP_RECLAIM_MASK, node);
if (unlikely(!area))
return NULL;
--
1.9.1
> 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.
>
okay, perhaps, another patch is applied to correct this weird name issue
in the future due to it is a separate issue now
we use get_count_order_long() to consist with get_order_long() now


\
 
 \ /
  Last update: 2016-08-11 00:01    [W:0.071 / U:7.332 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site