From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR03-AM5-obe.outbound.protection.outlook.com (mail-eopbgr30064.outbound.protection.outlook.com [40.107.3.64]) by dpdk.org (Postfix) with ESMTP id 1375D2BA1 for ; Thu, 15 Nov 2018 08:39:25 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=NvOcQJuYInuYcgvFNG9V7CAFASX2wLSmKO8KjQ1gOvg=; b=tKu43cfCSM6BO4vOZXh64t6Dbo3v30oS9Ok8rQ39Pxa+IB5rAuC7lVF4L9C1WT1nBHPcITBfGia6AVC0Hi/eUKpTta1CNIxUi9Vcz/UDdAJvDaeSZFE8KYQnVWCERIAmiae6v0wkjR3R8d2Auw+3vxLSepYTAnBmeKCVTE57u+Y= Received: from DB7PR05MB4426.eurprd05.prod.outlook.com (52.134.109.15) by DB7PR05MB4425.eurprd05.prod.outlook.com (52.134.109.14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1294.28; Thu, 15 Nov 2018 07:39:22 +0000 Received: from DB7PR05MB4426.eurprd05.prod.outlook.com ([fe80::bc22:c2f5:208d:826f]) by DB7PR05MB4426.eurprd05.prod.outlook.com ([fe80::bc22:c2f5:208d:826f%2]) with mapi id 15.20.1294.045; Thu, 15 Nov 2018 07:39:22 +0000 From: Shahaf Shuler To: Tom Barbette , "dev@dpdk.org" CC: Yongseok Koh Thread-Topic: [PATCH] mlx5: Report imissed stat Thread-Index: AQHUezn7GdEWLcTKrk2jn2J1oCKc+6VPYTIAgAATqgCAAPtOEA== Date: Thu, 15 Nov 2018 07:39:22 +0000 Message-ID: References: <1542104199-55746-1-git-send-email-barbette@kth.se>, <1542212300053.31332@kth.se> In-Reply-To: <1542212300053.31332@kth.se> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=shahafs@mellanox.com; x-originating-ip: [31.154.10.105] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DB7PR05MB4425; 6:f9EDySTmIBT9u6V6IT2dOoSF3r5r+UoXSW6Tu2TA1RP8ZklA7h7bNqIWqOVk3PKkSr8neBY+MxMtUxwsBGNmyOxKCYHeHc5Gl7Qc7A0n5zw1OltLSJyuln29BA0dB6q0xHz9Ur7w3M7XpMUMtcmtShKmXA7s+jWlcFBsS/4XsTDPR7vIIkj3whhbsnFt4bETU1u2bsokXHyiBZtlQJqucWglpdiwLsVkJuY4AFstNasVkOdn+6CRpbWKZrXa3rvnqaS4IGWa+keB7LozUyoudVOaxv8KAgGlNwAO/j45fh2bxRHtNczrZr2ofjH4VyeUpemHQo5p/K2Jq49BOtdaxJQYbufS08GWyheKImhnTL+0RB63FYAw8mPk788qrXaBdrX2PGUYojzoJvymjEza68Akuj9tN0qXlQrrxNdRTazQejppAPRXjcmVBlZwRGUypo5D9uFUTyOuuvsar9RXQw==; 5:fRrYw5bPusGfoRBpUMlV/FaR4OkRR8Tn0YtKrJsG+Su4ouPWyVUkyeSxcYPdl+LK7pbtmMwJQu59T+0cuZWohRhBYqvp2SXcMH1n3STAGOkVYGlXtZrAu9pswJBJbBjnF6UAv/ODdSLggBttcn1feP37VTcfJIOUGhrQS+q26YQ=; 7:jqUes+YwA+aeX1pPFKDCqly58VlDFilEED1+P+WrTGenPLkKG+ErItKEBB1p9QgTO+Paoy59nsXhTmOaxfytdqtXhx4L3u2bnQNAVgViXkwPv6jBUuVOzfD2nONb0CLmCDl/K0OlDExf3FmtOPU9mA== x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-correlation-id: 169f1fc2-0bef-4fdb-396c-08d64acd75f1 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390098)(7020095)(4652040)(8989299)(5600074)(711020)(4618075)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7153060)(7193020); SRVR:DB7PR05MB4425; x-ms-traffictypediagnostic: DB7PR05MB4425: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(5005006)(8121501046)(3231415)(944501410)(52105112)(93006095)(93001095)(3002001)(10201501046)(6055026)(148016)(149066)(150057)(6041310)(20161123558120)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123564045)(20161123560045)(20161123562045)(201708071742011)(7699051)(76991095); SRVR:DB7PR05MB4425; BCL:0; PCL:0; RULEID:; SRVR:DB7PR05MB4425; x-forefront-prvs: 08572BD77F x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(346002)(376002)(396003)(136003)(39860400002)(366004)(199004)(189003)(476003)(11346002)(2501003)(74316002)(446003)(26005)(71200400001)(2906002)(71190400001)(486006)(186003)(105586002)(478600001)(110136005)(33656002)(66066001)(76176011)(7696005)(102836004)(6506007)(97736004)(106356001)(99286004)(316002)(8676002)(55016002)(68736007)(2900100001)(9686003)(53936002)(6436002)(305945005)(81156014)(14444005)(256004)(81166006)(7736002)(229853002)(551984002)(3846002)(6116002)(86362001)(14454004)(8936002)(5660300001)(4326008)(6246003)(107886003)(25786009); DIR:OUT; SFP:1101; SCL:1; SRVR:DB7PR05MB4425; H:DB7PR05MB4426.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: cErulmDFmenJCzwh66LSm2unlAu3DqgEuY0ypkuF/Hywv/V2e/btMkJVZFDnnl9inKjlZL4O1MqZrSGvvmiiR0ivnId4jBPj/LL0QOOpE0xraI9x+VCiZNVpoPzWIpTbUW7MmKcrh8X7roYCDQXl0E58pcW/k1DrK24YVqLufviCRx3wYOCZn8I1u5jWpRVJYCRxgErSIvl+cjkr4fAjBF4plmGyW99WRdFUcWSfIeu2fvT0HHquSj9g7ClgWM7UvSR+FgOhLI8Jy/ofxdnBBW6Jx9YokuJTp9IFIOf0R1nlKI5Odln0+wpZ7rITP1FwAeCs50y+176nkwDRIyHDPm6LZT0NdlxLPKpjABEukhw= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 169f1fc2-0bef-4fdb-396c-08d64acd75f1 X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Nov 2018 07:39:22.6060 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB7PR05MB4425 Subject: Re: [dpdk-dev] [PATCH] mlx5: Report imissed stat X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Nov 2018 07:39:25 -0000 Wednesday, November 14, 2018 6:18 PM, Tom Barbette: > Subject: RE: [PATCH] mlx5: Report imissed stat >=20 > Hi Shahaf, >=20 > Yes we learned this distinction with rx_discards_phy the hard way. I woul= d > expect imissed to be only rx_out_of_buffer actually. I'd say people look= at > imissed to see if they consume packets fast enough. If the problem is pur= e > CPU power. And that is more the definition of imissed, it is "ie RX queue= s are > full", not "eg". > The *_phy counters are somehow unusual, 82599, xl710 and others would > simply never receive other packets. But I'll follow your lead. >=20 > If documentation of the limitations is okay for you, I can propose a v2, = also > adding rx_discards_phy no matter what in xstats as it is needed for the f= ull > picture. Then the documentation can mention that. OK, let's go this way, since we want to have a consist behavior between the= PMD: 1. the imissed will be only the out_of_buffer 2. the phy drop counters will be reported on xstat only 3. not need to doc the phy limitation since we allready have one. For the v2 please see more comments below=20 >=20 > Tom >=20 >=20 >=20 > ________________________________________ > De : Shahaf Shuler Envoy=E9 : mercredi 14 > novembre 2018 16:17 =C0 : Tom Barbette; dev@dpdk.org Cc : Yongseok Koh > Objet : RE: [PATCH] mlx5: Report imissed stat >=20 > Hi Again Tom, >=20 > Tuesday, November 13, 2018 12:17 PM, Tom Barbette: > > Subject: [PATCH] mlx5: Report imissed stat > > > > The imissed counters (number of packets dropped because the queues > > were > > full) were actually reported through xstats as "rx_out_of_buffer" > > but was not reported through stats. > > > > Following a recent discussion on the ML, as there is no way to tell > > the user if a counter is implemented or not, this should be considered > > a bug. Eg, user looking at imissed will think the packets are lost befo= re > reaching the device. > > > > As for xstats, I added a base counter to be able to "reset" imissed. > > >=20 > Yes, a bit of extra work is needed here. > the full definition of the missed is > " > uint64_t imissed; > /**< Total of RX packets dropped by the HW, > * because there are no available buffer (i.e. RX queues are full). > */ > " >=20 > It is not well defined (this is other topic), because it assumes the only= reason > to drop packets is because there is no avail buffer on the Rx queue. > It is not entirely correct, for example if you have large latency on your= system > it is possible that packets would be dropped on the physical port, not > because the application is slow and not replenish the buffers fast enough= , > rather because the NIC is not processing them on the needed rate. >=20 > I guess what application seek on imissed is the sum of two. It can be don= e by > summing up two counters: the out_of_buffer and the rx_discard_phy (which > exists on ethtool -S and need to be added to mlx5_counters_init > array). >=20 > Still, the output will be incorrect on the following cases: > 1. from VF, since VF cannot read the phy port counters 2. in the presence= of > representors, as they all share the same phy port counter. >=20 > I guess if we want to get such patch in, we need first to make the calcul= ation > correct, and document the limitations. >=20 >=20 > > Signed-off-by: Tom Barbette > > --- > > drivers/net/mlx5/mlx5.h | 2 ++ > > drivers/net/mlx5/mlx5_stats.c | 36 > > +++++++++++++++++++++++------------- > > 2 files changed, 25 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index > > a3a34cf..61054a8 100644 > > --- a/drivers/net/mlx5/mlx5.h > > +++ b/drivers/net/mlx5/mlx5.h > > @@ -77,6 +77,8 @@ struct mlx5_xstats_ctrl { > > /* Index in the device counters table. */ > > uint16_t dev_table_idx[MLX5_MAX_XSTATS]; > > uint64_t base[MLX5_MAX_XSTATS]; > > + /* Base for imissed counter. */ > > + uint64_t imissed_base; Since imissed is part of stats and not xstats it will probably be cleaner t= o have it on a different (new) struct called mlx5_stats_ctrl.=20 > > }; > > > > /* Flow list . */ > > diff --git a/drivers/net/mlx5/mlx5_stats.c > > b/drivers/net/mlx5/mlx5_stats.c index 91f3d47..1e75e85 100644 > > --- a/drivers/net/mlx5/mlx5_stats.c > > +++ b/drivers/net/mlx5/mlx5_stats.c > > @@ -119,6 +119,24 @@ static const struct mlx5_counter_ctrl > > mlx5_counters_init[] =3D { > > > > static const unsigned int xstats_n =3D RTE_DIM(mlx5_counters_init); > > > > +static inline void > > +mlx5_read_ib_stat(struct priv *priv, unsigned int idx, uint64_t Why not provide the counter name instead of the idx?=20 > > +*stat) { > > + FILE *file; > > + MKSTR(path, "%s/ports/1/hw_counters/%s", > > + priv->ibdev_path, > > + mlx5_counters_init[idx].ctr_name); > > + > > + file =3D fopen(path, "rb"); > > + if (file) { > > + int n =3D fscanf(file, "%" SCNu64, stat); > > + > > + fclose(file); > > + if (n !=3D 1) > > + stat =3D 0; > > + } > > +} > > + > > /** > > * Read device counters table. > > * > > @@ -155,19 +173,7 @@ mlx5_read_dev_counters(struct rte_eth_dev > *dev, > > uint64_t *stats) > > } > > for (i =3D 0; i !=3D xstats_n; ++i) { > > if (mlx5_counters_init[i].ib) { > > - FILE *file; > > - MKSTR(path, "%s/ports/1/hw_counters/%s", > > - priv->ibdev_path, > > - mlx5_counters_init[i].ctr_name); > > - > > - file =3D fopen(path, "rb"); > > - if (file) { > > - int n =3D fscanf(file, "%" SCNu64, &stats= [i]); > > - > > - fclose(file); > > - if (n !=3D 1) > > - stats[i] =3D 0; > > - } > > + mlx5_read_ib_stat(priv, i, &stats[i]); > > } else { > > stats[i] =3D (uint64_t) > > et_stats->data[xstats_ctrl- > > >dev_table_idx[i]]; > > @@ -281,6 +287,7 @@ mlx5_xstats_init(struct rte_eth_dev *dev) > > if (ret) > > DRV_LOG(ERR, "port %u cannot read device counters: %s", > > dev->data->port_id, strerror(rte_errno)); > > + mlx5_read_ib_stat(priv, 17, &xstats_ctrl->imissed_base); Since this function now is doing init for both stats and xstats better to c= all it mlx5_stats_init.=20 > > free: > > rte_free(strings); > > } > > @@ -389,6 +396,8 @@ mlx5_stats_get(struct rte_eth_dev *dev, struct > > rte_eth_stats *stats) #endif > > tmp.oerrors +=3D txq->stats.oerrors; > > } > > + mlx5_read_ib_stat(priv, 17, &tmp.imissed); > > + tmp.imissed -=3D priv->xstats_ctrl.imissed_base; > > #ifndef MLX5_PMD_SOFT_COUNTERS > > /* FIXME: retrieve and add hardware counters. */ #endif @@ > > -461,6 > > +470,7 @@ mlx5_xstats_reset(struct rte_eth_dev *dev) > > } > > for (i =3D 0; i !=3D n; ++i) > > xstats_ctrl->base[i] =3D counters[i]; > > + mlx5_read_ib_stat(priv, 17, &xstats_ctrl->imissed_base); The imissed is part of stats, not xstats, therefore it should be reset on m= lx5_stats_reset. > > } > > > > /** > > -- > > 2.7.4