On 5/10/2022 6:33 AM, Long Li wrote:
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.yi...@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.yi...@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?
Yes can you please send a v3 with some more description in the commit
log on the special case for the PMD, and perhaps some comments in the code.
Thanks,
ferruh
Long
[1]
It seems assignment was after but it is fixed on the way:
Commit 53ecfa24fbcd ("ethdev: fix overwriting driver-specific stats")
