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

[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.

@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

-- 
Gaëtan