From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 958AA5320 for ; Wed, 21 Jun 2017 10:33:14 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga105.fm.intel.com with ESMTP; 21 Jun 2017 01:33:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,368,1493708400"; d="scan'208";a="870196732" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.28]) by FMSMGA003.fm.intel.com with SMTP; 21 Jun 2017 01:32:53 -0700 Received: by (sSMTP sendmail emulation); Wed, 21 Jun 2017 09:32:52 +0100 Date: Wed, 21 Jun 2017 09:32:51 +0100 From: Bruce Richardson To: Hemant Agrawal Cc: "Wiles, Keith" , Shreyansh Jain , Adrien Mazarguil , "dev@dpdk.org" , "Yigit, Ferruh" , Thomas Monjalon Message-ID: <20170621083251.GA91808@bricha3-MOBL3.ger.corp.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> <063C1898-C5C7-4F5D-AC27-0792D51D729A@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.8.1 (2017-04-11) 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: Wed, 21 Jun 2017 08:33:15 -0000 On Wed, Jun 21, 2017 at 01:47:52PM +0530, Hemant Agrawal wrote: > On 6/20/2017 8:04 PM, Wiles, Keith wrote: > > > > > On Jun 20, 2017, at 5:43 AM, Hemant Agrawal > > > wrote: > > > > > > 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. > > > > Is there really a big objection to allowing this patch to be > > accepted? > > Bruce, Adrien Any opinion? > I don't have strong feelings either way. However, if there is only one user of these functions right now, I'd normally wait till there is a second user before moving them to a common area. If others feel differently, I'm ok with having them as common right now. /Bruce