From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1422723AbcHEPsc (ORCPT ); Fri, 5 Aug 2016 11:48:32 -0400 Received: from sender153-mail.zoho.com ([74.201.84.153]:21942 "EHLO sender153-mail.zoho.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759985AbcHEPsb (ORCPT ); Fri, 5 Aug 2016 11:48:31 -0400 DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=zapps768; d=zoho.com; h=subject:to:references:cc:from:message-id:date:user-agent:mime-version:in-reply-to:content-type; b=NvXARNe1/q6fLiOjUn8HnIAccLKXVn2Jcjweq+Vjn/d3bClUVddVVsgXOMD0F+3Xo8dlt2yiUU8D GbeR6rwGPIHUNhD+wB6w62aMeyxhAfgNc8ymWAsUyO1RcDapQIal Subject: Re: [PATCH] mm/vmalloc: fix align value calculation error To: Andrew Morton References: <57A2F6A3.9080908@zoho.com> <57A2FE7B.5070505@zoho.com> <20160804142421.576426492d629f0839298f9a@linux-foundation.org> Cc: tj@kernel.org, hannes@cmpxchg.org, mhocko@kernel.org, minchan@kernel.org, zijun_hu@htc.com, rientjes@google.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org From: zijun_hu Message-ID: Date: Fri, 5 Aug 2016 23:48:21 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160804142421.576426492d629f0839298f9a@linux-foundation.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>From 1fa79b706735908b2c7aed635dcad7ed9c0a2a87 Mon Sep 17 00:00:00 2001 From: zijun_hu 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 Signed-off-by: zijun_hu --- include/linux/bitops.h | 17 +++++++++++++++++ mm/vmalloc.c | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/include/linux/bitops.h b/include/linux/bitops.h index 299e76b..c18448d 100644 --- 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); -- 1.9.1