Re: [dpdk-dev] [RFC 4/4] net/ether: use bitops to speedup comparison

[Bruce Richardson]

On Thu, May 16, 2019 at 05:07:45PM +0100, Bruce Richardson wrote:
> On Thu, May 16, 2019 at 09:06:52AM -0700, Stephen Hemminger wrote:
> > On Thu, 16 May 2019 17:03:37 +0100
> > Bruce Richardson <bruce.richard...@intel.com> wrote:
> > 
> > > On Wed, May 15, 2019 at 03:19:52PM -0700, Stephen Hemminger wrote:
> > > > Using bit operations like or and xor is faster than a loop
> > > > on all architectures. Really just explicit unrolling.
> > > > 
> > > > Similar cast to uint16 unaligned is already done in
> > > > other functions here.
> > > > 
> > > > Signed-off-by: Stephen Hemminger <step...@networkplumber.org>
> > > > ---
> > > >  lib/librte_net/rte_ether.h | 17 +++++++----------
> > > >  1 file changed, 7 insertions(+), 10 deletions(-)
> > > >   
> > > Rather than casting to unaligned values, which gives compiler warnings in
> > > some cases, I believe we should just mark the ethernet addresses as always
> > > being 2-byte aligned and simplify things. [unless we have a good use case
> > > where we won't have 2-byte alignment???].
> > > 
> > > See patch: http://patches.dpdk.org/patch/53482/
> > > 
> > > Regards,
> > > /Bruce
> > 
> > I agree. Then you could also remove the unaligned_uint16_t that
> > already exists in rte_ether.h
> > 
> > Do you want me to put your patch in my series?
> 
> Sure, feel free.
> 

I've just found another problem in this area. Compiling l2fwd with gcc 9, I
get:

main.c:164:54: warning: taking address of packed member of ‘struct ether_hdr’ 
may result in an unaligned pointer value [-Waddress-of-packed-member]
  164 |  ether_addr_copy(&l2fwd_ports_eth_addr[dest_portid], &eth->s_addr);
      | 

Looking at some of the structures, it appears that not all the packet
attributes may be necessary. For example, while AFAIK there are not
absolute guarantees about structure padding, for all compilers I remember
using the following changes are safe:

diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index 8090b7c01..7d9f34791 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -57,7 +57,7 @@ extern "C" {
 struct ether_addr {
        /** Addr bytes in tx order */
        uint8_t addr_bytes[ETHER_ADDR_LEN] __rte_aligned(2);
-} __attribute__((__packed__));
+};

 #define ETHER_LOCAL_ADMIN_ADDR 0x02 /**< Locally assigned Eth. address. */
 #define ETHER_GROUP_ADDR       0x01 /**< Multicast or broadcast Eth. address. 
*/
@@ -273,7 +273,7 @@ struct ether_hdr {
        struct ether_addr d_addr; /**< Destination address. */
        struct ether_addr s_addr; /**< Source address. */
        uint16_t ether_type;      /**< Frame type. */
-} __attribute__((__packed__));
+};

 /**
  * Ethernet VLAN Header.
@@ -283,7 +283,7 @@ struct ether_hdr {
 struct vlan_hdr {
        uint16_t vlan_tci; /**< Priority (3) + CFI (1) + Identifier Code (12) */
        uint16_t eth_proto;/**< Ethernet type of encapsulated frame. */
-} __attribute__((__packed__));
+};

 /**
  * VXLAN protocol header.

I think we therefore should consider removing those packed attributes to
avoid application warnings.

/Bruce