From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id B3682A04C9; Tue, 11 Aug 2020 13:02:51 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C230A4C9D; Tue, 11 Aug 2020 13:02:50 +0200 (CEST) Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net [217.70.183.196]) by dpdk.org (Postfix) with ESMTP id F23D7F04 for ; Tue, 11 Aug 2020 13:02:49 +0200 (CEST) X-Originating-IP: 90.92.205.40 Received: from u256.net (lfbn-idf2-1-1144-40.w90-92.abo.wanadoo.fr [90.92.205.40]) (Authenticated sender: grive@u256.net) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 5DE57E0003; Tue, 11 Aug 2020 11:02:46 +0000 (UTC) Date: Tue, 11 Aug 2020 13:02:41 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Ivan Dyukov Cc: dev@dpdk.org, thomas@monjalon.net, ferruh.yigit@intel.com, stephen@networkplumber.org Message-ID: <20200811110241.tfucvkcgn7yhlvoo@u256.net> References: <20200427095737.11082-1-i.dyukov@samsung.com> <20200811085246.28735-1-i.dyukov@samsung.com> <20200811085246.28735-3-i.dyukov@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20200811085246.28735-3-i.dyukov@samsung.com> Subject: Re: [dpdk-dev] [PATCH v9 02/24] ethdev: add a link status text representation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 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