Hi Stephen, Some of your comments are addressed in the next patch, which reduces this code copied from the existing telemetry library. Some others I will fix for v5.
>From: Stephen Hemminger <[email protected]> >Sent: Friday 24 April 2020 16:30 >To: Power, Ciara <[email protected]> >Cc: [email protected]; Laatz, Kevin <[email protected]>; Pattan, Reshma ><[email protected]>; [email protected]; >[email protected]; Wiles, Keith <[email protected]>; >[email protected]; [email protected] >Subject: Re: [dpdk-dev] [PATCH v4 02/18] telemetry: move code to metrics for >later reuse > >On Fri, 24 Apr 2020 13:41:43 +0100 >Ciara Power <[email protected]> wrote: > >> This commit moves some of the telemetry library code to a new file in >> the metrics library. No modifications are made to the moved code, >> except what is needed to allow it to compile and run. The additional >> code in metrics is built only when the Jansson library is present. >> Telemetry functions as normal, using the functions from the >> metrics_telemetry file. This move will enable code be reused by the >> new version of telemetry in a later commit, to support backward >> compatibility with the existing telemetry usage. >> >> Signed-off-by: Ciara Power <[email protected]> > > >Minor comments, none of these are show stoppers. <snip> >> +static int32_t >> +rte_metrics_tel_update_metrics_ethdev(uint16_t port_id, int >> +reg_start_index) { >> + int ret, num_xstats, i; >> + struct rte_eth_xstat *eth_xstats; >> + >> + if (!rte_eth_dev_is_valid_port(port_id)) { >> + METRICS_LOG_ERR("port_id: %d is invalid", port_id); >> + return -EINVAL; >> + } >> + >> + ret = rte_metrics_tel_is_port_active(port_id); >> + if (ret < 1) >> + return -EINVAL; >> + >> + num_xstats = rte_eth_xstats_get(port_id, NULL, 0); >> + if (num_xstats < 0) { >> + METRICS_LOG_ERR("rte_eth_xstats_get(%u) failed: %d", >port_id, >> + num_xstats); >> + return -EPERM; >> + } > >The number of metrics on a port should not change (as long as it has not been >hot plugged). So you could optimize by knowing number of stats from last >query. > Requerying is safer, this code is for backward compatibility and the existing telemetry requeries the number of stats each time. <snip> Thanks, Ciara
