| Message ID | 20090812.222046.60033759.davem@davemloft.net |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Aug 12, 2009 at 10:20:46PM -0700, David Miller wrote: > From: Dave Jones <davej@redhat.com> > Date: Wed, 12 Aug 2009 13:13:33 -0400 > > > There's another instance of the same further in the file. > > > > Does this look right? > > Dave, Francois posted the following fix to lkml (he forgot > to CC: netdev and the people reporting this bug, oh well) Ah, I missed it (disk crashed on friday, so I've been backlogged since then). Thanks for the forward. > Did you see it? It should fix the bug. > > 8139cp: balance dma_map_single vs dma_unmap_single pair > > The driver always: > 1. allocate cp->rx_buf_sz + NET_IP_ALIGN > 2. map cp->rx_buf_sz > > Signed-off-by: Francois Romieu <romieu@fr.zoreil.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > --- > drivers/net/8139cp.c | 5 ++--- > 1 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/8139cp.c b/drivers/net/8139cp.c > index 50efde1..d0dbbf3 100644 > --- a/drivers/net/8139cp.c > +++ b/drivers/net/8139cp.c > @@ -515,7 +515,7 @@ rx_status_loop: > dma_addr_t mapping; > struct sk_buff *skb, *new_skb; > struct cp_desc *desc; > - unsigned buflen; > + const unsigned buflen = cp->rx_buf_sz; > > skb = cp->rx_skb[rx_tail]; > BUG_ON(!skb); > @@ -549,8 +549,7 @@ rx_status_loop: > pr_debug("%s: rx slot %d status 0x%x len %d\n", > dev->name, rx_tail, status, len); > > - buflen = cp->rx_buf_sz + NET_IP_ALIGN; > - new_skb = netdev_alloc_skb(dev, buflen); > + new_skb = netdev_alloc_skb(dev, buflen + NET_IP_ALIGN); > if (!new_skb) { > dev->stats.rx_dropped++; > goto rx_next; Below this, we're still doing an skb_reserve(NET_IP_ALIGN) on new_skb. Although the mapping is now constantly sized, aren't we still wastefully bumping the data/tail of the skb twice ? Dave -- 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
Dave Jones <davej@redhat.com> : [...] > > @@ -549,8 +549,7 @@ rx_status_loop: > > pr_debug("%s: rx slot %d status 0x%x len %d\n", > > dev->name, rx_tail, status, len); > > > > - buflen = cp->rx_buf_sz + NET_IP_ALIGN; > > - new_skb = netdev_alloc_skb(dev, buflen); > > + new_skb = netdev_alloc_skb(dev, buflen + NET_IP_ALIGN); > > if (!new_skb) { > > dev->stats.rx_dropped++; > > goto rx_next; > > Below this, we're still doing an skb_reserve(NET_IP_ALIGN) on new_skb. > Although the mapping is now constantly sized, aren't we still wastefully > bumping the data/tail of the skb twice ? $ grep -n NET_IP_ALIGN drivers/net/8139cp.c 552: new_skb = netdev_alloc_skb(dev, buflen + NET_IP_ALIGN); 558: skb_reserve(new_skb, NET_IP_ALIGN); 1059: skb = netdev_alloc_skb(dev, cp->rx_buf_sz + NET_IP_ALIGN); 1063: skb_reserve(skb, NET_IP_ALIGN); I do not get it : netdev_alloc_skb allocates but it does not "bump" as skb_reserve does (and skb_reserve does not allocate). Where would the double bump come from ? The mapping is constantly sized but we want the ethernet header following IP data to be evenly aligned and thus the mapping to be oddly aligned.
On Thu, Aug 13, 2009 at 09:23:26PM +0200, Francois Romieu wrote: > > Below this, we're still doing an skb_reserve(NET_IP_ALIGN) on new_skb. > > Although the mapping is now constantly sized, aren't we still wastefully > > bumping the data/tail of the skb twice ? > > $ grep -n NET_IP_ALIGN drivers/net/8139cp.c > 552: new_skb = netdev_alloc_skb(dev, buflen + NET_IP_ALIGN); > 558: skb_reserve(new_skb, NET_IP_ALIGN); > 1059: skb = netdev_alloc_skb(dev, cp->rx_buf_sz + NET_IP_ALIGN); > 1063: skb_reserve(skb, NET_IP_ALIGN); > > I do not get it : netdev_alloc_skb allocates but it does not "bump" as > skb_reserve does (and skb_reserve does not allocate). Where would the > double bump come from ? > > The mapping is constantly sized but we want the ethernet header following > IP data to be evenly aligned and thus the mapping to be oddly aligned. Ah, now I get it. Thanks for the explanation :) Dave -- 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/8139cp.c b/drivers/net/8139cp.c index 50efde1..d0dbbf3 100644 --- a/drivers/net/8139cp.c +++ b/drivers/net/8139cp.c @@ -515,7 +515,7 @@ rx_status_loop: dma_addr_t mapping; struct sk_buff *skb, *new_skb; struct cp_desc *desc; - unsigned buflen; + const unsigned buflen = cp->rx_buf_sz; skb = cp->rx_skb[rx_tail]; BUG_ON(!skb); @@ -549,8 +549,7 @@ rx_status_loop: pr_debug("%s: rx slot %d status 0x%x len %d\n", dev->name, rx_tail, status, len); - buflen = cp->rx_buf_sz + NET_IP_ALIGN; - new_skb = netdev_alloc_skb(dev, buflen); + new_skb = netdev_alloc_skb(dev, buflen + NET_IP_ALIGN); if (!new_skb) { dev->stats.rx_dropped++; goto rx_next;