[KJ][PATCH] is_power_of_2 in ia64mm

[KJ][PATCH] is_power_of_2 in ia64mm

[Vignesh Babu BM]

Replacing (n & (n-1)) in the context of power of 2 checks
with is_power_of_2


diff --git a/arch/ia64/mm/hugetlbpage.c b/arch/ia64/mm/hugetlbpage.c
index 0c7e94e..0ccc70e 100644
--- a/arch/ia64/mm/hugetlbpage.c
+++ b/arch/ia64/mm/hugetlbpage.c
@@ -16,6 +16,7 @@
 #include <linux/smp_lock.h>
 #include <linux/slab.h>
 #include <linux/sysctl.h>
+#include <linux/log2.h>
 #include <asm/mman.h>
 #include <asm/pgalloc.h>
 #include <asm/tlb.h>
@@ -175,7 +176,7 @@ static int __init hugetlb_setup_sz(char *str)
 		tr_pages = 0x15557000UL;
 
 	size = memparse(str, &str);
-	if (*str || (size & (size-1)) || !(tr_pages & size) ||
+	if (*str || !is_power_of_2(size) || !(tr_pages & size) ||
 		size <= PAGE_SIZE ||
 		size >= (1UL << PAGE_SHIFT << MAX_ORDER)) {
 		printk(KERN_WARNING "Invalid huge page size specified\n");

-- 
Regards,  
Vignesh Babu BM  
_____________________________________________________________  
"Why is it that every time I'm with you, makes me believe in magic?"

--
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>

Re: [KJ] [PATCH] is_power_of_2 in ia64mm

[Richard Knutsson]

Vignesh Babu BM wrote:
> Replacing (n & (n-1)) in the context of power of 2 checks
> with is_power_of_2
>
>
> diff --git a/arch/ia64/mm/hugetlbpage.c b/arch/ia64/mm/hugetlbpage.c
> index 0c7e94e..0ccc70e 100644
> --- a/arch/ia64/mm/hugetlbpage.c
> +++ b/arch/ia64/mm/hugetlbpage.c
> @@ -16,6 +16,7 @@
>  #include <linux/smp_lock.h>
>  #include <linux/slab.h>
>  #include <linux/sysctl.h>
> +#include <linux/log2.h>
>  #include <asm/mman.h>
>  #include <asm/pgalloc.h>
>  #include <asm/tlb.h>
> @@ -175,7 +176,7 @@ static int __init hugetlb_setup_sz(char *str)
>  		tr_pages = 0x15557000UL;
>  
>  	size = memparse(str, &str);
> -	if (*str || (size & (size-1)) || !(tr_pages & size) ||
> +	if (*str || !is_power_of_2(size) || !(tr_pages & size) ||
>  		size <= PAGE_SIZE ||
>  		size >= (1UL << PAGE_SHIFT << MAX_ORDER)) {
>  		printk(KERN_WARNING "Invalid huge page size specified\n");
>
>   
As we talked about before; is this really correct? !is_power_of_2(0) == 
true while (0 & (0-1)) == 0.

Richard Knutsson

--
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>

Re: [KJ] [PATCH] is_power_of_2 in ia64mm

[Andreas Schwab]

Richard Knutsson <ricknu-0@student.ltu.se> writes:

> Vignesh Babu BM wrote:
>> @@ -175,7 +176,7 @@ static int __init hugetlb_setup_sz(char *str)
>>  		tr_pages = 0x15557000UL;
>>   	size = memparse(str, &str);
>> -	if (*str || (size & (size-1)) || !(tr_pages & size) ||
>> +	if (*str || !is_power_of_2(size) || !(tr_pages & size) ||
>>  		size <= PAGE_SIZE ||
>>  		size >= (1UL << PAGE_SHIFT << MAX_ORDER)) {
>>  		printk(KERN_WARNING "Invalid huge page size specified\n");
>>
>>   
> As we talked about before; is this really correct? !is_power_of_2(0) ==
> true while (0 & (0-1)) == 0.

size == 0 is also covered by the next two conditions, so the overall value
does not change.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstrasse 5, 90409 Nurnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

--
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>

Re: [KJ] [PATCH] is_power_of_2 in ia64mm

[Richard Knutsson]

Andreas Schwab wrote:
> Richard Knutsson <ricknu-0@student.ltu.se> writes:
>
>   
>> Vignesh Babu BM wrote:
>>     
>>> @@ -175,7 +176,7 @@ static int __init hugetlb_setup_sz(char *str)
>>>  		tr_pages = 0x15557000UL;
>>>   	size = memparse(str, &str);
>>> -	if (*str || (size & (size-1)) || !(tr_pages & size) ||
>>> +	if (*str || !is_power_of_2(size) || !(tr_pages & size) ||
>>>  		size <= PAGE_SIZE ||
>>>  		size >= (1UL << PAGE_SHIFT << MAX_ORDER)) {
>>>  		printk(KERN_WARNING "Invalid huge page size specified\n");
>>>
>>>   
>>>       
>> As we talked about before; is this really correct? !is_power_of_2(0) ==
>> true while (0 & (0-1)) == 0.
>>     
>
> size == 0 is also covered by the next two conditions, so the overall value
> does not change.
>   
Yes, but is it meant to state that 'size' is not a power of two? 
Otherwise, imho, it should be left as-is.

Richard Knutsson

--
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>

Re: [KJ] [PATCH] is_power_of_2 in ia64mm

[Andreas Schwab]

Richard Knutsson <ricknu-0@student.ltu.se> writes:

> Andreas Schwab wrote:
>> Richard Knutsson <ricknu-0@student.ltu.se> writes:
>>
>>   
>>> Vignesh Babu BM wrote:
>>>     
>>>> @@ -175,7 +176,7 @@ static int __init hugetlb_setup_sz(char *str)
>>>>  		tr_pages = 0x15557000UL;
>>>>   	size = memparse(str, &str);
>>>> -	if (*str || (size & (size-1)) || !(tr_pages & size) ||
>>>> +	if (*str || !is_power_of_2(size) || !(tr_pages & size) ||
>>>>  		size <= PAGE_SIZE ||
>>>>  		size >= (1UL << PAGE_SHIFT << MAX_ORDER)) {
>>>>  		printk(KERN_WARNING "Invalid huge page size specified\n");
>>>>
>>>>         
>>> As we talked about before; is this really correct? !is_power_of_2(0) ==
>>> true while (0 & (0-1)) == 0.
>>>     
>>
>> size == 0 is also covered by the next two conditions, so the overall value
>> does not change.
>>   
> Yes, but is it meant to state that 'size' is not a power of two?

What else can it mean?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstrasse 5, 90409 Nurnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

--
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>

Re: [KJ] [PATCH] is_power_of_2 in ia64mm

[Richard Knutsson]

Andreas Schwab wrote:
> Richard Knutsson <ricknu-0@student.ltu.se> writes:
>   
>> Andreas Schwab wrote:
>>     
>>> Richard Knutsson <ricknu-0@student.ltu.se> writes:
>>>   
>>>       
>>>> Vignesh Babu BM wrote:
>>>>         
>>>>> @@ -175,7 +176,7 @@ static int __init hugetlb_setup_sz(char *str)
>>>>>  		tr_pages = 0x15557000UL;
>>>>>   	size = memparse(str, &str);
>>>>> -	if (*str || (size & (size-1)) || !(tr_pages & size) ||
>>>>> +	if (*str || !is_power_of_2(size) || !(tr_pages & size) ||
>>>>>  		size <= PAGE_SIZE ||
>>>>>  		size >= (1UL << PAGE_SHIFT << MAX_ORDER)) {
>>>>>  		printk(KERN_WARNING "Invalid huge page size specified\n");
>>>>>           
>>>> As we talked about before; is this really correct? !is_power_of_2(0) ==
>>>> true while (0 & (0-1)) == 0.
>>>>         
>>> size == 0 is also covered by the next two conditions, so the overall value
>>> does not change.
>>>       
>> Yes, but is it meant to state that 'size' is not a power of two?
>>     
>
> What else can it mean?
>   
What about !one_or_less_bit()? It has not been implemented (yet?) but 
been discussed. It ended by concluding that is_power_of_2() should be 
fixed up first and then we can see about it.
And as I stated before; !is_power_of_2(size) != (size & (size-1))! It 
may be a bug but then we have to be sure it is suppose to be the power of 2.

Richard Knutsson

--
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>

Re: [KJ] [PATCH] is_power_of_2 in ia64mm

[Andreas Schwab]

Richard Knutsson <ricknu-0@student.ltu.se> writes:

> Andreas Schwab wrote:
>> Richard Knutsson <ricknu-0@student.ltu.se> writes:
>>   
>>> Andreas Schwab wrote:
>>>     
>>>> Richard Knutsson <ricknu-0@student.ltu.se> writes:
>>>>         
>>>>> Vignesh Babu BM wrote:
>>>>>         
>>>>>> @@ -175,7 +176,7 @@ static int __init hugetlb_setup_sz(char *str)
>>>>>>  		tr_pages = 0x15557000UL;
>>>>>>   	size = memparse(str, &str);
>>>>>> -	if (*str || (size & (size-1)) || !(tr_pages & size) ||
>>>>>> +	if (*str || !is_power_of_2(size) || !(tr_pages & size) ||
>>>>>>  		size <= PAGE_SIZE ||
>>>>>>  		size >= (1UL << PAGE_SHIFT << MAX_ORDER)) {
>>>>>>  		printk(KERN_WARNING "Invalid huge page size specified\n");
>>>>>>           
>>>>> As we talked about before; is this really correct? !is_power_of_2(0) ==
>>>>> true while (0 & (0-1)) == 0.
>>>>>         
>>>> size == 0 is also covered by the next two conditions, so the overall value
>>>> does not change.
>>>>       
>>> Yes, but is it meant to state that 'size' is not a power of two?
>>>     
>>
>> What else can it mean?
>>   
> What about !one_or_less_bit()?

Obviously not.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstrasse 5, 90409 Nurnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

--
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>

Re: [KJ] [PATCH] is_power_of_2 in ia64mm

[Robert P. J. Day]

On Fri, 16 Feb 2007, Richard Knutsson wrote:

> Andreas Schwab wrote:
> > Richard Knutsson <ricknu-0@student.ltu.se> writes:
> >
> >
> > > Vignesh Babu BM wrote:
> > >
> > > > @@ -175,7 +176,7 @@ static int __init hugetlb_setup_sz(char *str)
> > > >  		tr_pages = 0x15557000UL;
> > > >   	size = memparse(str, &str);
> > > > -	if (*str || (size & (size-1)) || !(tr_pages & size) ||
> > > > +	if (*str || !is_power_of_2(size) || !(tr_pages & size) ||
> > > >  		size <= PAGE_SIZE ||
> > > >  		size >= (1UL << PAGE_SHIFT << MAX_ORDER)) {
> > > >  		printk(KERN_WARNING "Invalid huge page size specified\n");
> > > >
> > > >
> > > As we talked about before; is this really correct?
> > > !is_power_of_2(0) == true while (0 & (0-1)) == 0.
> >
> > size == 0 is also covered by the next two conditions, so the
> > overall value does not change.
> >
> Yes, but is it meant to state that 'size' is not a power of two?
> Otherwise, imho, it should be left as-is.

i think the above change is fine.  as long as the final, overall
semantics of the condition are identical, then there's no problem.

rday

-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================

--
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>

Re: [KJ] [PATCH] is_power_of_2 in ia64mm

[Robert P. J. Day]

i'm not clear on what the possible problem is here:

On Fri, 16 Feb 2007, Richard Knutsson wrote:

> Andreas Schwab wrote:
> > Richard Knutsson <ricknu-0@student.ltu.se> writes:
> >
> >> Andreas Schwab wrote:
> >>
> >>> Richard Knutsson <ricknu-0@student.ltu.se> writes:
> >>>
> >>>
> >>>> Vignesh Babu BM wrote:
> >>>>
> >>>>> @@ -175,7 +176,7 @@ static int __init hugetlb_setup_sz(char *str)
> >>>>>  		tr_pages = 0x15557000UL;
> >>>>>   	size = memparse(str, &str);
> >>>>> -	if (*str || (size & (size-1)) || !(tr_pages & size) ||
> >>>>> +	if (*str || !is_power_of_2(size) || !(tr_pages & size) ||
> >>>>>  		size <= PAGE_SIZE ||
> >>>>>  		size >= (1UL << PAGE_SHIFT << MAX_ORDER)) {
> >>>>>  		printk(KERN_WARNING "Invalid huge page size specified\n");
> >>>>>
> >>>> As we talked about before; is this really correct? !is_power_of_2(0) ==
> >>>> true while (0 & (0-1)) == 0.
> >>>>
> >>> size == 0 is also covered by the next two conditions, so the overall value
> >>> does not change.
> >>>
> >> Yes, but is it meant to state that 'size' is not a power of two?
> >>
> >
> > What else can it mean?

> What about !one_or_less_bit()? It has not been implemented (yet?)
> but been discussed.

but whether or not it's been implemented doesn't change whether or not
the code above can be simplified.  given what's being tested, and the
error message about whether a page size is valid, it seems fairly
clear that this is a power of two test.  what's the problem?

> It ended by concluding that is_power_of_2() should be fixed up first
> and then we can see about it.

there's nothing about is_power_of_2() that needs "fixing".  it's
correct as it's currently implemented.

> And as I stated before; !is_power_of_2(size) != (size & (size-1))!
> It may be a bug but then we have to be sure it is suppose to be the
> power of 2.

it may be that certain tests in the current code need to be examined
carefully to clarify what's being tested.  but the code above seems
straightforward.

rday

-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================

--
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>

Re: [KJ] [PATCH] is_power_of_2 in ia64mm

[Richard Knutsson]

Robert P. J. Day wrote:
> i'm not clear on what the possible problem is here:
>
> On Fri, 16 Feb 2007, Richard Knutsson wrote:
>
>   
>> Andreas Schwab wrote:
>>     
>>> Richard Knutsson <ricknu-0@student.ltu.se> writes:
>>>
>>>       
>>>> Andreas Schwab wrote:
>>>>
>>>>         
>>>>> Richard Knutsson <ricknu-0@student.ltu.se> writes:
>>>>>
>>>>>
>>>>>           
>>>>>> Vignesh Babu BM wrote:
>>>>>>
>>>>>>             
>>>>>>> @@ -175,7 +176,7 @@ static int __init hugetlb_setup_sz(char *str)
>>>>>>>  		tr_pages = 0x15557000UL;
>>>>>>>   	size = memparse(str, &str);
>>>>>>> -	if (*str || (size & (size-1)) || !(tr_pages & size) ||
>>>>>>> +	if (*str || !is_power_of_2(size) || !(tr_pages & size) ||
>>>>>>>  		size <= PAGE_SIZE ||
>>>>>>>  		size >= (1UL << PAGE_SHIFT << MAX_ORDER)) {
>>>>>>>  		printk(KERN_WARNING "Invalid huge page size specified\n");
>>>>>>>
>>>>>>>               
>>>>>> As we talked about before; is this really correct? !is_power_of_2(0) ==
>>>>>> true while (0 & (0-1)) == 0.
>>>>>>
>>>>>>             
>>>>> size == 0 is also covered by the next two conditions, so the overall value
>>>>> does not change.
>>>>>
>>>>>           
>>>> Yes, but is it meant to state that 'size' is not a power of two?
>>>>
>>>>         
>>> What else can it mean?
>>>       
>> What about !one_or_less_bit()? It has not been implemented (yet?)
>> but been discussed.
>>     
>
> but whether or not it's been implemented doesn't change whether or not
> the code above can be simplified.  given what's being tested, and the
> error message about whether a page size is valid, it seems fairly
> clear that this is a power of two test.  what's the problem?
>   
Fsck, I can't see that. But if that is what's intended, well then...

(5 min later)
Ok, now I think I see it. Sorry for the noise..
>   
>> It ended by concluding that is_power_of_2() should be fixed up first
>> and then we can see about it.
>>     
>
> there's nothing about is_power_of_2() that needs "fixing".  it's
> correct as it's currently implemented.
>   
Oh, I didn't mean that is_power_of_2() need to be fixed, I meant 
fixing/replacing the kernel with is_power_of_2().


Todays lesson: don't try to code while you have a cold...
Richard Knutsson

--
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>