From: David Christensen <drc@linux.vnet.ibm.com>
To: Thomas Monjalon <thomas@monjalon.net>, dev@dpdk.org
Cc: Bruce Richardson <bruce.richardson@intel.com>,
david.marchand@redhat.com, Jerin Jacob <jerinj@marvell.com>,
Gavin Hu <gavin.hu@arm.com>
Subject: Re: [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches
Date: Mon, 8 Jul 2019 10:24:14 -0700 [thread overview]
Message-ID: <6bc0b95e-89f3-8fef-5c32-ccf634c8b31c@linux.vnet.ibm.com> (raw)
In-Reply-To: <62492340.4kEa2brbSF@xps>
> 01/07/2019 22:41, Bruce Richardson:
>> On Mon, Jul 01, 2019 at 09:30:02PM +0200, Thomas Monjalon wrote:
>>> 29/05/2019 17:41, Bruce Richardson:
>>>> Use the flag checking functions and a couple of empty stubs to remove the
>>>> ifdefs from the middle of the C code, and replace them with more readable
>>>> regular if statements. Other ifdefs at the top of the file are kept, but
>>>> are not mixed with C code, so there is a clean separation.
>>>>
>>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>>>> ---
>>>> lib/librte_net/rte_net_crc.c | 38 ++++++++++++++++++++++++------------
>>>> 1 file changed, 25 insertions(+), 13 deletions(-)
>>>
>>> The result is more lines of code :)
>>>
>>>> --- a/lib/librte_net/rte_net_crc.c
>>>> +++ b/lib/librte_net/rte_net_crc.c
>>>> @@ -18,8 +18,17 @@
>>>>
>>>> #ifdef X86_64_SSE42_PCLMULQDQ
>>>> #include <net_crc_sse.h>
>>>> -#elif defined ARM64_NEON_PMULL
>>>> +#else
>>>> +/* define stubs for the SSE functions to avoid compiler errors */
>>>> +#define handlers_sse42 handlers_scalar
>>>> +#define rte_net_crc_sse42_init() do { } while(0)
>>>> +#endif
>>>> +
>>>> +#ifdef ARM64_NEON_PMULL
>>>> #include <net_crc_neon.h>
>>>> +#else
>>>> +#define handlers_neon handlers_scalar
>>>> +#define rte_net_crc_neon_init() do { } while(0)
>>>> #endif
>>>
>>> Looking at the need for stubs, I don't see the benefit.
>>>
>> Yes, one needs stubs, but those are placed in a single place, and the main
>> C-code itself is free of ifdefs running through it. I'd view this as a good
>> thing in limiting the scope of any ifdef-ery, since it annoys me looking at
>> #ifdefs in the middle of functions (since it messes up the indentation flow
>> of the code if nothing else!).
>>
>> If you don't see this as a big benefit, then there is not a lot else to
>> commend this set for you, sadly. It just seemed a nice improvement to me -
>> irrespective of net lines of code.
>
> Any other opinion?
I support adding a few lines of code in the slow path to provide cleaner
separation of architecture specific code, though the existing IFDEF code
doesn't appear very intrusive in this case. My preference would be
architecture specific header files and strong/weak linking to pull in
the appropriate code.
Dave
next prev parent reply other threads:[~2019-07-08 17:24 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-29 15:41 [dpdk-dev] [PATCH 0/4] Enhance CPU flag support Bruce Richardson
2019-05-29 15:41 ` [dpdk-dev] [PATCH 1/4] build: fix quoting on RTE_ARCH string value Bruce Richardson
2019-05-29 15:53 ` Luca Boccassi
2019-05-29 15:41 ` [dpdk-dev] [PATCH 2/4] config/arm: fix missing define for arm platforms Bruce Richardson
2019-05-29 15:41 ` [dpdk-dev] [PATCH 3/4] eal: allow checking CPU flags by name Bruce Richardson
2019-06-27 13:22 ` David Marchand
2019-06-28 12:40 ` Bruce Richardson
2019-06-28 13:34 ` David Marchand
2019-05-29 15:41 ` [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches Bruce Richardson
2019-07-01 19:30 ` Thomas Monjalon
2019-07-01 20:41 ` Bruce Richardson
2019-07-04 20:20 ` Thomas Monjalon
2019-07-08 17:24 ` David Christensen [this message]
2019-06-27 12:39 ` [dpdk-dev] [PATCH 0/4] Enhance CPU flag support Ananyev, Konstantin
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=6bc0b95e-89f3-8fef-5c32-ccf634c8b31c@linux.vnet.ibm.com \
--to=drc@linux.vnet.ibm.com \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=gavin.hu@arm.com \
--cc=jerinj@marvell.com \
--cc=thomas@monjalon.net \
/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).