[dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[Ravi Kerur]

Changes in v2
Make this a separate patch. 

rte_is_power_of_2 returns true for 0, fix by checking for n.

Ravi Kerur (1):
  Fix rte_is_power_of_2

 lib/librte_eal/common/include/rte_common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
1.9.1

[dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[Ravi Kerur]

rte_is_power_of_2 returns true for 0 and 0 is not power_of_2. Fix
by checking for n.

Signed-off-by: Ravi Kerur <rkerur@gmail.com>
---
 lib/librte_eal/common/include/rte_common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index 921b91f..8ac940c 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -203,7 +203,7 @@ extern int RTE_BUILD_BUG_ON_detected_error;
 static inline int
 rte_is_power_of_2(uint32_t n)
 {
-	return ((n-1) & n) == 0;
+	return n && !(n & (n - 1));
 }
 
 /**
-- 
1.9.1

Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[Neil Horman]

On Sat, Dec 27, 2014 at 10:30:44AM -0500, Ravi Kerur wrote:
> rte_is_power_of_2 returns true for 0 and 0 is not power_of_2. Fix
> by checking for n.
> 
> Signed-off-by: Ravi Kerur <rkerur@gmail.com>
> ---
>  lib/librte_eal/common/include/rte_common.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
> index 921b91f..8ac940c 100644
> --- a/lib/librte_eal/common/include/rte_common.h
> +++ b/lib/librte_eal/common/include/rte_common.h
> @@ -203,7 +203,7 @@ extern int RTE_BUILD_BUG_ON_detected_error;
>  static inline int
>  rte_is_power_of_2(uint32_t n)
>  {
> -	return ((n-1) & n) == 0;
> +	return n && !(n & (n - 1));
>  }
>  
>  /**
> -- 
> 1.9.1
> 
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[Thomas Monjalon]

> > rte_is_power_of_2 returns true for 0 and 0 is not power_of_2. Fix
> > by checking for n.
> > 
> > Signed-off-by: Ravi Kerur <rkerur@gmail.com>
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Applied

Thanks
-- 
Thomas

Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[Zhang, Helin]

Hi Kerur

It seems that your fix result in cannot launching applications.
I don't suspect the correction of your fix, but somewhere else needs to be corrected together with your fix.

Logs:
/************************************************************
RING: Cannot reserve memory for tailq
EAL: rte_eal_common_log_init(): cannot create log_history mempool
PANIC in rte_eal_init():
Cannot init logs
6: [./l3fwd() [0x41d7c5]]
5: [/lib64/libc.so.6(__libc_start_main+0xf5) [0x3d8a221d65]]
4: [./l3fwd(main+0x23) [0x41c493]]
3: [./l3fwd(rte_eal_init+0x108d) [0x466f7d]]
2: [./l3fwd(__rte_panic+0xc9) [0x41c358]]
1: [./l3fwd(rte_dump_stack+0x18) [0x46e258]]

Regards,
Helin

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman
> Sent: Sunday, December 28, 2014 4:49 AM
> To: Ravi Kerur
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2
> 
> On Sat, Dec 27, 2014 at 10:30:44AM -0500, Ravi Kerur wrote:
> > rte_is_power_of_2 returns true for 0 and 0 is not power_of_2. Fix by
> > checking for n.
> >
> > Signed-off-by: Ravi Kerur <rkerur@gmail.com>
> > ---
> >  lib/librte_eal/common/include/rte_common.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_eal/common/include/rte_common.h
> > b/lib/librte_eal/common/include/rte_common.h
> > index 921b91f..8ac940c 100644
> > --- a/lib/librte_eal/common/include/rte_common.h
> > +++ b/lib/librte_eal/common/include/rte_common.h
> > @@ -203,7 +203,7 @@ extern int RTE_BUILD_BUG_ON_detected_error;
> > static inline int  rte_is_power_of_2(uint32_t n)  {
> > -	return ((n-1) & n) == 0;
> > +	return n && !(n & (n - 1));
> >  }
> >
> >  /**
> > --
> > 1.9.1
> >
> >
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[David Marchand]

Hello,

On Mon, Jan 19, 2015 at 3:45 AM, Zhang, Helin <helin.zhang@intel.com> wrote:

> It seems that your fix result in cannot launching applications.
> I don't suspect the correction of your fix, but somewhere else needs to be
> corrected together with your fix.
>
> Logs:
> /************************************************************
> RING: Cannot reserve memory for tailq
>

I have a quick fix for this one (rte_malloc_socket refuses 0 alignment),
but looking at the change, I would say there are a lot of places to be
checked.
Were those places checked during review ?

Kerur, did you run a make test ?
All tests fail for me because of rte_malloc_socket.


-- 
David Marchand

Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[Thomas Monjalon]

2015-01-19 08:21, David Marchand:
> On Mon, Jan 19, 2015 at 3:45 AM, Zhang, Helin <helin.zhang@intel.com> wrote:
> 
> > It seems that your fix result in cannot launching applications.
> > I don't suspect the correction of your fix, but somewhere else needs to be
> > corrected together with your fix.
> >
> > Logs:
> > /************************************************************
> > RING: Cannot reserve memory for tailq
> 
> I have a quick fix for this one (rte_malloc_socket refuses 0 alignment),
> but looking at the change, I would say there are a lot of places to be
> checked.
> Were those places checked during review ?

http://dpdk.org/browse/dpdk/commit/?id=2fc8d6daa4c7a
This case demonstrates that an Acked-by line is not always sufficient
to apply a patch.

> Kerur, did you run a make test ?
> All tests fail for me because of rte_malloc_socket.

My dream would be to have a machine receiving patches, applying them in a sandbox,
run some basic tests and reports failures.

It should be fixed now:
        http://dpdk.org/browse/dpdk/commit/?id=8e3e06501660

-- 
Thomas

Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of David Marchand
> Sent: Monday, January 19, 2015 7:21 AM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2
> 
> Hello,
> 
> On Mon, Jan 19, 2015 at 3:45 AM, Zhang, Helin <helin.zhang@intel.com>
> wrote:
> 
> > It seems that your fix result in cannot launching applications.
> > I don't suspect the correction of your fix, but somewhere else needs to be
> > corrected together with your fix.
> >
> > Logs:
> >
> /**********************************************************
> **
> > RING: Cannot reserve memory for tailq
> >
> 
> I have a quick fix for this one (rte_malloc_socket refuses 0 alignment),
> but looking at the change, I would say there are a lot of places to be
> checked.
> Were those places checked during review ?
> 
> Kerur, did you run a make test ?
> All tests fail for me because of rte_malloc_socket.

I think this is already fixed by Declan in his last patch.

Pablo

> 
> 
> --
> David Marchand

Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[Qiu, Michael]

On 2015/1/19 17:14, Thomas Monjalon wrote:
> 2015-01-19 08:21, David Marchand:
>> On Mon, Jan 19, 2015 at 3:45 AM, Zhang, Helin <helin.zhang@intel.com> wrote:
>>
>>> It seems that your fix result in cannot launching applications.
>>> I don't suspect the correction of your fix, but somewhere else needs to be
>>> corrected together with your fix.
>>>
>>> Logs:
>>> /************************************************************
>>> RING: Cannot reserve memory for tailq
>> I have a quick fix for this one (rte_malloc_socket refuses 0 alignment),
>> but looking at the change, I would say there are a lot of places to be
>> checked.
>> Were those places checked during review ?
> http://dpdk.org/browse/dpdk/commit/?id=2fc8d6daa4c7a
> This case demonstrates that an Acked-by line is not always sufficient
> to apply a patch.
>
>> Kerur, did you run a make test ?
>> All tests fail for me because of rte_malloc_socket.
> My dream would be to have a machine receiving patches, applying them in a sandbox,
> run some basic tests and reports failures.

This could be, but I hope every contributor could do basic check like
build and run of at lease one app like test-pmd, then it will reduce
most of failure we faced.

Then I think we can have one next branch, all patches first been
reviewed and basic verified then applied to next, after all tested then
merge to master(more stable)

Then all the release could based on master branch, it also could avoid
lots of unstable RC release(like compile error we see before, it should
not happen in RC release in my mind).

Thanks,
Michael
>
> It should be fixed now:
>         http://dpdk.org/browse/dpdk/commit/?id=8e3e06501660
>

Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[David Marchand]

On Mon, Jan 19, 2015 at 10:30 AM, Qiu, Michael <michael.qiu@intel.com>
wrote:

>
> This could be, but I hope every contributor could do basic check like
> build and run of at lease one app like test-pmd, then it will reduce
> most of failure we faced.
>

testpmd is fine, but make test should be preferred as it requires no setup
and it requires no pmd to be configured.
Besides, testpmd is an application, it does not test libraries that it does
not use, while make test is supposed to test all libraries.


There is still the hugepages to configure when running make test so I would
say "make test" should tell you so.
This will avoid wasting 10 minutes to understand why all your tests fail ...

Can anyone help document this / make it more user friendly / easier ?


-- 
David Marchand

Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[Qiu, Michael]

On 2015/1/19 17:50, David Marchand wrote:
On Mon, Jan 19, 2015 at 10:30 AM, Qiu, Michael <michael.qiu@intel.com<mailto:michael.qiu@intel.com>> wrote:

This could be, but I hope every contributor could do basic check like
build and run of at lease one app like test-pmd, then it will reduce
most of failure we faced.

testpmd is fine, but make test should be preferred as it requires no setup and it requires no pmd to be configured.
Besides, testpmd is an application, it does not test libraries that it does not use, while make test is supposed to test all libraries.


I just use test-pmd for an example, the contributor should test his patches before send out, especially, the test need to focus on what he try to fixed.

It is a bad behavior to send out before test.

There is still the hugepages to configure when running make test so I would say "make test" should tell you so.
This will avoid wasting 10 minutes to understand why all your tests fail ...

Can anyone help document this / make it more user friendly / easier ?


--
David Marchand

Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[Qiu, Michael]

On 2015/1/19 17:50, David Marchand wrote:
On Mon, Jan 19, 2015 at 10:30 AM, Qiu, Michael <michael.qiu@intel.com<mailto:michael.qiu@intel.com>> wrote:

This could be, but I hope every contributor could do basic check like
build and run of at lease one app like test-pmd, then it will reduce
most of failure we faced.

testpmd is fine, but make test should be preferred as it requires no setup and it requires no pmd to be configured.
Besides, testpmd is an application, it does not test libraries that it does not use, while make test is supposed to test all libraries.




I just use test-pmd for an example, the contributor should test his patches before send out, especially, the test need to focus on what he try to fixed.



It is a bad behavior to send out *without* tested.


There is still the hugepages to configure when running make test so I would say "make test" should tell you so.
This will avoid wasting 10 minutes to understand why all your tests fail ...


Yes, it could be useful.
Can anyone help document this / make it more user friendly / easier ?




--
David Marchand

Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[Ravi Kerur]

Sorry for the inconvenience. I was not aware of "make test" utility which
does more elaborate testing. I will make a note of it and make sure future
patches will go through it. I had done basic testing with testpmd as the
changes were minimal.

Thanks,
Ravi

On Mon, Jan 19, 2015 at 1:13 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> 2015-01-19 08:21, David Marchand:
> > On Mon, Jan 19, 2015 at 3:45 AM, Zhang, Helin <helin.zhang@intel.com>
> wrote:
> >
> > > It seems that your fix result in cannot launching applications.
> > > I don't suspect the correction of your fix, but somewhere else needs
> to be
> > > corrected together with your fix.
> > >
> > > Logs:
> > > /************************************************************
> > > RING: Cannot reserve memory for tailq
> >
> > I have a quick fix for this one (rte_malloc_socket refuses 0 alignment),
> > but looking at the change, I would say there are a lot of places to be
> > checked.
> > Were those places checked during review ?
>
> http://dpdk.org/browse/dpdk/commit/?id=2fc8d6daa4c7a
> This case demonstrates that an Acked-by line is not always sufficient
> to apply a patch.
>
> > Kerur, did you run a make test ?
> > All tests fail for me because of rte_malloc_socket.
>
> My dream would be to have a machine receiving patches, applying them in a
> sandbox,
> run some basic tests and reports failures.
>
> It should be fixed now:
>         http://dpdk.org/browse/dpdk/commit/?id=8e3e06501660
>
> --
> Thomas
>

Re: [dpdk-dev] [PATCH v2] Fix rte_is_power_of_2

[Ravi Kerur]

Looks like "make test" was added recently? I have "1.8.0" version and when
I run "make test" after "make install T=x86_64-native-linuxapp-gcc" I get
following errors

root@user-PC:/home/rkerur/dpdk-new-5/dpdk# make test
No test found, please do a 'make build' first, or specify O=
root@user-PC:/home/rkerur/dpdk-new-5/dpdk#


However, When I pull latest code (as of 1/19) and follow same steps as
above  I see "make test" runs fine but there are 19 failures as shown
below. Any other prerequisites needed for this test to run successfully. If
it is documented and can be shared, it will really help new contributors to
not make same mistake I did.

root@user-PC:/home/rkerur/dpdk-new-6/dpdk# make test
/home/rkerur/dpdk-new-6/dpdk/build/app/test -c f -n 4

Test name                      Test result                      Test
Total
================================================================================
Start group_1:                 Fail [No prompt]              [00m 00s]
Timer autotest:                Fail [No prompt]              [00m 00s]
Debug autotest:                Fail [No prompt]              [00m 00s]
Errno autotest:                Fail [No prompt]              [00m 00s]
Meter autotest:                Fail [No prompt]              [00m 00s]
Common autotest:               Fail [No prompt]              [00m 00s]
Dump log history:              Fail [No prompt]              [00m 00s]
Dump rings:                    Fail [No prompt]              [00m 00s]
Dump mempools:                 Fail [No prompt]              [00m 00s] [00m
00s]
Start group_2:                 Success                       [00m 00s]
Memory autotest:               Success                       [00m 00s]
Read/write lock autotest:      Success                       [00m 00s]
Logs autotest:                 Success                       [00m 00s]
CPU flags autotest:            Success                       [00m 00s]
Version autotest:              Success                       [00m 00s]
EAL filesystem autotest:       Success                       [00m 00s]
EAL flags autotest:            Fail                          [00m 09s]
Hash autotest:                 Success                       [00m 00s] [00m
12s]
Start group_3:                 Success                       [00m 00s]
LPM autotest:                  Success                       [00m 23s]
IVSHMEM autotest:              Fail [Not found]              [00m 00s]
Memcpy autotest:               Success                       [00m 00s]
Memzone autotest:              Success                       [00m 00s]
String autotest:               Success                       [00m 00s]
Alarm autotest:                Success                       [00m 16s] [00m
54s]
Start group_4:                 Success                       [00m 00s]
PCI autotest:                  Fail                          [00m 00s]
Malloc autotest:               Success                       [00m 00s]
Multi-process autotest:        Fail                          [00m 01s]
Mbuf autotest:                 Success                       [00m 27s]
Per-lcore autotest:            Success                       [00m 05s]
Ring autotest:                 Success                       [00m 02s] [01m
31s]
Start group_5:                 Success                       [00m 00s]
Spinlock autotest:             Success                       [00m 15s]
Byte order autotest:           Success                       [00m 00s]
TAILQ autotest:                Success                       [00m 00s]
Command-line autotest:         Fail                          [00m 00s]
Interrupts autotest:           Success                       [00m 11s] [01m
59s]
Start group_6:                 Success                       [00m 00s]
Function reentrancy autotest:  Success                       [00m 00s]
Mempool autotest:              Fail                          [00m 00s]
Atomics autotest:              Success                       [00m 00s]
Prefetch autotest:             Success                       [00m 00s]
Red autotest:                  Success                       [01m 40s] [03m
40s]
Start group_7:                 Success                       [00m 00s]
PMD ring autotest:             Fail                          [00m 00s]
Access list control autotest:  Success                       [00m 00s]
Sched autotest:                Success                       [00m 00s] [03m
42s]
Start kni:                     Success                       [00m 00s]
KNI autotest:                  Fail [Crash]                  [00m 00s] [03m
43s]
Start mempool_perf:            Success                       [00m 00s]
Cycles autotest:               Success                       [00m 01s]
Mempool performance autotest:  Fail                          [00m 00s] [03m
45s]
Start memcpy_perf:             Success                       [00m 00s]
Memcpy performance autotest:   Success                       [00m 50s] [04m
37s]
Start hash_perf:               Success                       [00m 00s]
Hash performance autotest:     Success                       [01m 05s] [05m
43s]
Start power:                   Success                       [00m 00s]
Power autotest:                Success                       [00m 00s] [05m
44s]
Start power_acpi_cpufreq:      Success                       [00m 00s]
Power ACPI cpufreq autotest:   Success                       [00m 00s] [05m
45s]
Start power_kvm_vm:            Success                       [00m 00s]
Power KVM VM  autotest:        Fail                          [00m 00s] [05m
46s]
Start lpm6:                    Success                       [00m 00s]
LPM6 autotest:                 Fail                          [00m 00s] [05m
47s]
Start timer_perf:              Success                       [00m 00s]
Timer performance autotest:    Success                       [00m 11s] [05m
59s]
Start ring_perf:               Success                       [00m 00s]
Ring performance autotest:     Success                       [00m 04s] [06m
04s]
================================================================================
Total run time: 06m 04s
Number of failed tests: 19



On Mon, Jan 19, 2015 at 9:07 AM, Ravi Kerur <rkerur@gmail.com> wrote:

> Sorry for the inconvenience. I was not aware of "make test" utility which
> does more elaborate testing. I will make a note of it and make sure future
> patches will go through it. I had done basic testing with testpmd as the
> changes were minimal.
>
> Thanks,
> Ravi
>
> On Mon, Jan 19, 2015 at 1:13 AM, Thomas Monjalon <
> thomas.monjalon@6wind.com> wrote:
>
>> 2015-01-19 08:21, David Marchand:
>> > On Mon, Jan 19, 2015 at 3:45 AM, Zhang, Helin <helin.zhang@intel.com>
>> wrote:
>> >
>> > > It seems that your fix result in cannot launching applications.
>> > > I don't suspect the correction of your fix, but somewhere else needs
>> to be
>> > > corrected together with your fix.
>> > >
>> > > Logs:
>> > > /************************************************************
>> > > RING: Cannot reserve memory for tailq
>> >
>> > I have a quick fix for this one (rte_malloc_socket refuses 0 alignment),
>> > but looking at the change, I would say there are a lot of places to be
>> > checked.
>> > Were those places checked during review ?
>>
>> http://dpdk.org/browse/dpdk/commit/?id=2fc8d6daa4c7a
>> This case demonstrates that an Acked-by line is not always sufficient
>> to apply a patch.
>>
>> > Kerur, did you run a make test ?
>> > All tests fail for me because of rte_malloc_socket.
>>
>> My dream would be to have a machine receiving patches, applying them in a
>> sandbox,
>> run some basic tests and reports failures.
>>
>> It should be fixed now:
>>         http://dpdk.org/browse/dpdk/commit/?id=8e3e06501660
>>
>> --
>> Thomas
>>
>
>