[Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled

[Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled

[=?unknown-8bit?q?Bj=C3=B6rn_T=C3=B6pel?=]

When XDP is enabled, the driver will report incorrect
statistics. Received frames will reported as transmitted frames.

This commits fixes the i40e implementation of ndo_get_stats64 (struct
net_device_ops), so that iproute2 will report correct statistics
(e.g. when running "ip -stats link show dev eth0") even when XDP is
enabled.

Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
Fixes: 74608d17fe29 ("i40e: add support for XDP_TX action")
Signed-off-by: Bj?rn T?pel <bjorn.topel@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 24 +++++++++++----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index e40c023cc7b6..7c122dd3faa1 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -425,9 +425,9 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
 				  struct rtnl_link_stats64 *stats)
 {
 	struct i40e_netdev_priv *np = netdev_priv(netdev);
-	struct i40e_ring *tx_ring, *rx_ring;
 	struct i40e_vsi *vsi = np->vsi;
 	struct rtnl_link_stats64 *vsi_stats = i40e_get_vsi_stats_struct(vsi);
+	struct i40e_ring *ring;
 	int i;
 
 	if (test_bit(__I40E_VSI_DOWN, vsi->state))
@@ -441,24 +441,26 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
 		u64 bytes, packets;
 		unsigned int start;
 
-		tx_ring = READ_ONCE(vsi->tx_rings[i]);
-		if (!tx_ring)
+		ring = READ_ONCE(vsi->tx_rings[i]);
+		if (!ring)
 			continue;
-		i40e_get_netdev_stats_struct_tx(tx_ring, stats);
+		i40e_get_netdev_stats_struct_tx(ring, stats);
 
-		rx_ring = &tx_ring[1];
+		if (i40e_enabled_xdp_vsi(vsi)) {
+			ring++;
+			i40e_get_netdev_stats_struct_tx(ring, stats);
+		}
 
+		ring++;
 		do {
-			start = u64_stats_fetch_begin_irq(&rx_ring->syncp);
-			packets = rx_ring->stats.packets;
-			bytes   = rx_ring->stats.bytes;
-		} while (u64_stats_fetch_retry_irq(&rx_ring->syncp, start));
+			start   = u64_stats_fetch_begin_irq(&ring->syncp);
+			packets = ring->stats.packets;
+			bytes   = ring->stats.bytes;
+		} while (u64_stats_fetch_retry_irq(&ring->syncp, start));
 
 		stats->rx_packets += packets;
 		stats->rx_bytes   += bytes;
 
-		if (i40e_enabled_xdp_vsi(vsi))
-			i40e_get_netdev_stats_struct_tx(&rx_ring[1], stats);
 	}
 	rcu_read_unlock();
 
-- 
2.17.1

[Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled

[Jesper Dangaard Brouer]

On Fri, 24 Aug 2018 13:21:59 +0200
Bj?rn T?pel <bjorn.topel@intel.com> wrote:

> When XDP is enabled, the driver will report incorrect
> statistics. Received frames will reported as transmitted frames.
> 
> This commits fixes the i40e implementation of ndo_get_stats64 (struct
> net_device_ops), so that iproute2 will report correct statistics
> (e.g. when running "ip -stats link show dev eth0") even when XDP is
> enabled.
> 
> Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Fixes: 74608d17fe29 ("i40e: add support for XDP_TX action")

Stable candidate:
 $ git describe --contains 74608d17fe29
 v4.13-rc1~157^2~128^2~13

> Signed-off-by: Bj?rn T?pel <bjorn.topel@intel.com>

It works for me:

Tested-by: Jesper Dangaard Brouer <brouer@redhat.com>

I'm explicitly _not_ ACK'ing the patch, as I think the your code changes
below makes it harder to follow whether a TX or RX ring is getting
updated. But it is 100% up to the driver maintainers to say if this is
acceptable from a maintenance PoV.

> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 24 +++++++++++----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index e40c023cc7b6..7c122dd3faa1 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -425,9 +425,9 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
>  				  struct rtnl_link_stats64 *stats)
>  {
>  	struct i40e_netdev_priv *np = netdev_priv(netdev);
> -	struct i40e_ring *tx_ring, *rx_ring;
>  	struct i40e_vsi *vsi = np->vsi;
>  	struct rtnl_link_stats64 *vsi_stats = i40e_get_vsi_stats_struct(vsi);
> +	struct i40e_ring *ring;
>  	int i;
>  
>  	if (test_bit(__I40E_VSI_DOWN, vsi->state))
> @@ -441,24 +441,26 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
>  		u64 bytes, packets;
>  		unsigned int start;
>  
> -		tx_ring = READ_ONCE(vsi->tx_rings[i]);
> -		if (!tx_ring)
> +		ring = READ_ONCE(vsi->tx_rings[i]);
> +		if (!ring)
>  			continue;
> -		i40e_get_netdev_stats_struct_tx(tx_ring, stats);
> +		i40e_get_netdev_stats_struct_tx(ring, stats);
>  
> -		rx_ring = &tx_ring[1];
> +		if (i40e_enabled_xdp_vsi(vsi)) {
> +			ring++;
> +			i40e_get_netdev_stats_struct_tx(ring, stats);
> +		}
>  
> +		ring++;
>  		do {
> -			start = u64_stats_fetch_begin_irq(&rx_ring->syncp);
> -			packets = rx_ring->stats.packets;
> -			bytes   = rx_ring->stats.bytes;
> -		} while (u64_stats_fetch_retry_irq(&rx_ring->syncp, start));
> +			start   = u64_stats_fetch_begin_irq(&ring->syncp);
> +			packets = ring->stats.packets;
> +			bytes   = ring->stats.bytes;
> +		} while (u64_stats_fetch_retry_irq(&ring->syncp, start));
>  
>  		stats->rx_packets += packets;
>  		stats->rx_bytes   += bytes;
>  
> -		if (i40e_enabled_xdp_vsi(vsi))
> -			i40e_get_netdev_stats_struct_tx(&rx_ring[1], stats);
>  	}
>  	rcu_read_unlock();
>  



-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH] i40e: report correct statistics when XDP is enabled

[Jesper Dangaard Brouer]

On Fri, 24 Aug 2018 13:21:59 +0200
Björn Töpel <bjorn.topel@intel.com> wrote:

> When XDP is enabled, the driver will report incorrect
> statistics. Received frames will reported as transmitted frames.
> 
> This commits fixes the i40e implementation of ndo_get_stats64 (struct
> net_device_ops), so that iproute2 will report correct statistics
> (e.g. when running "ip -stats link show dev eth0") even when XDP is
> enabled.
> 
> Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Fixes: 74608d17fe29 ("i40e: add support for XDP_TX action")

Stable candidate:
 $ git describe --contains 74608d17fe29
 v4.13-rc1~157^2~128^2~13

> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>

It works for me:

Tested-by: Jesper Dangaard Brouer <brouer@redhat.com>

I'm explicitly _not_ ACK'ing the patch, as I think the your code changes
below makes it harder to follow whether a TX or RX ring is getting
updated. But it is 100% up to the driver maintainers to say if this is
acceptable from a maintenance PoV.

> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 24 +++++++++++----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index e40c023cc7b6..7c122dd3faa1 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -425,9 +425,9 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
>  				  struct rtnl_link_stats64 *stats)
>  {
>  	struct i40e_netdev_priv *np = netdev_priv(netdev);
> -	struct i40e_ring *tx_ring, *rx_ring;
>  	struct i40e_vsi *vsi = np->vsi;
>  	struct rtnl_link_stats64 *vsi_stats = i40e_get_vsi_stats_struct(vsi);
> +	struct i40e_ring *ring;
>  	int i;
>  
>  	if (test_bit(__I40E_VSI_DOWN, vsi->state))
> @@ -441,24 +441,26 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
>  		u64 bytes, packets;
>  		unsigned int start;
>  
> -		tx_ring = READ_ONCE(vsi->tx_rings[i]);
> -		if (!tx_ring)
> +		ring = READ_ONCE(vsi->tx_rings[i]);
> +		if (!ring)
>  			continue;
> -		i40e_get_netdev_stats_struct_tx(tx_ring, stats);
> +		i40e_get_netdev_stats_struct_tx(ring, stats);
>  
> -		rx_ring = &tx_ring[1];
> +		if (i40e_enabled_xdp_vsi(vsi)) {
> +			ring++;
> +			i40e_get_netdev_stats_struct_tx(ring, stats);
> +		}
>  
> +		ring++;
>  		do {
> -			start = u64_stats_fetch_begin_irq(&rx_ring->syncp);
> -			packets = rx_ring->stats.packets;
> -			bytes   = rx_ring->stats.bytes;
> -		} while (u64_stats_fetch_retry_irq(&rx_ring->syncp, start));
> +			start   = u64_stats_fetch_begin_irq(&ring->syncp);
> +			packets = ring->stats.packets;
> +			bytes   = ring->stats.bytes;
> +		} while (u64_stats_fetch_retry_irq(&ring->syncp, start));
>  
>  		stats->rx_packets += packets;
>  		stats->rx_bytes   += bytes;
>  
> -		if (i40e_enabled_xdp_vsi(vsi))
> -			i40e_get_netdev_stats_struct_tx(&rx_ring[1], stats);
>  	}
>  	rcu_read_unlock();
>  



-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

[Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Bj?rn T?pel
> Sent: Friday, August 24, 2018 4:22 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; intel-wired-
> lan at lists.osuosl.org
> Cc: brouer at redhat.com; Topel, Bjorn <bjorn.topel@intel.com>;
> magnus.karlsson at gmail.com; Karlsson, Magnus
> <magnus.karlsson@intel.com>
> Subject: [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is
> enabled
> 
> When XDP is enabled, the driver will report incorrect statistics. Received
> frames will reported as transmitted frames.
> 
> This commits fixes the i40e implementation of ndo_get_stats64 (struct
> net_device_ops), so that iproute2 will report correct statistics (e.g. when
> running "ip -stats link show dev eth0") even when XDP is enabled.
> 
> Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Fixes: 74608d17fe29 ("i40e: add support for XDP_TX action")
> Signed-off-by: Bj?rn T?pel <bjorn.topel@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 24 +++++++++++----------
>  1 file changed, 13 insertions(+), 11 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>

[Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled

[Paul Menzel]

Dear Bj?rn,


On 08/24/18 16:00, Jesper Dangaard Brouer wrote:
> On Fri, 24 Aug 2018 13:21:59 +0200
> Bj?rn T?pel <bjorn.topel@intel.com> wrote:
> 
>> When XDP is enabled, the driver will report incorrect
>> statistics. Received frames will reported as transmitted frames.
>>
>> This commits fixes the i40e implementation of ndo_get_stats64 (struct

Should you send a v2, then please use singular for *commit*:

This commit ?.

>> net_device_ops), so that iproute2 will report correct statistics
>> (e.g. when running "ip -stats link show dev eth0") even when XDP is
>> enabled.

In the future, I?d be great, if you could describe your fix in the
commit message too. For example, why the if statement needs to move up.

>> Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
>> Fixes: 74608d17fe29 ("i40e: add support for XDP_TX action")
> 
> Stable candidate:
>  $ git describe --contains 74608d17fe29
>  v4.13-rc1~157^2~128^2~13
> 
>> Signed-off-by: Bj?rn T?pel <bjorn.topel@intel.com>
> 
> It works for me:
> 
> Tested-by: Jesper Dangaard Brouer <brouer@redhat.com>
> 
> I'm explicitly _not_ ACK'ing the patch, as I think the your code changes
> below makes it harder to follow whether a TX or RX ring is getting
> updated. But it is 100% up to the driver maintainers to say if this is
> acceptable from a maintenance PoV.
> 
>> ---
>>  drivers/net/ethernet/intel/i40e/i40e_main.c | 24 +++++++++++----------
>>  1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> index e40c023cc7b6..7c122dd3faa1 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> @@ -425,9 +425,9 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
>>  				  struct rtnl_link_stats64 *stats)
>>  {
>>  	struct i40e_netdev_priv *np = netdev_priv(netdev);
>> -	struct i40e_ring *tx_ring, *rx_ring;
>>  	struct i40e_vsi *vsi = np->vsi;
>>  	struct rtnl_link_stats64 *vsi_stats = i40e_get_vsi_stats_struct(vsi);
>> +	struct i40e_ring *ring;
>>  	int i;
>>  
>>  	if (test_bit(__I40E_VSI_DOWN, vsi->state))
>> @@ -441,24 +441,26 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
>>  		u64 bytes, packets;
>>  		unsigned int start;
>>  
>> -		tx_ring = READ_ONCE(vsi->tx_rings[i]);
>> -		if (!tx_ring)
>> +		ring = READ_ONCE(vsi->tx_rings[i]);
>> +		if (!ring)
>>  			continue;
>> -		i40e_get_netdev_stats_struct_tx(tx_ring, stats);
>> +		i40e_get_netdev_stats_struct_tx(ring, stats);
>>  
>> -		rx_ring = &tx_ring[1];
>> +		if (i40e_enabled_xdp_vsi(vsi)) {
>> +			ring++;
>> +			i40e_get_netdev_stats_struct_tx(ring, stats);
>> +		}
>>  
>> +		ring++;
>>  		do {
>> -			start = u64_stats_fetch_begin_irq(&rx_ring->syncp);
>> -			packets = rx_ring->stats.packets;
>> -			bytes   = rx_ring->stats.bytes;
>> -		} while (u64_stats_fetch_retry_irq(&rx_ring->syncp, start));
>> +			start   = u64_stats_fetch_begin_irq(&ring->syncp);
>> +			packets = ring->stats.packets;
>> +			bytes   = ring->stats.bytes;
>> +		} while (u64_stats_fetch_retry_irq(&ring->syncp, start));
>>  
>>  		stats->rx_packets += packets;
>>  		stats->rx_bytes   += bytes;
>>  
>> -		if (i40e_enabled_xdp_vsi(vsi))
>> -			i40e_get_netdev_stats_struct_tx(&rx_ring[1], stats);
>>  	}
>>  	rcu_read_unlock();
>>  


Kind regards,

Paul

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5174 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20180828/3c4619b4/attachment.p7s>

Re: [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled

[Paul Menzel]

[-- Attachment #1: Type: text/plain, Size: 3498 bytes --]

Dear Björn,


On 08/24/18 16:00, Jesper Dangaard Brouer wrote:
> On Fri, 24 Aug 2018 13:21:59 +0200
> Björn Töpel <bjorn.topel@intel.com> wrote:
> 
>> When XDP is enabled, the driver will report incorrect
>> statistics. Received frames will reported as transmitted frames.
>>
>> This commits fixes the i40e implementation of ndo_get_stats64 (struct

Should you send a v2, then please use singular for *commit*:

This commit ….

>> net_device_ops), so that iproute2 will report correct statistics
>> (e.g. when running "ip -stats link show dev eth0") even when XDP is
>> enabled.

In the future, I’d be great, if you could describe your fix in the
commit message too. For example, why the if statement needs to move up.

>> Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
>> Fixes: 74608d17fe29 ("i40e: add support for XDP_TX action")
> 
> Stable candidate:
>  $ git describe --contains 74608d17fe29
>  v4.13-rc1~157^2~128^2~13
> 
>> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> 
> It works for me:
> 
> Tested-by: Jesper Dangaard Brouer <brouer@redhat.com>
> 
> I'm explicitly _not_ ACK'ing the patch, as I think the your code changes
> below makes it harder to follow whether a TX or RX ring is getting
> updated. But it is 100% up to the driver maintainers to say if this is
> acceptable from a maintenance PoV.
> 
>> ---
>>  drivers/net/ethernet/intel/i40e/i40e_main.c | 24 +++++++++++----------
>>  1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> index e40c023cc7b6..7c122dd3faa1 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>> @@ -425,9 +425,9 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
>>  				  struct rtnl_link_stats64 *stats)
>>  {
>>  	struct i40e_netdev_priv *np = netdev_priv(netdev);
>> -	struct i40e_ring *tx_ring, *rx_ring;
>>  	struct i40e_vsi *vsi = np->vsi;
>>  	struct rtnl_link_stats64 *vsi_stats = i40e_get_vsi_stats_struct(vsi);
>> +	struct i40e_ring *ring;
>>  	int i;
>>  
>>  	if (test_bit(__I40E_VSI_DOWN, vsi->state))
>> @@ -441,24 +441,26 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
>>  		u64 bytes, packets;
>>  		unsigned int start;
>>  
>> -		tx_ring = READ_ONCE(vsi->tx_rings[i]);
>> -		if (!tx_ring)
>> +		ring = READ_ONCE(vsi->tx_rings[i]);
>> +		if (!ring)
>>  			continue;
>> -		i40e_get_netdev_stats_struct_tx(tx_ring, stats);
>> +		i40e_get_netdev_stats_struct_tx(ring, stats);
>>  
>> -		rx_ring = &tx_ring[1];
>> +		if (i40e_enabled_xdp_vsi(vsi)) {
>> +			ring++;
>> +			i40e_get_netdev_stats_struct_tx(ring, stats);
>> +		}
>>  
>> +		ring++;
>>  		do {
>> -			start = u64_stats_fetch_begin_irq(&rx_ring->syncp);
>> -			packets = rx_ring->stats.packets;
>> -			bytes   = rx_ring->stats.bytes;
>> -		} while (u64_stats_fetch_retry_irq(&rx_ring->syncp, start));
>> +			start   = u64_stats_fetch_begin_irq(&ring->syncp);
>> +			packets = ring->stats.packets;
>> +			bytes   = ring->stats.bytes;
>> +		} while (u64_stats_fetch_retry_irq(&ring->syncp, start));
>>  
>>  		stats->rx_packets += packets;
>>  		stats->rx_bytes   += bytes;
>>  
>> -		if (i40e_enabled_xdp_vsi(vsi))
>> -			i40e_get_netdev_stats_struct_tx(&rx_ring[1], stats);
>>  	}
>>  	rcu_read_unlock();
>>  


Kind regards,

Paul


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

[Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled

[=?unknown-8bit?q?Bj=C3=B6rn_T=C3=B6pel?=]

On 2018-08-28 19:00, Paul Menzel wrote:
> Dear Bj?rn,
> 
> 
> On 08/24/18 16:00, Jesper Dangaard Brouer wrote:
>> On Fri, 24 Aug 2018 13:21:59 +0200
>> Bj?rn T?pel <bjorn.topel@intel.com> wrote:
>>
>>> When XDP is enabled, the driver will report incorrect
>>> statistics. Received frames will reported as transmitted frames.
>>>
>>> This commits fixes the i40e implementation of ndo_get_stats64 (struct
> 
> Should you send a v2, then please use singular for *commit*:
> 
> This commit ?.
> 
>>> net_device_ops), so that iproute2 will report correct statistics
>>> (e.g. when running "ip -stats link show dev eth0") even when XDP is
>>> enabled.
> 
> In the future, I?d be great, if you could describe your fix in the
> commit message too. For example, why the if statement needs to move up.
>

Thanks for the review, Paul. I'll address your comments, if we'll end up
with V2.


Bj?rn

>>> Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>> Fixes: 74608d17fe29 ("i40e: add support for XDP_TX action")
>>
>> Stable candidate:
>>   $ git describe --contains 74608d17fe29
>>   v4.13-rc1~157^2~128^2~13
>>
>>> Signed-off-by: Bj?rn T?pel <bjorn.topel@intel.com>
>>
>> It works for me:
>>
>> Tested-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>
>> I'm explicitly _not_ ACK'ing the patch, as I think the your code changes
>> below makes it harder to follow whether a TX or RX ring is getting
>> updated. But it is 100% up to the driver maintainers to say if this is
>> acceptable from a maintenance PoV.
>>
>>> ---
>>>   drivers/net/ethernet/intel/i40e/i40e_main.c | 24 +++++++++++----------
>>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> index e40c023cc7b6..7c122dd3faa1 100644
>>> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> @@ -425,9 +425,9 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
>>>   				  struct rtnl_link_stats64 *stats)
>>>   {
>>>   	struct i40e_netdev_priv *np = netdev_priv(netdev);
>>> -	struct i40e_ring *tx_ring, *rx_ring;
>>>   	struct i40e_vsi *vsi = np->vsi;
>>>   	struct rtnl_link_stats64 *vsi_stats = i40e_get_vsi_stats_struct(vsi);
>>> +	struct i40e_ring *ring;
>>>   	int i;
>>>   
>>>   	if (test_bit(__I40E_VSI_DOWN, vsi->state))
>>> @@ -441,24 +441,26 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
>>>   		u64 bytes, packets;
>>>   		unsigned int start;
>>>   
>>> -		tx_ring = READ_ONCE(vsi->tx_rings[i]);
>>> -		if (!tx_ring)
>>> +		ring = READ_ONCE(vsi->tx_rings[i]);
>>> +		if (!ring)
>>>   			continue;
>>> -		i40e_get_netdev_stats_struct_tx(tx_ring, stats);
>>> +		i40e_get_netdev_stats_struct_tx(ring, stats);
>>>   
>>> -		rx_ring = &tx_ring[1];
>>> +		if (i40e_enabled_xdp_vsi(vsi)) {
>>> +			ring++;
>>> +			i40e_get_netdev_stats_struct_tx(ring, stats);
>>> +		}
>>>   
>>> +		ring++;
>>>   		do {
>>> -			start = u64_stats_fetch_begin_irq(&rx_ring->syncp);
>>> -			packets = rx_ring->stats.packets;
>>> -			bytes   = rx_ring->stats.bytes;
>>> -		} while (u64_stats_fetch_retry_irq(&rx_ring->syncp, start));
>>> +			start   = u64_stats_fetch_begin_irq(&ring->syncp);
>>> +			packets = ring->stats.packets;
>>> +			bytes   = ring->stats.bytes;
>>> +		} while (u64_stats_fetch_retry_irq(&ring->syncp, start));
>>>   
>>>   		stats->rx_packets += packets;
>>>   		stats->rx_bytes   += bytes;
>>>   
>>> -		if (i40e_enabled_xdp_vsi(vsi))
>>> -			i40e_get_netdev_stats_struct_tx(&rx_ring[1], stats);
>>>   	}
>>>   	rcu_read_unlock();
>>>   
> 
> 
> Kind regards,
> 
> Paul
> 

Re: [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled

[Björn Töpel]

On 2018-08-28 19:00, Paul Menzel wrote:
> Dear Björn,
> 
> 
> On 08/24/18 16:00, Jesper Dangaard Brouer wrote:
>> On Fri, 24 Aug 2018 13:21:59 +0200
>> Björn Töpel <bjorn.topel@intel.com> wrote:
>>
>>> When XDP is enabled, the driver will report incorrect
>>> statistics. Received frames will reported as transmitted frames.
>>>
>>> This commits fixes the i40e implementation of ndo_get_stats64 (struct
> 
> Should you send a v2, then please use singular for *commit*:
> 
> This commit ….
> 
>>> net_device_ops), so that iproute2 will report correct statistics
>>> (e.g. when running "ip -stats link show dev eth0") even when XDP is
>>> enabled.
> 
> In the future, I’d be great, if you could describe your fix in the
> commit message too. For example, why the if statement needs to move up.
>

Thanks for the review, Paul. I'll address your comments, if we'll end up
with V2.


Björn

>>> Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>> Fixes: 74608d17fe29 ("i40e: add support for XDP_TX action")
>>
>> Stable candidate:
>>   $ git describe --contains 74608d17fe29
>>   v4.13-rc1~157^2~128^2~13
>>
>>> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
>>
>> It works for me:
>>
>> Tested-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>
>> I'm explicitly _not_ ACK'ing the patch, as I think the your code changes
>> below makes it harder to follow whether a TX or RX ring is getting
>> updated. But it is 100% up to the driver maintainers to say if this is
>> acceptable from a maintenance PoV.
>>
>>> ---
>>>   drivers/net/ethernet/intel/i40e/i40e_main.c | 24 +++++++++++----------
>>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> index e40c023cc7b6..7c122dd3faa1 100644
>>> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
>>> @@ -425,9 +425,9 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
>>>   				  struct rtnl_link_stats64 *stats)
>>>   {
>>>   	struct i40e_netdev_priv *np = netdev_priv(netdev);
>>> -	struct i40e_ring *tx_ring, *rx_ring;
>>>   	struct i40e_vsi *vsi = np->vsi;
>>>   	struct rtnl_link_stats64 *vsi_stats = i40e_get_vsi_stats_struct(vsi);
>>> +	struct i40e_ring *ring;
>>>   	int i;
>>>   
>>>   	if (test_bit(__I40E_VSI_DOWN, vsi->state))
>>> @@ -441,24 +441,26 @@ static void i40e_get_netdev_stats_struct(struct net_device *netdev,
>>>   		u64 bytes, packets;
>>>   		unsigned int start;
>>>   
>>> -		tx_ring = READ_ONCE(vsi->tx_rings[i]);
>>> -		if (!tx_ring)
>>> +		ring = READ_ONCE(vsi->tx_rings[i]);
>>> +		if (!ring)
>>>   			continue;
>>> -		i40e_get_netdev_stats_struct_tx(tx_ring, stats);
>>> +		i40e_get_netdev_stats_struct_tx(ring, stats);
>>>   
>>> -		rx_ring = &tx_ring[1];
>>> +		if (i40e_enabled_xdp_vsi(vsi)) {
>>> +			ring++;
>>> +			i40e_get_netdev_stats_struct_tx(ring, stats);
>>> +		}
>>>   
>>> +		ring++;
>>>   		do {
>>> -			start = u64_stats_fetch_begin_irq(&rx_ring->syncp);
>>> -			packets = rx_ring->stats.packets;
>>> -			bytes   = rx_ring->stats.bytes;
>>> -		} while (u64_stats_fetch_retry_irq(&rx_ring->syncp, start));
>>> +			start   = u64_stats_fetch_begin_irq(&ring->syncp);
>>> +			packets = ring->stats.packets;
>>> +			bytes   = ring->stats.bytes;
>>> +		} while (u64_stats_fetch_retry_irq(&ring->syncp, start));
>>>   
>>>   		stats->rx_packets += packets;
>>>   		stats->rx_bytes   += bytes;
>>>   
>>> -		if (i40e_enabled_xdp_vsi(vsi))
>>> -			i40e_get_netdev_stats_struct_tx(&rx_ring[1], stats);
>>>   	}
>>>   	rcu_read_unlock();
>>>   
> 
> 
> Kind regards,
> 
> Paul
>