From: Adrien Mazarguil <adrien.mazarguil@6wind.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 1/2] eal: add static endianness conversion macros
Date: Thu, 8 Jun 2017 11:14:16 +0200 [thread overview]
Message-ID: <20170608091416.GU1758@6wind.com> (raw)
In-Reply-To: <44029570.D8ug5AmCbY@xps>
On Wed, Jun 07, 2017 at 04:16:58PM +0200, Thomas Monjalon wrote:
> Hi, some comments below:
>
> 18/05/2017 12:14, Adrien Mazarguil:
> > These macros resolve to constant expressions that allow developers to
> > perform endianness conversion on static/const objects, even outside of
> > function scope as they do not translate to function calls.
> >
> > This is most useful for static initializers and constant values (whenever
> > it has to be performed at compilation time). Run-time endianness conversion
> > of variable values should keep using rte_*_to_*() calls for best
> > performance.
> >
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> [...]
> > +#define RTE_STATIC_BSWAP64(v) \
> > + ((((uint64_t)(v) & UINT64_C(0x00000000000000ff)) << 56) | \
> > + (((uint64_t)(v) & UINT64_C(0x000000000000ff00)) << 40) | \
> > + (((uint64_t)(v) & UINT64_C(0x0000000000ff0000)) << 24) | \
> > + (((uint64_t)(v) & UINT64_C(0x00000000ff000000)) << 8) | \
> > + (((uint64_t)(v) & UINT64_C(0x000000ff00000000)) >> 8) | \
> > + (((uint64_t)(v) & UINT64_C(0x0000ff0000000000)) >> 24) | \
> > + (((uint64_t)(v) & UINT64_C(0x00ff000000000000)) >> 40) | \
> > + (((uint64_t)(v) & UINT64_C(0xff00000000000000)) >> 56))
>
> Minor nit: you could align lines by inserting a space before 8.
I think alignment attempts past the mandatory line indentation often end up
in a failure (e.g. when grouping macros by name, one of them inevitably
happens to be longer than initially envisioned, same for structure fields
and trailing comment blocks, etc.) Since I'm not convinced it improves
readability, I tend to avoid them altogether for consistency.
It's a matter of style but I can change that if you prefer.
> > +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN
> > +#define RTE_BE16(v) (uint16_t)(v)
> > +#define RTE_BE32(v) (uint32_t)(v)
> > +#define RTE_BE64(v) (uint64_t)(v)
> > +#define RTE_LE16(v) RTE_STATIC_BSWAP16(v)
> > +#define RTE_LE32(v) RTE_STATIC_BSWAP32(v)
> > +#define RTE_LE64(v) RTE_STATIC_BSWAP64(v)
> > +#elif RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > +#define RTE_BE16(v) RTE_STATIC_BSWAP16(v)
> > +#define RTE_BE32(v) RTE_STATIC_BSWAP32(v)
> > +#define RTE_BE64(v) RTE_STATIC_BSWAP64(v)
> > +#define RTE_LE16(v) (uint16_t)(v)
> > +#define RTE_LE32(v) (uint32_t)(v)
> > +#define RTE_LE64(v) (uint64_t)(v)
>
> This naming is confusing.
> Let's take RTE_BE16() as example, it does not say wether the input value
> is big endian or the output value will be big endian.
> I think we should mimic the wording of run-time conversions:
> RTE_BE_TO_CPU_16()
>
> Any other ideas?
First I'd like to keep those macro names as short as possible, ideally not
much larger than simply casting the provided value to the target type for
usability and readability purposes. Think about files full of static
initializers, while there are not many examples right now, the definition of
static rte_flow rules and capability trees will need to use these macros
extensively.
The fact you suggested RTE_BE_TO_CPU_16() instead of RTE_CPU_TO_BE_16() as a
replacement for RTE_BE16() highlights the misunderstanding. However I find
"CPU_TO" overly verbose, particularly since the reverse macros won't exist,
remember these are made for static conversions of integer constants resolved
at compilation time, not variables. Users may additionally confuse
RTE_CPU_TO_BE_16() with its similarly-named inline function counterpart.
Functions and macros are typically named after their output, not their
input. In that sense and without further precision, RTE_BE16() is fine in my
opinion.
Remember this [1]? I think we could make everything clearer by perhaps
applying it and casting the results of these macros to the proper type,
e.g.:
#define RTE_BE16(v) (rte_be16_t)(v)
I can probably modify this series to introduce the new types first, use them
in the conversion macro and then later clarify existing structure
fields. How about this?
[1] http://dpdk.org/ml/archives/dev/2016-November/050060.html
--
Adrien Mazarguil
6WIND
next prev parent reply other threads:[~2017-06-08 9:14 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-18 10:14 Adrien Mazarguil
2017-05-18 10:14 ` [dpdk-dev] [PATCH 2/2] ethdev: tidy up endianness handling in flow API Adrien Mazarguil
2017-06-07 14:16 ` [dpdk-dev] [PATCH 1/2] eal: add static endianness conversion macros Thomas Monjalon
2017-06-08 9:14 ` Adrien Mazarguil [this message]
2017-06-08 16:35 ` Thomas Monjalon
2017-06-15 15:48 ` [dpdk-dev] [PATCH v2 1/3] eal: introduce big and little endian types Adrien Mazarguil
2017-06-16 14:12 ` Thomas Monjalon
2017-06-15 15:48 ` [dpdk-dev] [PATCH v2 2/3] eal: add static endianness conversion macros Adrien Mazarguil
2017-06-15 15:48 ` [dpdk-dev] [PATCH v2 3/3] ethdev: tidy up endianness handling in flow API Adrien Mazarguil
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=20170608091416.GU1758@6wind.com \
--to=adrien.mazarguil@6wind.com \
--cc=dev@dpdk.org \
--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).