From mboxrd@z Thu Jan 1 00:00:00 1970 From: Adrien Mazarguil Subject: Re: [PATCH 01/38] eal: add support for 24 40 and 48 bit operations Date: Fri, 16 Jun 2017 12:34:02 +0200 Message-ID: <20170616103402.GB1758@6wind.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> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Bruce Richardson , "dev@dpdk.org" , "ferruh.yigit@intel.com" , Hemant Agrawal , Thomas Monjalon To: Shreyansh Jain Return-path: Received: from mail-wr0-f169.google.com (mail-wr0-f169.google.com [209.85.128.169]) by dpdk.org (Postfix) with ESMTP id 4D32C2BB9 for ; Fri, 16 Jun 2017 12:34:11 +0200 (CEST) Received: by mail-wr0-f169.google.com with SMTP id 36so35934524wry.3 for ; Fri, 16 Jun 2017 03:34:11 -0700 (PDT) Content-Disposition: inline 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" 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 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. -- Adrien Mazarguil 6WIND