[dpdk-dev] [RFC 0/4] net/ether: improvements

[dpdk-dev] [RFC 0/4] net/ether: improvements

[Stephen Hemminger]

A bunch of little changes to net/ether

Stephen Hemminger (4):
  net/ether: deinline non-critical functions
  net/ether: add eth_unformat_addr
  ethdev: use eth_unformat_addr
  net/ether: use bitops to speedup comparison

 lib/librte_ethdev/rte_class_eth.c  |  9 +----
 lib/librte_net/Makefile            |  1 +
 lib/librte_net/rte_ether.c         | 41 ++++++++++++++++++++++
 lib/librte_net/rte_ether.h         | 55 ++++++++++++++----------------
 lib/librte_net/rte_net_version.map |  9 +++++
 5 files changed, 77 insertions(+), 38 deletions(-)
 create mode 100644 lib/librte_net/rte_ether.c

-- 
2.20.1

[dpdk-dev] [RFC 1/4] net/ether: deinline non-critical functions

[Stephen Hemminger]

Formatting ethernet address and getting a random value are
not in critical path so they should not be inlined.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_net/Makefile            |  1 +
 lib/librte_net/rte_ether.c         | 29 +++++++++++++++++++++++++++++
 lib/librte_net/rte_ether.h         | 24 ++++--------------------
 lib/librte_net/rte_net_version.map |  8 ++++++++
 4 files changed, 42 insertions(+), 20 deletions(-)
 create mode 100644 lib/librte_net/rte_ether.c

diff --git a/lib/librte_net/Makefile b/lib/librte_net/Makefile
index c3082069ab50..f68b42cd0e8f 100644
--- a/lib/librte_net/Makefile
+++ b/lib/librte_net/Makefile
@@ -14,6 +14,7 @@ LIBABIVER := 1
 
 SRCS-$(CONFIG_RTE_LIBRTE_NET) := rte_net.c
 SRCS-$(CONFIG_RTE_LIBRTE_NET) += rte_net_crc.c
+SRCS-$(CONFIG_RTE_LIBRTE_NET) := rte_ether.c
 SRCS-$(CONFIG_RTE_LIBRTE_NET) += rte_arp.c
 
 # install includes
diff --git a/lib/librte_net/rte_ether.c b/lib/librte_net/rte_ether.c
new file mode 100644
index 000000000000..d4b41f122a16
--- /dev/null
+++ b/lib/librte_net/rte_ether.c
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2014 Intel Corporation
+ */
+
+#include <rte_ether.h>
+
+void
+eth_random_addr(uint8_t *addr)
+{
+	uint64_t rand = rte_rand();
+	uint8_t *p = (uint8_t *)&rand;
+
+	rte_memcpy(addr, p, ETHER_ADDR_LEN);
+	addr[0] &= (uint8_t)~ETHER_GROUP_ADDR;	/* clear multicast bit */
+	addr[0] |= ETHER_LOCAL_ADMIN_ADDR;	/* set local assignment bit */
+}
+
+void
+ether_format_addr(char *buf, uint16_t size,
+		  const struct ether_addr *eth_addr)
+{
+	snprintf(buf, size, "%02X:%02X:%02X:%02X:%02X:%02X",
+		 eth_addr->addr_bytes[0],
+		 eth_addr->addr_bytes[1],
+		 eth_addr->addr_bytes[2],
+		 eth_addr->addr_bytes[3],
+		 eth_addr->addr_bytes[4],
+		 eth_addr->addr_bytes[5]);
+}
diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index 3a87ff184900..46d40412763c 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -204,15 +204,8 @@ static inline int is_valid_assigned_ether_addr(const struct ether_addr *ea)
  * @param addr
  *   A pointer to Ethernet address.
  */
-static inline void eth_random_addr(uint8_t *addr)
-{
-	uint64_t rand = rte_rand();
-	uint8_t *p = (uint8_t *)&rand;
-
-	rte_memcpy(addr, p, ETHER_ADDR_LEN);
-	addr[0] &= (uint8_t)~ETHER_GROUP_ADDR;       /* clear multicast bit */
-	addr[0] |= ETHER_LOCAL_ADMIN_ADDR;  /* set local assignment bit */
-}
+void
+eth_random_addr(uint8_t *addr);
 
 /**
  * Fast copy an Ethernet address.
@@ -251,18 +244,9 @@ static inline void ether_addr_copy(const struct ether_addr *ea_from,
  * @param eth_addr
  *   A pointer to a ether_addr structure.
  */
-static inline void
+void
 ether_format_addr(char *buf, uint16_t size,
-		  const struct ether_addr *eth_addr)
-{
-	snprintf(buf, size, "%02X:%02X:%02X:%02X:%02X:%02X",
-		 eth_addr->addr_bytes[0],
-		 eth_addr->addr_bytes[1],
-		 eth_addr->addr_bytes[2],
-		 eth_addr->addr_bytes[3],
-		 eth_addr->addr_bytes[4],
-		 eth_addr->addr_bytes[5]);
-}
+		  const struct ether_addr *eth_addr);
 
 /**
  * Ethernet header: Contains the destination address, source address
diff --git a/lib/librte_net/rte_net_version.map b/lib/librte_net/rte_net_version.map
index 26c06e7c7ae7..49d34093781c 100644
--- a/lib/librte_net/rte_net_version.map
+++ b/lib/librte_net/rte_net_version.map
@@ -13,6 +13,14 @@ DPDK_17.05 {
 
 } DPDK_16.11;
 
+DPDK_19.08 {
+	global:
+
+	eth_random_addr;
+	eth_format_addr;
+} DPDK_17.05;
+
+
 EXPERIMENTAL {
 	global:
 
-- 
2.20.1

[dpdk-dev] [RFC 2/4] net/ether: add eth_unformat_addr

[Stephen Hemminger]

Make a function that coresponds with eth_aton_r which can
be used to convert string to ether_addr.

This also allows rte_ethdev to no longer depend on the
cmdline library.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_net/rte_ether.c         | 12 ++++++++++++
 lib/librte_net/rte_ether.h         | 14 ++++++++++++++
 lib/librte_net/rte_net_version.map |  1 +
 3 files changed, 27 insertions(+)

diff --git a/lib/librte_net/rte_ether.c b/lib/librte_net/rte_ether.c
index d4b41f122a16..ca7c841db197 100644
--- a/lib/librte_net/rte_ether.c
+++ b/lib/librte_net/rte_ether.c
@@ -27,3 +27,15 @@ ether_format_addr(char *buf, uint16_t size,
 		 eth_addr->addr_bytes[4],
 		 eth_addr->addr_bytes[5]);
 }
+
+int __rte_experimental
+ether_unformat_addr(const char *str, struct ether_addr *eth_addr)
+{
+	return (sscanf(str, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
+		       &eth_addr->addr_bytes[0],
+		       &eth_addr->addr_bytes[1],
+		       &eth_addr->addr_bytes[2],
+		       &eth_addr->addr_bytes[3],
+		       &eth_addr->addr_bytes[4],
+		       &eth_addr->addr_bytes[5]) == 6) ? 0 : -1;
+}
diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index 46d40412763c..b94e64b2195e 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -248,6 +248,20 @@ void
 ether_format_addr(char *buf, uint16_t size,
 		  const struct ether_addr *eth_addr);
 
+/**
+ * Convert string with Ethernet address to an ether_addr.
+ *
+ * @param str
+ *   A pointer to buffer contains the formatted MAC address.
+ * @param eth_addr
+ *   A pointer to a ether_addr structure.
+ * @return
+ *   0 if successful
+ *   -1 and sets rte_errno if invalid string
+ */
+int __rte_experimental
+ether_unformat_addr(const char *str, struct ether_addr *eth_addr);
+
 /**
  * Ethernet header: Contains the destination address, source address
  * and frame type.
diff --git a/lib/librte_net/rte_net_version.map b/lib/librte_net/rte_net_version.map
index 49d34093781c..bbf14ff1cdfa 100644
--- a/lib/librte_net/rte_net_version.map
+++ b/lib/librte_net/rte_net_version.map
@@ -26,4 +26,5 @@ EXPERIMENTAL {
 
 	rte_net_make_rarp_packet;
 	rte_net_skip_ip6_ext;
+	eth_unformat_addr;
 };
-- 
2.20.1

[dpdk-dev] [RFC 3/4] ethdev: use eth_unformat_addr

[Stephen Hemminger]

Use eth_unformat_addr, so that ethdev can be built and work
without the cmdline library. The dependency on cmdline was
an arrangement of convenience anyway.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_ethdev/rte_class_eth.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/lib/librte_ethdev/rte_class_eth.c b/lib/librte_ethdev/rte_class_eth.c
index cb99c92ece93..40ca936230c6 100644
--- a/lib/librte_ethdev/rte_class_eth.c
+++ b/lib/librte_ethdev/rte_class_eth.c
@@ -4,7 +4,6 @@
 
 #include <string.h>
 
-#include <cmdline_parse_etheraddr.h>
 #include <rte_class.h>
 #include <rte_compat.h>
 #include <rte_errno.h>
@@ -43,19 +42,13 @@ static int
 eth_mac_cmp(const char *key __rte_unused,
 		const char *value, void *opaque)
 {
-	int ret;
 	struct ether_addr mac;
 	const struct rte_eth_dev_data *data = opaque;
 	struct rte_eth_dev_info dev_info;
 	uint32_t index;
 
 	/* Parse devargs MAC address. */
-	/*
-	 * cannot use ether_aton_r(value, &mac)
-	 * because of include conflict with rte_ether.h
-	 */
-	ret = cmdline_parse_etheraddr(NULL, value, &mac, sizeof(mac));
-	if (ret < 0)
+	if (ether_unformat_addr(value, &mac) < 0)
 		return -1; /* invalid devargs value */
 
 	/* Return 0 if devargs MAC is matching one of the device MACs. */
-- 
2.20.1

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

[Stephen Hemminger]

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 <stephen@networkplumber.org>
---
 lib/librte_net/rte_ether.h | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index b94e64b2195e..5d9242cda230 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -78,11 +78,10 @@ struct ether_addr {
 static inline int is_same_ether_addr(const struct ether_addr *ea1,
 				     const struct ether_addr *ea2)
 {
-	int i;
-	for (i = 0; i < ETHER_ADDR_LEN; i++)
-		if (ea1->addr_bytes[i] != ea2->addr_bytes[i])
-			return 0;
-	return 1;
+	const unaligned_uint16_t *w1 = (const uint16_t *)ea1;
+	const unaligned_uint16_t *w2 = (const uint16_t *)ea2;
+
+	return ((w1[0] ^ w2[0]) | (w1[1] ^ w2[1]) | (w1[2] ^ w2[2])) == 0;
 }
 
 /**
@@ -97,11 +96,9 @@ static inline int is_same_ether_addr(const struct ether_addr *ea1,
  */
 static inline int is_zero_ether_addr(const struct ether_addr *ea)
 {
-	int i;
-	for (i = 0; i < ETHER_ADDR_LEN; i++)
-		if (ea->addr_bytes[i] != 0x00)
-			return 0;
-	return 1;
+	const unaligned_uint16_t *w = (const uint16_t *)ea;
+
+	return (w[0] | w[1] | w[2]) == 0;
 }
 
 /**
-- 
2.20.1

Re: [dpdk-dev] [RFC 1/4] net/ether: deinline non-critical functions

[David Marchand]

On Thu, May 16, 2019 at 12:20 AM Stephen Hemminger <
stephen@networkplumber.org> wrote:

> Formatting ethernet address and getting a random value are
> not in critical path so they should not be inlined.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_net/Makefile            |  1 +
>  lib/librte_net/rte_ether.c         | 29 +++++++++++++++++++++++++++++
>  lib/librte_net/rte_ether.h         | 24 ++++--------------------
>  lib/librte_net/rte_net_version.map |  8 ++++++++
>  4 files changed, 42 insertions(+), 20 deletions(-)
>  create mode 100644 lib/librte_net/rte_ether.c
>
> diff --git a/lib/librte_net/Makefile b/lib/librte_net/Makefile
> index c3082069ab50..f68b42cd0e8f 100644
> --- a/lib/librte_net/Makefile
> +++ b/lib/librte_net/Makefile
> @@ -14,6 +14,7 @@ LIBABIVER := 1
>
>  SRCS-$(CONFIG_RTE_LIBRTE_NET) := rte_net.c
>  SRCS-$(CONFIG_RTE_LIBRTE_NET) += rte_net_crc.c
> +SRCS-$(CONFIG_RTE_LIBRTE_NET) := rte_ether.c
>  SRCS-$(CONFIG_RTE_LIBRTE_NET) += rte_arp.c
>
>  # install includes
>

Missing the meson counterpart.


diff --git a/lib/librte_net/rte_ether.c b/lib/librte_net/rte_ether.c
> new file mode 100644
> index 000000000000..d4b41f122a16
> --- /dev/null
> +++ b/lib/librte_net/rte_ether.c
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2014 Intel Corporation
> + */
> +
> +#include <rte_ether.h>
> +
> +void
> +eth_random_addr(uint8_t *addr)
> +{
> +       uint64_t rand = rte_rand();
> +       uint8_t *p = (uint8_t *)&rand;
> +
> +       rte_memcpy(addr, p, ETHER_ADDR_LEN);
> +       addr[0] &= (uint8_t)~ETHER_GROUP_ADDR;  /* clear multicast bit */
> +       addr[0] |= ETHER_LOCAL_ADMIN_ADDR;      /* set local assignment
> bit */
> +}
> +
> +void
> +ether_format_addr(char *buf, uint16_t size,
> +                 const struct ether_addr *eth_addr)
> +{
> +       snprintf(buf, size, "%02X:%02X:%02X:%02X:%02X:%02X",
> +                eth_addr->addr_bytes[0],
> +                eth_addr->addr_bytes[1],
> +                eth_addr->addr_bytes[2],
> +                eth_addr->addr_bytes[3],
> +                eth_addr->addr_bytes[4],
> +                eth_addr->addr_bytes[5]);
> +}
> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index 3a87ff184900..46d40412763c 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -204,15 +204,8 @@ static inline int is_valid_assigned_ether_addr(const
> struct ether_addr *ea)
>   * @param addr
>   *   A pointer to Ethernet address.
>   */
> -static inline void eth_random_addr(uint8_t *addr)
> -{
> -       uint64_t rand = rte_rand();
> -       uint8_t *p = (uint8_t *)&rand;
> -
> -       rte_memcpy(addr, p, ETHER_ADDR_LEN);
> -       addr[0] &= (uint8_t)~ETHER_GROUP_ADDR;       /* clear multicast
> bit */
> -       addr[0] |= ETHER_LOCAL_ADMIN_ADDR;  /* set local assignment bit */
> -}
> +void
> +eth_random_addr(uint8_t *addr);
>
>  /**
>   * Fast copy an Ethernet address.
> @@ -251,18 +244,9 @@ static inline void ether_addr_copy(const struct
> ether_addr *ea_from,
>   * @param eth_addr
>   *   A pointer to a ether_addr structure.
>   */
> -static inline void
> +void
>  ether_format_addr(char *buf, uint16_t size,
> -                 const struct ether_addr *eth_addr)
> -{
> -       snprintf(buf, size, "%02X:%02X:%02X:%02X:%02X:%02X",
> -                eth_addr->addr_bytes[0],
> -                eth_addr->addr_bytes[1],
> -                eth_addr->addr_bytes[2],
> -                eth_addr->addr_bytes[3],
> -                eth_addr->addr_bytes[4],
> -                eth_addr->addr_bytes[5]);
> -}
> +                 const struct ether_addr *eth_addr);
>
>  /**
>   * Ethernet header: Contains the destination address, source address
> diff --git a/lib/librte_net/rte_net_version.map
> b/lib/librte_net/rte_net_version.map
> index 26c06e7c7ae7..49d34093781c 100644
> --- a/lib/librte_net/rte_net_version.map
> +++ b/lib/librte_net/rte_net_version.map
> @@ -13,6 +13,14 @@ DPDK_17.05 {
>
>  } DPDK_16.11;
>
> +DPDK_19.08 {
> +       global:
> +
> +       eth_random_addr;
> +       eth_format_addr;
> +} DPDK_17.05;
>

A bit sad to introduce unprefixed symbols in ABI.
These were inlined before, so the existing users already have their own
copy of this code embedded.
Can't we go and add rte_eth_(random/format)_addr from the start ?


+
> +
>

(nit: one empty line is enough.)

 EXPERIMENTAL {
>         global:
>
> --
> 2.20.1
>
>


-- 
David Marchand

Re: [dpdk-dev] [RFC 2/4] net/ether: add eth_unformat_addr

[David Marchand]

On Thu, May 16, 2019 at 12:20 AM Stephen Hemminger <
stephen@networkplumber.org> wrote:

> Make a function that coresponds with eth_aton_r which can
>

nit: corresponds

be used to convert string to ether_addr.
>
> This also allows rte_ethdev to no longer depend on the
> cmdline library.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_net/rte_ether.c         | 12 ++++++++++++
>  lib/librte_net/rte_ether.h         | 14 ++++++++++++++
>  lib/librte_net/rte_net_version.map |  1 +
>  3 files changed, 27 insertions(+)
>
> diff --git a/lib/librte_net/rte_ether.c b/lib/librte_net/rte_ether.c
> index d4b41f122a16..ca7c841db197 100644
> --- a/lib/librte_net/rte_ether.c
> +++ b/lib/librte_net/rte_ether.c
> @@ -27,3 +27,15 @@ ether_format_addr(char *buf, uint16_t size,
>                  eth_addr->addr_bytes[4],
>                  eth_addr->addr_bytes[5]);
>  }
> +
> +int __rte_experimental
>

You don't need the experimental tag in the .c.


+ether_unformat_addr(const char *str, struct ether_addr *eth_addr)
> +{
> +       return (sscanf(str, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
> +                      &eth_addr->addr_bytes[0],
> +                      &eth_addr->addr_bytes[1],
> +                      &eth_addr->addr_bytes[2],
> +                      &eth_addr->addr_bytes[3],
> +                      &eth_addr->addr_bytes[4],
> +                      &eth_addr->addr_bytes[5]) == 6) ? 0 : -1;
> +}
>

Using scanf this way, you won't detect trailing characters

And I am not sure this format would detect something like:
00:11:22:33:44:555


diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index 46d40412763c..b94e64b2195e 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -248,6 +248,20 @@ void
>  ether_format_addr(char *buf, uint16_t size,
>                   const struct ether_addr *eth_addr);
>
> +/**
> + * Convert string with Ethernet address to an ether_addr.
> + *
> + * @param str
> + *   A pointer to buffer contains the formatted MAC address.
> + * @param eth_addr
> + *   A pointer to a ether_addr structure.
> + * @return
> + *   0 if successful
> + *   -1 and sets rte_errno if invalid string
> + */
> +int __rte_experimental
> +ether_unformat_addr(const char *str, struct ether_addr *eth_addr);
> +
>  /**
>   * Ethernet header: Contains the destination address, source address
>   * and frame type.
> diff --git a/lib/librte_net/rte_net_version.map
> b/lib/librte_net/rte_net_version.map
> index 49d34093781c..bbf14ff1cdfa 100644
> --- a/lib/librte_net/rte_net_version.map
> +++ b/lib/librte_net/rte_net_version.map
> @@ -26,4 +26,5 @@ EXPERIMENTAL {
>
>         rte_net_make_rarp_packet;
>         rte_net_skip_ip6_ext;
> +       eth_unformat_addr;
>

Ditto previous patch, can we prefix this with rte_ ?


-- 
David Marchand

Re: [dpdk-dev] [RFC 3/4] ethdev: use eth_unformat_addr

[David Marchand]

On Thu, May 16, 2019 at 12:20 AM Stephen Hemminger <
stephen@networkplumber.org> wrote:

> Use eth_unformat_addr, so that ethdev can be built and work
> without the cmdline library. The dependency on cmdline was
> an arrangement of convenience anyway.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_ethdev/rte_class_eth.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_class_eth.c
> b/lib/librte_ethdev/rte_class_eth.c
> index cb99c92ece93..40ca936230c6 100644
> --- a/lib/librte_ethdev/rte_class_eth.c
> +++ b/lib/librte_ethdev/rte_class_eth.c
> @@ -4,7 +4,6 @@
>
>  #include <string.h>
>
> -#include <cmdline_parse_etheraddr.h>
>  #include <rte_class.h>
>  #include <rte_compat.h>
>  #include <rte_errno.h>
> @@ -43,19 +42,13 @@ static int
>  eth_mac_cmp(const char *key __rte_unused,
>                 const char *value, void *opaque)
>  {
> -       int ret;
>         struct ether_addr mac;
>         const struct rte_eth_dev_data *data = opaque;
>         struct rte_eth_dev_info dev_info;
>         uint32_t index;
>
>         /* Parse devargs MAC address. */
> -       /*
> -        * cannot use ether_aton_r(value, &mac)
> -        * because of include conflict with rte_ether.h
> -        */
> -       ret = cmdline_parse_etheraddr(NULL, value, &mac, sizeof(mac));
> -       if (ret < 0)
> +       if (ether_unformat_addr(value, &mac) < 0)
>

I had a look at cmdline_parse_etheraddr.
I can see it supports a different format, like "XXXX:XXXX:XXXX" for a mac
address.
So here, we might be breaking some existing users assumptions.


                return -1; /* invalid devargs value */
>
>         /* Return 0 if devargs MAC is matching one of the device MACs. */
> --
> 2.20.1
>
>

-- 
David Marchand

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

[Mattias Rönnblom]

On 2019-05-16 00:19, 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 <stephen@networkplumber.org>
> ---
>   lib/librte_net/rte_ether.h | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index b94e64b2195e..5d9242cda230 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -78,11 +78,10 @@ struct ether_addr {
>   static inline int is_same_ether_addr(const struct ether_addr *ea1,
>   				     const struct ether_addr *ea2)
>   {
> -	int i;
> -	for (i = 0; i < ETHER_ADDR_LEN; i++)
> -		if (ea1->addr_bytes[i] != ea2->addr_bytes[i])
> -			return 0;
> -	return 1;
> +	const unaligned_uint16_t *w1 = (const uint16_t *)ea1;
> +	const unaligned_uint16_t *w2 = (const uint16_t *)ea2;
> +
> +	return ((w1[0] ^ w2[0]) | (w1[1] ^ w2[1]) | (w1[2] ^ w2[2])) == 0;
>   }
>   

If you want to shave off a couple of instructions, you can switch the 
three 16-bit loads to one 32-bit and one 16-bit load.

Something like:

	const uint8_t *ea1_b = (const uint8_t *)ea1;
	const uint8_t *ea2_b = (const uint8_t *)ea2;
	uint32_t ea1_h;
	uint32_t ea2_h;
	uint16_t ea1_l;
	uint16_t ea2_l;

	memcpy(&ea1_h, &ea1_b[0], sizeof(ea1_h));
	memcpy(&ea1_l, &ea1_b[sizeof(ea1_h)], sizeof(ea1_l));

	memcpy(&ea2_h, &ea2_b[0], sizeof(ea2_h));
	memcpy(&ea2_l, &ea2_b[sizeof(ea2_h)], sizeof(ea2_l));

	return ((ea1_l ^ ea2_l) | (ea1_h ^ ea2_h)) == 0;

Code is not as clean as your solution though.

>   /**
> @@ -97,11 +96,9 @@ static inline int is_same_ether_addr(const struct ether_addr *ea1,
>    */
>   static inline int is_zero_ether_addr(const struct ether_addr *ea)
>   {
> -	int i;
> -	for (i = 0; i < ETHER_ADDR_LEN; i++)
> -		if (ea->addr_bytes[i] != 0x00)
> -			return 0;
> -	return 1;
> +	const unaligned_uint16_t *w = (const uint16_t *)ea;
> +
> +	return (w[0] | w[1] | w[2]) == 0;
>   }
>   
>   /**
> 

Re: [dpdk-dev] [RFC 3/4] ethdev: use eth_unformat_addr

[Bruce Richardson]

On Wed, May 15, 2019 at 03:19:51PM -0700, Stephen Hemminger wrote:
> Use eth_unformat_addr, so that ethdev can be built and work
> without the cmdline library. The dependency on cmdline was
> an arrangement of convenience anyway.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_ethdev/rte_class_eth.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
I think this patch should also modify the make and meson.build files to
remove that dependency then.

/Bruce

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

[Stephen Hemminger]

On Thu, 16 May 2019 11:03:10 +0200
Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:

> On 2019-05-16 00:19, 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 <stephen@networkplumber.org>
> > ---
> >   lib/librte_net/rte_ether.h | 17 +++++++----------
> >   1 file changed, 7 insertions(+), 10 deletions(-)
> > 
> > diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> > index b94e64b2195e..5d9242cda230 100644
> > --- a/lib/librte_net/rte_ether.h
> > +++ b/lib/librte_net/rte_ether.h
> > @@ -78,11 +78,10 @@ struct ether_addr {
> >   static inline int is_same_ether_addr(const struct ether_addr *ea1,
> >   				     const struct ether_addr *ea2)
> >   {
> > -	int i;
> > -	for (i = 0; i < ETHER_ADDR_LEN; i++)
> > -		if (ea1->addr_bytes[i] != ea2->addr_bytes[i])
> > -			return 0;
> > -	return 1;
> > +	const unaligned_uint16_t *w1 = (const uint16_t *)ea1;
> > +	const unaligned_uint16_t *w2 = (const uint16_t *)ea2;
> > +
> > +	return ((w1[0] ^ w2[0]) | (w1[1] ^ w2[1]) | (w1[2] ^ w2[2])) == 0;
> >   }
> >     
> 
> If you want to shave off a couple of instructions, you can switch the 
> three 16-bit loads to one 32-bit and one 16-bit load.
> 
> Something like:
> 
> 	const uint8_t *ea1_b = (const uint8_t *)ea1;
> 	const uint8_t *ea2_b = (const uint8_t *)ea2;
> 	uint32_t ea1_h;
> 	uint32_t ea2_h;
> 	uint16_t ea1_l;
> 	uint16_t ea2_l;
> 
> 	memcpy(&ea1_h, &ea1_b[0], sizeof(ea1_h));
> 	memcpy(&ea1_l, &ea1_b[sizeof(ea1_h)], sizeof(ea1_l));
> 
> 	memcpy(&ea2_h, &ea2_b[0], sizeof(ea2_h));
> 	memcpy(&ea2_l, &ea2_b[sizeof(ea2_h)], sizeof(ea2_l));
> 
> 	return ((ea1_l ^ ea2_l) | (ea1_h ^ ea2_h)) == 0;
> 
> Code is not as clean as your solution though.
> 
> >   /**
> > @@ -97,11 +96,9 @@ static inline int is_same_ether_addr(const struct ether_addr *ea1,
> >    */
> >   static inline int is_zero_ether_addr(const struct ether_addr *ea)
> >   {
> > -	int i;
> > -	for (i = 0; i < ETHER_ADDR_LEN; i++)
> > -		if (ea->addr_bytes[i] != 0x00)
> > -			return 0;
> > -	return 1;
> > +	const unaligned_uint16_t *w = (const uint16_t *)ea;
> > +
> > +	return (w[0] | w[1] | w[2]) == 0;
> >   }
> >   
> >   /**
> >   

You can even do 64 bit load and then mask, which is what Linux kernel
does. But not sure it matters. The cost of the loop is what is expensive

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

[Bruce Richardson]

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 <stephen@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

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

[Stephen Hemminger]

On Thu, 16 May 2019 17:03:37 +0100
Bruce Richardson <bruce.richardson@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 <stephen@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?

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

[Bruce Richardson]

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.richardson@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 <stephen@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.

Thanks,
/Bruce

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.richardson@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 <stephen@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

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

[Stephen Hemminger]

On Thu, 16 May 2019 17:36:43 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> 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.richardson@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 <stephen@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

Agree if structure is naturally packed, adding packed attribute is a mistake.

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

[Bruce Richardson]

On Thu, May 16, 2019 at 10:04:54AM -0700, Stephen Hemminger wrote:
> On Thu, 16 May 2019 17:36:43 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > 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.richardson@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 <stephen@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
> 
> Agree if structure is naturally packed, adding packed attribute is a mistake.

Do you want to also include such a change in your patchset, or will I do up
a patch for it - perhaps a two-patch set with this and my previous alignment
patch?

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

[Bruce Richardson]

On Thu, May 16, 2019 at 09:37:42PM +0100, Bruce Richardson wrote:
> On Thu, May 16, 2019 at 10:04:54AM -0700, Stephen Hemminger wrote:
> > Agree if structure is naturally packed, adding packed attribute is a mistake.
> 
> Do you want to also include such a change in your patchset, or will I do up
> a patch for it - perhaps a two-patch set with this and my previous alignment
> patch?

Never mind, see patchset on list now...