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

[Hemant Agrawal]

On 6/20/2017 8:04 PM, Wiles, Keith wrote:
>
>> On Jun 20, 2017, at 5:43 AM, Hemant Agrawal <hemant.agrawal at nxp.com> wrote:
>>
>> On 6/19/2017 7:22 PM, Wiles, Keith wrote:
>>>
>>>> On Jun 19, 2017, at 6:00 AM, Shreyansh Jain <shreyansh.jain at nxp.com> wrote:
>>>>
>>>> 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 at intel.com]
>>>>>>> Sent: Friday, June 16, 2017 2:27 PM
>>>>>>> To: Shreyansh Jain <shreyansh.jain at nxp.com>
>>>>>>> Cc: dev at dpdk.org; ferruh.yigit at intel.com; Hemant Agrawal
>>>>>>> <hemant.agrawal at 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 at nxp.com>
>>>>>>>>
>>>>>>>> Bit Swap and LE<=>BE conversions for 23, 40 and 48 bit width
>>>>>>>>
>>>>>>>> Signed-off-by: Hemant Agrawal <hemant.agrawal at 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
>>>
>>> These are all static inline functions, so no real code increase unless used and having them in one spot is the best place IMO.
>>>
>>>
>>> Regards,
>>> Keith
>>
>> Yes! these are simple static inline functions with no core unless used.
>> Many hardware accelerators usages 40 bit & 48 bits data. we thought, it can be usable by others as well.
>>
>> currently we are seeing a mixed opinion.
>>
>> In next revision, We will move them within our driver code. If someone need them in future, we can always bring them to eal.
>
> Is there really a big objection to allowing this patch to be accepted?

Bruce, Adrien
	Any opinion?

Regards,
Hemant
>
>>
>> Regards,
>> Hemant
>>
>>>
>>>
>>
>>
>
> Regards,
> Keith
>
>