> On Jun 19, 2017, at 6:00 AM, Shreyansh Jain <shreyansh.j...@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.richard...@intel.com] >>>> Sent: Friday, June 16, 2017 2:27 PM >>>> To: Shreyansh Jain <shreyansh.j...@nxp.com> >>>> Cc: dev@dpdk.org; ferruh.yi...@intel.com; Hemant Agrawal >>>> <hemant.agra...@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.agra...@nxp.com> >>>>> >>>>> Bit Swap and LE<=>BE conversions for 23, 40 and 48 bit width >>>>> >>>>> Signed-off-by: Hemant Agrawal <hemant.agra...@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
