From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH 01/38] eal: add support for 24 40 and 48 bit operations Date: Mon, 19 Jun 2017 16:30:45 +0530 Message-ID: <142b5789-3234-0036-4965-cf4fc78d942d@nxp.com> References: <1497591668-3320-1-git-send-email-shreyansh.jain@nxp.com> <1497591668-3320-2-git-send-email-shreyansh.jain@nxp.com> <20170616085719.GB82628@bricha3-MOBL3.ger.corp.intel.com> <20170616103402.GB1758@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: Bruce Richardson , "dev@dpdk.org" , "ferruh.yigit@intel.com" , Hemant Agrawal , Thomas Monjalon To: Adrien Mazarguil Return-path: Received: from NAM03-DM3-obe.outbound.protection.outlook.com (mail-dm3nam03on0051.outbound.protection.outlook.com [104.47.41.51]) by dpdk.org (Postfix) with ESMTP id 292EC2C8 for ; Mon, 19 Jun 2017 12:52:08 +0200 (CEST) In-Reply-To: <20170616103402.GB1758@6wind.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >>> Cc: dev@dpdk.org; ferruh.yigit@intel.com; Hemant Agrawal >>> >>> 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 >>>> >>>> Bit Swap and LE<=>BE conversions for 23, 40 and 48 bit width >>>> >>>> Signed-off-by: Hemant Agrawal >>>> --- >>>> .../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