From 8c49d5f1c219d986bb129c61dfa2e155d7f2b1da Mon Sep 17 00:00:00 2001 From: Lee Daly Date: Thu, 12 Oct 2017 14:31:28 +0100 Subject: [PATCH] ethdev: rework xstats retrieve by id MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Fix xstats functions, rte_eth_xstats_get_names_by_id() and rte_eth_xstats_get_by_id(), in current implementation ethdev level reads all xstat values and filters out the ones requested by the application. This behavior doesn't benefit from PMD ops and doesn't provide the benefit the API was created in the first place for. APIs are also unnecessarily complicated. Both APIs have different returns for the same params. In this fix, instead of reading all the stats and finding the requested value, drivers can provide ops to get selected xstats. API no longer crashes with certain params, rte_eth_get_by_id returned seg fault with "ids = NULL && values != NULL && n Reviewed-by: Ferruh Yigit --- lib/librte_ether/rte_ethdev.c | 291 ++++++++++------------------------ lib/librte_ether/rte_ethdev.h | 10 +- 2 files changed, 87 insertions(+), 214 deletions(-) diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index 8f027c71b3..0b1e928949 100644 --- a/lib/librte_ether/rte_ethdev.c +++ b/lib/librte_ether/rte_ethdev.c @@ -1623,121 +1623,69 @@ rte_eth_xstats_get_id_by_name(uint16_t port_id, const char *xstat_name, return -EINVAL; } +/* retrieve ethdev extended statistics names */ int rte_eth_xstats_get_names_by_id(uint16_t port_id, struct rte_eth_xstat_name *xstats_names, unsigned int size, uint64_t *ids) { - /* Get all xstats */ + struct rte_eth_xstat_name *xstats_names_copy; + unsigned int expected_entries; + struct rte_eth_dev *dev; + unsigned int i; + + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); + expected_entries = get_xstats_count(port_id); + dev = &rte_eth_devices[port_id]; + + /* Return max number of stats if no ids given */ if (!ids) { - struct rte_eth_dev *dev; - int cnt_used_entries; - int cnt_expected_entries; - int cnt_driver_entries; - uint32_t idx, id_queue; - uint16_t num_q; - - cnt_expected_entries = get_xstats_count(port_id); - if (xstats_names == NULL || cnt_expected_entries < 0 || - (int)size < cnt_expected_entries) - return cnt_expected_entries; - - /* port_id checked in get_xstats_count() */ - dev = &rte_eth_devices[port_id]; - cnt_used_entries = 0; - - for (idx = 0; idx < RTE_NB_STATS; idx++) { - snprintf(xstats_names[cnt_used_entries].name, - sizeof(xstats_names[0].name), - "%s", rte_stats_strings[idx].name); - cnt_used_entries++; - } - num_q = RTE_MIN(dev->data->nb_rx_queues, - RTE_ETHDEV_QUEUE_STAT_CNTRS); - for (id_queue = 0; id_queue < num_q; id_queue++) { - for (idx = 0; idx < RTE_NB_RXQ_STATS; idx++) { - snprintf(xstats_names[cnt_used_entries].name, - sizeof(xstats_names[0].name), - "rx_q%u%s", - id_queue, - rte_rxq_stats_strings[idx].name); - cnt_used_entries++; - } + if (!xstats_names) + return expected_entries; + else if (xstats_names && size < expected_entries) + return expected_entries; + } - } - num_q = RTE_MIN(dev->data->nb_tx_queues, - RTE_ETHDEV_QUEUE_STAT_CNTRS); - for (id_queue = 0; id_queue < num_q; id_queue++) { - for (idx = 0; idx < RTE_NB_TXQ_STATS; idx++) { - snprintf(xstats_names[cnt_used_entries].name, - sizeof(xstats_names[0].name), - "tx_q%u%s", - id_queue, - rte_txq_stats_strings[idx].name); - cnt_used_entries++; - } - } + if (ids && !xstats_names) + return -EINVAL; - if (dev->dev_ops->xstats_get_names_by_id != NULL) { - /* If there are any driver-specific xstats, append them - * to end of list. - */ - cnt_driver_entries = - (*dev->dev_ops->xstats_get_names_by_id)( - dev, - xstats_names + cnt_used_entries, - NULL, - size - cnt_used_entries); - if (cnt_driver_entries < 0) - return cnt_driver_entries; - cnt_used_entries += cnt_driver_entries; - - } else if (dev->dev_ops->xstats_get_names != NULL) { - /* If there are any driver-specific xstats, append them - * to end of list. - */ - cnt_driver_entries = (*dev->dev_ops->xstats_get_names)( - dev, - xstats_names + cnt_used_entries, - size - cnt_used_entries); - if (cnt_driver_entries < 0) - return cnt_driver_entries; - cnt_used_entries += cnt_driver_entries; - } + if (dev->dev_ops->xstats_get_names_by_id != NULL) + return (*dev->dev_ops->xstats_get_names_by_id)( + dev, xstats_names, ids, size); - return cnt_used_entries; + /* Retrieve all stats */ + if (!ids) { + int num_stats = rte_eth_xstats_get_names(port_id, xstats_names, + expected_entries); + if (num_stats < 0 || num_stats > (int)expected_entries) + return num_stats; + else + return expected_entries; } - /* Get only xstats given by IDS */ - else { - uint16_t len, i; - struct rte_eth_xstat_name *xstats_names_copy; - len = rte_eth_xstats_get_names_by_id(port_id, NULL, 0, NULL); + xstats_names_copy = calloc(expected_entries, + sizeof(struct rte_eth_xstat_name)); - xstats_names_copy = - malloc(sizeof(struct rte_eth_xstat_name) * len); - if (!xstats_names_copy) { - RTE_PMD_DEBUG_TRACE( - "ERROR: can't allocate memory for values_copy\n"); + if (!xstats_names_copy) { + RTE_PMD_DEBUG_TRACE("ERROR: can't allocate memory"); + return -ENOMEM; + } + + /* Fill xstats_names_copy structure */ + rte_eth_xstats_get_names(port_id, xstats_names_copy, expected_entries); + + /* Filter stats */ + for (i = 0; i < size; i++) { + if (ids[i] >= expected_entries) { + RTE_PMD_DEBUG_TRACE("ERROR: id value isn't valid\n"); free(xstats_names_copy); return -1; } - - rte_eth_xstats_get_names_by_id(port_id, xstats_names_copy, - len, NULL); - - for (i = 0; i < size; i++) { - if (ids[i] >= len) { - RTE_PMD_DEBUG_TRACE( - "ERROR: id value isn't valid\n"); - return -1; - } - strcpy(xstats_names[i].name, - xstats_names_copy[ids[i]].name); - } - free(xstats_names_copy); - return size; + xstats_names[i] = xstats_names_copy[ids[i]]; } + + free(xstats_names_copy); + return size; } int @@ -1808,128 +1756,53 @@ rte_eth_xstats_get_names(uint16_t port_id, /* retrieve ethdev extended statistics */ int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids, - uint64_t *values, unsigned int n) + uint64_t *values, unsigned int size) { - /* If need all xstats */ - if (!ids) { - struct rte_eth_stats eth_stats; - struct rte_eth_dev *dev; - unsigned int count = 0, i, q; - signed int xcount = 0; - uint64_t val, *stats_ptr; - uint16_t nb_rxqs, nb_txqs; - - RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); - dev = &rte_eth_devices[port_id]; - - nb_rxqs = RTE_MIN(dev->data->nb_rx_queues, - RTE_ETHDEV_QUEUE_STAT_CNTRS); - nb_txqs = RTE_MIN(dev->data->nb_tx_queues, - RTE_ETHDEV_QUEUE_STAT_CNTRS); - - /* Return generic statistics */ - count = RTE_NB_STATS + (nb_rxqs * RTE_NB_RXQ_STATS) + - (nb_txqs * RTE_NB_TXQ_STATS); - - - /* implemented by the driver */ - if (dev->dev_ops->xstats_get_by_id != NULL) { - /* Retrieve the xstats from the driver at the end of the - * xstats struct. Retrieve all xstats. - */ - xcount = (*dev->dev_ops->xstats_get_by_id)(dev, - NULL, - values ? values + count : NULL, - (n > count) ? n - count : 0); - - if (xcount < 0) - return xcount; - /* implemented by the driver */ - } else if (dev->dev_ops->xstats_get != NULL) { - /* Retrieve the xstats from the driver at the end of the - * xstats struct. Retrieve all xstats. - * Compatibility for PMD without xstats_get_by_ids - */ - unsigned int size = (n > count) ? n - count : 1; - struct rte_eth_xstat xstats[size]; - - xcount = (*dev->dev_ops->xstats_get)(dev, - values ? xstats : NULL, size); - - if (xcount < 0) - return xcount; - - if (values != NULL) - for (i = 0 ; i < (unsigned int)xcount; i++) - values[i + count] = xstats[i].value; - } + unsigned int num_xstats_filled; + uint16_t expected_entries; + struct rte_eth_dev *dev; + unsigned int i; - if (n < count + xcount || values == NULL) - return count + xcount; + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); + expected_entries = get_xstats_count(port_id); + struct rte_eth_xstat xstats[expected_entries]; + dev = &rte_eth_devices[port_id]; - /* now fill the xstats structure */ - count = 0; - rte_eth_stats_get(port_id, ð_stats); + /* Return max number of stats if no ids given */ + if (!ids) { + if (!values) + return expected_entries; + else if (values && size < expected_entries) + return expected_entries; + } - /* global stats */ - for (i = 0; i < RTE_NB_STATS; i++) { - stats_ptr = RTE_PTR_ADD(ð_stats, - rte_stats_strings[i].offset); - val = *stats_ptr; - values[count++] = val; - } + if (ids && !values) + return -EINVAL; - /* per-rxq stats */ - for (q = 0; q < nb_rxqs; q++) { - for (i = 0; i < RTE_NB_RXQ_STATS; i++) { - stats_ptr = RTE_PTR_ADD(ð_stats, - rte_rxq_stats_strings[i].offset + - q * sizeof(uint64_t)); - val = *stats_ptr; - values[count++] = val; - } - } + if (dev->dev_ops->xstats_get_by_id != NULL) + return (*dev->dev_ops->xstats_get_by_id)(dev, ids, values, + size); - /* per-txq stats */ - for (q = 0; q < nb_txqs; q++) { - for (i = 0; i < RTE_NB_TXQ_STATS; i++) { - stats_ptr = RTE_PTR_ADD(ð_stats, - rte_txq_stats_strings[i].offset + - q * sizeof(uint64_t)); - val = *stats_ptr; - values[count++] = val; - } - } + /* Fill the xstats structure */ + num_xstats_filled = rte_eth_xstats_get(port_id, xstats, + expected_entries); - return count + xcount; + /* Return all stats */ + if (!ids) { + for (i = 0; i < num_xstats_filled; i++) + values[i] = xstats[i].value; + return expected_entries; } - /* Need only xstats given by IDS array */ - else { - uint16_t i, size; - uint64_t *values_copy; - - size = rte_eth_xstats_get_by_id(port_id, NULL, NULL, 0); - values_copy = malloc(sizeof(*values_copy) * size); - if (!values_copy) { - RTE_PMD_DEBUG_TRACE( - "ERROR: can't allocate memory for values_copy\n"); + /* Filter stats */ + for (i = 0; i < size; i++) { + if (ids[i] >= expected_entries) { + RTE_PMD_DEBUG_TRACE("ERROR: id value isn't valid\n"); return -1; } - - rte_eth_xstats_get_by_id(port_id, NULL, values_copy, size); - - for (i = 0; i < n; i++) { - if (ids[i] >= size) { - RTE_PMD_DEBUG_TRACE( - "ERROR: id value isn't valid\n"); - return -1; - } - values[i] = values_copy[ids[i]]; - } - free(values_copy); - return n; + values[i] = xstats[ids[i]].value; } + return size; } int diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index 3f19117b2c..aaf02b33d3 100644 --- a/lib/librte_ether/rte_ethdev.h +++ b/lib/librte_ether/rte_ethdev.h @@ -2462,23 +2462,23 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id, * @param ids * A pointer to an ids array passed by application. This tells which * statistics values function should retrieve. This parameter - * can be set to NULL if n is 0. In this case function will retrieve + * can be set to NULL if size is 0. In this case function will retrieve * all avalible statistics. * @param values * A pointer to a table to be filled with device statistics values. - * @param n + * @param size * The size of the ids array (number of elements). * @return - * - A positive value lower or equal to n: success. The return value + * - A positive value lower or equal to size: success. The return value * is the number of entries filled in the stats table. - * - A positive value higher than n: error, the given statistics table + * - A positive value higher than size: error, the given statistics table * is too small. The return value corresponds to the size that should * be given to succeed. The entries in the table are not valid and * shall not be used by the caller. * - A negative value on error (invalid port id). */ int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids, - uint64_t *values, unsigned int n); + uint64_t *values, unsigned int size); /** * Gets the ID of a statistic from its name. -- 2.20.1