| Message ID | b838d871c0d507787373061e4edd91a624d62475.1339073391.git.rkagan@parallels.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
[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(-)