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

[David Marchand <david.marchand@redhat.com>]

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