| Message ID | 1285243291-4520-1-git-send-email-sgruszka@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Stanislaw Gruszka <sgruszka@redhat.com> : > We have fedora bug report where driver fail to initialize after > suspend/resume because of memory allocation errors: > https://bugzilla.redhat.com/show_bug.cgi?id=629158 > > To fix use GFP_KERNEL allocation where possible. Feel free to add a Acked-by: Francois Romieu <romieu@fr.zoreil.com> as soon as it will have been explicitely reported to improve the situation (it is not clear in the PR above).
On Thu, 23 Sep 2010 23:20:12 +0200 Francois Romieu <romieu@fr.zoreil.com> wrote: > Stanislaw Gruszka <sgruszka@redhat.com> : > > We have fedora bug report where driver fail to initialize after > > suspend/resume because of memory allocation errors: > > https://bugzilla.redhat.com/show_bug.cgi?id=629158 > > > > To fix use GFP_KERNEL allocation where possible. > > Feel free to add a Acked-by: Francois Romieu <romieu@fr.zoreil.com> > as soon as it will have been explicitely reported to improve the > situation (it is not clear in the PR above). I'm pretty sure patch fix the problem, however yes, we do not have confirmation from reporter yet. Anyway atomic allocation should not be used in process context. Stanislaw -- 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
Stanislaw Gruszka <sgruszka@redhat.com> : > Francois Romieu <romieu@fr.zoreil.com> wrote: [...] > > Feel free to add a Acked-by: Francois Romieu <romieu@fr.zoreil.com> > > as soon as it will have been explicitely reported to improve the > > situation (it is not clear in the PR above). > > I'm pretty sure patch fix the problem, however yes, we do not have > confirmation from reporter yet. A success report would help me swallow a 6 parameters rtl8169_alloc_rx_skb method. The driver is not smart wrt partially allocated rx ring, especially at init time. It considers a single init time skb allocation failure fatal. > Anyway atomic allocation should not be used in process context. What do you mean ? tg3->open() does not seem to bother. It is not alone.
From: Francois Romieu <romieu@fr.zoreil.com> Date: Sat, 25 Sep 2010 00:24:34 +0200 > Stanislaw Gruszka <sgruszka@redhat.com> : >> Francois Romieu <romieu@fr.zoreil.com> wrote: > [...] >> Anyway atomic allocation should not be used in process context. > > What do you mean ? tg3->open() does not seem to bother. It is not alone. I think this is merely an indication that r8169 is more often used in systems that actually suspend/resume than tg3 is. tg3 ought to be doing this too for correctness, as should pretty much every network driver. Stanislaw's patches are very reasonable, especially if the problem is happening. But yes a "Tested-by: " confirming the fix would really be appeciated before we apply this. -- 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
Le vendredi 24 septembre 2010 à 22:37 -0700, David Miller a écrit : > From: Francois Romieu <romieu@fr.zoreil.com> > Date: Sat, 25 Sep 2010 00:24:34 +0200 > > > Stanislaw Gruszka <sgruszka@redhat.com> : > >> Francois Romieu <romieu@fr.zoreil.com> wrote: > > [...] > >> Anyway atomic allocation should not be used in process context. > > > > What do you mean ? tg3->open() does not seem to bother. It is not alone. > > I think this is merely an indication that r8169 is more often > used in systems that actually suspend/resume than tg3 is. tg3 > ought to be doing this too for correctness, as should pretty much > every network driver. > Sure but most people use MTU=1500, so tg3 works well. Only r8169 allocates high order pages even with normal MTU > Stanislaw's patches are very reasonable, especially if the problem > is happening. > > But yes a "Tested-by: " confirming the fix would really be > appeciated before we apply this. > -- Patch solves the suspend/resume, probably, but as soon as we receive trafic, we can hit the allocation error anyway... -- 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
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Sat, 25 Sep 2010 08:06:37 +0200 > Patch solves the suspend/resume, probably, but as soon as we receive > trafic, we can hit the allocation error anyway... It allocates 1536 + N, where N can be NET_IP_ALIGN, or some small value like 8. This is in the same ballpark as what tg3 allocates for RX buffers. SLAB/SLUB/whatever just wants multi-order page allocations even for SKBs which are about this size. Furthermore, the sleeping allocations we do at ->open() time to allocate the entire RX ring all at once will buddy up a lot of pages and make 1-order allocs more likely. -- 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
Le samedi 25 septembre 2010 à 00:13 -0700, David Miller a écrit : > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Sat, 25 Sep 2010 08:06:37 +0200 > > > Patch solves the suspend/resume, probably, but as soon as we receive > > trafic, we can hit the allocation error anyway... > > It allocates 1536 + N, where N can be NET_IP_ALIGN, or some small > value like 8. > > This is in the same ballpark as what tg3 allocates for RX buffers. > > SLAB/SLUB/whatever just wants multi-order page allocations even > for SKBs which are about this size. > > Furthermore, the sleeping allocations we do at ->open() time to > allocate the entire RX ring all at once will buddy up a lot of > pages and make 1-order allocs more likely. Yes, I forgot this problem about SLUB (I ended using SLAB on servers because of this order-3 problem on kmalloc(2048)) bnx2 uses GFP_KERNEL allocations at init time, but tg3 uses GFP_ATOMIC (because tp->lock is held). The r8169 current problem is its currently copying all incoming frames. I guess nobody cares or noticed the performance drop. (But commit c0cd884a is recent (2.6.34), this is not yet in production...) -- 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 Sat, 2010-09-25 at 11:12 +0200, Eric Dumazet wrote: [...] > The r8169 current problem is its currently copying all incoming frames. > I guess nobody cares or noticed the performance drop. > (But commit c0cd884a is recent (2.6.34), this is not yet in > production...) Since that was a security fix it was backported to 2.6.32.12 and probably most distribution kernels. So yes it is in production. Ben.
Le dimanche 26 septembre 2010 à 00:33 +0100, Ben Hutchings a écrit : > On Sat, 2010-09-25 at 11:12 +0200, Eric Dumazet wrote: > [...] > > The r8169 current problem is its currently copying all incoming frames. > > I guess nobody cares or noticed the performance drop. > > (But commit c0cd884a is recent (2.6.34), this is not yet in > > production...) > > Since that was a security fix it was backported to 2.6.32.12 and > probably most distribution kernels. So yes it is in production. Maybe in your company, not a single machine in mine ;) Most linux servers in production run much older kernels. rhel 5 -> 2.6.18 debian 5 -> 2.6.26.x -- 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 Sun, 2010-09-26 at 08:09 +0200, Eric Dumazet wrote: > Le dimanche 26 septembre 2010 à 00:33 +0100, Ben Hutchings a écrit : > > On Sat, 2010-09-25 at 11:12 +0200, Eric Dumazet wrote: > > [...] > > > The r8169 current problem is its currently copying all incoming frames. > > > I guess nobody cares or noticed the performance drop. > > > (But commit c0cd884a is recent (2.6.34), this is not yet in > > > production...) > > > > Since that was a security fix it was backported to 2.6.32.12 and > > probably most distribution kernels. So yes it is in production. > > Maybe in your company, not a single machine in mine ;) > > Most linux servers in production run much older kernels. > > rhel 5 -> 2.6.18 > debian 5 -> 2.6.26.x Those both have the fix. Look for CVE-2009-4537 in the changelog. Ben.
diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c index 5490033..4f94073 100644 --- a/drivers/net/r8169.c +++ b/drivers/net/r8169.c @@ -4006,7 +4006,7 @@ static inline void rtl8169_map_to_asic(struct RxDesc *desc, dma_addr_t mapping, static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev, struct net_device *dev, struct RxDesc *desc, int rx_buf_sz, - unsigned int align) + unsigned int align, gfp_t gfp) { struct sk_buff *skb; dma_addr_t mapping; @@ -4014,7 +4014,7 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev, pad = align ? align : NET_IP_ALIGN; - skb = netdev_alloc_skb(dev, rx_buf_sz + pad); + skb = __netdev_alloc_skb(dev, rx_buf_sz + pad, gfp); if (!skb) goto err_out; @@ -4045,7 +4045,7 @@ static void rtl8169_rx_clear(struct rtl8169_private *tp) } static u32 rtl8169_rx_fill(struct rtl8169_private *tp, struct net_device *dev, - u32 start, u32 end) + u32 start, u32 end, gfp_t gfp) { u32 cur; @@ -4060,7 +4060,7 @@ static u32 rtl8169_rx_fill(struct rtl8169_private *tp, struct net_device *dev, skb = rtl8169_alloc_rx_skb(tp->pci_dev, dev, tp->RxDescArray + i, - tp->rx_buf_sz, tp->align); + tp->rx_buf_sz, tp->align, gfp); if (!skb) break; @@ -4088,7 +4088,7 @@ static int rtl8169_init_ring(struct net_device *dev) memset(tp->tx_skb, 0x0, NUM_TX_DESC * sizeof(struct ring_info)); memset(tp->Rx_skbuff, 0x0, NUM_RX_DESC * sizeof(struct sk_buff *)); - if (rtl8169_rx_fill(tp, dev, 0, NUM_RX_DESC) != NUM_RX_DESC) + if (rtl8169_rx_fill(tp, dev, 0, NUM_RX_DESC, GFP_KERNEL) != NUM_RX_DESC) goto err_out; rtl8169_mark_as_last_descriptor(tp->RxDescArray + NUM_RX_DESC - 1); @@ -4587,7 +4587,7 @@ static int rtl8169_rx_interrupt(struct net_device *dev, count = cur_rx - tp->cur_rx; tp->cur_rx = cur_rx; - delta = rtl8169_rx_fill(tp, dev, tp->dirty_rx, tp->cur_rx); + delta = rtl8169_rx_fill(tp, dev, tp->dirty_rx, tp->cur_rx, GFP_ATOMIC); if (!delta && count) netif_info(tp, intr, dev, "no Rx buffer allocated\n"); tp->dirty_rx += delta;
We have fedora bug report where driver fail to initialize after suspend/resume because of memory allocation errors: https://bugzilla.redhat.com/show_bug.cgi?id=629158 To fix use GFP_KERNEL allocation where possible. Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com> --- drivers/net/r8169.c | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)