[dpdk-dev,01/38] eal: add support for 24 40 and 48 bit operations
Checks
Commit Message
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(+)
Comments
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.
/Bruce
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.
-
Shreyansh
16/06/2017 11:21, Shreyansh Jain:
> Hi Bruce,
>
> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > 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.
I prefer keeping common functions in a common place.
So I like this patch.
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 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.
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
> On Jun 19, 2017, at 6:00 AM, Shreyansh Jain <shreyansh.jain@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@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
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
On 6/19/2017 7:22 PM, Wiles, Keith wrote:
>
>> On Jun 19, 2017, at 6:00 AM, Shreyansh Jain <shreyansh.jain@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@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
>
> 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.
Regards,
Hemant
>
>
> On Jun 20, 2017, at 5:43 AM, Hemant Agrawal <hemant.agrawal@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@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@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
>>
>> 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?
>
> Regards,
> Hemant
>
>>
>>
>
>
Regards,
Keith
On 6/20/2017 8:04 PM, Wiles, Keith wrote:
>
>> On Jun 20, 2017, at 5:43 AM, Hemant Agrawal <hemant.agrawal@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@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@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
>>>
>>> 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
>
>
On Wed, Jun 21, 2017 at 01:47:52PM +0530, Hemant Agrawal wrote:
> On 6/20/2017 8:04 PM, Wiles, Keith wrote:
> >
> > > On Jun 20, 2017, at 5:43 AM, Hemant Agrawal
> > > <hemant.agrawal@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@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@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
> > > >
> > > > 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?
>
I don't have strong feelings either way. However, if there is only one
user of these functions right now, I'd normally wait till there is a
second user before moving them to a common area. If others feel
differently, I'm ok with having them as common right now.
/Bruce
On Wed, Jun 21, 2017 at 01:47:52PM +0530, Hemant Agrawal wrote:
> On 6/20/2017 8:04 PM, Wiles, Keith wrote:
> >
> >>On Jun 20, 2017, at 5:43 AM, Hemant Agrawal <hemant.agrawal@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@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@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
> >>>
> >>>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?
Well, I'm not against adding them, however as described in my previous
reply, the fact input/output values are not necessarily aligned correctly
due to their type width must be documented then to avoid issues later (a
couple of ascii art diagrams should clear that up).
To summarize, this is the same issue as storing the result of a 32-bit
conversion macro as a 64-bit value:
uint32_t be64 = htonl(0x12345678);
On little endian systems, be64 looks like:
12 34 56 78 00 00 00 00
While on big endian systems:
00 00 00 00 12 34 56 78
Therefore if this value was written as is to a 64-bit field of a packet sent
over the network, it would correctly work only if the host CPU was big
endian. It's not really "be64" but "be32 stored inside uint64_t and padded
in host CPU order", which could be error prone if left undocumented.
On 06/16/2017 08:40 AM, 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(+)
>
> diff --git a/lib/librte_eal/common/include/generic/rte_byteorder.h b/lib/librte_eal/common/include/generic/rte_byteorder.h
> index e00bccb..8903ff6 100644
> --- a/lib/librte_eal/common/include/generic/rte_byteorder.h
> +++ b/lib/librte_eal/common/include/generic/rte_byteorder.h
> @@ -122,6 +122,84 @@ rte_constant_bswap64(uint64_t x)
> ((x & 0xff00000000000000ULL) >> 56);
> }
>
> +/*
> + * An internal function to swap bytes of a 48-bit value.
> + */
> +static inline uint64_t
> +rte_constant_bswap48(uint64_t x)
> +{
> + return ((x & 0x0000000000ffULL) << 40) |
> + ((x & 0x00000000ff00ULL) << 24) |
> + ((x & 0x000000ff0000ULL) << 8) |
> + ((x & 0x0000ff000000ULL) >> 8) |
> + ((x & 0x00ff00000000ULL) >> 24) |
> + ((x & 0xff0000000000ULL) >> 40);
> +}
> +
Won't something like bswap64(x << 16) be much more efficient? Two
instructions for the non-constant case, compared to 15-20 here.
@@ -122,6 +122,84 @@ rte_constant_bswap64(uint64_t x)
((x & 0xff00000000000000ULL) >> 56);
}
+/*
+ * An internal function to swap bytes of a 48-bit value.
+ */
+static inline uint64_t
+rte_constant_bswap48(uint64_t x)
+{
+ return ((x & 0x0000000000ffULL) << 40) |
+ ((x & 0x00000000ff00ULL) << 24) |
+ ((x & 0x000000ff0000ULL) << 8) |
+ ((x & 0x0000ff000000ULL) >> 8) |
+ ((x & 0x00ff00000000ULL) >> 24) |
+ ((x & 0xff0000000000ULL) >> 40);
+}
+
+/*
+ * An internal function to swap bytes of a 40-bit value.
+ */
+static inline uint64_t
+rte_constant_bswap40(uint64_t x)
+{
+ return ((x & 0x00000000ffULL) << 32) |
+ ((x & 0x000000ff00ULL) << 16) |
+ ((x & 0x0000ff0000ULL)) |
+ ((x & 0x00ff000000ULL) >> 16) |
+ ((x & 0xff00000000ULL) >> 32);
+}
+
+/*
+ * An internal function to swap bytes of a 24-bit value.
+ */
+static inline uint32_t
+rte_constant_bswap24(uint32_t x)
+{
+ return ((x & 0x0000ffULL) << 16) |
+ ((x & 0x00ff00ULL)) |
+ ((x & 0xff0000ULL) >> 16);
+}
+
+#define rte_bswap24 rte_constant_bswap24
+#define rte_bswap40 rte_constant_bswap40
+#define rte_bswap48 rte_constant_bswap48
+
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+
+#define rte_cpu_to_le_24(x) (x)
+#define rte_cpu_to_le_40(x) (x)
+#define rte_cpu_to_le_48(x) (x)
+
+#define rte_cpu_to_be_24(x) rte_bswap24(x)
+#define rte_cpu_to_be_40(x) rte_bswap40(x)
+#define rte_cpu_to_be_48(x) rte_bswap48(x)
+
+#define rte_le_to_cpu_24(x) (x)
+#define rte_le_to_cpu_40(x) (x)
+#define rte_le_to_cpu_48(x) (x)
+
+#define rte_be_to_cpu_24(x) rte_bswap24(x)
+#define rte_be_to_cpu_40(x) rte_bswap40(x)
+#define rte_be_to_cpu_48(x) rte_bswap48(x)
+
+#else /* RTE_BIG_ENDIAN */
+
+#define rte_cpu_to_le_24(x) rte_bswap24(x)
+#define rte_cpu_to_le_40(x) rte_bswap40(x)
+#define rte_cpu_to_le_48(x) rte_bswap48(x)
+
+#define rte_cpu_to_be_24(x) (x)
+#define rte_cpu_to_be_40(x) (x)
+#define rte_cpu_to_be_48(x) (x)
+
+#define rte_le_to_cpu_24(x) rte_bswap24(x)
+#define rte_le_to_cpu_40(x) rte_bswap40(x)
+#define rte_le_to_cpu_48(x) rte_bswap48(x)
+
+#define rte_be_to_cpu_24(x) (x)
+#define rte_be_to_cpu_40(x) (x)
+#define rte_be_to_cpu_48(x) (x)
+#endif
#ifdef __DOXYGEN__