* [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled
@ 2018-08-24 11:21 =?unknown-8bit?q?Bj=C3=B6rn_T=C3=B6pel?=
2018-08-24 14:00 ` Jesper Dangaard Brouer
2018-08-28 16:50 ` Bowers, AndrewX
0 siblings, 2 replies; 8+ messages in thread
From: =?unknown-8bit?q?Bj=C3=B6rn_T=C3=B6pel?= @ 2018-08-24 11:21 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled 2018-08-24 11:21 [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled =?unknown-8bit?q?Bj=C3=B6rn_T=C3=B6pel?= @ 2018-08-24 14:00 ` Jesper Dangaard Brouer 2018-08-28 16:50 ` Bowers, AndrewX 1 sibling, 0 replies; 8+ messages in thread From: Jesper Dangaard Brouer @ 2018-08-24 14:00 UTC (permalink / raw) To: intel-wired-lan 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] 8+ messages in thread
* Re: [PATCH] i40e: report correct statistics when XDP is enabled @ 2018-08-24 14:00 ` Jesper Dangaard Brouer 0 siblings, 0 replies; 8+ 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] 8+ messages in thread
* [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled 2018-08-24 14:00 ` Jesper Dangaard Brouer @ 2018-08-28 17:00 ` Paul Menzel -1 siblings, 0 replies; 8+ messages in thread From: Paul Menzel @ 2018-08-28 17:00 UTC (permalink / raw) To: intel-wired-lan 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled @ 2018-08-28 17:00 ` Paul Menzel 0 siblings, 0 replies; 8+ 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] 8+ messages in thread
* [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled 2018-08-28 17:00 ` Paul Menzel @ 2018-08-28 17:10 ` Björn Töpel -1 siblings, 0 replies; 8+ messages in thread From: =?unknown-8bit?q?Bj=C3=B6rn_T=C3=B6pel?= @ 2018-08-28 17:10 UTC (permalink / raw) To: intel-wired-lan 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] 8+ messages in thread
* Re: [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled @ 2018-08-28 17:10 ` Björn Töpel 0 siblings, 0 replies; 8+ 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] 8+ messages in thread
* [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled 2018-08-24 11:21 [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled =?unknown-8bit?q?Bj=C3=B6rn_T=C3=B6pel?= 2018-08-24 14:00 ` Jesper Dangaard Brouer @ 2018-08-28 16:50 ` Bowers, AndrewX 1 sibling, 0 replies; 8+ messages in thread From: Bowers, AndrewX @ 2018-08-28 16:50 UTC (permalink / raw) To: intel-wired-lan > -----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> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-08-28 21:03 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-08-24 11:21 [Intel-wired-lan] [PATCH] i40e: report correct statistics when XDP is enabled =?unknown-8bit?q?Bj=C3=B6rn_T=C3=B6pel?= 2018-08-24 14:00 ` Jesper Dangaard Brouer 2018-08-24 14:00 ` Jesper Dangaard Brouer 2018-08-28 17:00 ` [Intel-wired-lan] " Paul Menzel 2018-08-28 17:00 ` Paul Menzel 2018-08-28 17:10 ` =?unknown-8bit?q?Bj=C3=B6rn_T=C3=B6pel?= 2018-08-28 17:10 ` Björn Töpel 2018-08-28 16:50 ` Bowers, AndrewX
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.