[dpdk-dev] [PATCH v7 1/6] lib: add information metrics library

[Remy Horton]

On 17/01/2017 11:01, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Remy Horton
[..]
> Nit: why the _s in rte_metrics_meta_s? Is there a reason why struct
> rte_metrics_meta {  is not suitable? Same question for
> rte_metrics_data_s.

Think it was originally to avoid a namespace collision, and I left the 
suffix in because this is an internal data-structure.


>> +	if (memzone == NULL)
>> +		rte_exit(EXIT_FAILURE, "Unable to allocate stats memzone\n");
>> +	stats = memzone->addr;
>> +	memset(stats, 0, sizeof(struct rte_metrics_data_s));
>> +	rte_spinlock_init(&stats->lock);
>
> Should this spinlock be initialized before the creation of the
> memzone? Creating the memzone first, and later initializing the
> locking-structure for it feels racey to me. The lock should probably
> be taken and unlocked again after init and memset.

Problem is the lock being part of the reserved memzone allocation. It is 
in there so that secondary processes can use the lock.


> Nit: would rte_metrics_reg_name() be a better function name?
[..]
> Nit: would rte_metrics_reg_names() be a better function name?

Agree. Done.


> Would rte_metrics_update_single() be a better function name?
[..]
> Would rte_metrics_update() be a better function name?

Think rte_metrics_update_value & rte_metrics_update_values would make a 
better pair. My preference at the moment is not to nominate a preferred one.


>> +	if (port_id != RTE_METRICS_GLOBAL &&
>> +			(port_id < 0 || port_id > RTE_MAX_ETHPORTS))
>> +		return -EINVAL;
>> +
>> +	rte_metrics_init();
>
> See above comments on rte_metrics_init() taking a socket_id parameter. Here any core could call update_metrics(), and if the library was not yet initialized, the memory for metrics would end up on the socket of this core. This should be avoided by
> A) adding socket_id parameter to the metrics_init() function
> B) Demanding that metrics_init() is called by the application before any core uses update_metrics()

Done. Should also help alleviate the race.


> What is a "set border"? I think this ensures that one set of metrics
> cannot overwrite the index's of another metrics set - correct?

It is intended to stop things like:

  idx1 = rte_metrics_reg_names(some_names, 2);
  idx2 = rte_metrics_reg_names(more_names, 3);
   ...
  rte_metrics_update_values(port_id, idx1, &new_values, 5);

as the above assumes idx2=idx1+2 which is not guaranteed in concurrent 
enviornments

..Remy