Re: [dpdk-dev] [PATCH 02/11] telemetry: add initial connection socket

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Tuesday, August 28, 2018 5:40 PM
> To: Power, Ciara <ciara.power@intel.com>
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Archbold, Brian
> <brian.archbold@intel.com>; Kenny, Emma <emma.kenny@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 02/11] telemetry: add initial connection
> socket

<snip>

> > +int32_t
> > +rte_telemetry_check_port_activity(int port_id)
> > +{
> > +	int pid;
> > +
> > +	RTE_ETH_FOREACH_DEV(pid) {
> > +		if (pid == port_id)
> > +			return 1;
> > +	}
> > +	TELEMETRY_LOG_ERR("Error - port_id: %d is invalid, not active\n",
> > +		port_id);
> > +	return 0;
> > +}
> > +
> 
> This function seems more about ethdev than telemetry.
> Maybe add it as part of librte_ethdev.

Yep that might be a better place, making it a generic ethdev function.


> The "active" semantic is blurry however. With this implementation, this
> is checking that the device is currently attached and owned by the
> default user. Should telemetry be limited to devices owned by default
> user?

Good question - perhaps one that we can get input on from others.
Would you (app X) want App Y to read telemetry from your ethdev/cryptodev?

Perhaps keeping telemetry to an "owned" port is a feature, perhaps
a limitation.

I'd like input on this one :D


> Also, this function does not seem used in this patch, can it be added
> when used?

Sure.


> > +static int32_t
> > +rte_telemetry_reg_ethdev_to_metrics(uint16_t port_id)
> 
> "_to" might not be necessary.
> 
> > +{
> > +	int ret, num_xstats, start_index, i;
> > +	struct rte_eth_xstat *eth_xstats;
> > +
> > +	if (!rte_eth_dev_is_valid_port(port_id)) {
> > +		TELEMETRY_LOG_ERR("Error - port_id: %d is invalid\n", port_id);
> > +		return -EINVAL;
> > +	}
> > +
> > +	num_xstats = rte_eth_xstats_get(port_id, NULL, 0);
> > +	if (num_xstats < 0) {
> > +		TELEMETRY_LOG_ERR("Error - rte_eth_xstats_get(%u) failed:"
> > +			" %d\n", port_id, num_xstats);
> 
> I guess there isn't really a consensus yet, as the checkpatch.sh tool
> might be misconfigured, but the cesura is very awkward here.
> 
> Same for other logs.

Agreed, improvements possible here.


> > +		return -EPERM;
> > +	}
> > +
> > +	eth_xstats = malloc(sizeof(struct rte_eth_xstat) * num_xstats);
> > +	if (eth_xstats == NULL) {
> > +		TELEMETRY_LOG_ERR("Error - Failed to malloc memory for"
> > +			" xstats\n");
> > +		return -ENOMEM;
> > +	}
> > +	ret = rte_eth_xstats_get(port_id, eth_xstats, num_xstats);
> > +	if (ret < 0 || ret > num_xstats) {
> > +		free(eth_xstats);
> > +		TELEMETRY_LOG_ERR("Error - rte_eth_xstats_get(%u) len%i"
> > +			" failed: %d\n", port_id, num_xstats, ret);
> > +		return -EPERM;
> > +	}
> > +	struct rte_eth_xstat_name *eth_xstats_names;
> > +	eth_xstats_names = malloc(sizeof(struct rte_eth_xstat_name) *
> > +		 num_xstats);
> > +	if (eth_xstats_names == NULL) {
> 
> You are sometimes checking pointers against NULL, sometimes using "!".
> You can choose either in your library, but it would be better to be
> consistent and use a unified coding style.

Heh, I guess that's down to dev-style, and you'll note multiple sign-offs ;)

Can review for v2.


> > +		free(eth_xstats);
> > +		TELEMETRY_LOG_ERR("Error - Failed to malloc memory for"
> > +			" xstats_names\n");
> > +		return -ENOMEM;
> > +	}
> > +	ret = rte_eth_xstats_get_names(port_id, eth_xstats_names,
> > +		 num_xstats);
> > +	if (ret < 0 || ret > num_xstats) {
> > +		free(eth_xstats);
> > +		free(eth_xstats_names);
> > +		TELEMETRY_LOG_ERR("Error - rte_eth_xstats_get_names(%u)"
> > +			" len%i failed: %d\n", port_id, num_xstats,
> > +				 ret);
> > +		return -EPERM;
> > +	}
> > +	const char *xstats_names[num_xstats];
> > +
> > +	for (i = 0; i < num_xstats; i++)
> > +		xstats_names[i] = eth_xstats_names[eth_xstats[i].id].name;
> > +
> > +	start_index = rte_metrics_reg_names(xstats_names, num_xstats);
> > +
> > +	if (start_index < 0) {
> > +		TELEMETRY_LOG_ERR("Error - rte_metrics_reg_names failed -"
> > +			" metrics may already be registered\n");
> > +		free(eth_xstats);
> > +		free(eth_xstats_names);
> > +		return -1;
> > +	}
> > +	free(eth_xstats_names);
> > +	free(eth_xstats);
> 
> At this point, you have repeated 3 times those frees().
> It would be cleaner to define proper labels and use goto instead.

Agreed.

Thanks for review!