DPDK patches and discussions
 help / color / mirror / Atom feed
* [Patch v2] net/netvsc: report correct stats values
@ 2022-03-24 17:45 longli
  2022-04-26 21:56 ` Ferruh Yigit
  0 siblings, 1 reply; 16+ messages in thread
From: longli @ 2022-03-24 17:45 UTC (permalink / raw)
  To: dev, Stephen Hemminger; +Cc: Long Li, stable

From: Long Li <longli@microsoft.com>

The netvsc should add to the values from the VF and report the sum.

Fixes: 4e9c73e96e ("net/netvsc: add Hyper-V network device")
Cc: stable@dpdk.org
Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/net/netvsc/hn_ethdev.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/netvsc/hn_ethdev.c b/drivers/net/netvsc/hn_ethdev.c
index 0a357d3645..a6202d898b 100644
--- a/drivers/net/netvsc/hn_ethdev.c
+++ b/drivers/net/netvsc/hn_ethdev.c
@@ -804,8 +804,8 @@ static int hn_dev_stats_get(struct rte_eth_dev *dev,
 		stats->oerrors += txq->stats.errors;
 
 		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;
 		}
 	}
 
@@ -821,12 +821,12 @@ static int hn_dev_stats_get(struct rte_eth_dev *dev,
 		stats->imissed += rxq->stats.ring_full;
 
 		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
-			stats->q_ipackets[i] = rxq->stats.packets;
-			stats->q_ibytes[i] = rxq->stats.bytes;
+			stats->q_ipackets[i] += rxq->stats.packets;
+			stats->q_ibytes[i] += rxq->stats.bytes;
 		}
 	}
 
-	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
+	stats->rx_nombuf += dev->data->rx_mbuf_alloc_failed;
 	return 0;
 }
 
-- 
2.32.0


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Patch v2] net/netvsc: report correct stats values
  2022-03-24 17:45 [Patch v2] net/netvsc: report correct stats values longli
@ 2022-04-26 21:56 ` Ferruh Yigit
  2022-04-26 22:45   ` Stephen Hemminger
  0 siblings, 1 reply; 16+ messages in thread
From: Ferruh Yigit @ 2022-04-26 21:56 UTC (permalink / raw)
  To: longli, dev, Stephen Hemminger; +Cc: Long Li, stable

On 3/24/2022 5:45 PM, longli@linuxonhyperv.com wrote:
> From: Long Li <longli@microsoft.com>
> 
> The netvsc should add to the values from the VF and report the sum.
> 

Per port stats already accumulated, like:
'stats->opackets += txq->stats.packets;'

> Fixes: 4e9c73e96e ("net/netvsc: add Hyper-V network device")
> Cc: stable@dpdk.org
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>   drivers/net/netvsc/hn_ethdev.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/netvsc/hn_ethdev.c b/drivers/net/netvsc/hn_ethdev.c
> index 0a357d3645..a6202d898b 100644
> --- a/drivers/net/netvsc/hn_ethdev.c
> +++ b/drivers/net/netvsc/hn_ethdev.c
> @@ -804,8 +804,8 @@ static int hn_dev_stats_get(struct rte_eth_dev *dev,
>   		stats->oerrors += txq->stats.errors;
>   
>   		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.

>   		}
>   	}
>   
> @@ -821,12 +821,12 @@ static int hn_dev_stats_get(struct rte_eth_dev *dev,
>   		stats->imissed += rxq->stats.ring_full;
>   
>   		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
> -			stats->q_ipackets[i] = rxq->stats.packets;
> -			stats->q_ibytes[i] = rxq->stats.bytes;
> +			stats->q_ipackets[i] += rxq->stats.packets;
> +			stats->q_ibytes[i] += rxq->stats.bytes;
>   		}
>   	}
>   
> -	stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
> +	stats->rx_nombuf += dev->data->rx_mbuf_alloc_failed;

Why '+='? Is 'dev->data->rx_mbuf_alloc_failed' reset somewhere between 
two consecutive stats get call?

Anyway, above line has no affect, since the 'stats->rx_nombuf' is 
overwritten by 'rte_eth_stats_get()'. So above line can be removed.

>   	return 0;
>   }
>   


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Patch v2] net/netvsc: report correct stats values
  2022-04-26 21:56 ` Ferruh Yigit
@ 2022-04-26 22:45   ` Stephen Hemminger
  2022-05-03 18:18     ` Long Li
  0 siblings, 1 reply; 16+ messages in thread
From: Stephen Hemminger @ 2022-04-26 22:45 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: longli, dev, Stephen Hemminger, Long Li, stable

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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [Patch v2] net/netvsc: report correct stats values
  2022-04-26 22:45   ` Stephen Hemminger
@ 2022-05-03 18:18     ` Long Li
  2022-05-03 19:03       ` Ferruh Yigit
  0 siblings, 1 reply; 16+ messages in thread
From: Long Li @ 2022-05-03 18:18 UTC (permalink / raw)
  To: Stephen Hemminger, Ferruh Yigit; +Cc: longli, dev, Stephen Hemminger, stable

> 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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Patch v2] net/netvsc: report correct stats values
  2022-05-03 18:18     ` Long Li
@ 2022-05-03 19:03       ` Ferruh Yigit
  2022-05-03 19:14         ` Long Li
  0 siblings, 1 reply; 16+ messages in thread
From: Ferruh Yigit @ 2022-05-03 19:03 UTC (permalink / raw)
  To: Long Li, Stephen Hemminger; +Cc: longli, dev, Stephen Hemminger, stable

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?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [Patch v2] net/netvsc: report correct stats values
  2022-05-03 19:03       ` Ferruh Yigit
@ 2022-05-03 19:14         ` Long Li
  2022-05-03 19:55           ` Ferruh Yigit
  0 siblings, 1 reply; 16+ messages in thread
From: Long Li @ 2022-05-03 19:14 UTC (permalink / raw)
  To: Ferruh Yigit, Stephen Hemminger; +Cc: longli, dev, Stephen Hemminger, stable

> 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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Patch v2] net/netvsc: report correct stats values
  2022-05-03 19:14         ` Long Li
@ 2022-05-03 19:55           ` Ferruh Yigit
  2022-05-03 20:48             ` Long Li
  0 siblings, 1 reply; 16+ messages in thread
From: Ferruh Yigit @ 2022-05-03 19:55 UTC (permalink / raw)
  To: Long Li, Stephen Hemminger; +Cc: longli, dev, Stephen Hemminger, stable

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.

And still 'stats->rx_nombuf' change is not required right? If so can you 
remove it in the next version?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [Patch v2] net/netvsc: report correct stats values
  2022-05-03 19:55           ` Ferruh Yigit
@ 2022-05-03 20:48             ` Long Li
  2022-05-04 12:33               ` Ferruh Yigit
  0 siblings, 1 reply; 16+ messages in thread
From: Long Li @ 2022-05-03 20:48 UTC (permalink / raw)
  To: Ferruh Yigit, Stephen Hemminger; +Cc: longli, dev, Stephen Hemminger, stable

> 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.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Patch v2] net/netvsc: report correct stats values
  2022-05-03 20:48             ` Long Li
@ 2022-05-04 12:33               ` Ferruh Yigit
  2022-05-04 18:38                 ` Long Li
  0 siblings, 1 reply; 16+ messages in thread
From: Ferruh Yigit @ 2022-05-04 12:33 UTC (permalink / raw)
  To: Long Li, Stephen Hemminger; +Cc: longli, dev, Stephen Hemminger, stable

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?

[1]
https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.c?h=v22.03#n2518

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [Patch v2] net/netvsc: report correct stats values
  2022-05-04 12:33               ` Ferruh Yigit
@ 2022-05-04 18:38                 ` Long Li
  2022-05-05 16:28                   ` Ferruh Yigit
  0 siblings, 1 reply; 16+ messages in thread
From: Long Li @ 2022-05-04 18:38 UTC (permalink / raw)
  To: Ferruh Yigit, Stephen Hemminger; +Cc: longli, dev, Stephen Hemminger, stable

> 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.

> 
> [1]
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.dpdk
> .org%2Fdpdk%2Ftree%2Flib%2Fethdev%2Frte_ethdev.c%3Fh%3Dv22.03%23n25
> 18&amp;data=05%7C01%7Clongli%40microsoft.com%7Cea473df2344c460d575
> d08da2dca3e53%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63787
> 2643902917430%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ
> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sd
> ata=FZO%2B%2BnWtLGstHHIZ2aXsDUKNI%2Fi9tbj6jONhp174qKw%3D&amp;res
> erved=0

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Patch v2] net/netvsc: report correct stats values
  2022-05-04 18:38                 ` Long Li
@ 2022-05-05 16:28                   ` Ferruh Yigit
  2022-05-05 16:40                     ` Stephen Hemminger
  0 siblings, 1 reply; 16+ messages in thread
From: Ferruh Yigit @ 2022-05-05 16:28 UTC (permalink / raw)
  To: Long Li, Stephen Hemminger; +Cc: longli, dev, Stephen Hemminger, stable

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?

>>
>> [1]
>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.dpdk
>> .org%2Fdpdk%2Ftree%2Flib%2Fethdev%2Frte_ethdev.c%3Fh%3Dv22.03%23n25
>> 18&amp;data=05%7C01%7Clongli%40microsoft.com%7Cea473df2344c460d575
>> d08da2dca3e53%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63787
>> 2643902917430%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ
>> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sd
>> ata=FZO%2B%2BnWtLGstHHIZ2aXsDUKNI%2Fi9tbj6jONhp174qKw%3D&amp;res
>> erved=0


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Patch v2] net/netvsc: report correct stats values
  2022-05-05 16:28                   ` Ferruh Yigit
@ 2022-05-05 16:40                     ` Stephen Hemminger
  2022-05-05 16:57                       ` Ferruh Yigit
  0 siblings, 1 reply; 16+ messages in thread
From: Stephen Hemminger @ 2022-05-05 16:40 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Long Li, longli, dev, Stephen Hemminger, stable

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.

For the case of nested device like netvsc PMD that value needs to
be summed.

What happens with VF would be:

Call of rte_eth_stats_get on the netvsc PMD port.
    stats->rx_nombuf comes from rx_mbuf_alloc_failed of the vmbus part
Netvsc PMD stats get calls rte_eth_stats_get on the VF
Nested rte_eth_stats_get will set stats->rx_nombuf to the value from VF
   the previous value is overwritten.

At end of Netvsc PMD stats get it should be adding the value of any
allocation failures in the VMBUS part.

Summary:
rte_eth_stats_get:      stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;  // original
rte_eth_stats_get(vf):  stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;  // VF alloc failed
hn_dev_stats_get:       stats->rx_nombuf += dev->data->rx_mbuf_alloc_failed; // Vmbus alloc failed



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Patch v2] net/netvsc: report correct stats values
  2022-05-05 16:40                     ` Stephen Hemminger
@ 2022-05-05 16:57                       ` Ferruh Yigit
  2022-05-10  5:33                         ` Long Li
  0 siblings, 1 reply; 16+ messages in thread
From: Ferruh Yigit @ 2022-05-05 16:57 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Long Li, longli, dev, Stephen Hemminger, stable

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.


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [Patch v2] net/netvsc: report correct stats values
  2022-05-05 16:57                       ` Ferruh Yigit
@ 2022-05-10  5:33                         ` Long Li
  2022-05-10 11:29                           ` Ferruh Yigit
  0 siblings, 1 reply; 16+ messages in thread
From: Long Li @ 2022-05-10  5:33 UTC (permalink / raw)
  To: Ferruh Yigit, Stephen Hemminger; +Cc: longli, dev, Stephen Hemminger, stable

> 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")

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Patch v2] net/netvsc: report correct stats values
  2022-05-10  5:33                         ` Long Li
@ 2022-05-10 11:29                           ` Ferruh Yigit
  2022-05-10 18:03                             ` Long Li
  0 siblings, 1 reply; 16+ messages in thread
From: Ferruh Yigit @ 2022-05-10 11:29 UTC (permalink / raw)
  To: Long Li, Stephen Hemminger; +Cc: longli, dev, Stephen Hemminger, stable

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.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?
> 

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")


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [Patch v2] net/netvsc: report correct stats values
  2022-05-10 11:29                           ` Ferruh Yigit
@ 2022-05-10 18:03                             ` Long Li
  0 siblings, 0 replies; 16+ messages in thread
From: Long Li @ 2022-05-10 18:03 UTC (permalink / raw)
  To: Ferruh Yigit, Stephen Hemminger; +Cc: longli, dev, Stephen Hemminger, stable

> Subject: Re: [Patch v2] net/netvsc: report correct stats values
> 
> 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.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?
> >
> 
> 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

Yes, will send out shortly.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2022-05-10 18:03 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-24 17:45 [Patch v2] net/netvsc: report correct stats values longli
2022-04-26 21:56 ` Ferruh Yigit
2022-04-26 22:45   ` Stephen Hemminger
2022-05-03 18:18     ` Long Li
2022-05-03 19:03       ` Ferruh Yigit
2022-05-03 19:14         ` Long Li
2022-05-03 19:55           ` Ferruh Yigit
2022-05-03 20:48             ` Long Li
2022-05-04 12:33               ` Ferruh Yigit
2022-05-04 18:38                 ` Long Li
2022-05-05 16:28                   ` Ferruh Yigit
2022-05-05 16:40                     ` Stephen Hemminger
2022-05-05 16:57                       ` Ferruh Yigit
2022-05-10  5:33                         ` Long Li
2022-05-10 11:29                           ` Ferruh Yigit
2022-05-10 18:03                             ` Long Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).