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

  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>
---
 .../common/include/generic/rte_byteorder.h      | 58 +++++++++++++++-----
 1 file changed, 44 insertions(+), 14 deletions(-)
  

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.

> +#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?
  

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
  

08/06/2017 11:14, 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:
> > > +#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.

I agree
Here it is just adding a space in front of the single digit to make
bits numbers aligned on 2 digits :)

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

You're right.
RTE_BE_TO_CPU_16 does not make sense.
I think you could add a comment like that:
RTE_XE_NN is equivalent to rte_cpu_to_Xe_NN run-time conversion

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

Good point.

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

Yes good idea.