From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f171.google.com (mail-wr0-f171.google.com [209.85.128.171]) by dpdk.org (Postfix) with ESMTP id 80D722BA1 for ; Thu, 8 Jun 2017 11:14:23 +0200 (CEST) Received: by mail-wr0-f171.google.com with SMTP id v111so15730355wrc.3 for ; Thu, 08 Jun 2017 02:14:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=BaUG6HlkbajMZ13uoPhiotpmAhYZxs2jcNbE+kuUJmc=; b=PnIQX8BQ/FwRJ7MHhw9lbqT0ClcaGoiP6O7E7z3Ds665/KrnX5fUfGpTw7AvxxQraa hHFRyIDESBq/wVUoXCgyJk6S4Z+nXs1VMW8jdy5gym7358+X3aT2BZHiXX9rHobPipUm kQoRckwI88WZKTEejgopSwu8oP81nSPsXlRPtDO8bUCHwaTyqzcKSnjS+07vkCcF1Xac K4H+LryIvsRFPuoqbgidcJLmerz1S0IDGJabuh+AWqEOFc+iPyBQ7THFuF0GLQTIHiXA gH0opGN1CpJPenvYEZ/RK9PA7nGJzrhwc2WAfn3ZZN/TjF0wXBsYNT31Wv6psF+XQUlt IhuQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=BaUG6HlkbajMZ13uoPhiotpmAhYZxs2jcNbE+kuUJmc=; b=YhcnX1pAyYCIu+hXCgMTsi2briJYv4IbrtbmzZH696ogyhkkuXI0OhUeRz1cFA/TcB X0x4M+XlGhC5p9lzVxcYI5XUci8gObKWrZaZeMzobzdNbxPTuc/4/s7AfrWSt3Xpq0A8 279iQ1rsocUfw0SHElH3c6jdhfYl6vrCYl4gca4bkbWQpb4nHSrktYhN6Nx2LoSD2AA4 kEGtaVu8pm5+0Tq0Vwt+5m8b50+0eeko12/yq/F2HR3saN8wlQwZGK+2b27Jy29iubpB mLKe9UXDStpavNa3U2+UH2zm7pu9DE621LN7ObrlGleULoR2paUJ5ItDdEbmqRCIY4j0 uBTw== X-Gm-Message-State: AODbwcBHPj2RBfkONxVhoT4X8lFbEMhvCu+i0nsLZQapg50cqjuIzPyB r/gVxClXdyMWEPCt5lY= X-Received: by 10.223.160.68 with SMTP id l4mr27503195wrl.130.1496913262987; Thu, 08 Jun 2017 02:14:22 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 143sm5893106wmj.6.2017.06.08.02.14.22 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 08 Jun 2017 02:14:22 -0700 (PDT) Date: Thu, 8 Jun 2017 11:14:16 +0200 From: Adrien Mazarguil To: Thomas Monjalon Cc: dev@dpdk.org Message-ID: <20170608091416.GU1758@6wind.com> References: <840342851720fc237214aeb30d38565615293b58.1495101988.git.adrien.mazarguil@6wind.com> <44029570.D8ug5AmCbY@xps> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <44029570.D8ug5AmCbY@xps> Subject: Re: [dpdk-dev] [PATCH 1/2] eal: add static endianness conversion macros 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: Thu, 08 Jun 2017 09:14:23 -0000 On Wed, Jun 07, 2017 at 04:16:58PM +0200, Thomas Monjalon wrote: > Hi, some comments below: > > 18/05/2017 12:14, Adrien Mazarguil: > > These macros resolve to constant expressions that allow developers to > > perform endianness conversion on static/const objects, even outside of > > function scope as they do not translate to function calls. > > > > This is most useful for static initializers and constant values (whenever > > it has to be performed at compilation time). Run-time endianness conversion > > of variable values should keep using rte_*_to_*() calls for best > > performance. > > > > Signed-off-by: Adrien Mazarguil > [...] > > +#define RTE_STATIC_BSWAP64(v) \ > > + ((((uint64_t)(v) & UINT64_C(0x00000000000000ff)) << 56) | \ > > + (((uint64_t)(v) & UINT64_C(0x000000000000ff00)) << 40) | \ > > + (((uint64_t)(v) & UINT64_C(0x0000000000ff0000)) << 24) | \ > > + (((uint64_t)(v) & UINT64_C(0x00000000ff000000)) << 8) | \ > > + (((uint64_t)(v) & UINT64_C(0x000000ff00000000)) >> 8) | \ > > + (((uint64_t)(v) & UINT64_C(0x0000ff0000000000)) >> 24) | \ > > + (((uint64_t)(v) & UINT64_C(0x00ff000000000000)) >> 40) | \ > > + (((uint64_t)(v) & UINT64_C(0xff00000000000000)) >> 56)) > > Minor nit: you could align lines by inserting a space before 8. I think alignment attempts past the mandatory line indentation often end up in a failure (e.g. when grouping macros by name, one of them inevitably happens to be longer than initially envisioned, same for structure fields and trailing comment blocks, etc.) Since I'm not convinced it improves readability, I tend to avoid them altogether for consistency. It's a matter of style but I can change that if you prefer. > > +#if RTE_BYTE_ORDER == RTE_BIG_ENDIAN > > +#define RTE_BE16(v) (uint16_t)(v) > > +#define RTE_BE32(v) (uint32_t)(v) > > +#define RTE_BE64(v) (uint64_t)(v) > > +#define RTE_LE16(v) RTE_STATIC_BSWAP16(v) > > +#define RTE_LE32(v) RTE_STATIC_BSWAP32(v) > > +#define RTE_LE64(v) RTE_STATIC_BSWAP64(v) > > +#elif RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN > > +#define RTE_BE16(v) RTE_STATIC_BSWAP16(v) > > +#define RTE_BE32(v) RTE_STATIC_BSWAP32(v) > > +#define RTE_BE64(v) RTE_STATIC_BSWAP64(v) > > +#define RTE_LE16(v) (uint16_t)(v) > > +#define RTE_LE32(v) (uint32_t)(v) > > +#define RTE_LE64(v) (uint64_t)(v) > > This naming is confusing. > Let's take RTE_BE16() as example, it does not say wether the input value > is big endian or the output value will be big endian. > I think we should mimic the wording of run-time conversions: > RTE_BE_TO_CPU_16() > > Any other ideas? First I'd like to keep those macro names as short as possible, ideally not much larger than simply casting the provided value to the target type for usability and readability purposes. Think about files full of static initializers, while there are not many examples right now, the definition of static rte_flow rules and capability trees will need to use these macros extensively. The fact you suggested RTE_BE_TO_CPU_16() instead of RTE_CPU_TO_BE_16() as a replacement for RTE_BE16() highlights the misunderstanding. However I find "CPU_TO" overly verbose, particularly since the reverse macros won't exist, remember these are made for static conversions of integer constants resolved at compilation time, not variables. Users may additionally confuse RTE_CPU_TO_BE_16() with its similarly-named inline function counterpart. Functions and macros are typically named after their output, not their input. In that sense and without further precision, RTE_BE16() is fine in my opinion. Remember this [1]? I think we could make everything clearer by perhaps applying it and casting the results of these macros to the proper type, e.g.: #define RTE_BE16(v) (rte_be16_t)(v) I can probably modify this series to introduce the new types first, use them in the conversion macro and then later clarify existing structure fields. How about this? [1] http://dpdk.org/ml/archives/dev/2016-November/050060.html -- Adrien Mazarguil 6WIND