[dpdk-dev] spinlock: Move constructor function out of header file
> On 15 Jul 2016, at 17:08, Thomas Monjalon > wrote: > > 2016-07-15 16:37, Thomas Monjalon: >> I will apply it with trivial changes suggested by Jan and >> the small needed changes that I describe below: >> >> 2016-07-14 20:03, Jan Viktorin: >>> On Thu, 14 Jul 2016 15:27:29 +0200 >>> damarion at cisco.com wrote: --- /dev/null +++ b/lib/librte_eal/common/arch/x86/rte_spinlock.c >> [...] +uint8_t rte_rtm_supported; /* cache the flag to avoid the overhead +of the rte_cpu_get_flag_enabled function */ >> >> This variable must be exported in the .map file. >> --- a/lib/librte_eal/linuxapp/eal/Makefile +++ b/lib/librte_eal/linuxapp/eal/Makefile @@ -106,6 +106,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_keepalive.c # from arch dir SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_cpuflags.c +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_spinlock.c >>> >>> This is not good, you provide rte_spinlock.c only for x86. Building >>> for any other arch would fail to find this file. >> >> This change do the trick: >> >> -SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_spinlock.c >> +SRCS-$(CONFIG_RTE_ARCH_X86) += rte_spinlock.c >> >> (Note that CONFIG_RTE_EXEC_ENV_LINUXAPP check is not needed inside linuxapp >> EAL) >> >>> Moreover, the bsdapp/eal/Makefile should reflect this situation as >>> well. >> >> Yes > > Applied with above changes, thanks Thanks!
[dpdk-dev] spinlock: Move constructor function out of header file
> On 15 Jul 2016, at 12:09, Thomas Monjalon > wrote: > > 2016-07-15 09:54, Damjan Marion: >> So we don?t have much pending beside 2 patches for i40e which >> Jeff submitted yesterday and they will i guess need to wait for 16.11. > > Yes these i40e patches will probably have to wait 16.11. > >> Only one which I have on my mind is: >> >> https://git.fd.io/cgit/vpp/tree/dpdk/dpdk-16.04_patches/0005-Allow-applications-to-override-rte_delay_us.patch >> >> This is big issue for us when running single-core, as some >> drivers tend to call rte_delay_us for a long time, and that is >> causing packet drops. I.e. if you do stop/start on one interface >> and you are running BFD on another one, BFD will fail? >> >> Current patch is hack, it basically allows us to override >> delay function so we can de-schedule it, >> do some other useful work while waiting for delay to finish >> and then give control back to original function? >> >> Maybe we can fix this by providing a delay callback functionality... > > Yes it could be an idea. > Please send a RFC patch. OK, I will ask one of our guys to work on it...
[dpdk-dev] spinlock: Move constructor function out of header file
2016-07-15 16:37, Thomas Monjalon: > I will apply it with trivial changes suggested by Jan and > the small needed changes that I describe below: > > 2016-07-14 20:03, Jan Viktorin: > > On Thu, 14 Jul 2016 15:27:29 +0200 > > damarion at cisco.com wrote: > > > --- /dev/null > > > +++ b/lib/librte_eal/common/arch/x86/rte_spinlock.c > [...] > > > +uint8_t rte_rtm_supported; /* cache the flag to avoid the overhead > > > + of the rte_cpu_get_flag_enabled function */ > > This variable must be exported in the .map file. > > > > --- a/lib/librte_eal/linuxapp/eal/Makefile > > > +++ b/lib/librte_eal/linuxapp/eal/Makefile > > > @@ -106,6 +106,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += > > > rte_keepalive.c > > > > > > # from arch dir > > > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_cpuflags.c > > > +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_spinlock.c > > > > This is not good, you provide rte_spinlock.c only for x86. Building > > for any other arch would fail to find this file. > > This change do the trick: > > -SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_spinlock.c > +SRCS-$(CONFIG_RTE_ARCH_X86) += rte_spinlock.c > > (Note that CONFIG_RTE_EXEC_ENV_LINUXAPP check is not needed inside linuxapp > EAL) > > > Moreover, the bsdapp/eal/Makefile should reflect this situation as > > well. > > Yes Applied with above changes, thanks
[dpdk-dev] spinlock: Move constructor function out of header file
I will apply it with trivial changes suggested by Jan and the small needed changes that I describe below: 2016-07-14 20:03, Jan Viktorin: > On Thu, 14 Jul 2016 15:27:29 +0200 > damarion at cisco.com wrote: > > --- /dev/null > > +++ b/lib/librte_eal/common/arch/x86/rte_spinlock.c [...] > > +uint8_t rte_rtm_supported; /* cache the flag to avoid the overhead > > + of the rte_cpu_get_flag_enabled function */ This variable must be exported in the .map file. > > --- a/lib/librte_eal/linuxapp/eal/Makefile > > +++ b/lib/librte_eal/linuxapp/eal/Makefile > > @@ -106,6 +106,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_keepalive.c > > > > # from arch dir > > SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_cpuflags.c > > +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_spinlock.c > > This is not good, you provide rte_spinlock.c only for x86. Building > for any other arch would fail to find this file. This change do the trick: -SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_spinlock.c +SRCS-$(CONFIG_RTE_ARCH_X86) += rte_spinlock.c (Note that CONFIG_RTE_EXEC_ENV_LINUXAPP check is not needed inside linuxapp EAL) > Moreover, the bsdapp/eal/Makefile should reflect this situation as > well. Yes
[dpdk-dev] spinlock: Move constructor function out of header file
2016-07-15 09:54, Damjan Marion: > So we don?t have much pending beside 2 patches for i40e which > Jeff submitted yesterday and they will i guess need to wait for 16.11. Yes these i40e patches will probably have to wait 16.11. > Only one which I have on my mind is: > > https://git.fd.io/cgit/vpp/tree/dpdk/dpdk-16.04_patches/0005-Allow-applications-to-override-rte_delay_us.patch > > This is big issue for us when running single-core, as some > drivers tend to call rte_delay_us for a long time, and that is > causing packet drops. I.e. if you do stop/start on one interface > and you are running BFD on another one, BFD will fail? > > Current patch is hack, it basically allows us to override > delay function so we can de-schedule it, > do some other useful work while waiting for delay to finish > and then give control back to original function? > > Maybe we can fix this by providing a delay callback functionality... Yes it could be an idea. Please send a RFC patch.
[dpdk-dev] spinlock: Move constructor function out of header file
2016-07-14 22:20, Damjan Marion: > > > On 15 Jul 2016, at 00:06, Thomas Monjalon > > wrote: > > > > 2016-07-14 18:10, Damjan Marion: > >> Dear Jan, > >> > >> Thank you for your comments. A bit too much overhead to submit simple patch > >> so let?s forget about it. I will just add it as it is to our private > >> collection of patches. > > > > These are changes trivial to fix when applying. > > I strongly prefer that you upstream patches instead of keeping patches > > in the VPP repository. I will help you in this task. > > Thanks for the effort. > > Yeah, I agree with you, unfortunately it is all about time, > for me it is still cheaper to rebase them. > > I respect your rules, but I just don?t have enough free cycles > to spend learning all of them, for my occasional patch submission. We know there is a learning curve for the submission process. That's why we do not expect it to be fully satisfied, especially from occasional contributors. I am used to do some not significant changes before applying to help newcomers patches being accepted. That's what I will do in your case. As I said previously I will help you to drop your local patches. Please continue sending other patches with a detailed explanation and we will take care of them.
[dpdk-dev] spinlock: Move constructor function out of header file
> On 15 Jul 2016, at 10:31, Thomas Monjalon > wrote: > > 2016-07-14 22:20, Damjan Marion: >> >>> On 15 Jul 2016, at 00:06, Thomas Monjalon >>> wrote: >>> >>> 2016-07-14 18:10, Damjan Marion: Dear Jan, Thank you for your comments. A bit too much overhead to submit simple patch so let?s forget about it. I will just add it as it is to our private collection of patches. >>> >>> These are changes trivial to fix when applying. >>> I strongly prefer that you upstream patches instead of keeping patches >>> in the VPP repository. I will help you in this task. >>> Thanks for the effort. >> >> Yeah, I agree with you, unfortunately it is all about time, >> for me it is still cheaper to rebase them. >> >> I respect your rules, but I just don?t have enough free cycles >> to spend learning all of them, for my occasional patch submission. > > We know there is a learning curve for the submission process. > That's why we do not expect it to be fully satisfied, especially > from occasional contributors. > I am used to do some not significant changes before applying to > help newcomers patches being accepted. That's what I will do in > your case. > As I said previously I will help you to drop your local patches. > > Please continue sending other patches with a detailed explanation > and we will take care of them. Ok, Thanks! So we don?t have much pending beside 2 patches for i40e which Jeff submitted yesterday and they will i guess need to wait for 16.11. Only one which I have on my mind is: https://git.fd.io/cgit/vpp/tree/dpdk/dpdk-16.04_patches/0005-Allow-applications-to-override-rte_delay_us.patch This is big issue for us when running single-core, as some drivers tend to call rte_delay_us for a long time, and that is causing packet drops. I.e. if you do stop/start on one interface and you are running BFD on another one, BFD will fail? Current patch is hack, it basically allows us to override delay function so we can de-schedule it, do some other useful work while waiting for delay to finish and then give control back to original function? Maybe we can fix this by providing a delay callback functionality... Thanks, Damjan
[dpdk-dev] spinlock: Move constructor function out of header file
2016-07-14 18:10, Damjan Marion: > Dear Jan, > > Thank you for your comments. A bit too much overhead to submit simple patch > so let?s forget about it. I will just add it as it is to our private > collection of patches. These are changes trivial to fix when applying. I strongly prefer that you upstream patches instead of keeping patches in the VPP repository. I will help you in this task. Thanks for the effort. > If anybody wants to pick it from here, please do... I can update the bsdapp Makefile and do the trivial changes for you, if we agree that we do not need to touch to other archs (see below). > > On 14 Jul 2016, at 20:03, Jan Viktorin wrote: > >> --- a/lib/librte_eal/linuxapp/eal/Makefile > >> +++ b/lib/librte_eal/linuxapp/eal/Makefile > >> @@ -106,6 +106,7 @@ SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_keepalive.c > >> > >> # from arch dir > >> SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_cpuflags.c > >> +SRCS-$(CONFIG_RTE_EXEC_ENV_LINUXAPP) += rte_spinlock.c > > > > This is not good, you provide rte_spinlock.c only for x86. Building > > for any other arch would fail to find this file. It is not used in other archs. It is really x86 specific. > > Moreover, the bsdapp/eal/Makefile should reflect this situation as > > well. Good catch.
[dpdk-dev] spinlock: Move constructor function out of header file
> On 15 Jul 2016, at 00:06, Thomas Monjalon > wrote: > > 2016-07-14 18:10, Damjan Marion: >> Dear Jan, >> >> Thank you for your comments. A bit too much overhead to submit simple patch >> so let?s forget about it. I will just add it as it is to our private >> collection of patches. > > These are changes trivial to fix when applying. > I strongly prefer that you upstream patches instead of keeping patches > in the VPP repository. I will help you in this task. > Thanks for the effort. Yeah, I agree with you, unfortunately it is all about time, for me it is still cheaper to rebase them. I respect your rules, but I just don?t have enough free cycles to spend learning all of them, for my occasional patch submission.
[dpdk-dev] spinlock: Move constructor function out of header file
Hello Damjan,
thank you for the patch. It makes sense to me.
Next time, please CC the appropriate maintainers.
(See the MAINTAINERS file in the root of the DPDK source tree.)
In the subject after "spinlock:" you should start with a lower case
letter, so "move constructor..."
On Thu, 14 Jul 2016 15:27:29 +0200
damarion at cisco.com wrote:
> From: Damjan Marion
>
> Having constructor function in the header gile is generaly
I'd write:
Having constructor functions in header files is generally a bad idea.
Anyway:
s/gile/file/
> bad idea, as it will eventually be implanted to 3rd party
> library.
>
> In this case it is causing linking issues with 3rd party
it causes linking issues
> libraries when application is not linked to dpdk, due to missing
an application
to dpdk due to a missing gymbol (no comma)
> symbol called by constructor.
Please include the following line:
Fixes: ba7468997ea6 ("spinlock: add HTM lock elision for x86")
>
> Signed-off-by: Damjan Marion
>
> ---
> lib/librte_eal/common/arch/x86/rte_spinlock.c | 45 ++
> .../common/include/arch/x86/rte_spinlock.h | 13 ++-
> lib/librte_eal/linuxapp/eal/Makefile | 1 +
> 3 files changed, 49 insertions(+), 10 deletions(-)
> create mode 100644 lib/librte_eal/common/arch/x86/rte_spinlock.c
>
> diff --git a/lib/librte_eal/common/arch/x86/rte_spinlock.c
> b/lib/librte_eal/common/arch/x86/rte_spinlock.c
> new file mode 100644
> index 000..ad8cc5a
> --- /dev/null
> +++ b/lib/librte_eal/common/arch/x86/rte_spinlock.c
> @@ -0,0 +1,45 @@
> +/*-
> + * BSD LICENSE
> + *
> + * Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * * Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in
> + * the documentation and/or other materials provided with the
> + * distribution.
> + * * Neither the name of Intel Corporation nor the names of its
> + * contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "rte_cpuflags.h"
> +
> +#include
According to:
http://dpdk.org/doc/guides-16.04/contributing/coding_style.html#coding-style
you should change the order of these includes.
> +
> +uint8_t rte_rtm_supported; /* cache the flag to avoid the overhead
> + of the rte_cpu_get_flag_enabled function */
The comment should be placed before the declaration or use the /**< */
Doxygen style. I'd prefer to placed it before. Can you fix it with this
patch?
> +
> +static void __attribute__((constructor))
> +rte_rtm_init(void)
> +{
> + rte_rtm_supported = rte_cpu_get_flag_enabled(RTE_CPUFLAG_RTM);
> +}
> diff --git a/lib/librte_eal/common/include/arch/x86/rte_spinlock.h
> b/lib/librte_eal/common/include/arch/x86/rte_spinlock.h
> index 02f95cb..8e630c2 100644
> --- a/lib/librte_eal/common/include/arch/x86/rte_spinlock.h
> +++ b/lib/librte_eal/common/include/arch/x86/rte_spinlock.h
> @@ -94,24 +94,17 @@ rte_spinlock_trylock (rte_spinlock_t *sl)
> }
> #endif
>
> -static uint8_t rtm_supported; /* cache the flag to avoid the overhead
> - of the rte_cpu_get_flag_enabled function */
> -
> -static inline void __attribute__((constructor))
> -rte_rtm_init(void)
> -{
> - rtm_supported = rte_cpu_get_flag_enabled(RTE_CPUFLAG_RTM);
> -}
> +extern uint8_t rte_rtm_supported;
>
> static inline int rte_tm_supported(void)
> {
> - return rtm_supported;
> + return rte_rtm_supported;
> }
>
> static inline int
> rte_try_tm(volatile int *lock)
> {
> - if (!rtm_supported)
> + if (!rte_rtm_supported)
> return 0;
>
[dpdk-dev] spinlock: Move constructor function out of header file
Dear Jan,
Thank you for your comments. A bit too much overhead to submit simple patch
so let?s forget about it. I will just add it as it is to our private
collection of patches.
If anybody wants to pick it from here, please do...
Thanks,
Damjan
> On 14 Jul 2016, at 20:03, Jan Viktorin wrote:
>
> Hello Damjan,
>
> thank you for the patch. It makes sense to me.
>
> Next time, please CC the appropriate maintainers.
> (See the MAINTAINERS file in the root of the DPDK source tree.)
>
> In the subject after "spinlock:" you should start with a lower case
> letter, so "move constructor..."
>
> On Thu, 14 Jul 2016 15:27:29 +0200
> damarion at cisco.com wrote:
>
>> From: Damjan Marion
>>
>> Having constructor function in the header gile is generaly
>
> I'd write:
>
> Having constructor functions in header files is generally a bad idea.
>
> Anyway:
>
> s/gile/file/
>
>> bad idea, as it will eventually be implanted to 3rd party
>> library.
>>
>> In this case it is causing linking issues with 3rd party
>
> it causes linking issues
>
>> libraries when application is not linked to dpdk, due to missing
>
> an application
>
> to dpdk due to a missing gymbol (no comma)
>
>> symbol called by constructor.
>
> Please include the following line:
>
> Fixes: ba7468997ea6 ("spinlock: add HTM lock elision for x86")
>
>>
>> Signed-off-by: Damjan Marion
>>
>> ---
>> lib/librte_eal/common/arch/x86/rte_spinlock.c | 45
>> ++
>> .../common/include/arch/x86/rte_spinlock.h | 13 ++-
>> lib/librte_eal/linuxapp/eal/Makefile | 1 +
>> 3 files changed, 49 insertions(+), 10 deletions(-)
>> create mode 100644 lib/librte_eal/common/arch/x86/rte_spinlock.c
>>
>> diff --git a/lib/librte_eal/common/arch/x86/rte_spinlock.c
>> b/lib/librte_eal/common/arch/x86/rte_spinlock.c
>> new file mode 100644
>> index 000..ad8cc5a
>> --- /dev/null
>> +++ b/lib/librte_eal/common/arch/x86/rte_spinlock.c
>> @@ -0,0 +1,45 @@
>> +/*-
>> + * BSD LICENSE
>> + *
>> + * Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
>> + * All rights reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * * Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * * Redistributions in binary form must reproduce the above copyright
>> + * notice, this list of conditions and the following disclaimer in
>> + * the documentation and/or other materials provided with the
>> + * distribution.
>> + * * Neither the name of Intel Corporation nor the names of its
>> + * contributors may be used to endorse or promote products derived
>> + * from this software without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>> +
>> +#include "rte_cpuflags.h"
>> +
>> +#include
>
> According to:
> http://dpdk.org/doc/guides-16.04/contributing/coding_style.html#coding-style
>
> you should change the order of these includes.
>
>> +
>> +uint8_t rte_rtm_supported; /* cache the flag to avoid the overhead
>> + of the rte_cpu_get_flag_enabled function */
>
> The comment should be placed before the declaration or use the /**< */
> Doxygen style. I'd prefer to placed it before. Can you fix it with this
> patch?
>
>> +
>> +static void __attribute__((constructor))
>> +rte_rtm_init(void)
>> +{
>> +rte_rtm_supported = rte_cpu_get_flag_enabled(RTE_CPUFLAG_RTM);
>> +}
>> diff --git a/lib/librte_eal/common/include/arch/x86/rte_spinlock.h
>> b/lib/librte_eal/common/include/arch/x86/rte_spinlock.h
>> index 02f95cb..8e630c2 100644
>> --- a/lib/librte_eal/common/include/arch/x86/rte_spinlock.h
>> +++ b/lib/librte_eal/common/include/arch/x86/rte_spinlock.h
>> @@ -94,24 +94,17 @@ rte_spinlock_trylock (rte_spinlock_t *sl)
>> }
>> #endif
>>
>> -static uint8_t rtm_supported; /* cache the flag to avoid the overhead
>> - of the rte_cpu_get_flag_enabled func
