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 at 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 at cisco.com wrote:
>
>> From: Damjan Marion <damarion at 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 at 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)
