* Re: [PATCH] i40e: report correct statistics when XDP is enabled
[not found] <20180824112159.32298-1-bjorn.topel@intel.com>
@ 2018-08-24 14:00 ` Jesper Dangaard Brouer
2018-08-28 17:00 ` [Intel-wired-lan] " Paul Menzel
0 siblings, 1 reply; 3+ messages in thread
From: Jesper Dangaard Brouer @ 2018-08-24 14:00 UTC (permalink / raw)
To: Björn Töpel
Cc: jeffrey.t.kirsher, intel-wired-lan, magnus.karlsson,
magnus.karlsson, jacob.e.keller, bjorn.topel, brouer,
netdev@vger.kernel.org
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled
2018-08-24 14:00 ` [PATCH] i40e: report correct statistics when XDP is enabled Jesper Dangaard Brouer
@ 2018-08-28 17:00 ` Paul Menzel
2018-08-28 17:10 ` Björn Töpel
0 siblings, 1 reply; 3+ messages in thread
From: Paul Menzel @ 2018-08-28 17:00 UTC (permalink / raw)
To: Björn Töpel
Cc: Jesper Dangaard Brouer, netdev, intel-wired-lan, Magnus Karlsson,
Magnus Karlsson
[-- 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 --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled
2018-08-28 17:00 ` [Intel-wired-lan] " Paul Menzel
@ 2018-08-28 17:10 ` Björn Töpel
0 siblings, 0 replies; 3+ messages in thread
From: Björn Töpel @ 2018-08-28 17:10 UTC (permalink / raw)
To: Paul Menzel
Cc: Jesper Dangaard Brouer, netdev, intel-wired-lan, Magnus Karlsson,
Magnus Karlsson
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-08-28 21:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20180824112159.32298-1-bjorn.topel@intel.com>
2018-08-24 14:00 ` [PATCH] i40e: report correct statistics when XDP is enabled Jesper Dangaard Brouer
2018-08-28 17:00 ` [Intel-wired-lan] " Paul Menzel
2018-08-28 17:10 ` Björn Töpel
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).