[PATCH] e1000: Real time packets and bytes statistics

[PATCH] e1000: Real time packets and bytes statistics

[Jean Delvare]

Hi all,

This patch is posted for review and comments.

Let the e1000 driver report the most important statistics (rx/tx_bytes
and rx/tx_packets) in real time, rather than every other second. This
is similar to what the e100 driver is doing.

The current asynchronous statistics refresh model makes it impossible
to monitor the network traffic with an interval which isn't a multiple
of 2 seconds. For example, an interval of 5 seconds would result in a
sawtooth diagram (+20%, -20%) for a constant transfer rate. With a 1
second interval it's even worse (0, 200%) of course. This has been
annoying users for years, but was never actually fixed:

LKML thread "e1000 statistics timer", August 2003:
  http://lkml.org/lkml/2003/8/2/127
Bug filled against SUSE LINUX 9.0 Professional, September 2003:
  https://bugzilla.novell.com/show_bug.cgi?id=46464
  (access is restricted, sorry about that)
LKML thread "minor e1000 bug", December 2003:
  http://marc.theaimsgroup.com/?t=107190957000001
Bug filled against Ubuntu Dapper Drake, August 2006:
  https://launchpad.net/distros/ubuntu/+source/linux-source-2.6.15/+bug/55989
Kernel bug #6986, August 2006:
  http://bugzilla.kernel.org/show_bug.cgi?id=6986

rx/tx_bytes will show slightly lower values than before, because the
hardware appears to include the 4-byte ethernet frame CRC into the
frame length, while the driver doesn't. It's probably OK as the
e100, 3c59x and 8139too drivers don't include it either.

I additionally noted a difference of 6 bytes on some TX frames, which
I am not able to explain. It's probably small and rare enough not to
be considered a problem, but if someone can explain it, I would be
grateful.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/net/e1000/e1000_main.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

--- linux-2.6.19-rc1.orig/drivers/net/e1000/e1000_main.c	2006-10-11 10:53:49.000000000 +0200
+++ linux-2.6.19-rc1/drivers/net/e1000/e1000_main.c	2006-10-11 11:34:41.000000000 +0200
@@ -3118,6 +3118,8 @@
 	               e1000_tx_map(adapter, tx_ring, skb, first,
 	                            max_per_txd, nr_frags, mss));
 
+	adapter->net_stats.tx_packets++;
+	adapter->net_stats.tx_bytes += skb->len;
 	netdev->trans_start = jiffies;
 
 	/* Make sure there is space in the ring for the next send. */
@@ -3384,10 +3386,8 @@
 
 	/* Fill out the OS statistics structure */
 
-	adapter->net_stats.rx_packets = adapter->stats.gprc;
-	adapter->net_stats.tx_packets = adapter->stats.gptc;
-	adapter->net_stats.rx_bytes = adapter->stats.gorcl;
-	adapter->net_stats.tx_bytes = adapter->stats.gotcl;
+	/* rx/tx_packets and rx/tx_bytes are computed in real time by
+	   the driver */
 	adapter->net_stats.multicast = adapter->stats.mprc;
 	adapter->net_stats.collisions = adapter->stats.colc;
 
@@ -3833,6 +3833,7 @@
 				  ((uint32_t)(rx_desc->errors) << 24),
 				  le16_to_cpu(rx_desc->csum), skb);
 
+		length = skb->len;	/* Save actual length for stats */
 		skb->protocol = eth_type_trans(skb, netdev);
 #ifdef CONFIG_E1000_NAPI
 		if (unlikely(adapter->vlgrp &&
@@ -3853,6 +3854,8 @@
 			netif_rx(skb);
 		}
 #endif /* CONFIG_E1000_NAPI */
+		adapter->net_stats.rx_packets++;
+		adapter->net_stats.rx_bytes += length;
 		netdev->last_rx = jiffies;
 
 next_desc:
@@ -4009,6 +4012,7 @@
 copydone:
 		e1000_rx_checksum(adapter, staterr,
 				  le16_to_cpu(rx_desc->wb.lower.hi_dword.csum_ip.csum), skb);
+		length = skb->len;	/* Save actual length for stats */
 		skb->protocol = eth_type_trans(skb, netdev);
 
 		if (likely(rx_desc->wb.upper.header_status &
@@ -4031,6 +4035,8 @@
 			netif_rx(skb);
 		}
 #endif /* CONFIG_E1000_NAPI */
+		adapter->net_stats.rx_packets++;
+		adapter->net_stats.rx_bytes += length;
 		netdev->last_rx = jiffies;
 
 next_desc:

Thanks,
-- 
Jean Delvare

Re: [PATCH] e1000: Real time packets and bytes statistics

[Jesse Brandeburg]

On 10/11/06, Jean Delvare <khali@linux-fr.org> wrote:
> Hi all,
>
> This patch is posted for review and comments.
>
> Let the e1000 driver report the most important statistics (rx/tx_bytes
> and rx/tx_packets) in real time, rather than every other second. This
> is similar to what the e100 driver is doing.
>
> The current asynchronous statistics refresh model makes it impossible
> to monitor the network traffic with an interval which isn't a multiple
> of 2 seconds. For example, an interval of 5 seconds would result in a
> sawtooth diagram (+20%, -20%) for a constant transfer rate. With a 1
> second interval it's even worse (0, 200%) of course. This has been
> annoying users for years, but was never actually fixed:

I think the idea is good, however, see below.

<snip
> rx/tx_bytes will show slightly lower values than before, because the
> hardware appears to include the 4-byte ethernet frame CRC into the
> frame length, while the driver doesn't. It's probably OK as the
> e100, 3c59x and 8139too drivers don't include it either.

this is okay.

> I additionally noted a difference of 6 bytes on some TX frames, which
> I am not able to explain. It's probably small and rare enough not to
> be considered a problem, but if someone can explain it, I would be
> grateful.

now, that sounds odd, however, once again, see below.

> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
>  drivers/net/e1000/e1000_main.c |   14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> --- linux-2.6.19-rc1.orig/drivers/net/e1000/e1000_main.c        2006-10-11 10:53:49.000000000 +0200
> +++ linux-2.6.19-rc1/drivers/net/e1000/e1000_main.c     2006-10-11 11:34:41.000000000 +0200
> @@ -3118,6 +3118,8 @@
>                        e1000_tx_map(adapter, tx_ring, skb, first,
>                                     max_per_txd, nr_frags, mss));
>
> +       adapter->net_stats.tx_packets++;
> +       adapter->net_stats.tx_bytes += skb->len;
>         netdev->trans_start = jiffies;

this is the part I'm most worried about.  as I believe it to be
incorrect for TSO packets.  Maybe something like?
+       if (skb_shinfo(skb)->gso_segs)
+              adapter->net_stats.tx_packets += skb_shinfo(skb)->gso_segs;
+       else
+              adapter->net_stats.tx_packets++;
+       adapter->net_stats.tx_bytes += skb->len;
        netdev->trans_start = jiffies;

skb len will still be off by some amount, because the skb->data
(header) is replicated across each gso segment but only counted once
this way, but hopefully someone will pipe up with a good way to
compute that.

The rest of the patch seems fine, barring any other comments.

Jesse

Re: [PATCH] e1000: Real time packets and bytes statistics

[Stephen Hemminger]

On Wed, 11 Oct 2006 10:44:12 -0700
"Jesse Brandeburg" <jesse.brandeburg@gmail.com> wrote:

> On 10/11/06, Jean Delvare <khali@linux-fr.org> wrote:
> > Hi all,
> >
> > This patch is posted for review and comments.
> >
> > Let the e1000 driver report the most important statistics (rx/tx_bytes
> > and rx/tx_packets) in real time, rather than every other second. This
> > is similar to what the e100 driver is doing.
> >
> > The current asynchronous statistics refresh model makes it impossible
> > to monitor the network traffic with an interval which isn't a multiple
> > of 2 seconds. For example, an interval of 5 seconds would result in a
> > sawtooth diagram (+20%, -20%) for a constant transfer rate. With a 1
> > second interval it's even worse (0, 200%) of course. This has been
> > annoying users for years, but was never actually fixed:
> 
> I think the idea is good, however, see below.
> 
> <snip
> > rx/tx_bytes will show slightly lower values than before, because the
> > hardware appears to include the 4-byte ethernet frame CRC into the
> > frame length, while the driver doesn't. It's probably OK as the
> > e100, 3c59x and 8139too drivers don't include it either.
> 
> this is okay.
> 
> > I additionally noted a difference of 6 bytes on some TX frames, which
> > I am not able to explain. It's probably small and rare enough not to
> > be considered a problem, but if someone can explain it, I would be
> > grateful.
> 
> now, that sounds odd, however, once again, see below.
> 
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > ---
> >  drivers/net/e1000/e1000_main.c |   14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > --- linux-2.6.19-rc1.orig/drivers/net/e1000/e1000_main.c        2006-10-11 10:53:49.000000000 +0200
> > +++ linux-2.6.19-rc1/drivers/net/e1000/e1000_main.c     2006-10-11 11:34:41.000000000 +0200
> > @@ -3118,6 +3118,8 @@
> >                        e1000_tx_map(adapter, tx_ring, skb, first,
> >                                     max_per_txd, nr_frags, mss));
> >
> > +       adapter->net_stats.tx_packets++;
> > +       adapter->net_stats.tx_bytes += skb->len;
> >         netdev->trans_start = jiffies;
> 
> this is the part I'm most worried about.  as I believe it to be
> incorrect for TSO packets.  Maybe something like?
> +       if (skb_shinfo(skb)->gso_segs)
> +              adapter->net_stats.tx_packets += skb_shinfo(skb)->gso_segs;
> +       else
> +              adapter->net_stats.tx_packets++;
> +       adapter->net_stats.tx_bytes += skb->len;
>         netdev->trans_start = jiffies;
> 
> skb len will still be off by some amount, because the skb->data
> (header) is replicated across each gso segment but only counted once
> this way, but hopefully someone will pipe up with a good way to
> compute that.
> 
> The rest of the patch seems fine, barring any other comments.
> 
> Jesse

You might want to put the tx values in a per-cpu structure and
sum later.  Incrementing statistics can actually be a performance
bottleneck on SMP tests, because it causes lots of cache thrashing.


-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [PATCH] e1000: Real time packets and bytes statistics

[Jean Delvare]

Hi Stephen,

On 10/11/06, Stephen Hemminger wrote:
> On Wed, 11 Oct 2006, Jesse Brandeburg wrote:
> > On 10/11/06, Jean Delvare wrote:
> > > Let the e1000 driver report the most important statistics (rx/tx_bytes
> > > and rx/tx_packets) in real time, rather than every other second. This
> > > is similar to what the e100 driver is doing.
> > > (...)
> > > --- linux-2.6.19-rc1.orig/drivers/net/e1000/e1000_main.c        2006-10-11 10:53:49.000000000 +0200
> > > +++ linux-2.6.19-rc1/drivers/net/e1000/e1000_main.c     2006-10-11 11:34:41.000000000 +0200
> > > @@ -3118,6 +3118,8 @@
> > >                        e1000_tx_map(adapter, tx_ring, skb, first,
> > >                                     max_per_txd, nr_frags, mss));
> > >
> > > +       adapter->net_stats.tx_packets++;
> > > +       adapter->net_stats.tx_bytes += skb->len;
> > >         netdev->trans_start = jiffies;
> > 
> > this is the part I'm most worried about.  as I believe it to be
> > incorrect for TSO packets.  Maybe something like?
> > +       if (skb_shinfo(skb)->gso_segs)
> > +              adapter->net_stats.tx_packets += skb_shinfo(skb)->gso_segs;
> > +       else
> > +              adapter->net_stats.tx_packets++;
> > +       adapter->net_stats.tx_bytes += skb->len;
> >         netdev->trans_start = jiffies;
> > 
> > skb len will still be off by some amount, because the skb->data
> > (header) is replicated across each gso segment but only counted once
> > this way, but hopefully someone will pipe up with a good way to
> > compute that.
> 
> You might want to put the tx values in a per-cpu structure and
> sum later.  Incrementing statistics can actually be a performance
> bottleneck on SMP tests, because it causes lots of cache thrashing.

I don't really see how this would be implemented. Can you please point
me to other drivers which do it that way?

Thanks,
-- 
Jean Delvare

Re: [PATCH] e1000: Real time packets and bytes statistics

[Stephen Hemminger]

On Thu, 12 Oct 2006 23:02:52 +0200
Jean Delvare <khali@linux-fr.org> wrote:

> Hi Stephen,
> 
> On 10/11/06, Stephen Hemminger wrote:
> > On Wed, 11 Oct 2006, Jesse Brandeburg wrote:
> > > On 10/11/06, Jean Delvare wrote:
> > > > Let the e1000 driver report the most important statistics (rx/tx_bytes
> > > > and rx/tx_packets) in real time, rather than every other second. This
> > > > is similar to what the e100 driver is doing.
> > > > (...)
> > > > --- linux-2.6.19-rc1.orig/drivers/net/e1000/e1000_main.c        2006-10-11 10:53:49.000000000 +0200
> > > > +++ linux-2.6.19-rc1/drivers/net/e1000/e1000_main.c     2006-10-11 11:34:41.000000000 +0200
> > > > @@ -3118,6 +3118,8 @@
> > > >                        e1000_tx_map(adapter, tx_ring, skb, first,
> > > >                                     max_per_txd, nr_frags, mss));
> > > >
> > > > +       adapter->net_stats.tx_packets++;
> > > > +       adapter->net_stats.tx_bytes += skb->len;
> > > >         netdev->trans_start = jiffies;
> > > 
> > > this is the part I'm most worried about.  as I believe it to be
> > > incorrect for TSO packets.  Maybe something like?
> > > +       if (skb_shinfo(skb)->gso_segs)
> > > +              adapter->net_stats.tx_packets += skb_shinfo(skb)->gso_segs;
> > > +       else
> > > +              adapter->net_stats.tx_packets++;
> > > +       adapter->net_stats.tx_bytes += skb->len;
> > >         netdev->trans_start = jiffies;
> > > 
> > > skb len will still be off by some amount, because the skb->data
> > > (header) is replicated across each gso segment but only counted once
> > > this way, but hopefully someone will pipe up with a good way to
> > > compute that.
> > 
> > You might want to put the tx values in a per-cpu structure and
> > sum later.  Incrementing statistics can actually be a performance
> > bottleneck on SMP tests, because it causes lots of cache thrashing.
> 
> I don't really see how this would be implemented. Can you please point
> me to other drivers which do it that way?
> 
> Thanks,

Loopback (drivers/net/loopback.c) does it, but it is simpler since it doesn't
have to support multiple interfaces. In a normal driver you would have to use
indirection and alloc_percpu() like af_inet.c does.

-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [PATCH] e1000: Real time packets and bytes statistics

[Jean Delvare]

Hi Jesse,

On 10/11/06, Jesse Brandeburg wrote:
> On 10/11/06, Jean Delvare wrote:
> > Let the e1000 driver report the most important statistics (rx/tx_bytes
> > and rx/tx_packets) in real time, rather than every other second. This
> > is similar to what the e100 driver is doing.
> >
> > The current asynchronous statistics refresh model makes it impossible
> > to monitor the network traffic with an interval which isn't a multiple
> > of 2 seconds. For example, an interval of 5 seconds would result in a
> > sawtooth diagram (+20%, -20%) for a constant transfer rate. With a 1
> > second interval it's even worse (0, 200%) of course. This has been
> > annoying users for years, but was never actually fixed:
> 
> I think the idea is good, however, see below.

Good news :)

> > I additionally noted a difference of 6 bytes on some TX frames, which
> > I am not able to explain. It's probably small and rare enough not to
> > be considered a problem, but if someone can explain it, I would be
> > grateful.
> 
> now, that sounds odd, however, once again, see below.

What you say below about TSO can't possibly explain this difference, as
your fix is about tx_packets while the difference I observed was on
tx_bytes only, the packet count was always correct. I'll investigate
tomorrow, if I can find a pattern for these differences I might discover
what these bytes are. For now the only idea I have is that 6 bytes is
ETH_ALEN, the size of an ethernet MAC address - but that doesn't
explain anything per se.

> >  drivers/net/e1000/e1000_main.c |   14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > --- linux-2.6.19-rc1.orig/drivers/net/e1000/e1000_main.c        2006-10-11 10:53:49.000000000 +0200
> > +++ linux-2.6.19-rc1/drivers/net/e1000/e1000_main.c     2006-10-11 11:34:41.000000000 +0200
> > @@ -3118,6 +3118,8 @@
> >                        e1000_tx_map(adapter, tx_ring, skb, first,
> >                                     max_per_txd, nr_frags, mss));
> >
> > +       adapter->net_stats.tx_packets++;
> > +       adapter->net_stats.tx_bytes += skb->len;
> >         netdev->trans_start = jiffies;
> 
> this is the part I'm most worried about.  as I believe it to be
> incorrect for TSO packets.  Maybe something like?

I have to admit I have very little experience with network drivers and
I didn't have the slightest idea what TSO was until I looked into
wikipedia two minutes ago. So I seem to understand that the skb
structure in the code above could correspond to several packets sent on
the wire by the ethernet adapter when TSO is used? Seems to be very
recent, 2.6.16 didn't have that.

> +       if (skb_shinfo(skb)->gso_segs)
> +              adapter->net_stats.tx_packets += skb_shinfo(skb)->gso_segs;
> +       else
> +              adapter->net_stats.tx_packets++;
> +       adapter->net_stats.tx_bytes += skb->len;
>         netdev->trans_start = jiffies;

My comparisons between hardware and software-computed statistics did
not reveal any difference with regards to tx_packets, while there
should have been one if the change above is needed. This suggests that
my tests (run on 2.6.18) did not trigger any TSO packet? Can you
suggest a way to generate such packets so that I am sure I exercise
this code path?

I found an inline function in include/net/tcp.h, tcp_skb_pcount(),
which evaluates to skb_shinfo(skb)->gso_segs. I guess I should use that
instead of the above?

> skb len will still be off by some amount, because the skb->data
> (header) is replicated across each gso segment but only counted once
> this way, but hopefully someone will pipe up with a good way to
> compute that.

And nearby there is another inline function, tcp_skb_mss(), which
evaluates to skb_shinfo(skb)->gso_size... I can't experiment with that
until I know a way to trigger TSO packets, but could it be the size
we're after?

Thanks a lot for your guidance.

-- 
Jean Delvare