Re: [dpdk-dev] [PATCH 01/38] eal: add support for 24 40 and 48 bit operations

[Shreyansh Jain <shreyansh.jain@nxp.com>]

Hello Adrien,

On Friday 16 June 2017 04:04 PM, Adrien Mazarguil wrote:
> Hi Shreyansh,
> 
> On Fri, Jun 16, 2017 at 09:21:35AM +0000, Shreyansh Jain wrote:
>> Hi Bruce,
>>
>>> -----Original Message-----
>>> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
>>> Sent: Friday, June 16, 2017 2:27 PM
>>> To: Shreyansh Jain <shreyansh.jain@nxp.com>
>>> Cc: dev@dpdk.org; ferruh.yigit@intel.com; Hemant Agrawal
>>> <hemant.agrawal@nxp.com>
>>> Subject: Re: [dpdk-dev] [PATCH 01/38] eal: add support for 24 40 and 48 bit
>>> operations
>>>
>>> On Fri, Jun 16, 2017 at 11:10:31AM +0530, Shreyansh Jain wrote:
>>>> From: Hemant Agrawal <hemant.agrawal@nxp.com>
>>>>
>>>> Bit Swap and LE<=>BE conversions for 23, 40 and 48 bit width
>>>>
>>>> Signed-off-by: Hemant Agrawal <hemant.agrawal@nxp.com>
>>>> ---
>>>>   .../common/include/generic/rte_byteorder.h         | 78
>>> ++++++++++++++++++++++
>>>>   1 file changed, 78 insertions(+)
>>>>
>>> Are these really common enough for inclusion in an generic EAL file?
>>> Would they be better being driver specific, so that we don't end up with
>>> lots of extra byte-swap routines for each possible size used by a
>>> driver.
>>   
>> Reasoning was to keep all bit/byte swap at a single place and if it is
>> useful for others.
>>
>>  From DPAA perspective, these macro can be anywhere. In case someone else too
>> has use of this (now or in near-future), probably then we can consider this
>> in EAL.
>> Else, if I don't get much responses in a few days, I will shift them to
>> DPAA driver in next version of this series.
> 
> While I'm not against exposing exotic byte swapping functions, they are not
> completely safe and I'm not sure they should be part of public header files
> on that basis.
> 
> Problem is their storage size is larger than the number of bytes they deal
> with, which raises the question: are filler bytes prepended or appended to
> the converted value? How about input values in non-native order? Answering
> that is not so easy as it depends on the use case. We actually had a similar
> issue when defining VXLAN's VNI field for rte_flow, which is 24-bit in
> network order.
> 
> Take rte_constant_bswap48() for instance, assuming input value is
> little-endian, output is supposed to be big-endian. While the shifts are
> correct, filler bytes are not in the right place for a big-endian system,
> and the resulting value stored on uint64_t cannot be used as-is. Again, that
> depends on the use case, it could be correct if the resulting value was to
> be used as is on a little-endian system.

I understand what you have stated - the application or any user needs to 
be context aware about what they are using and the side-effect of such 
conversions.

> 
> I think the only safe way to deal with that is by defining specific types of
> the proper size, e.g.:
> 
>   typedef uint8_t uint48_t[6];
> 
> These are cumbersome and cannot be used like normal integers though. With
> such types, byte-swapping functions become meaningless.
> 
> Since these are supposed to be rather simple functions, I'm not sure
> handling/documenting all this complexity in rte_byteorder.h makes sense.
> 

I have no issues moving these into DPAA specific code. Hemant added them 
in generic just in case they would be of use to others.

-
Shreyansh