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

["Damjan Marion (damarion)" <damarion@cisco.com>]

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)