diff mbox

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

Message ID 20170820200535.25987-1-qi.z.zhang@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Qi Zhang Aug. 20, 2017, 8:05 p.m. UTC 
  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(-)

Comments

Ferruh Yigit Aug. 21, 2017, 12:42 p.m. UTC | #1 
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 +
>
Qi Zhang Sept. 15, 2017, 10:33 a.m. UTC | #2 
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
Ferruh Yigit Sept. 15, 2017, 10:47 a.m. UTC | #3 
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
>
Ferruh Yigit Sept. 15, 2017, 12:21 p.m. UTC | #4 
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.
diff mbox

Patch

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 +