[dpdk-dev] [PATCH 1/2] eal: add static endianness conversion macros

[Adrien Mazarguil]

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 at 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