From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id D0FD55A58 for ; Sun, 5 Jul 2015 11:27:16 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP; 05 Jul 2015 02:27:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,409,1432623600"; d="scan'208";a="519076373" Received: from irsmsx110.ger.corp.intel.com ([163.33.3.25]) by FMSMGA003.fm.intel.com with ESMTP; 05 Jul 2015 02:27:15 -0700 Received: from irsmsx109.ger.corp.intel.com ([169.254.13.200]) by irsmsx110.ger.corp.intel.com ([163.33.3.25]) with mapi id 14.03.0224.002; Sun, 5 Jul 2015 10:27:13 +0100 From: "Tahhan, Maryam" To: Olivier MATZ , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v3 2/7] ixgbe: add functions to get and reset xstats Thread-Index: AQHQsA/02/X1giN3pEarMnxmZb9IlJ3Js/kAgALyedA= Date: Sun, 5 Jul 2015 09:27:12 +0000 Message-ID: <1A27633A6DA49C4A92FCD5D4312DBF536A45ADC2@IRSMSX109.ger.corp.intel.com> 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> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Sun, 05 Jul 2015 09:27:17 -0000 Best Regards,=20 Maryam > -----Original Message----- > From: Olivier MATZ [mailto:olivier.matz@6wind.com] > Sent: Friday, July 3, 2015 2:16 PM > To: Tahhan, Maryam; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v3 2/7] ixgbe: add functions to get and re= set > xstats >=20 > Hi Maryam, >=20 > 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 > =3D { > > .allmulticast_disable =3D ixgbe_dev_allmulticast_disable, > > .link_update =3D ixgbe_dev_link_update, > > .stats_get =3D ixgbe_dev_stats_get, > > + .xstats_get =3D ixgbe_dev_xstats_get, > > .stats_reset =3D ixgbe_dev_stats_reset, > > + .xstats_reset =3D ixgbe_dev_xstats_reset, > > .queue_stats_mapping_set =3D ixgbe_dev_queue_stats_mapping_set, > > .dev_infos_get =3D ixgbe_dev_info_get, > > .mtu_set =3D ixgbe_dev_mtu_set, > > @@ -414,6 +419,33 @@ static const struct eth_dev_ops > ixgbevf_eth_dev_ops =3D { > > .set_mc_addr_list =3D 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[= ] =3D { > > + {"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])) > > + >=20 > Maybe RTE_NB_XSTATS should be renamed in IXGBE_NB_XSTATS? >=20 >=20 > > /** > > * 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 =3D > > + IXGBE_DEV_PRIVATE_TO_HW(dev->data- > >dev_private); > > + struct ixgbe_hw_stats *hw_stats =3D > > + 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 =3D RTE_NB_XSTATS; > > + > > + if (n =3D=3D 0) > > + return count; >=20 > I think it does not exactly matches the API described in rte_ethdev.h: >=20 > * @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) >=20 > I think it should be: >=20 > if (n < count) > return count >=20 I was using 0 on the first call just to return the size of the extended sta= ts structure that needs to be allocated by the application. It's passed as = 0 from ethdev_get_stats to retrieve the size. But I will update it. >=20 > > + > > + total_missed_rx =3D 0; > > + total_qbrc =3D 0; > > + total_qprc =3D 0; > > + total_qprdc =3D 0; > > + rxnfgpc =3D 0; > > + txdgpc =3D 0; > > + count =3D 0; > > + > > + ixgbe_read_stats_registers(hw, hw_stats, &total_missed_rx, > &total_qbrc, > > + &total_qprc, > &rxnfgpc, &txdgpc, &total_qprdc); > > + > > + if (!xstats) > > + return 0; >=20 This is the mechanism used to reset stats for normal stats and xstats. The = reset functions call the stats function with the nullified stats structure.= So I think this is ok to leave in, I will add a comment about it being par= t of the reset functionality > this cannot happen except if n =3D=3D 0. > This condition is already tested above, and "count" should be returned. >=20 > > + > > + /* Error stats */ > > + for (i =3D 0; i < RTE_NB_XSTATS; i++) { > > + snprintf(xstats[count].name, sizeof(xstats[count].name), > > + "%s", rte_ixgbe_stats_strings[i].name); > > + xstats[count++].value =3D *(uint64_t *)(((char *)hw_stats) + > > + > rte_ixgbe_stats_strings[i].offset); > > + } > > + > > + return count; > > +} >=20 > Shouldn't it be xstats[i] instead of xstats[count] ? No, i is just used to cycle through the stats string structure but we use t= he count for xstats because that is the index that is passed in from rte_e= th_xstats_get(). Remember we are filling out the xstats structure with gene= ric stats at the ethdev level first and then passing the xstats structure t= o the driver for the rest of it to be filled out. >=20 > Does it work when using "show port in test-pmd"? Yes, I tested it before patch submission and it works. >=20 > > + > > +static void > > +ixgbe_dev_xstats_reset(struct rte_eth_dev *dev) { > > + struct ixgbe_hw_stats *stats =3D > > + 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) { > >