From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 2CC5D2C8 for ; Mon, 19 Jun 2017 15:52:11 +0200 (CEST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Jun 2017 06:52:10 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,361,1493708400"; d="scan'208";a="116668029" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga006.fm.intel.com with ESMTP; 19 Jun 2017 06:52:10 -0700 Received: from fmsmsx113.amr.corp.intel.com ([169.254.13.162]) by FMSMSX108.amr.corp.intel.com ([169.254.9.133]) with mapi id 14.03.0319.002; Mon, 19 Jun 2017 06:52:09 -0700 From: "Wiles, Keith" To: Shreyansh Jain CC: Adrien Mazarguil , "Richardson, Bruce" , "dev@dpdk.org" , "Yigit, Ferruh" , Hemant Agrawal , "Thomas Monjalon" Thread-Topic: [dpdk-dev] [PATCH 01/38] eal: add support for 24 40 and 48 bit operations Thread-Index: AQHS6QM+9h6e+P2AiE2diAHo5Dt0Ng== Date: Mon, 19 Jun 2017 13:52:08 +0000 Message-ID: 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> In-Reply-To: <142b5789-3234-0036-4965-cf4fc78d942d@nxp.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.255.77.189] Content-Type: text/plain; charset="us-ascii" Content-ID: <5F72B3417417924F86B63AA1DBE56C09@intel.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 01/38] eal: add support for 24 40 and 48 bit operations X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Jun 2017 13:52:12 -0000 > On Jun 19, 2017, at 6:00 AM, Shreyansh Jain wrot= e: >=20 > Hello Adrien, >=20 > 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, >>>=20 >>>> -----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 4= 8 bit >>>> operations >>>>=20 >>>> On Fri, Jun 16, 2017 at 11:10:31AM +0530, Shreyansh Jain wrote: >>>>> From: Hemant Agrawal >>>>>=20 >>>>> Bit Swap and LE<=3D>BE conversions for 23, 40 and 48 bit width >>>>>=20 >>>>> Signed-off-by: Hemant Agrawal >>>>> --- >>>>> .../common/include/generic/rte_byteorder.h | 78 >>>> ++++++++++++++++++++++ >>>>> 1 file changed, 78 insertions(+) >>>>>=20 >>>> 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 wi= th >>>> 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. >>>=20 >>> From DPAA perspective, these macro can be anywhere. In case someone els= e 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 fi= les >> on that basis. >> Problem is their storage size is larger than the number of bytes they de= al >> with, which raises the question: are filler bytes prepended or appended = to >> the converted value? How about input values in non-native order? Answeri= ng >> that is not so easy as it depends on the use case. We actually had a sim= ilar >> 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. >=20 > 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 conv= ersions. >=20 >> I think the only safe way to deal with that is by defining specific type= s of >> the proper size, e.g.: >> typedef uint8_t uint48_t[6]; >> These are cumbersome and cannot be used like normal integers though. Wit= h >> 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. >=20 > 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. >=20 > - > 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