From mboxrd@z Thu Jan 1 00:00:00 1970 From: "David Harton (dharton)" Subject: Re: [RFC PATCH v1 1/3] rte: change xstats to use integer keys Date: Fri, 29 Apr 2016 13:17:35 +0000 Message-ID: References: <1460731462-21229-1-git-send-email-remy.horton@intel.com> <1460731462-21229-2-git-send-email-remy.horton@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable To: Remy Horton , "dev@dpdk.org" , "Thomas Monjalon" Return-path: Received: from rcdn-iport-1.cisco.com (rcdn-iport-1.cisco.com [173.37.86.72]) by dpdk.org (Postfix) with ESMTP id 78F5E5598 for ; Fri, 29 Apr 2016 15:17:38 +0200 (CEST) In-Reply-To: <1460731462-21229-2-git-send-email-remy.horton@intel.com> Content-Language: en-US List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Remy Horton > Sent: Friday, April 15, 2016 10:44 AM > To: dev@dpdk.org; Thomas Monjalon > Subject: [dpdk-dev] [RFC PATCH v1 1/3] rte: change xstats to use integer > keys >=20 > Signed-off-by: Remy Horton > --- > lib/librte_ether/rte_ethdev.c | 87 > +++++++++++++++++++++++++++++++++++++++---- > lib/librte_ether/rte_ethdev.h | 38 +++++++++++++++++++ > 2 files changed, 117 insertions(+), 8 deletions(-) >=20 > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.= c > index a31018e..cdd0685 100644 > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -1507,6 +1507,82 @@ rte_eth_stats_reset(uint8_t port_id) > dev->data->rx_mbuf_alloc_failed =3D 0; > } >=20 > +static int > +rte_eth_xstats_count(uint8_t port_id) Thanks for adding this. I believe an overt API is much more clear. > +{ > + struct rte_eth_dev *dev; > + int count; > + > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL); > + dev =3D &rte_eth_devices[port_id]; > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->xstats_names, -ENOTSUP); > + count =3D (*dev->dev_ops->xstats_names)(dev, NULL, 0); > + if (count >=3D 0) { > + count +=3D RTE_NB_STATS; > + count +=3D dev->data->nb_rx_queues * RTE_NB_RXQ_STATS; > + count +=3D dev->data->nb_tx_queues * RTE_NB_TXQ_STATS; > + } > + return count; > +} > + > +int > +rte_eth_xstats_names(uint8_t port_id, struct rte_eth_xstats_name > *ptr_names, > + unsigned limit) > +{ > + struct rte_eth_dev *dev; > + int cnt_used_entries; > + int cnt_expected_entries; > + uint32_t idx, id_queue; > + int offset; > + > + cnt_expected_entries =3D rte_eth_xstats_count(port_id); > + if (cnt_expected_entries < 0 || ptr_names =3D=3D NULL) > + return cnt_expected_entries; I suggest we don't provide two ways to get the number of stats and=20 that users always call rte_eth_xstats_count(). Recommend returning -EINVAL if ptr_names is NULL. > + > + if ((int)limit < cnt_expected_entries) > + return -ERANGE; > + > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL); > + dev =3D &rte_eth_devices[port_id]; > + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->xstats_names, -ENOTSUP); I think this check is too restrictive. There are drivers that do not=20 provide device specific xstats today but the xstats API will still=20 return the per q stats. Recommend skipping the device specific steps that follow. > + cnt_used_entries =3D (*dev->dev_ops->xstats_names)( > + dev, ptr_names, limit); > + > + if (cnt_used_entries < 0) > + return cnt_used_entries; > + > + offset =3D cnt_used_entries * RTE_ETH_XSTATS_NAME_SIZE; > + for (idx =3D 0; idx < RTE_NB_STATS; idx++) { > + snprintf(ptr_names[cnt_used_entries].name, > + sizeof(ptr_names[0].name), > + "%s", rte_stats_strings[idx].name); > + offset +=3D RTE_ETH_XSTATS_NAME_SIZE; > + cnt_used_entries++; > + } > + for (id_queue =3D 0; id_queue < dev->data->nb_rx_queues; id_queue++) { > + for (idx =3D 0; idx < RTE_NB_RXQ_STATS; idx++) { > + snprintf(ptr_names[cnt_used_entries].name, > + sizeof(ptr_names[0].name), > + "rx_q%u%s", > + id_queue, rte_rxq_stats_strings[idx].name); > + offset +=3D RTE_ETH_XSTATS_NAME_SIZE; > + cnt_used_entries++; > + } > + > + } > + for (id_queue =3D 0; id_queue < dev->data->nb_tx_queues; id_queue++) { > + for (idx =3D 0; idx < RTE_NB_TXQ_STATS; idx++) { > + snprintf(ptr_names[cnt_used_entries].name, > + sizeof(ptr_names[0].name), > + "tx_q%u%s", > + id_queue, rte_txq_stats_strings[idx].name); > + offset +=3D RTE_ETH_XSTATS_NAME_SIZE; > + cnt_used_entries++; > + } > + } > + return cnt_used_entries; > +} > + > /* retrieve ethdev extended statistics */ > int > rte_eth_xstats_get(uint8_t port_id, struct rte_eth_xstats *xstats, > @@ -1551,8 +1627,7 @@ rte_eth_xstats_get(uint8_t port_id, struct > rte_eth_xstats *xstats, > stats_ptr =3D RTE_PTR_ADD(ð_stats, > rte_stats_strings[i].offset); > val =3D *stats_ptr; > - snprintf(xstats[count].name, sizeof(xstats[count].name), > - "%s", rte_stats_strings[i].name); > + xstats[count].key =3D count + xcount; Suggest setting adding: xstats[count].name[0] =3D '\0' until name is removed. > xstats[count++].value =3D val; > } >=20 > @@ -1563,9 +1638,7 @@ rte_eth_xstats_get(uint8_t port_id, struct > rte_eth_xstats *xstats, > rte_rxq_stats_strings[i].offset + > q * sizeof(uint64_t)); > val =3D *stats_ptr; > - snprintf(xstats[count].name, sizeof(xstats[count].name), > - "rx_q%u_%s", q, > - rte_rxq_stats_strings[i].name); > + xstats[count].key =3D count + xcount; Same name comment. > xstats[count++].value =3D val; > } > } > @@ -1577,9 +1650,7 @@ rte_eth_xstats_get(uint8_t port_id, struct > rte_eth_xstats *xstats, > rte_txq_stats_strings[i].offset + > q * sizeof(uint64_t)); > val =3D *stats_ptr; > - snprintf(xstats[count].name, sizeof(xstats[count].name), > - "tx_q%u_%s", q, > - rte_txq_stats_strings[i].name); Same name comment. > + xstats[count].key =3D count + xcount; > xstats[count++].value =3D val; > } > } > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.= h > index 022733e..4b81c59 100644 > --- a/lib/librte_ether/rte_ethdev.h > +++ b/lib/librte_ether/rte_ethdev.h > @@ -941,10 +941,23 @@ struct rte_eth_txq_info { > * structure. > */ > struct rte_eth_xstats { > + /* FIXME: Remove name[] once remaining drivers converted */ > char name[RTE_ETH_XSTATS_NAME_SIZE]; > + uint64_t key; > uint64_t value; > }; >=20 > +/** > + * A name-key lookup element for extended statistics. > + * > + * This structure is used to map between names and ID numbers > + * for extended ethernet statistics. > + */ > +struct rte_eth_xstats_name { > + char name[RTE_ETH_XSTATS_NAME_SIZE]; > + uint64_t id; Suggest naming this key as well or modifying rte_eth_xstats to use id. > +}; > + > #define ETH_DCB_NUM_TCS 8 > #define ETH_MAX_VMDQ_POOL 64 >=20 > @@ -1080,6 +1093,10 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev > *dev, > typedef void (*eth_xstats_reset_t)(struct rte_eth_dev *dev); > /**< @internal Reset extended stats of an Ethernet device. */ >=20 > +typedef int (*eth_xstats_names_t)(struct rte_eth_dev *dev, > + struct rte_eth_xstats_name *ptr_names, unsigned limit); > +/**< @internal Get names of extended stats of an Ethernet device. */ > + > typedef int (*eth_queue_stats_mapping_set_t)(struct rte_eth_dev *dev, > uint16_t queue_id, > uint8_t stat_idx, > @@ -1427,6 +1444,8 @@ struct eth_dev_ops { > eth_stats_reset_t stats_reset; /**< Reset generic device > statistics. */ > eth_xstats_get_t xstats_get; /**< Get extended device > statistics. */ > eth_xstats_reset_t xstats_reset; /**< Reset extended device > statistics. */ > + eth_xstats_names_t xstats_names; > + /**< Get names of extended statistics. */ > eth_queue_stats_mapping_set_t queue_stats_mapping_set; > /**< Configure per queue stat counter mapping. */ > eth_dev_infos_get_t dev_infos_get; /**< Get device info. */ > @@ -2279,6 +2298,25 @@ int rte_eth_stats_get(uint8_t port_id, struct > rte_eth_stats *stats); > void rte_eth_stats_reset(uint8_t port_id); >=20 > /** > + * Retrieve names of extended statistics of an Ethernet device. > + * > + * Names within ptr_strings will be aligned to RTE_ETH_XSTATS_NAME_SIZE > and > + * will be listed in ascending mapping order. > + * > + * @param port_id > + * The port identifier of the Ethernet device. > + * @param ptr_names > + * Block of memory to insert names into. Must be at least limit in size= . > + * If NULL, function returns how many statistics are available. > + * @param limit > + * Capacity of ptr_strings (number of names). Ignored if ptr_string is > NULL. > + * @return > + * If successful, number of statistics; negative on error. > + */ > +int rte_eth_xstats_names(uint8_t port_id, struct rte_eth_xstats_name > *ptr_names, > + unsigned limit); > + > +/** > * Retrieve extended statistics of an Ethernet device. > * > * @param port_id > -- > 2.5.5 Nice work. Regards, Dave