From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hemant Agrawal Subject: Re: [PATCH 01/38] eal: add support for 24 40 and 48 bit operations Date: Tue, 20 Jun 2017 16:13:53 +0530 Message-ID: <21801a3d-2512-3664-f40a-6510b5e6e8b4@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> <142b5789-3234-0036-4965-cf4fc78d942d@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: Adrien Mazarguil , "Richardson, Bruce" , "dev@dpdk.org" , "Yigit, Ferruh" , Thomas Monjalon To: "Wiles, Keith" , Shreyansh Jain Return-path: Received: from NAM02-CY1-obe.outbound.protection.outlook.com (mail-cys01nam02on0045.outbound.protection.outlook.com [104.47.37.45]) by dpdk.org (Postfix) with ESMTP id BED002C72 for ; Tue, 20 Jun 2017 12:44:03 +0200 (CEST) In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 6/19/2017 7:22 PM, Wiles, Keith wrote: > >> On Jun 19, 2017, at 6:00 AM, Shreyansh Jain 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 >>>>> 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 > > 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 > >