From mboxrd@z Thu Jan 1 00:00:00 1970 From: "David Harton (dharton)" Subject: Re: [PATCH v2 1/2] ethdev: stop overriding rx_nombuf by rte_eth_stats_get Date: Wed, 23 Aug 2017 22:19:34 +0000 Message-ID: References: <20170823011937.37579-1-dharton@cisco.com> <20170823025555.19022-1-dharton@cisco.com> <20170823145631.5fce68dc@xeon-e3> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "thomas@monjalon.net" , "dev@dpdk.org" To: Stephen Hemminger Return-path: Received: from rcdn-iport-6.cisco.com (rcdn-iport-6.cisco.com [173.37.86.77]) by dpdk.org (Postfix) with ESMTP id A857C7D6E for ; Thu, 24 Aug 2017 00:19:36 +0200 (CEST) In-Reply-To: <20170823145631.5fce68dc@xeon-e3> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Wednesday, August 23, 2017 5:57 PM > To: David Harton (dharton) > Cc: thomas@monjalon.net; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v2 1/2] ethdev: stop overriding rx_nombuf > by rte_eth_stats_get >=20 > On Tue, 22 Aug 2017 22:55:55 -0400 > David Harton wrote: >=20 > > rte_eth_stats_get() unconditonally would set rx_nombuf even if the > > device was setting the value. A check has been added in > > rte_eth_stats_get() to leave the device value in-tact when non-zero. > > > > Signed-off-by: David Harton > > --- > > > > v2: Fixed braces complaint required by other coding standards. > > > > lib/librte_ether/rte_ethdev.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/lib/librte_ether/rte_ethdev.c > > b/lib/librte_ether/rte_ethdev.c index 0597641..0a1d3b8 100644 > > --- a/lib/librte_ether/rte_ethdev.c > > +++ b/lib/librte_ether/rte_ethdev.c > > @@ -1336,8 +1336,11 @@ struct rte_eth_dev * > > memset(stats, 0, sizeof(*stats)); > > > > RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP); > > - stats->rx_nombuf =3D dev->data->rx_mbuf_alloc_failed; > > (*dev->dev_ops->stats_get)(dev, stats); > > + /* only set rx_nombuf if not set by the device */ > > + if (!stats->rx_nombuf) > > + stats->rx_nombuf =3D dev->data->rx_mbuf_alloc_failed; > > + > > return 0; > > } > > >=20 > This seems backwards. It seems like the original way worked fine. > If device specific code wanted to override rx_nombuf it could do so eithe= r > by adding it's additional value or just setting rx_nombuf. >=20 > Adding special cases seems like it would start a bad precedent and the > could would end up quite complex as some values had one semantic and > others were only from driver. Eternal apologies. This is another example of me trying to upstream a fix = we've held on to for far too long and not realizing it has been addressed. = I see that this was fixed here: 53ecfa24fbcd17d9855937391ce347f37434fbfa Again, apologies...I'll be careful publishing any further fixes trying to d= etermine if others have fixed in different ways. Regards, Dave