[dpdk-dev] [PATCH] spinlock: Move constructor function out of header file

[dpdk-dev] [PATCH] spinlock: Move constructor function out of header file

[damarion]

From: Damjan Marion <damarion@cisco.com>

Having constructor function in the header gile is generaly
bad idea, as it will eventually be implanted to 3rd party
library.

In this case it is causing linking issues with 3rd party
libraries when application is not linked to dpdk, due to missing
symbol called by constructor.

Signed-off-by: Damjan Marion <damarion@cisco.com>
---
 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 0000000..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 <stdint.h>
+
+uint8_t rte_rtm_supported; /* cache the flag to avoid the overhead
+			      of the rte_cpu_get_flag_enabled function */
+
+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;
 
 	int retries = RTE_RTM_MAX_RETRIES;
diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
index 1a97693..7287d13 100644
--- 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
 
 CFLAGS_eal_common_cpuflags.o := $(CPUFLAGS_LIST)
 
-- 
2.7.4

Re: [dpdk-dev] spinlock: Move constructor function out of header file

[Jan Viktorin]

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@cisco.com wrote:

> From: Damjan Marion <damarion@cisco.com>
> 
> 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 <damarion@cisco.com>
> 
> ---
> 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 0000000..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 <stdint.h>

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;
>  
>  	int retries = RTE_RTM_MAX_RETRIES;
> diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
> index 1a97693..7287d13 100644
> --- 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.
  
Moreover, the bsdapp/eal/Makefile should reflect this situation as
well.

Regards
Jan

>  
>  CFLAGS_eal_common_cpuflags.o := $(CPUFLAGS_LIST)
>

Re: [dpdk-dev] spinlock: Move constructor function out of header file

[Damjan Marion (damarion)]

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 <viktorin@rehivetech.com> 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@cisco.com wrote:
> 
>> From: Damjan Marion <damarion@cisco.com>
>> 
>> 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 <damarion@cisco.com>
>> 
>> ---
>> 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 0000000..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 <stdint.h>
> 
> 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;
>> 
>> 	int retries = RTE_RTM_MAX_RETRIES;
>> diff --git a/lib/librte_eal/linuxapp/eal/Makefile b/lib/librte_eal/linuxapp/eal/Makefile
>> index 1a97693..7287d13 100644
>> --- 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.
> 
> Moreover, the bsdapp/eal/Makefile should reflect this situation as
> well.
> 
> Regards
> Jan
> 
>> 
>> CFLAGS_eal_common_cpuflags.o := $(CPUFLAGS_LIST)

Re: [dpdk-dev] spinlock: Move constructor function out of header file

[Thomas Monjalon]

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 <viktorin@rehivetech.com> 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.

Re: [dpdk-dev] spinlock: Move constructor function out of header file

[Damjan Marion (damarion)]

> On 15 Jul 2016, at 00:06, Thomas Monjalon <thomas.monjalon@6wind.com> 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.

Re: [dpdk-dev] spinlock: Move constructor function out of header file

[Thomas Monjalon]

2016-07-14 22:20, Damjan Marion:
> 
> > On 15 Jul 2016, at 00:06, Thomas Monjalon <thomas.monjalon@6wind.com> 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.

Re: [dpdk-dev] spinlock: Move constructor function out of header file

[Damjan Marion (damarion)]

> On 15 Jul 2016, at 10:31, Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> 
> 2016-07-14 22:20, Damjan Marion:
>> 
>>> On 15 Jul 2016, at 00:06, Thomas Monjalon <thomas.monjalon@6wind.com> 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

Re: [dpdk-dev] spinlock: Move constructor function out of header file

[Thomas Monjalon]

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.

Re: [dpdk-dev] spinlock: Move constructor function out of header file

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

Re: [dpdk-dev] spinlock: Move constructor function out of header file

[Thomas Monjalon]

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

Re: [dpdk-dev] spinlock: Move constructor function out of header file

[Damjan Marion (damarion)]

> On 15 Jul 2016, at 12:09, Thomas Monjalon <thomas.monjalon@6wind.com> 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...

Re: [dpdk-dev] spinlock: Move constructor function out of header file

[Damjan Marion (damarion)]

> On 15 Jul 2016, at 12:09, Thomas Monjalon <thomas.monjalon@6wind.com> 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...

Re: [dpdk-dev] spinlock: Move constructor function out of header file

[Damjan Marion (damarion)]

> On 15 Jul 2016, at 17:08, Thomas Monjalon <thomas.monjalon@6wind.com> 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@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!