[Upstream commit 31c15a2f24ebdab14333d9bf5df49757842ae2ec with paths
adjusted to compensate for the drivers/net/ethernet/intel reorg in
dee1ad47f2ee75f5146d83ca757c1b7861c34c3b]
Author: Dean Nelson <[email protected]>
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 <[email protected]>
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 <[email protected]>
Acked-by: Jeff Kirsher <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Signed-off-by: Roman Kagan <[email protected]>
---
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
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 <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> Acked-by: Jeff Kirsher <[email protected]>
> Signed-off-by: David S. Miller <[email protected]>
> Signed-off-by: Roman Kagan <[email protected]>
> ---
> 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
From: Jeff Kirsher <[email protected]>
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.
On 06/07/2012 02:43 PM, David Miller wrote:
> From: Jeff Kirsher <[email protected]>
> 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.
On Thu, Jun 07, 2012 at 02:43:58PM -0700, David Miller wrote:
> From: Jeff Kirsher <[email protected]>
> 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
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 <[email protected]>
> > 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.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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 <[email protected]>
> > > 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.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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 <[email protected]>
> > > > 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