RE: [Patch v2] net/netvsc: report correct stats values

[Long Li <longli@microsoft.com>]

> Subject: Re: [Patch v2] net/netvsc: report correct stats values
> 
> On 5/5/2022 5:40 PM, Stephen Hemminger wrote:
> > On Thu, 5 May 2022 17:28:38 +0100
> > Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
> >
> >> On 5/4/2022 7:38 PM, Long Li wrote:
> >>>> Subject: Re: [Patch v2] net/netvsc: report correct stats values
> >>>>
> >>>> On 5/3/2022 9:48 PM, Long Li wrote:
> >>>>>> Subject: Re: [Patch v2] net/netvsc: report correct stats values
> >>>>>>
> >>>>>> On 5/3/2022 8:14 PM, Long Li wrote:
> >>>>>>>> Subject: Re: [Patch v2] net/netvsc: report correct stats values
> >>>>>>>>
> >>>>>>>> On 5/3/2022 7:18 PM, Long Li wrote:
> >>>>>>>>>> Subject: Re: [Patch v2] net/netvsc: report correct stats
> >>>>>>>>>> values
> >>>>>>>>>>
> >>>>>>>>>> On Tue, 26 Apr 2022 22:56:14 +0100 Ferruh Yigit
> >>>>>>>>>> <ferruh.yigit@xilinx.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>>>>        		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
> >>>>>>>>>>>> -			stats->q_opackets[i] = txq->stats.packets;
> >>>>>>>>>>>> -			stats->q_obytes[i] = txq->stats.bytes;
> >>>>>>>>>>>> +			stats->q_opackets[i] += txq-
> >stats.packets;
> >>>>>>>>>>>> +			stats->q_obytes[i] += txq->stats.bytes;
> >>>>>>>>>>>
> >>>>>>>>>>> This is per queue stats, 'stats->q_opackets[i]', in next
> >>>>>>>>>>> iteration of the loop, 'i' will be increased and 'txq' will
> >>>>>>>>>>> be updated, so as far as I can see the above change has no affect.
> >>>>>>>>>>
> >>>>>>>>>> Agree, that is why it was just assignment originally.
> >>>>>>>>>
> >>>>>>>>> The condition here is a little different. NETVSC is a master
> >>>>>>>>> device with
> >>>>>>>> another PMD running as a slave. When reporting stats values, it
> >>>>>>>> needs to add the values from the slave PMD. The original code
> >>>>>>>> just overwrites the values from its slave PMD.
> >>>>>>>>
> >>>>>>>> Where the initial values are coming from, 'hn_vf_stats_get()'?
> >>>>>>>>
> >>>>>>>> If 'hn_vf_stats_get()' fills the stats, what are the values
> >>>>>>>> kept in
> >>>>>>>> 'txq-
> >>>>>>> stats.*'
> >>>>>>>> in above updated loop?
> >>>>>>>
> >>>>>>> Yes, hn_vf_stats_get() fills in the stats from the slave PMD.
> >>>>>>> txq->stats
> >>>>>> values are from the master PMD. Those values are different and
> >>>>>> accounted separated from the values from the slave PMD.
> >>>>>>
> >>>>>> I see, since this is a little different than what most of the
> >>>>>> PMDs do, can you please put a little more info to the commit log?
> >>>>>> Or perhaps can add some comments to the code.
> >>>>>
> >>>>> Ok, will do.
> >>>>>
> >>>>>>
> >>>>>> And still 'stats->rx_nombuf' change is not required right? If so
> >>>>>> can you remove it in the next version?
> >>>>>
> >>>>> It is still needed. NETVSC unconditionally calls the slave PMD to
> >>>>> receive
> >>>> packets, even if it can't allocate a mbuf to receive a synthetic
> >>>> packet itself. The accounting of rx_nombuf is valid because the
> >>>> synthetic packets (to NETVSC) and VF packets (to slave PMD) are routed
> separately from Hyper-V.
> >>>>
> >>>> I am not referring to the "+=" update, my comment was because
> >>>> 'stats-
> >>>>> rx_nombuf' is overwritten in 'rte_eth_stats_get()' [1].
> >>>> Is it still required?
> >>>
> >>> Yes, it is still needed. NETVSC calls the rte_eth_stats_get() on its slave PMD
> first, and stats->rx_nombuf is updated (overwritten) for its slave PMD. Afte that,
> it needs to add to its own dev->data->rx_mbuf_alloc_failed back to stats-
> >rx_nombuf.
> >>>
> >>
> >> But its own stat also will be overwritten (not in PMD function, but
> >> in ethdev layer).
> >> 'stats->rx_nombuf' assignment in the PMD seems has no effect and can
> >> be removed.
> >>
> >> I can't see how it is needed, can you please put a call stack to describe?
> >
> > This here:
> >
> >
> > int
> > rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats) {
> > 	struct rte_eth_dev *dev;
> >
> > 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > 	dev = &rte_eth_devices[port_id];
> >
> > 	if (stats == NULL) {
> > 		RTE_ETHDEV_LOG(ERR, "Cannot get ethdev port %u stats to
> NULL\n",
> > 			port_id);
> > 		return -EINVAL;
> > 	}
> >
> > 	memset(stats, 0, sizeof(*stats));
> >
> > 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->stats_get, -ENOTSUP);
> > 	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
> > 	return eth_err(port_id, (*dev->dev_ops->stats_get)(dev, stats)); }
> >
> > Will fill in rx_nombuf from the current rx_mbuf_alloc_failed.
> > But it happens before the PMD specific stats function.
> >
> 
> I keep seeing the ethdev assignment as *after* the dev_ops, but it is not [1], so
> code is OK as it is.

Hi Ferruh,

Do you still want me to send a v3, or this patch is good as it is?

Long

> 
> 
> [1]
> It seems assignment was after but it is fixed on the way:
> Commit 53ecfa24fbcd ("ethdev: fix overwriting driver-specific stats")