[dpdk-dev] [PATCH 01/38] eal: add support for 24 40 and 48 bit operations

Bruce Richardson bruce.richardson at intel.com
Wed Jun 21 10:32:51 CEST 2017


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
> > > <hemant.agrawal at nxp.com> wrote:
> > > 
> > > On 6/19/2017 7:22 PM, Wiles, Keith wrote:
> > > > 
> > > > > On Jun 19, 2017, at 6:00 AM, Shreyansh Jain
> > > > > <shreyansh.jain at nxp.com> 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 at intel.com] Sent: Friday, June
> > > > > > > > 16, 2017 2:27 PM To: Shreyansh Jain
> > > > > > > > <shreyansh.jain at nxp.com> Cc: dev at dpdk.org;
> > > > > > > > ferruh.yigit at intel.com; Hemant Agrawal
> > > > > > > > <hemant.agrawal at nxp.com> 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 <hemant.agrawal at nxp.com>
> > > > > > > > > 
> > > > > > > > > Bit Swap and LE<=>BE conversions for 23, 40 and 48 bit
> > > > > > > > > width
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Hemant Agrawal <hemant.agrawal at nxp.com>
> > > > > > > > > --- .../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


More information about the dev mailing list