From: Olivier MATZ <olivier.matz@6wind.com>
To: "Wiles, Keith" <keith.wiles@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v8 1/2] mk:Simplify the ifdefs in rte.app.mk
Date: Thu, 14 May 2015 14:21:11 +0200 [thread overview]
Message-ID: <55549337.2040106@6wind.com> (raw)
In-Reply-To: <D178CD17.200B2%keith.wiles@intel.com>
Hi Keith,
On 05/13/2015 04:43 PM, Wiles, Keith wrote:
>
>
> On 5/13/15, 9:28 AM, "Olivier MATZ" <olivier.matz@6wind.com> wrote:
>
>>
>> On 05/13/2015 04:04 PM, Wiles, Keith wrote:
>>>
>>>
>>> On 5/13/15, 8:56 AM, "Olivier MATZ" <olivier.matz@6wind.com> wrote:
>>>
>>>> Hi Keith,
>>>>
>>>> On 05/13/2015 03:17 PM, Wiles, Keith wrote:
>>>>>>>
>>>>>>> endif # ifeq ($(NO_AUTOLIBS),)
>>>>>>>
>>>>>>> -LDLIBS += $(CPU_LDLIBS)
>>>>>>> +LDLIBS += $(_LDLIBS-y) $(EXTRA_LDLIBS)
>>>>>>>
>>>>>>
>>>>>> As discussed in the previous mail, all things that are about
>>>>>> EXTRA_LDLIBS should be moved in the second patch. Therefore,
>>>>>> the title of the second patch should not be "update doc...", but
>>>>>> something like "mk: introduce EXTRA_LDLIBS...".
>>>>>>
>>>>>> By the way, I missed that before, but it seems that your
>>>>>> patch removes CPU_LDLIBS, I don't think it's correct.
>>>>>
>>>>> I found no reference to CPU_LDLIBS in the docs or code other then then
>>>>> one
>>>>> line. We now have EXTRA_LDLIBS for the command line, right?
>>>>
>>>> Yes, but your patch says "simplify the ifdef". Removing
>>>> a variable (even if it is not used) in this patch is not
>>>> a good idea.
>>>>
>>>> Now, the CPU_CFLAGS, CPU_LDFLAGS, CPU_LDLIBS can be defined internally
>>>> by the rte.vars.mk in mk/arch/ or mk/machine/ directories.
>>>
>>> No docs for CPU_LDLIBS or reference to that variable, which means it
>>> does
>>> not exist, right?
>>> If it was used or documented then I would agree. Having magic variables
>>> is
>>> not a good idea. I will add the CPU_LDLIBS in to the line, but someone
>>> will have to document that variable.
>>
>> First, this variable is internal to DPDK framework. It is not
>
> Not referenced in the code any place except the one line and not
> referenced in the docs, which means no one used that variable. By
> definition this is some magic variable that someone could use only because
> he knew about that variable.
>
> As I stated before, I will add that variable back, but it must be
> documented, correct?
>
> Look at the code a number of CPU_XXXX flags are commented about in the
> code.
It could be added at the same place than other CPU_* variables,
in another patch.
Olivier
>
> machine/power8/rte.vars.mk:39:# - can define CPU_CFLAGS variable
> (overridden by cmdline value) that
> machine/power8/rte.vars.mk:41:# - can define CPU_LDFLAGS variable
> (overridden by cmdline value) that
> machine/power8/rte.vars.mk:43:# - can define CPU_ASFLAGS variable
> (overridden by cmdline value) that
> machine/power8/rte.vars.mk:53:# CPU_CFLAGS =
> machine/power8/rte.vars.mk:54:# CPU_LDFLAGS =
> machine/power8/rte.vars.mk:55:# CPU_ASFLAGS =
> machine/wsm/rte.vars.mk:40:# - can define CPU_CFLAGS variable (overriden
> by cmdline value) that
> machine/wsm/rte.vars.mk:42:# - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/wsm/rte.vars.mk:44:# - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/wsm/rte.vars.mk:54:# CPU_CFLAGS =
> machine/wsm/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/wsm/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/native/rte.vars.mk:40:# - can define CPU_CFLAGS variable
> (overriden by cmdline value) that
> machine/native/rte.vars.mk:42:# - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/native/rte.vars.mk:44:# - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/native/rte.vars.mk:54:# CPU_CFLAGS =
> machine/native/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/native/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/native/rte.vars.mk:66: CPU_SSE42_SUPPORT = $(shell grep SSE4\.2
> /var/run/dmesg.boot 2>/dev/null)
> machine/native/rte.vars.mk:67: ifneq ($(CPU_SSE42_SUPPORT),)
> machine/atm/rte.vars.mk:40:# - can define CPU_CFLAGS variable (overriden
> by cmdline value) that
> machine/atm/rte.vars.mk:42:# - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/atm/rte.vars.mk:44:# - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/atm/rte.vars.mk:54:# CPU_CFLAGS =
> machine/atm/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/atm/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/hsw/rte.vars.mk:40:# - can define CPU_CFLAGS variable (overriden
> by cmdline value) that
> machine/hsw/rte.vars.mk:42:# - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/hsw/rte.vars.mk:44:# - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/hsw/rte.vars.mk:54:# CPU_CFLAGS =
> machine/hsw/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/hsw/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/ivb/rte.vars.mk:40:# - can define CPU_CFLAGS variable (overriden
> by cmdline value) that
> machine/ivb/rte.vars.mk:42:# - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/ivb/rte.vars.mk:44:# - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/ivb/rte.vars.mk:54:# CPU_CFLAGS =
> machine/ivb/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/ivb/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/snb/rte.vars.mk:40:# - can define CPU_CFLAGS variable (overriden
> by cmdline value) that
> machine/snb/rte.vars.mk:42:# - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/snb/rte.vars.mk:44:# - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/snb/rte.vars.mk:54:# CPU_CFLAGS =
> machine/snb/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/snb/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/nhm/rte.vars.mk:40:# - can define CPU_CFLAGS variable (overriden
> by cmdline value) that
> machine/nhm/rte.vars.mk:42:# - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/nhm/rte.vars.mk:44:# - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/nhm/rte.vars.mk:54:# CPU_CFLAGS =
> machine/nhm/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/nhm/rte.vars.mk:56:# CPU_ASFLAGS =
> machine/default/rte.vars.mk:40:# - can define CPU_CFLAGS variable
> (overriden by cmdline value) that
> machine/default/rte.vars.mk:42:# - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> machine/default/rte.vars.mk:44:# - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> machine/default/rte.vars.mk:54:# CPU_CFLAGS =
> machine/default/rte.vars.mk:55:# CPU_LDFLAGS =
> machine/default/rte.vars.mk:56:# CPU_ASFLAGS =
> rte.lib.mk:43:CPU_LDFLAGS += --version-script=$(SRCDIR)/$(EXPORT_MAP)
> rte.lib.mk:67:LD := $(CC) $(CPU_CFLAGS)
> rte.lib.mk:68:_CPU_LDFLAGS := $(call linkerprefix,$(CPU_LDFLAGS))
> rte.lib.mk:70:_CPU_LDFLAGS := $(CPU_LDFLAGS)
> rte.lib.mk:82:O_TO_S = $(LD) $(_CPU_LDFLAGS) -shared $(OBJS-y)
> -Wl,-soname,$(LIB) -o $(LIB)
> rte.app.mk:144:LDLIBS += $(_LDLIBS-y) $(CPU_LDLIBS) $(EXTRA_LDLIBS)
> arch/x86_64/rte.vars.mk:39:# - define CPU_CFLAGS variable (overriden by
> cmdline or previous
> arch/x86_64/rte.vars.mk:41:# - define CPU_LDFLAGS variable (overriden by
> cmdline or previous
> arch/x86_64/rte.vars.mk:43:# - define CPU_ASFLAGS variable (overriden by
> cmdline or previous
> arch/x86_64/rte.vars.mk:55:CPU_CFLAGS ?= -m64
> arch/x86_64/rte.vars.mk:56:CPU_LDFLAGS ?=
> arch/x86_64/rte.vars.mk:57:CPU_ASFLAGS ?= -felf64
> arch/x86_64/rte.vars.mk:59:export ARCH CROSS CPU_CFLAGS CPU_LDFLAGS
> CPU_ASFLAGS
> arch/i686/rte.vars.mk:39:# - define CPU_CFLAGS variable (overriden by
> cmdline or previous
> arch/i686/rte.vars.mk:41:# - define CPU_LDFLAGS variable (overriden by
> cmdline or previous
> arch/i686/rte.vars.mk:43:# - define CPU_ASFLAGS variable (overriden by
> cmdline or previous
> arch/i686/rte.vars.mk:55:CPU_CFLAGS ?= -m32
> arch/i686/rte.vars.mk:56:CPU_LDFLAGS ?= -melf_i386
> arch/i686/rte.vars.mk:57:CPU_ASFLAGS ?= -felf
> arch/i686/rte.vars.mk:59:export ARCH CROSS CPU_CFLAGS CPU_LDFLAGS
> CPU_ASFLAGS
> arch/x86_x32/rte.vars.mk:39:# - define CPU_CFLAGS variable (overridden
> by cmdline or previous
> arch/x86_x32/rte.vars.mk:41:# - define CPU_LDFLAGS variable (overridden
> by cmdline or previous
> arch/x86_x32/rte.vars.mk:43:# - define CPU_ASFLAGS variable (overridden
> by cmdline or previous
> arch/x86_x32/rte.vars.mk:54:CPU_CFLAGS ?= -mx32
> arch/x86_x32/rte.vars.mk:55:CPU_LDFLAGS ?= -melf32_x86_64
> arch/x86_x32/rte.vars.mk:56:#CPU_ASFLAGS ?= -felf64
> arch/x86_x32/rte.vars.mk:59:ifneq ($(shell echo | $(CC) $(CPU_CFLAGS) -E -
> 2>/dev/null 1>/dev/null && echo 0), 0)
> arch/x86_x32/rte.vars.mk:63:export ARCH CROSS CPU_CFLAGS CPU_LDFLAGS
> CPU_ASFLAGS
> arch/ppc_64/rte.vars.mk:35:CPU_CFLAGS ?= -m64 -DRTE_CACHE_LINE_SIZE=128
> arch/ppc_64/rte.vars.mk:36:CPU_LDFLAGS ?=
> arch/ppc_64/rte.vars.mk:37:CPU_ASFLAGS ?= -felf64
> arch/ppc_64/rte.vars.mk:39:export ARCH CROSS CPU_CFLAGS CPU_LDFLAGS
> CPU_ASFLAGS
> target/generic/rte.vars.mk:46:# - can define CPU_CFLAGS variable
> (overriden by cmdline value) that
> target/generic/rte.vars.mk:48:# - can define CPU_LDFLAGS variable
> (overriden by cmdline value) that
> target/generic/rte.vars.mk:50:# - can define CPU_ASFLAGS variable
> (overriden by cmdline value) that
> target/generic/rte.vars.mk:64:# - define CPU_CFLAGS variable (overriden
> by cmdline or previous
> target/generic/rte.vars.mk:66:# - define CPU_LDFLAGS variable (overriden
> by cmdline or previous
> target/generic/rte.vars.mk:68:# - define CPU_ASFLAGS variable (overriden
> by cmdline or previous
> target/generic/rte.vars.mk:110:CFLAGS := $(CPU_CFLAGS) $(EXECENV_CFLAGS)
> $(TOOLCHAIN_CFLAGS) $(MACHINE_CFLAGS)
> target/generic/rte.vars.mk:114:LDFLAGS := $(CPU_LDFLAGS)
> $(EXECENV_LDFLAGS) $(TOOLCHAIN_LDFLAGS) $(MACHINE_LDFLAGS)
> target/generic/rte.vars.mk:118:ASFLAGS := $(CPU_ASFLAGS)
> $(EXECENV_ASFLAGS) $(TOOLCHAIN_ASFLAGS) $(MACHINE_ASFLAGS)
> rte.sharelib.mk:52:LD := $(CC) $(CPU_CFLAGS)
> rte.sharelib.mk:53:O_TO_S = $(LD) $(call linkerprefix,$(CPU_LDFLAGS)) \
> rte.sharelib.mk:56:O_TO_S = $(LD) $(CPU_LDFLAGS) \
>
>
>
>> documented, because the goal of the documentation is not to
>> document all internal variables. In one word, it's not a magic
>> variable at all, it is simply a variable.
>>
>> Now, the heart of the matter is that your patch silently remove
>> this line. This is not acceptable and that's why I'm commenting
>> on it. The goal of patch splitting and proper title is to separate
>> features, and win time when searching in the git log for the cause
>> of a problem.
>>
>> This is the same problem with EXTRA_LDLIBS. How can you justify
>> having 2 patches:
>> - "simplify the ifdef" that also adds the EXTRA_LDLIBS
>> then
>> - "update the documentation for EXTRA_LDLIBS"
>>
>> Regards,
>> Olivier
>>
>>
>>
>>>>
>>>> Regards,
>>>> Olivier
>>>>
>>>
>>
>
>
next prev parent reply other threads:[~2015-05-14 12:21 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-13 14:43 Wiles, Keith
2015-05-14 12:21 ` Olivier MATZ [this message]
-- strict thread matches above, loose matches on Subject: below --
2015-05-11 23:14 [dpdk-dev] [PATCH v7 1/2] Simplify " Keith Wiles
2015-05-12 19:11 ` [dpdk-dev] [PATCH v8 1/2] mk:Simplify " Keith Wiles
2015-05-13 7:41 ` Olivier MATZ
2015-05-13 13:17 ` Wiles, Keith
2015-05-13 13:56 ` Olivier MATZ
2015-05-13 14:04 ` Wiles, Keith
2015-05-13 14:28 ` Olivier MATZ
2015-05-13 14:34 ` Wiles, Keith
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55549337.2040106@6wind.com \
--to=olivier.matz@6wind.com \
--cc=dev@dpdk.org \
--cc=keith.wiles@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).