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

[Van Haaren, Harry]

> -----Original Message-----
> From: Horton, Remy
> Sent: Tuesday, January 17, 2017 1:40 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>; dev at dpdk.org
> Cc: Thomas Monjalon <thomas.monjalon at 6wind.com>
> Subject: Re: [dpdk-dev] [PATCH v7 1/6] lib: add information metrics library
> 
> 
> 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.

OK.

> >> +	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.

Ah ok - apologies I missed that the lock is in the memory itself.

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

Great

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

Cool.


> > 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


That confirms my understanding - thanks.