[dpdk-stable] [PATCH] net/i40e: fix packet count for PF

[dpdk-stable] [PATCH] net/i40e: fix packet count for PF

[Qi Zhang]

Previously, for PF statistics we use VSI register for packet count
but use port's register for packet bytes, that cause inconsistent
situation of PF statistics when some VF is active,  since it will
cover VF's packet bytes but not packet count.
The patch will take port register for PF packet count back, but still
exclude main vsi's discard packet count.
Just like pervious fix, its still not perfect,(since RX packet number
is over counted when there is VF discard packet) but seems it make the
overall better).

Fixes: 9aace75fc82e ("i40e: fix statistics")
Cc: stable@dpdk.org

Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 5f26e24..63acbb8 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -2664,13 +2664,14 @@ i40e_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 	/* call read registers - updates values, now write them to struct */
 	i40e_read_stats_registers(pf, hw);
 
-	stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
-			pf->main_vsi->eth_stats.rx_multicast +
-			pf->main_vsi->eth_stats.rx_broadcast -
+	stats->ipackets = ns->eth.rx_unicast +
+			ns->eth.rx_multicast +
+			ns->eth.rx_broadcast -
+			ns->eth.rx_discards -
 			pf->main_vsi->eth_stats.rx_discards;
-	stats->opackets = pf->main_vsi->eth_stats.tx_unicast +
-			pf->main_vsi->eth_stats.tx_multicast +
-			pf->main_vsi->eth_stats.tx_broadcast;
+	stats->opackets = ns->eth.tx_unicast +
+			ns->eth.tx_multicast +
+			ns->eth.tx_broadcast;
 	stats->ibytes   = ns->eth.rx_bytes;
 	stats->obytes   = ns->eth.tx_bytes;
 	stats->oerrors  = ns->eth.tx_errors +
-- 
2.9.4

Re: [dpdk-stable] [PATCH] net/i40e: fix packet count for PF

[Ferruh Yigit]

On 8/20/2017 9:05 PM, Qi Zhang wrote:
> Previously, for PF statistics we use VSI register for packet count
> but use port's register for packet bytes, that cause inconsistent
> situation of PF statistics when some VF is active,  since it will
> cover VF's packet bytes but not packet count.
> The patch will take port register for PF packet count back, but still
> exclude main vsi's discard packet count.
> Just like pervious fix, its still not perfect,(since RX packet number
> is over counted when there is VF discard packet) but seems it make the
> overall better).

What does Linux do for stats calculation?
I believe it is good to be consistent with it.

> 
> Fixes: 9aace75fc82e ("i40e: fix statistics")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
> index 5f26e24..63acbb8 100644
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -2664,13 +2664,14 @@ i40e_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>  	/* call read registers - updates values, now write them to struct */
>  	i40e_read_stats_registers(pf, hw);
>  
> -	stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
> -			pf->main_vsi->eth_stats.rx_multicast +
> -			pf->main_vsi->eth_stats.rx_broadcast -
> +	stats->ipackets = ns->eth.rx_unicast +
> +			ns->eth.rx_multicast +
> +			ns->eth.rx_broadcast -
> +			ns->eth.rx_discards -
>  			pf->main_vsi->eth_stats.rx_discards;

Both port rx_discards and PF rx_discards excluded, is this intentional?
Won't this cause double exclusion of some rx_discards packets?

> -	stats->opackets = pf->main_vsi->eth_stats.tx_unicast +
> -			pf->main_vsi->eth_stats.tx_multicast +
> -			pf->main_vsi->eth_stats.tx_broadcast;
> +	stats->opackets = ns->eth.tx_unicast +
> +			ns->eth.tx_multicast +
> +			ns->eth.tx_broadcast;
>  	stats->ibytes   = ns->eth.rx_bytes;
>  	stats->obytes   = ns->eth.tx_bytes;
>  	stats->oerrors  = ns->eth.tx_errors +
> 

Re: [dpdk-stable] [PATCH] net/i40e: fix packet count for PF

[Zhang, Qi Z]

Sorry for late reply

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, August 21, 2017 8:43 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [dpdk-stable] [PATCH] net/i40e: fix packet count for PF
> 
> On 8/20/2017 9:05 PM, Qi Zhang wrote:
> > Previously, for PF statistics we use VSI register for packet count but
> > use port's register for packet bytes, that cause inconsistent
> > situation of PF statistics when some VF is active,  since it will
> > cover VF's packet bytes but not packet count.
> > The patch will take port register for PF packet count back, but still
> > exclude main vsi's discard packet count.
> > Just like pervious fix, its still not perfect,(since RX packet number
> > is over counted when there is VF discard packet) but seems it make the
> > overall better).
> 
> What does Linux do for stats calculation?
> I believe it is good to be consistent with it.

Kernel driver is quite different on the stats calucation, I don't think this patch is going to cover this.
It just try to fix the mismatch between rxbytes and rx_packets on PF.
But your suggestion is considerable

> 
> >
> > Fixes: 9aace75fc82e ("i40e: fix statistics")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> >  drivers/net/i40e/i40e_ethdev.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev.c
> > b/drivers/net/i40e/i40e_ethdev.c index 5f26e24..63acbb8 100644
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -2664,13 +2664,14 @@ i40e_dev_stats_get(struct rte_eth_dev *dev,
> struct rte_eth_stats *stats)
> >  	/* call read registers - updates values, now write them to struct */
> >  	i40e_read_stats_registers(pf, hw);
> >
> > -	stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
> > -			pf->main_vsi->eth_stats.rx_multicast +
> > -			pf->main_vsi->eth_stats.rx_broadcast -
> > +	stats->ipackets = ns->eth.rx_unicast +
> > +			ns->eth.rx_multicast +
> > +			ns->eth.rx_broadcast -
> > +			ns->eth.rx_discards -
> >  			pf->main_vsi->eth_stats.rx_discards;
> 
> Both port rx_discards and PF rx_discards excluded, is this intentional?
> Won't this cause double exclusion of some rx_discards packets?

Yes, this is intentional, port rx_discard and VSI rx_discard counts on different part of total drop packets.
> 
> > -	stats->opackets = pf->main_vsi->eth_stats.tx_unicast +
> > -			pf->main_vsi->eth_stats.tx_multicast +
> > -			pf->main_vsi->eth_stats.tx_broadcast;
> > +	stats->opackets = ns->eth.tx_unicast +
> > +			ns->eth.tx_multicast +
> > +			ns->eth.tx_broadcast;
> >  	stats->ibytes   = ns->eth.rx_bytes;
> >  	stats->obytes   = ns->eth.tx_bytes;
> >  	stats->oerrors  = ns->eth.tx_errors +
> 

Regards
Qi

Re: [dpdk-stable] [PATCH] net/i40e: fix packet count for PF

[Ferruh Yigit]

On 9/15/2017 11:33 AM, Zhang, Qi Z wrote:
> Sorry for late reply
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Monday, August 21, 2017 8:43 PM
>> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
>> Cc: dev@dpdk.org; stable@dpdk.org
>> Subject: Re: [dpdk-stable] [PATCH] net/i40e: fix packet count for PF
>>
>> On 8/20/2017 9:05 PM, Qi Zhang wrote:
>>> Previously, for PF statistics we use VSI register for packet count but
>>> use port's register for packet bytes, that cause inconsistent
>>> situation of PF statistics when some VF is active,  since it will
>>> cover VF's packet bytes but not packet count.
>>> The patch will take port register for PF packet count back, but still
>>> exclude main vsi's discard packet count.
>>> Just like pervious fix, its still not perfect,(since RX packet number
>>> is over counted when there is VF discard packet) but seems it make the
>>> overall better).
>>
>> What does Linux do for stats calculation?
>> I believe it is good to be consistent with it.
> 
> Kernel driver is quite different on the stats calucation, I don't think this patch is going to cover this.
> It just try to fix the mismatch between rxbytes and rx_packets on PF.
> But your suggestion is considerable

Method can be different, but numbers should be same :)
Not for this patch, bur for long term, I think having exact stats
numbers as Linux should be the target, if this is not the case already.

> 
>>
>>>
>>> Fixes: 9aace75fc82e ("i40e: fix statistics")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>>> ---
>>>  drivers/net/i40e/i40e_ethdev.c | 13 +++++++------
>>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/i40e/i40e_ethdev.c
>>> b/drivers/net/i40e/i40e_ethdev.c index 5f26e24..63acbb8 100644
>>> --- a/drivers/net/i40e/i40e_ethdev.c
>>> +++ b/drivers/net/i40e/i40e_ethdev.c
>>> @@ -2664,13 +2664,14 @@ i40e_dev_stats_get(struct rte_eth_dev *dev,
>> struct rte_eth_stats *stats)
>>>  	/* call read registers - updates values, now write them to struct */
>>>  	i40e_read_stats_registers(pf, hw);
>>>
>>> -	stats->ipackets = pf->main_vsi->eth_stats.rx_unicast +
>>> -			pf->main_vsi->eth_stats.rx_multicast +
>>> -			pf->main_vsi->eth_stats.rx_broadcast -
>>> +	stats->ipackets = ns->eth.rx_unicast +
>>> +			ns->eth.rx_multicast +
>>> +			ns->eth.rx_broadcast -
>>> +			ns->eth.rx_discards -
>>>  			pf->main_vsi->eth_stats.rx_discards;
>>
>> Both port rx_discards and PF rx_discards excluded, is this intentional?
>> Won't this cause double exclusion of some rx_discards packets?
> 
> Yes, this is intentional, port rx_discard and VSI rx_discard counts on different part of total drop packets.
>>
>>> -	stats->opackets = pf->main_vsi->eth_stats.tx_unicast +
>>> -			pf->main_vsi->eth_stats.tx_multicast +
>>> -			pf->main_vsi->eth_stats.tx_broadcast;
>>> +	stats->opackets = ns->eth.tx_unicast +
>>> +			ns->eth.tx_multicast +
>>> +			ns->eth.tx_broadcast;
>>>  	stats->ibytes   = ns->eth.rx_bytes;
>>>  	stats->obytes   = ns->eth.tx_bytes;
>>>  	stats->oerrors  = ns->eth.tx_errors +
>>
> 
> Regards
> Qi
> 

Re: [dpdk-stable] [PATCH] net/i40e: fix packet count for PF

[Ferruh Yigit]

On 8/20/2017 9:05 PM, Qi Zhang wrote:
> Previously, for PF statistics we use VSI register for packet count
> but use port's register for packet bytes, that cause inconsistent
> situation of PF statistics when some VF is active,  since it will
> cover VF's packet bytes but not packet count.
> The patch will take port register for PF packet count back, but still
> exclude main vsi's discard packet count.
> Just like pervious fix, its still not perfect,(since RX packet number
> is over counted when there is VF discard packet) but seems it make the
> overall better).
> 
> Fixes: 9aace75fc82e ("i40e: fix statistics")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>

Applied to dpdk-next-net/master, thanks.