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.dyu...@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
>
