From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 53FC9106B for ; Fri, 3 Jul 2015 15:19:13 +0200 (CEST) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1ZB0wl-0006Kl-Pa; Fri, 03 Jul 2015 15:24:06 +0200 Message-ID: <55968BC8.1060704@6wind.com> Date: Fri, 03 Jul 2015 15:19:04 +0200 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.7.0 MIME-Version: 1.0 To: Maryam Tahhan , dev@dpdk.org References: <1435323558-169985-1-git-send-email-maryam.tahhan@intel.com> <1435323558-169985-3-git-send-email-maryam.tahhan@intel.com> <55968B1C.6070405@6wind.com> In-Reply-To: <55968B1C.6070405@6wind.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v3 2/7] ixgbe: add functions to get and reset xstats X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 03 Jul 2015 13:19:13 -0000 On 07/03/2015 03:16 PM, Olivier MATZ wrote: > Hi Maryam, > > On 06/26/2015 02:59 PM, Maryam Tahhan wrote: >> Implement ixgbe_dev_xstats_reset and ixgbe_dev_xstats_get. >> >> Signed-off-by: Maryam Tahhan >> --- >> drivers/net/ixgbe/ixgbe_ethdev.c | 85 ++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 85 insertions(+) >> >> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c >> index 927021f..0f62bcb 100644 >> --- a/drivers/net/ixgbe/ixgbe_ethdev.c >> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c >> @@ -131,7 +131,10 @@ static int ixgbe_dev_link_update(struct rte_eth_dev *dev, >> int wait_to_complete); >> static void ixgbe_dev_stats_get(struct rte_eth_dev *dev, >> struct rte_eth_stats *stats); >> +static int ixgbe_dev_xstats_get(struct rte_eth_dev *dev, >> + struct rte_eth_xstats *xstats, unsigned n); >> static void ixgbe_dev_stats_reset(struct rte_eth_dev *dev); >> +static void ixgbe_dev_xstats_reset(struct rte_eth_dev *dev); >> static int ixgbe_dev_queue_stats_mapping_set(struct rte_eth_dev *eth_dev, >> uint16_t queue_id, >> uint8_t stat_idx, >> @@ -334,7 +337,9 @@ static const struct eth_dev_ops ixgbe_eth_dev_ops = { >> .allmulticast_disable = ixgbe_dev_allmulticast_disable, >> .link_update = ixgbe_dev_link_update, >> .stats_get = ixgbe_dev_stats_get, >> + .xstats_get = ixgbe_dev_xstats_get, >> .stats_reset = ixgbe_dev_stats_reset, >> + .xstats_reset = ixgbe_dev_xstats_reset, >> .queue_stats_mapping_set = ixgbe_dev_queue_stats_mapping_set, >> .dev_infos_get = ixgbe_dev_info_get, >> .mtu_set = ixgbe_dev_mtu_set, >> @@ -414,6 +419,33 @@ static const struct eth_dev_ops ixgbevf_eth_dev_ops = { >> .set_mc_addr_list = ixgbe_dev_set_mc_addr_list, >> }; >> >> +/* store statistics names and its offset in stats structure */ struct >> +rte_ixgbe_xstats_name_off { >> + char name[RTE_ETH_XSTATS_NAME_SIZE]; >> + unsigned offset; >> +}; >> + >> +static const struct rte_ixgbe_xstats_name_off rte_ixgbe_stats_strings[] = { >> + {"rx_illegal_byte_err", offsetof(struct ixgbe_hw_stats, errbc)}, >> + {"rx_len_err", offsetof(struct ixgbe_hw_stats, rlec)}, >> + {"rx_undersize_count", offsetof(struct ixgbe_hw_stats, ruc)}, >> + {"rx_oversize_count", offsetof(struct ixgbe_hw_stats, roc)}, >> + {"rx_fragment_count", offsetof(struct ixgbe_hw_stats, rfc)}, >> + {"rx_jabber_count", offsetof(struct ixgbe_hw_stats, rjc)}, >> + {"l3_l4_xsum_error", offsetof(struct ixgbe_hw_stats, xec)}, >> + {"mac_local_fault", offsetof(struct ixgbe_hw_stats, mlfc)}, >> + {"mac_remote_fault", offsetof(struct ixgbe_hw_stats, mrfc)}, >> + {"mac_short_pkt_discard", offsetof(struct ixgbe_hw_stats, mspdc)}, >> + {"fccrc_error", offsetof(struct ixgbe_hw_stats, fccrc)}, >> + {"fcoe_drop", offsetof(struct ixgbe_hw_stats, fcoerpdc)}, >> + {"fc_last_error", offsetof(struct ixgbe_hw_stats, fclast)}, >> + {"rx_broadcast_packets", offsetof(struct ixgbe_hw_stats, bprc)}, >> + {"mgmt_pkts_dropped", offsetof(struct ixgbe_hw_stats, mngpdc)}, >> +}; >> + >> +#define RTE_NB_XSTATS (sizeof(rte_ixgbe_stats_strings) / \ >> + sizeof(rte_ixgbe_stats_strings[0])) >> + > > Maybe RTE_NB_XSTATS should be renamed in IXGBE_NB_XSTATS? > > >> /** >> * Atomically reads the link status information from global >> * structure rte_eth_dev. >> @@ -1982,6 +2014,59 @@ ixgbe_dev_stats_reset(struct rte_eth_dev *dev) >> memset(stats, 0, sizeof(*stats)); >> } >> >> +static int >> +ixgbe_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstats *xstats, >> + unsigned n) >> +{ >> + struct ixgbe_hw *hw = >> + IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private); >> + struct ixgbe_hw_stats *hw_stats = >> + IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private); >> + uint64_t total_missed_rx, total_qbrc, total_qprc, total_qprdc; >> + uint64_t rxnfgpc, txdgpc; >> + unsigned i, count = RTE_NB_XSTATS; >> + >> + if (n == 0) >> + return count; > > I think it does not exactly matches the API described in rte_ethdev.h: > > * @return > * - positive value lower or equal to n: success. The return value > * is the number of entries filled in the stats table. > * - positive value higher than n: 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. > * - negative value on error (invalid port id) > > I think it should be: > > if (n < count) > return count > > >> + >> + total_missed_rx = 0; >> + total_qbrc = 0; >> + total_qprc = 0; >> + total_qprdc = 0; >> + rxnfgpc = 0; >> + txdgpc = 0; >> + count = 0; >> + >> + ixgbe_read_stats_registers(hw, hw_stats, &total_missed_rx, &total_qbrc, >> + &total_qprc, &rxnfgpc, &txdgpc, &total_qprdc); >> + >> + if (!xstats) >> + return 0; > > this cannot happen except if n == 0. > This condition is already tested above, and "count" should be returned. > >> + >> + /* Error stats */ >> + for (i = 0; i < RTE_NB_XSTATS; i++) { >> + snprintf(xstats[count].name, sizeof(xstats[count].name), >> + "%s", rte_ixgbe_stats_strings[i].name); >> + xstats[count++].value = *(uint64_t *)(((char *)hw_stats) + >> + rte_ixgbe_stats_strings[i].offset); >> + } >> + >> + return count; >> +} > > Shouldn't it be xstats[i] instead of xstats[count] ? > > Does it work when using "show port in test-pmd"? ok I missed the 'count = 0' above. So why using count instead of i ? Also, I think it would be clearer to use the constant IXGBE_NB_XSTATS instead of count at the beginning of the function. > >> + >> +static void >> +ixgbe_dev_xstats_reset(struct rte_eth_dev *dev) >> +{ >> + struct ixgbe_hw_stats *stats = >> + IXGBE_DEV_PRIVATE_TO_STATS(dev->data->dev_private); >> + >> + /* HW registers are cleared on read */ >> + ixgbe_dev_xstats_get(dev, NULL, RTE_NB_XSTATS); >> + >> + /* Reset software totals */ >> + memset(stats, 0, sizeof(*stats)); >> +} >> + >> static void >> ixgbevf_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats) >> { >>