userspace: Define and use struct eth_addr.

Define struct eth_addr and use it instead of a uint8_t array for all ethernet addresses in OVS userspace. The struct is always the right size, and it can be assigned without an explicit memcpy, which makes code more readable. "struct eth_addr" is a good type name for this as many utility functions are already named accordingly. struct eth_addr can be accessed as bytes as well as ovs_be16's, which makes the struct 16-bit aligned. All use seems to be 16-bit aligned, so some algorithms on the ethernet addresses can be made a bit more efficient making use of this fact. As the struct fits into a register (in 64-bit systems) we pass it by value when possible. This patch also changes the few uses of Linux specific ETH_ALEN to OVS's own ETH_ADDR_LEN, and removes the OFP_ETH_ALEN, as it is no longer needed. This work stemmed from a desire to make all struct flow members assignable for unrelated exploration purposes. However, I think this might be a nice code readability improvement by itself. Signed-off-by: Jarno Rajahalme <[email protected]>
kevinmel2000 · Aug 28, 2015 · 74ff329 · 74ff329
1 parent d8ef07e
commit 74ff329
Show file tree

Hide file tree

Showing 79 changed files with 697 additions and 765 deletions.
diff --git a/build-aux/check-structs b/build-aux/check-structs
@@ -15,6 +15,7 @@ types['ovs_be16'] = {"size": 2, "alignment": 2}
 types['ovs_be32'] = {"size": 4, "alignment": 4}
 types['ovs_be64'] = {"size": 8, "alignment": 8}
 types['ovs_32aligned_be64'] = {"size": 8, "alignment": 4}
+types['struct eth_addr'] = {"size": 6, "alignment": 1}
 
 token = None
 line = ""

diff --git a/build-aux/extract-odp-netlink-h b/build-aux/extract-odp-netlink-h
@@ -20,11 +20,14 @@ $i\
 #include "OvsDpInterfaceExt.h"\
 #endif\
 
+# Use OVS's own struct eth_addr instead of a 6-byte char array.
+s,<linux/types\.h>,"openvswitch/types.h",
+s,#.*<linux/if_ether\.h>,,
+s/__u8[ \t]*\([a-zA-Z0-9_]*\)[ \t]*\[[ \t]*ETH_ALEN[ \t]*\]/struct eth_addr \1/
 
 # Transform most Linux-specific __u<N> types into C99 uint<N>_t types,
 # and most Linux-specific __be<N> into Open vSwitch ovs_be<N>,
 # and use the appropriate userspace header.
-s,<linux/types\.h>,"openvswitch/types.h",
 s/__u32/uint32_t/g
 s/__u16/uint16_t/g
 s/__u8/uint8_t/g
@@ -36,7 +39,3 @@ s/__be16/ovs_be16/g
 # boundary.
 s/__u64/ovs_32aligned_u64/g
 s/__be64/ovs_32aligned_be64/g
-
-# Use OVS's own ETH_ADDR_LEN instead of Linux-specific ETH_ALEN.
-s,<linux/if_ether\.h>,"packets.h",
-s/ETH_ALEN/ETH_ADDR_LEN/
diff --git a/include/openflow/openflow-1.0.h b/include/openflow/openflow-1.0.h
@@ -96,7 +96,7 @@ enum ofp10_port_features {
 /* Description of a physical port */
 struct ofp10_phy_port {
     ovs_be16 port_no;
-    uint8_t hw_addr[OFP_ETH_ALEN];
+    struct eth_addr hw_addr;
     char name[OFP_MAX_PORT_NAME_LEN]; /* Null-terminated */
 
     ovs_be32 config;        /* Bitmap of OFPPC_* and OFPPC10_* flags. */
@@ -114,11 +114,10 @@ OFP_ASSERT(sizeof(struct ofp10_phy_port) == 48);
 /* Modify behavior of the physical port */
 struct ofp10_port_mod {
     ovs_be16 port_no;
-    uint8_t hw_addr[OFP_ETH_ALEN]; /* The hardware address is not
-                                      configurable.  This is used to
-                                      sanity-check the request, so it must
-                                      be the same as returned in an
-                                      ofp10_phy_port struct. */
+    struct eth_addr hw_addr; /* The hardware address is not configurable.  This
+                                is used to sanity-check the request, so it must
+                                be the same as returned in an ofp10_phy_port
+                                struct. */
 
     ovs_be32 config;        /* Bitmap of OFPPC_* flags. */
     ovs_be32 mask;          /* Bitmap of OFPPC_* flags to be changed. */
@@ -234,8 +233,8 @@ enum ofp10_flow_wildcards {
 struct ofp10_match {
     ovs_be32 wildcards;        /* Wildcard fields. */
     ovs_be16 in_port;          /* Input switch port. */
-    uint8_t dl_src[OFP_ETH_ALEN]; /* Ethernet source address. */
-    uint8_t dl_dst[OFP_ETH_ALEN]; /* Ethernet destination address. */
+    struct eth_addr dl_src;    /* Ethernet source address. */
+    struct eth_addr dl_dst;    /* Ethernet destination address. */
     ovs_be16 dl_vlan;          /* Input VLAN. */
     uint8_t dl_vlan_pcp;       /* Input VLAN priority. */
     uint8_t pad1[1];           /* Align to 64-bits. */

diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h
@@ -110,7 +110,7 @@ enum ofp11_port_features {
 struct ofp11_port {
     ovs_be32 port_no;
     uint8_t pad[4];
-    uint8_t hw_addr[OFP_ETH_ALEN];
+    struct eth_addr hw_addr;
     uint8_t pad2[2];                  /* Align to 64 bits. */
     char name[OFP_MAX_PORT_NAME_LEN]; /* Null-terminated */
 
@@ -133,11 +133,10 @@ OFP_ASSERT(sizeof(struct ofp11_port) == 64);
 struct ofp11_port_mod {
     ovs_be32 port_no;
     uint8_t pad[4];
-    uint8_t hw_addr[OFP_ETH_ALEN]; /* The hardware address is not
-                                      configurable.  This is used to
-                                      sanity-check the request, so it must
-                                      be the same as returned in an
-                                      ofp11_port struct. */
+    struct eth_addr hw_addr; /* The hardware address is not configurable.  This
+                                is used to sanity-check the request, so it must
+                                be the same as returned in an ofp11_port
+                                struct. */
     uint8_t pad2[2];        /* Pad to 64 bits. */
     ovs_be32 config;        /* Bitmap of OFPPC_* flags. */
     ovs_be32 mask;          /* Bitmap of OFPPC_* flags to be changed. */
@@ -194,10 +193,10 @@ struct ofp11_match {
     struct ofp11_match_header omh;
     ovs_be32 in_port;          /* Input switch port. */
     ovs_be32 wildcards;        /* Wildcard fields. */
-    uint8_t dl_src[OFP_ETH_ALEN]; /* Ethernet source address. */
-    uint8_t dl_src_mask[OFP_ETH_ALEN]; /* Ethernet source address mask.  */
-    uint8_t dl_dst[OFP_ETH_ALEN]; /* Ethernet destination address. */
-    uint8_t dl_dst_mask[OFP_ETH_ALEN]; /* Ethernet destination address mask. */
+    struct eth_addr dl_src;    /* Ethernet source address. */
+    struct eth_addr dl_src_mask; /* Ethernet source address mask.  */
+    struct eth_addr dl_dst;    /* Ethernet destination address. */
+    struct eth_addr dl_dst_mask; /* Ethernet destination address mask. */
     ovs_be16 dl_vlan;          /* Input VLAN id. */
     uint8_t dl_vlan_pcp;       /* Input VLAN priority. */
     uint8_t pad1[1];           /* Align to 32-bits */

diff --git a/include/openflow/openflow-1.4.h b/include/openflow/openflow-1.4.h
@@ -72,7 +72,7 @@ struct ofp14_port {
     ovs_be32 port_no;
     ovs_be16 length;
     uint8_t pad[2];
-    uint8_t hw_addr[OFP_ETH_ALEN];
+    struct eth_addr hw_addr;
     uint8_t pad2[2];                  /* Align to 64 bits. */
     char name[OFP_MAX_PORT_NAME_LEN]; /* Null-terminated */
 
@@ -106,7 +106,7 @@ OFP_ASSERT(sizeof(struct ofp14_port_mod_prop_ethernet) == 8);
 struct ofp14_port_mod {
     ovs_be32 port_no;
     uint8_t pad[4];
-    uint8_t hw_addr[OFP_ETH_ALEN];
+    struct eth_addr hw_addr;
     uint8_t pad2[2];
     ovs_be32 config;        /* Bitmap of OFPPC_* flags. */
     ovs_be32 mask;          /* Bitmap of OFPPC_* flags to be changed. */

diff --git a/include/openflow/openflow-common.h b/include/openflow/openflow-common.h
@@ -113,8 +113,6 @@ enum ofp_version {
 #define OFP_OLD_PORT  6633
 #define OFP_PORT  6653
 
-#define OFP_ETH_ALEN 6          /* Bytes in an Ethernet address. */
-
 #define OFP_DEFAULT_MISS_SEND_LEN   128
 
 /* Values below this cutoff are 802.3 packets and the two bytes

diff --git a/include/openvswitch/types.h b/include/openvswitch/types.h
@@ -106,4 +106,14 @@ typedef uint32_t OVS_BITWISE ofp11_port_t;
 #define ODP_PORT_C(X) ((OVS_FORCE odp_port_t) (X))
 #define OFP11_PORT_C(X) ((OVS_FORCE ofp11_port_t) (X))
 
+/* Using this stuct instead of a bare array makes an ethernet address field
+ * assignable.  The size of the array is also part of the type, so it is easier
+ * to deal with. */
+struct eth_addr {
+    union {
+        uint8_t ea[6];
+        ovs_be16 be16[3];
+    };
+};
+
 #endif /* openvswitch/types.h */
diff --git a/lib/bfd.c b/lib/bfd.c
@@ -170,10 +170,10 @@ struct bfd {
 
     uint32_t rmt_disc;            /* bfd.RemoteDiscr. */
 
-    uint8_t local_eth_src[ETH_ADDR_LEN]; /* Local eth src address. */
-    uint8_t local_eth_dst[ETH_ADDR_LEN]; /* Local eth dst address. */
+    struct eth_addr local_eth_src; /* Local eth src address. */
+    struct eth_addr local_eth_dst; /* Local eth dst address. */
 
-    uint8_t rmt_eth_dst[ETH_ADDR_LEN];   /* Remote eth dst address. */
+    struct eth_addr rmt_eth_dst;   /* Remote eth dst address. */
 
     ovs_be32 ip_src;              /* IPv4 source address. */
     ovs_be32 ip_dst;              /* IPv4 destination address. */
@@ -357,7 +357,7 @@ bfd_configure(struct bfd *bfd, const char *name, const struct smap *cfg,
     bool cpath_down, forwarding_if_rx;
     const char *hwaddr, *ip_src, *ip_dst;
     struct in_addr in_addr;
-    uint8_t ea[ETH_ADDR_LEN];
+    struct eth_addr ea;
 
     if (!cfg || !smap_get_bool(cfg, "enable", false)) {
         bfd_unref(bfd);
@@ -441,24 +441,24 @@ bfd_configure(struct bfd *bfd, const char *name, const struct smap *cfg,
     }
 
     hwaddr = smap_get(cfg, "bfd_local_src_mac");
-    if (hwaddr && eth_addr_from_string(hwaddr, ea)) {
-        memcpy(bfd->local_eth_src, ea, ETH_ADDR_LEN);
+    if (hwaddr && eth_addr_from_string(hwaddr, &ea)) {
+        bfd->local_eth_src = ea;
     } else {
-        memset(bfd->local_eth_src, 0, ETH_ADDR_LEN);
+        bfd->local_eth_src = eth_addr_zero;
     }
 
     hwaddr = smap_get(cfg, "bfd_local_dst_mac");
-    if (hwaddr && eth_addr_from_string(hwaddr, ea)) {
-        memcpy(bfd->local_eth_dst, ea, ETH_ADDR_LEN);
+    if (hwaddr && eth_addr_from_string(hwaddr, &ea)) {
+        bfd->local_eth_dst = ea;
     } else {
-        memset(bfd->local_eth_dst, 0, ETH_ADDR_LEN);
+        bfd->local_eth_dst = eth_addr_zero;
     }
 
     hwaddr = smap_get(cfg, "bfd_remote_dst_mac");
-    if (hwaddr && eth_addr_from_string(hwaddr, ea)) {
-        memcpy(bfd->rmt_eth_dst, ea, ETH_ADDR_LEN);
+    if (hwaddr && eth_addr_from_string(hwaddr, &ea)) {
+        bfd->rmt_eth_dst = ea;
     } else {
-        memset(bfd->rmt_eth_dst, 0, ETH_ADDR_LEN);
+        bfd->rmt_eth_dst = eth_addr_zero;
     }
 
     ip_src = smap_get(cfg, "bfd_src_ip");
@@ -589,7 +589,7 @@ bfd_should_send_packet(const struct bfd *bfd) OVS_EXCLUDED(mutex)
 
 void
 bfd_put_packet(struct bfd *bfd, struct dp_packet *p,
-               uint8_t eth_src[ETH_ADDR_LEN]) OVS_EXCLUDED(mutex)
+               const struct eth_addr eth_src) OVS_EXCLUDED(mutex)
 {
     long long int min_tx, min_rx;
     struct udp_header *udp;
@@ -614,14 +614,10 @@ bfd_put_packet(struct bfd *bfd, struct dp_packet *p,
 
     dp_packet_reserve(p, 2); /* Properly align after the ethernet header. */
     eth = dp_packet_put_uninit(p, sizeof *eth);
-    memcpy(eth->eth_src,
-           eth_addr_is_zero(bfd->local_eth_src) ? eth_src
-                                                : bfd->local_eth_src,
-           ETH_ADDR_LEN);
-    memcpy(eth->eth_dst,
-           eth_addr_is_zero(bfd->local_eth_dst) ? eth_addr_bfd
-                                                : bfd->local_eth_dst,
-           ETH_ADDR_LEN);
+    eth->eth_src = eth_addr_is_zero(bfd->local_eth_src)
+        ? eth_src : bfd->local_eth_src;
+    eth->eth_dst = eth_addr_is_zero(bfd->local_eth_dst)
+        ? eth_addr_bfd : bfd->local_eth_dst;
     eth->eth_type = htons(ETH_TYPE_IP);
 
     ip = dp_packet_put_zeros(p, sizeof *ip);
@@ -678,7 +674,7 @@ bfd_should_process_flow(const struct bfd *bfd_, const struct flow *flow,
     if (!eth_addr_is_zero(bfd->rmt_eth_dst)) {
         memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
 
-        if (memcmp(bfd->rmt_eth_dst, flow->dl_dst, ETH_ADDR_LEN)) {
+        if (!eth_addr_equals(bfd->rmt_eth_dst, flow->dl_dst)) {
             return false;
         }
     }

diff --git a/lib/bfd.h b/lib/bfd.h
@@ -36,7 +36,7 @@ void bfd_run(struct bfd *);
 
 bool bfd_should_send_packet(const struct bfd *);
 void bfd_put_packet(struct bfd *bfd, struct dp_packet *packet,
-                    uint8_t eth_src[ETH_ADDR_LEN]);
+                    const struct eth_addr eth_src);
 
 bool bfd_should_process_flow(const struct bfd *, const struct flow *,
                              struct flow_wildcards *);

diff --git a/lib/cfm.c b/lib/cfm.c
@@ -44,11 +44,10 @@ VLOG_DEFINE_THIS_MODULE(cfm);
 #define CFM_MAX_RMPS 256
 
 /* Ethernet destination address of CCM packets. */
-static const uint8_t eth_addr_ccm[ETH_ADDR_LEN] = {
-    0x01, 0x80, 0xC2, 0x00, 0x00, 0x30 };
-static const uint8_t eth_addr_ccm_x[ETH_ADDR_LEN] = {
-    0x01, 0x23, 0x20, 0x00, 0x00, 0x30
-};
+static const struct eth_addr eth_addr_ccm = {
+    { { 0x01, 0x80, 0xC2, 0x00, 0x00, 0x30 } } };
+static const struct eth_addr eth_addr_ccm_x = {
+    { { 0x01, 0x23, 0x20, 0x00, 0x00, 0x30 } } };
 
 #define ETH_TYPE_CFM 0x8902
 
@@ -185,7 +184,7 @@ cfm_rx_packets(const struct cfm *cfm) OVS_REQUIRES(mutex)
     }
 }
 
-static const uint8_t *
+static struct eth_addr
 cfm_ccm_addr(struct cfm *cfm)
 {
     bool extended;
@@ -565,7 +564,7 @@ cfm_should_send_ccm(struct cfm *cfm) OVS_EXCLUDED(mutex)
  * should be sent whenever cfm_should_send_ccm() indicates. */
 void
 cfm_compose_ccm(struct cfm *cfm, struct dp_packet *packet,
-                const uint8_t eth_src[ETH_ADDR_LEN]) OVS_EXCLUDED(mutex)
+                const struct eth_addr eth_src) OVS_EXCLUDED(mutex)
 {
     uint16_t ccm_vlan;
     struct ccm *ccm;

diff --git a/lib/cfm.h b/lib/cfm.h
@@ -94,7 +94,7 @@ void cfm_unref(struct cfm *);
 void cfm_run(struct cfm *);
 bool cfm_should_send_ccm(struct cfm *);
 void cfm_compose_ccm(struct cfm *, struct dp_packet *,
-                     const uint8_t eth_src[ETH_ADDR_LEN]);
+                     const struct eth_addr eth_src);
 long long int cfm_wait(struct cfm *);
 bool cfm_configure(struct cfm *, const struct cfm_settings *);
 void cfm_set_netdev(struct cfm *, const struct netdev *);

diff --git a/lib/csum.c b/lib/csum.c
@@ -114,19 +114,15 @@ recalc_csum32(ovs_be16 old_csum, ovs_be32 old_u32, ovs_be32 new_u32)
 
 /* Returns the new checksum for a packet in which the checksum field previously
  * contained 'old_csum' and in which a field that contained the 6 bytes at
- * 'old_bytes' was changed to contain the 6 bytes at 'new_bytes'. */
+ * 'old_mac' was changed to contain the 6 bytes at 'new_mac'. */
 ovs_be16
-recalc_csum48(ovs_be16 old_csum, const void *old_bytes,
-              const void *new_bytes)
+recalc_csum48(ovs_be16 old_csum, const struct eth_addr old_mac,
+              const struct eth_addr new_mac)
 {
     ovs_be16 new_csum = old_csum;
-    const uint16_t *p16_old = old_bytes,
-                   *p16_new = new_bytes;
-    int i;
 
-    for (i = 0; i < 3; ++i) {
-        new_csum = recalc_csum16(new_csum, get_unaligned_be16(&p16_old[i]),
-                                 get_unaligned_be16(&p16_new[i]));
+    for (int i = 0; i < 3; ++i) {
+        new_csum = recalc_csum16(new_csum, old_mac.be16[i], new_mac.be16[i]);
     }
 
     return new_csum;

diff --git a/lib/csum.h b/lib/csum.h
@@ -28,8 +28,8 @@ uint32_t csum_continue(uint32_t partial, const void *, size_t);
 ovs_be16 csum_finish(uint32_t partial);
 ovs_be16 recalc_csum16(ovs_be16 old_csum, ovs_be16 old_u16, ovs_be16 new_u16);
 ovs_be16 recalc_csum32(ovs_be16 old_csum, ovs_be32 old_u32, ovs_be32 new_u32);
-ovs_be16 recalc_csum48(ovs_be16 old_csum, const void *old_bytes,
-                       const void *new_bytes);
+ovs_be16 recalc_csum48(ovs_be16 old_csum, const struct eth_addr old_mac,
+                       const struct eth_addr new_mac);
 ovs_be16 recalc_csum128(ovs_be16 old_csum, ovs_16aligned_be32 old_u32[4],
                         const ovs_be32 new_u32[4]);