[dpdk-dev] [PATCH v9 02/24] ethdev: add a link status text representation

[Ivan Dyukov]

11.08.2020 14:02, Gaëtan Rivet пишет:
> On 11/08/20 11:52 +0300, Ivan Dyukov wrote:
>> Link status structure keeps complicated values which are hard to
>> represent to end user. e.g. link_speed has INT_MAX value which
>> means that speed is unknown, link_duplex equal to 0 means
>> 'half-duplex' etc. To simplify processing of the values
>> in application, new dpdk function is introduced.
>>
>> This commit adds function which treat link status structure
>> and format it to text representation. User may create custom
>> link status string using format string. If format string is NULL,
>> the function construct standard link status string.
>>
>> Signed-off-by: Ivan Dyukov <i.dyukov at samsung.com>
> Hello Ivan,
>
> I don't see much difference for this patch, from what I read in previous
> thread on the principle it does not seem motivated enough?
>
> I'd have a few nits on the implementation, but on the principle: I think
> this can be an incentive to get properly formatted port status strings.
>
> The API is a little awkward however, and it is definitely complex to
> maintain a format-based function. I think we could do without.
>
> I've tried a smaller alternative.
>
>   + simpler to use.
>   + simpler to maintain.
>   + safer in general.
>   + no need to declare local string to store intermediate output.
>
>   - one ugly macro.

It would be good for all values except link_speed because link speed 
should be formated using sprintf e.g.

char str[15];

...

char *rte_eth_link_speed_str(uint32_t link_speed) {

if (link_speed == UINT32_MAX)

return "Unknown";

else

snprintf(str,sizeof(str),"%d",link_speed);

return str;

}

so rte_eth_link_speed_str will require some global string, not local.
>
> @Thomas, Stephen: would something like this be easier to accept
> in librte_ethdev?
>
> index d79699e2ed..9d72a0b70e 100644
> --- a/examples/ip_pipeline/cli.c
> +++ b/examples/ip_pipeline/cli.c
> @@ -273,13 +273,13 @@ print_link_info(struct link *link, char *out, size_t out_size)
>                  "\n"
>                  "%s: flags=<%s> mtu %u\n"
>                  "\tether %02X:%02X:%02X:%02X:%02X:%02X rxqueues %u txqueues %u\n"
> -               "\tport# %u  speed %u Mbps\n"
> +               "\tport# %u  speed %s\n"
>                  "\tRX packets %" PRIu64"  bytes %" PRIu64"\n"
>                  "\tRX errors %" PRIu64"  missed %" PRIu64"  no-mbuf %" PRIu64"\n"
>                  "\tTX packets %" PRIu64"  bytes %" PRIu64"\n"
>                  "\tTX errors %" PRIu64"\n",
>                  link->name,
> -               eth_link.link_status == 0 ? "DOWN" : "UP",
> +               rte_eth_link_status_str(eth_link.link_status),
>                  mtu,
>                  mac_addr.addr_bytes[0], mac_addr.addr_bytes[1],
>                  mac_addr.addr_bytes[2], mac_addr.addr_bytes[3],
> @@ -287,7 +287,7 @@ print_link_info(struct link *link, char *out, size_t out_size)
>                  link->n_rxq,
>                  link->n_txq,
>                  link->port_id,
> -               eth_link.link_speed,
> +               rte_eth_link_speed_str(eth_link.link_speed),
>                  stats.ipackets,
>                  stats.ibytes,
>                  stats.ierrors,
> diff --git a/examples/ipv4_multicast/main.c b/examples/ipv4_multicast/main.c
> index 7e255c35a3..1350313ee9 100644
> --- a/examples/ipv4_multicast/main.c
> +++ b/examples/ipv4_multicast/main.c
> @@ -591,14 +591,8 @@ check_all_ports_link_status(uint32_t port_mask)
>                          }
>                          /* print link status if flag set */
>                          if (print_flag == 1) {
> -                               if (link.link_status)
> -                                       printf(
> -                                       "Port%d Link Up. Speed %u Mbps - %s\n",
> -                                       portid, link.link_speed,
> -                               (link.link_duplex == ETH_LINK_FULL_DUPLEX) ?
> -                                       ("full-duplex") : ("half-duplex"));
> -                               else
> -                                       printf("Port %d Link Down\n", portid);
> +                               printf("Port %s " RTE_ETH_LINK_FMT "\n",
> +                                      RTE_ETH_LINK_STR(link));
>                                  continue;
>                          }
>                          /* clear all_ports_up flag if any link down */
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 57e4a6ca58..f81e876d49 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4993,6 +4993,102 @@ rte_eth_tx_buffer(uint16_t port_id, uint16_t queue_id,
>          return rte_eth_tx_buffer_flush(port_id, queue_id, buffer);
>   }
>
> +/**
> + * Missing: doc.
> + */
> +static inline const char *
> +rte_eth_link_speed_str(uint32_t speed)
> +{
> +       struct {
> +               const char *str;
> +               uint32_t speed;
> +       } speed_str_map[] = {
> +               { "Unknown", ETH_SPEED_NUM_NONE },
> +               { "10 Mbps", ETH_SPEED_NUM_10M },
> +               { "100 Mbps", ETH_SPEED_NUM_100M },
> +               { "1 Gbps", ETH_SPEED_NUM_1G },
> +               { "2.5 Gbps", ETH_SPEED_NUM_2_5G },
> +               { "5 Gbps", ETH_SPEED_NUM_5G },
> +               { "10 Gbps", ETH_SPEED_NUM_10G },
> +               { "20 Gbps", ETH_SPEED_NUM_20G },
> +               { "25 Gbps", ETH_SPEED_NUM_25G },
> +               { "40 Gbps", ETH_SPEED_NUM_40G },
> +               { "50 Gbps", ETH_SPEED_NUM_50G },
> +               { "56 Gbps", ETH_SPEED_NUM_56G },
> +               { "100 Gbps", ETH_SPEED_NUM_100G },
> +               { "200 Gbps", ETH_SPEED_NUM_200G },
> +       };
> +       size_t i;
> +
> +       for (i = 0; i < RTE_DIM(speed_str_map); i++) {
> +               if (speed == speed_str_map[i].speed)
> +                       return speed_str_map[i].str;
> +       }
> +
> +       return speed_str_map[0].str;
> +}
> +
> +/**
> + * Missing: doc.
> + */
> +static inline const char *
> +rte_eth_link_duplex_str(uint16_t duplex)
> +{
> +       const char *str[] = {
> +               [0] = "HDX",
> +               [1] = "FDX",
> +       };
> +
> +       return str[!!duplex];
> +}
> +
> +/**
> + * Missing: doc.
> + */
> +static inline const char *
> +rte_eth_link_autoneg_str(uint16_t autoneg)
> +{
> +       const char *str[] = {
> +               [0] = "Fixed",
> +               [1] = "Autoneg",
> +       };
> +
> +       return str[!!autoneg];
> +}
> +
> +/**
> + * Missing: doc.
> + */
> +static inline const char *
> +rte_eth_link_status_str(uint16_t status)
> +{
> +       const char *str[] = {
> +               [0] = "Down",
> +               [1] = "Up",
> +       };
> +
> +       return str[!!status];
> +}
> +
> +/* internal. */
> +#define RTE_ETH_LINK_DOWN_OR_WHAT_(link, what, sep) \
> +       ((link).link_status == ETH_LINK_DOWN ? "" : (what)), \
> +       ((link).link_status == ETH_LINK_DOWN ? "" : (sep)) \
> +
> +/**
> + * Missing: doc.
> + */
> +#define RTE_ETH_LINK_FMT "%s%s%s%s%s%s"
> +
> +/**
> + * Missing: doc.
> + */
> +#define RTE_ETH_LINK_STR(link) \
> +       ((link).link_status == ETH_LINK_DOWN ? "Link down" : "Link up at "), \
> +       RTE_ETH_LINK_DOWN_OR_WHAT_(link, rte_eth_link_speed_str((link).link_speed), " "), \
> +       RTE_ETH_LINK_DOWN_OR_WHAT_(link, rte_eth_link_duplex_str((link).link_duplex), " "), \
> +       RTE_ETH_LINK_DOWN_OR_WHAT_(link, rte_eth_link_autoneg_str((link).link_autoneg), "")
> +
>   #ifdef __cplusplus
>   }
>   #endif
>