* [PATCH] mm/vmalloc: fix align value calculation error @ 2016-08-04 8:02 zijun_hu 2016-08-04 8:36 ` zijun_hu 0 siblings, 1 reply; 7+ messages in thread From: zijun_hu @ 2016-08-04 8:02 UTC (permalink / raw) To: Andrew Morton, tj, hannes Cc: mhocko, minchan, zijun_hu, rientjes, linux-kernel, linux-mm ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/vmalloc: fix align value calculation error 2016-08-04 8:02 [PATCH] mm/vmalloc: fix align value calculation error zijun_hu @ 2016-08-04 8:36 ` zijun_hu 2016-08-04 21:24 ` Andrew Morton 0 siblings, 1 reply; 7+ messages in thread From: zijun_hu @ 2016-08-04 8:36 UTC (permalink / raw) To: Andrew Morton, tj, hannes Cc: mhocko, minchan, zijun_hu, rientjes, linux-kernel, linux-mm On 08/04/2016 04:02 PM, zijun_hu wrote: >>From e40d1066f61394992e0167f259001ae9d2581dc1 Mon Sep 17 00:00:00 2001 > From: zijun_hu <zijun_hu@htc.com> > 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 <zijun_hu@htc.com> > --- > mm/vmalloc.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c > index 91f44e7..8b17c51 100644 > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -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; > + 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 <zijun_hu@htc.com> 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 <zijun_hu@htc.com> --- mm/vmalloc.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 91f44e7..dbbca8a 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -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)) -- 1.9.1 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/vmalloc: fix align value calculation error 2016-08-04 8:36 ` zijun_hu @ 2016-08-04 21:24 ` Andrew Morton 2016-08-05 2:27 ` zijun_hu 2016-08-05 15:48 ` zijun_hu 0 siblings, 2 replies; 7+ messages in thread From: Andrew Morton @ 2016-08-04 21:24 UTC (permalink / raw) To: zijun_hu Cc: tj, hannes, mhocko, minchan, zijun_hu, rientjes, linux-kernel, linux-mm > > 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 > > ... > > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -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)) 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 majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/vmalloc: fix align value calculation error 2016-08-04 21:24 ` Andrew Morton @ 2016-08-05 2:27 ` zijun_hu 2016-08-05 15:48 ` zijun_hu 1 sibling, 0 replies; 7+ messages in thread From: zijun_hu @ 2016-08-05 2:27 UTC (permalink / raw) To: Andrew Morton Cc: tj, hannes, mhocko, minchan, zijun_hu, rientjes, linux-kernel, linux-mm On 08/05/2016 05:24 AM, Andrew Morton wrote: >> >> 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 >> >> ... >> >> --- a/mm/vmalloc.c >> +++ b/mm/vmalloc.c >> @@ -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)) > > 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 > 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 > 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 > 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 > > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/vmalloc: fix align value calculation error 2016-08-04 21:24 ` Andrew Morton 2016-08-05 2:27 ` zijun_hu @ 2016-08-05 15:48 ` zijun_hu 2016-08-09 21:28 ` Andrew Morton 1 sibling, 1 reply; 7+ messages in thread From: zijun_hu @ 2016-08-05 15:48 UTC (permalink / raw) To: Andrew Morton Cc: tj, hannes, mhocko, minchan, zijun_hu, rientjes, linux-kernel, linux-mm On 2016/8/5 5:24, Andrew Morton wrote: >> >> 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 >> >> ... >> >> --- a/mm/vmalloc.c >> +++ b/mm/vmalloc.c >> @@ -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)) > > 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 > 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 > 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 > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/vmalloc: fix align value calculation error 2016-08-05 15:48 ` zijun_hu @ 2016-08-09 21:28 ` Andrew Morton 2016-08-10 5:32 ` zijun_hu 0 siblings, 1 reply; 7+ messages in thread From: Andrew Morton @ 2016-08-09 21:28 UTC (permalink / raw) To: zijun_hu Cc: tj, hannes, mhocko, minchan, zijun_hu, rientjes, linux-kernel, linux-mm 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()? 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. --- a/include/linux/bitops.h~mm-vmalloc-fix-align-value-calculation-error-fix +++ a/include/linux/bitops.h @@ -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 * --- a/mm/vmalloc.c~mm-vmalloc-fix-align-value-calculation-error-fix +++ a/mm/vmalloc.c @@ -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); _ -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mm/vmalloc: fix align value calculation error 2016-08-09 21:28 ` Andrew Morton @ 2016-08-10 5:32 ` zijun_hu 0 siblings, 0 replies; 7+ messages in thread From: zijun_hu @ 2016-08-10 5:32 UTC (permalink / raw) To: Andrew Morton Cc: tj, hannes, mhocko, minchan, zijun_hu, rientjes, linux-kernel, linux-mm 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-08-10 5:33 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-08-04 8:02 [PATCH] mm/vmalloc: fix align value calculation error zijun_hu 2016-08-04 8:36 ` zijun_hu 2016-08-04 21:24 ` Andrew Morton 2016-08-05 2:27 ` zijun_hu 2016-08-05 15:48 ` zijun_hu 2016-08-09 21:28 ` Andrew Morton 2016-08-10 5:32 ` zijun_hu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).