From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wiles, Keith" Subject: Re: [PATCH 01/38] eal: add support for 24 40 and 48 bit operations Date: Tue, 20 Jun 2017 14:34:24 +0000 Message-ID: <063C1898-C5C7-4F5D-AC27-0792D51D729A@intel.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> <21801a3d-2512-3664-f40a-6510b5e6e8b4@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: Shreyansh Jain , Adrien Mazarguil , "Richardson, Bruce" , "dev@dpdk.org" , "Yigit, Ferruh" , Thomas Monjalon To: Hemant Agrawal Return-path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id 493A04BE1 for ; Tue, 20 Jun 2017 16:34:26 +0200 (CEST) In-Reply-To: <21801a3d-2512-3664-f40a-6510b5e6e8b4@nxp.com> Content-Language: en-US Content-ID: 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 Jun 20, 2017, at 5:43 AM, Hemant Agrawal wrot= e: >=20 > On 6/19/2017 7:22 PM, Wiles, Keith wrote: >>=20 >>> On Jun 19, 2017, at 6:00 AM, Shreyansh Jain wr= ote: >>>=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= 48 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 = 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 i= s >>>>> useful for others. >>>>>=20 >>>>> From DPAA perspective, these macro can be anywhere. In case someone e= lse too >>>>> has use of this (now or in near-future), probably then we can conside= r 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 ar= e 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 appende= d to >>>> the converted value? How about input values in non-native order? Answe= ring >>>> that is not so easy as it depends on the use case. We actually had a s= imilar >>>> 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 a= re >>>> correct, filler bytes are not in the right place for a big-endian syst= em, >>>> 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 wa= s to >>>> be used as is on a little-endian system. >>>=20 >>> I understand what you have stated - the application or any user needs t= o be context aware about what they are using and the side-effect of such co= nversions. >>>=20 >>>> I think the only safe way to deal with that is by defining specific ty= pes of >>>> the proper size, e.g.: >>>> typedef uint8_t uint48_t[6]; >>>> These are cumbersome and cannot be used like normal integers though. W= ith >>>> 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 sens= e. >>>=20 >>> I have no issues moving these into DPAA specific code. Hemant added the= m in generic just in case they would be of use to others. >>>=20 >>> - >>> Shreyansh >>=20 >> These are all static inline functions, so no real code increase unless u= sed and having them in one spot is the best place IMO. >>=20 >>=20 >> Regards, >> Keith >=20 > Yes! these are simple static inline functions with no core unless used. > Many hardware accelerators usages 40 bit & 48 bits data. we thought, it c= an be usable by others as well. >=20 > currently we are seeing a mixed opinion. >=20 > In next revision, We will move them within our driver code. If someone ne= ed them in future, we can always bring them to eal. Is there really a big objection to allowing this patch to be accepted? >=20 > Regards, > Hemant >=20 >>=20 >>=20 >=20 >=20 Regards, Keith