[PATCH] e1000: save skb counts in TX to avoid cache misses

[PATCH] e1000: save skb counts in TX to avoid cache misses

[Roman Kagan]

[Upstream commit 31c15a2f24ebdab14333d9bf5df49757842ae2ec with paths
adjusted to compensate for the drivers/net/ethernet/intel reorg in
dee1ad47f2ee75f5146d83ca757c1b7861c34c3b]

Author: Dean Nelson <dnelson@redhat.com>
Date:   Thu Aug 25 14:39:24 2011 +0000

    e1000: save skb counts in TX to avoid cache misses

    Virtual Machines with emulated e1000 network adapter running on Parallels'
    server were seeing kernel panics due to the e1000 driver dereferencing an
    unexpected NULL pointer retrieved from buffer_info->skb.

    The problem has been addressed for the e1000e driver, but not for the e1000.
    Since the two drivers share similar code in the affected area, a port of the
    following e1000e driver commit solves the issue for the e1000 driver:

    commit 9ed318d546a29d7a591dbe648fd1a2efe3be1180
    Author: Tom Herbert <therbert@google.com>
    Date:   Wed May 5 14:02:27 2010 +0000

        e1000e: save skb counts in TX to avoid cache misses

        In e1000_tx_map, precompute number of segements and bytecounts which
        are derived from fields in skb; these are stored in buffer_info.  When
        cleaning tx in e1000_clean_tx_irq use the values in the associated
        buffer_info for statistics counting, this eliminates cache misses
        on skb fields.

    Signed-off-by: Dean Nelson <dnelson@redhat.com>
    Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Roman Kagan <rkagan@parallels.com>
---
 drivers/net/e1000/e1000.h      |    2 ++
 drivers/net/e1000/e1000_main.c |   18 +++++++++---------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
index 8676899..2c71884 100644
--- a/drivers/net/e1000/e1000.h
+++ b/drivers/net/e1000/e1000.h
@@ -150,6 +150,8 @@ struct e1000_buffer {
 	unsigned long time_stamp;
 	u16 length;
 	u16 next_to_watch;
+	unsigned int segs;
+	unsigned int bytecount;
 	u16 mapped_as_page;
 };
 
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 76e8af0..99525f9 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -2798,7 +2798,7 @@ static int e1000_tx_map(struct e1000_adapter *adapter,
 	struct e1000_buffer *buffer_info;
 	unsigned int len = skb_headlen(skb);
 	unsigned int offset = 0, size, count = 0, i;
-	unsigned int f;
+	unsigned int f, bytecount, segs;
 
 	i = tx_ring->next_to_use;
 
@@ -2899,7 +2899,13 @@ static int e1000_tx_map(struct e1000_adapter *adapter,
 		}
 	}
 
+	segs = skb_shinfo(skb)->gso_segs ?: 1;
+	/* multiply data chunks by size of headers */
+	bytecount = ((segs - 1) * skb_headlen(skb)) + skb->len;
+
 	tx_ring->buffer_info[i].skb = skb;
+	tx_ring->buffer_info[i].segs = segs;
+	tx_ring->buffer_info[i].bytecount = bytecount;
 	tx_ring->buffer_info[first].next_to_watch = i;
 
 	return count;
@@ -3573,14 +3579,8 @@ static bool e1000_clean_tx_irq(struct e1000_adapter *adapter,
 			cleaned = (i == eop);
 
 			if (cleaned) {
-				struct sk_buff *skb = buffer_info->skb;
-				unsigned int segs, bytecount;
-				segs = skb_shinfo(skb)->gso_segs ?: 1;
-				/* multiply data chunks by size of headers */
-				bytecount = ((segs - 1) * skb_headlen(skb)) +
-				            skb->len;
-				total_tx_packets += segs;
-				total_tx_bytes += bytecount;
+				total_tx_packets += buffer_info->segs;
+				total_tx_bytes += buffer_info->bytecount;
 			}
 			e1000_unmap_and_free_tx_resource(adapter, buffer_info);
 			tx_desc->upper.data = 0;
-- 
1.7.10.2

Re: [PATCH] e1000: save skb counts in TX to avoid cache misses

[Jeff Kirsher]

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

On 06/07/2012 05:49 AM, Roman Kagan wrote:
> [Upstream commit 31c15a2f24ebdab14333d9bf5df49757842ae2ec with paths
> adjusted to compensate for the drivers/net/ethernet/intel reorg in
> dee1ad47f2ee75f5146d83ca757c1b7861c34c3b]
>
> Author: Dean Nelson <dnelson@redhat.com>
> Date:   Thu Aug 25 14:39:24 2011 +0000
>
>     e1000: save skb counts in TX to avoid cache misses
>
>     Virtual Machines with emulated e1000 network adapter running on Parallels'
>     server were seeing kernel panics due to the e1000 driver dereferencing an
>     unexpected NULL pointer retrieved from buffer_info->skb.
>
>     The problem has been addressed for the e1000e driver, but not for the e1000.
>     Since the two drivers share similar code in the affected area, a port of the
>     following e1000e driver commit solves the issue for the e1000 driver:
>
>     commit 9ed318d546a29d7a591dbe648fd1a2efe3be1180
>     Author: Tom Herbert <therbert@google.com>
>     Date:   Wed May 5 14:02:27 2010 +0000
>
>         e1000e: save skb counts in TX to avoid cache misses
>
>         In e1000_tx_map, precompute number of segements and bytecounts which
>         are derived from fields in skb; these are stored in buffer_info.  When
>         cleaning tx in e1000_clean_tx_irq use the values in the associated
>         buffer_info for statistics counting, this eliminates cache misses
>         on skb fields.
>
>     Signed-off-by: Dean Nelson <dnelson@redhat.com>
>     Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Roman Kagan <rkagan@parallels.com>
> ---
>  drivers/net/e1000/e1000.h      |    2 ++
>  drivers/net/e1000/e1000_main.c |   18 +++++++++---------
>  2 files changed, 11 insertions(+), 9 deletions(-)
Thanks! I have applied the patch to my queue


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]

Re: [PATCH] e1000: save skb counts in TX to avoid cache misses

[David Miller]

From: Jeff Kirsher <tarbal@gmail.com>
Date: Thu, 07 Jun 2012 14:38:17 -0700

> Thanks! I have applied the patch to my queue

Why?

My impression is that this is a patch already in the tree, and it's
being submitted for -stable but such minor performance hacks are
absolutely not appropriate for -stable submission.

Re: [PATCH] e1000: save skb counts in TX to avoid cache misses

[Jeff Kirsher]

[-- Attachment #1.1: Type: text/plain, Size: 784 bytes --]

On 06/07/2012 02:43 PM, David Miller wrote:
> From: Jeff Kirsher <tarbal@gmail.com>
> Date: Thu, 07 Jun 2012 14:38:17 -0700
>
>> Thanks! I have applied the patch to my queue
> Why?
>
> My impression is that this is a patch already in the tree, and it's
> being submitted for -stable but such minor performance hacks are
> absolutely not appropriate for -stable submission.
I did not catch that Roman was trying to get this into stable because
there was no mention of what stable kernels this was applicable back to
(and the fact that it was a performance).

I thought he had found an issue with the previous commits and was
suggesting a fix to the previous patches.

Since he is trying to get this into -stable, disregard my statement
about adding it to my tree.


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]

[-- Attachment #2: Type: text/plain, Size: 395 bytes --]

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]

_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH] e1000: save skb counts in TX to avoid cache misses

[Greg KH]

On Thu, Jun 07, 2012 at 02:43:58PM -0700, David Miller wrote:
> From: Jeff Kirsher <tarbal@gmail.com>
> Date: Thu, 07 Jun 2012 14:38:17 -0700
> 
> > Thanks! I have applied the patch to my queue
> 
> Why?
> 
> My impression is that this is a patch already in the tree, and it's
> being submitted for -stable but such minor performance hacks are
> absolutely not appropriate for -stable submission.

The patch description says it is fixing reported oopses, but the
Subject: isn't all that helpful there.

So which is this?  Should I accept it for a stable release or not?

thanks,

greg k-h

Re: [PATCH] e1000: save skb counts in TX to avoid cache misses

[Roman Kagan]

On Fri, 2012-06-08 at 06:15 +0400, Greg KH wrote:
> On Thu, Jun 07, 2012 at 02:43:58PM -0700, David Miller wrote:
> > From: Jeff Kirsher <tarbal@gmail.com>
> > Date: Thu, 07 Jun 2012 14:38:17 -0700
> > 
> > > Thanks! I have applied the patch to my queue
> > 
> > Why?
> > 
> > My impression is that this is a patch already in the tree, and it's
> > being submitted for -stable but such minor performance hacks are
> > absolutely not appropriate for -stable submission.
> 
> The patch description says it is fixing reported oopses,

Exactly.

> but the Subject: isn't all that helpful there.

Well I just preserved the original subject from the upstream commit.
Want me to resubmit with a more alarming one?

> So which is this?  Should I accept it for a stable release or not?

IMO yes ;)

Thanks,
Roman.

Re: [PATCH] e1000: save skb counts in TX to avoid cache misses

[Roman Kagan]

On Fri, 2012-06-08 at 11:37 +0400, Roman Kagan wrote:
> On Fri, 2012-06-08 at 06:15 +0400, Greg KH wrote:
> > On Thu, Jun 07, 2012 at 02:43:58PM -0700, David Miller wrote:
> > > From: Jeff Kirsher <tarbal@gmail.com>
> > > Date: Thu, 07 Jun 2012 14:38:17 -0700
> > > 
> > > > Thanks! I have applied the patch to my queue
> > > 
> > > Why?
> > > 
> > > My impression is that this is a patch already in the tree, and it's
> > > being submitted for -stable but such minor performance hacks are
> > > absolutely not appropriate for -stable submission.
> > 
> > The patch description says it is fixing reported oopses,
> 
> Exactly.
> 
> > but the Subject: isn't all that helpful there.
> 
> Well I just preserved the original subject from the upstream commit.
> Want me to resubmit with a more alarming one?
> 
> > So which is this?  Should I accept it for a stable release or not?
> 
> IMO yes ;)

What came out of this discussion?  Should I resubmit with a different
subject, or the original one is good enough?

The patch resolves a real oops; we've seen it multiple times when
running Ubuntu-11.10 in virtual machines.  Upstream and RHEL have the
fix since long.  Ubuntu is waiting for 3.0-stable to merge it
(https://bugs.launchpad.net/bugs/1009545).

I'd appreciate any suggestion on how to proceed.

Thanks,
Roman.

Re: [PATCH] e1000: save skb counts in TX to avoid cache misses

[Greg KH]

On Wed, Jun 13, 2012 at 03:12:17PM +0400, Roman Kagan wrote:
> On Fri, 2012-06-08 at 11:37 +0400, Roman Kagan wrote:
> > On Fri, 2012-06-08 at 06:15 +0400, Greg KH wrote:
> > > On Thu, Jun 07, 2012 at 02:43:58PM -0700, David Miller wrote:
> > > > From: Jeff Kirsher <tarbal@gmail.com>
> > > > Date: Thu, 07 Jun 2012 14:38:17 -0700
> > > > 
> > > > > Thanks! I have applied the patch to my queue
> > > > 
> > > > Why?
> > > > 
> > > > My impression is that this is a patch already in the tree, and it's
> > > > being submitted for -stable but such minor performance hacks are
> > > > absolutely not appropriate for -stable submission.
> > > 
> > > The patch description says it is fixing reported oopses,
> > 
> > Exactly.
> > 
> > > but the Subject: isn't all that helpful there.
> > 
> > Well I just preserved the original subject from the upstream commit.
> > Want me to resubmit with a more alarming one?
> > 
> > > So which is this?  Should I accept it for a stable release or not?
> > 
> > IMO yes ;)
> 
> What came out of this discussion?  Should I resubmit with a different
> subject, or the original one is good enough?
> 
> The patch resolves a real oops; we've seen it multiple times when
> running Ubuntu-11.10 in virtual machines.  Upstream and RHEL have the
> fix since long.  Ubuntu is waiting for 3.0-stable to merge it
> (https://bugs.launchpad.net/bugs/1009545).

That's pretty funny that Ubuntu is letting me be the gatekeeper of fixes
to get to their customers, there's just so much wrong in that it's sad.

Anyway, I've queued it up for the next 3.0-stable release.

thanks,

greg k-h

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired