> -----Original Message----- > From: Horton, Remy > Sent: Tuesday, January 17, 2017 1:40 PM > To: Van Haaren, Harry <harry.van.haa...@intel.com>; dev@dpdk.org > Cc: Thomas Monjalon <thomas.monja...@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-boun...@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.
