[PATCH 1/2 -next] r8169: allocate with GFP_KERNEL flag when able to sleep

[PATCH 1/2 -next] r8169: allocate with GFP_KERNEL flag when able to sleep

[Stanislaw Gruszka]

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(-)

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;
-- 
1.7.0.1

[PATCH 2/2 -next] r8169: use device model DMA API

[Stanislaw Gruszka]

Use DMA API as PCI equivalents will be deprecated. This change also
allow to allocate with GFP_KERNEL where possible.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/r8169.c |   53 +++++++++++++++++++++++++++-----------------------
 1 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 4f94073..51dd9ac 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -1217,7 +1217,8 @@ static void rtl8169_update_counters(struct net_device *dev)
 	if ((RTL_R8(ChipCmd) & CmdRxEnb) == 0)
 		return;
 
-	counters = pci_alloc_consistent(tp->pci_dev, sizeof(*counters), &paddr);
+	counters = dma_alloc_coherent(&tp->pci_dev->dev, sizeof(*counters),
+				      &paddr, GFP_KERNEL);
 	if (!counters)
 		return;
 
@@ -1238,7 +1239,8 @@ static void rtl8169_update_counters(struct net_device *dev)
 	RTL_W32(CounterAddrLow, 0);
 	RTL_W32(CounterAddrHigh, 0);
 
-	pci_free_consistent(tp->pci_dev, sizeof(*counters), counters, paddr);
+	dma_free_coherent(&tp->pci_dev->dev, sizeof(*counters), counters,
+			  paddr);
 }
 
 static void rtl8169_get_ethtool_stats(struct net_device *dev,
@@ -3298,15 +3300,15 @@ static int rtl8169_open(struct net_device *dev)
 
 	/*
 	 * Rx and Tx desscriptors needs 256 bytes alignment.
-	 * pci_alloc_consistent provides more.
+	 * dma_alloc_coherent provides more.
 	 */
-	tp->TxDescArray = pci_alloc_consistent(pdev, R8169_TX_RING_BYTES,
-					       &tp->TxPhyAddr);
+	tp->TxDescArray = dma_alloc_coherent(&pdev->dev, R8169_TX_RING_BYTES,
+					     &tp->TxPhyAddr, GFP_KERNEL);
 	if (!tp->TxDescArray)
 		goto err_pm_runtime_put;
 
-	tp->RxDescArray = pci_alloc_consistent(pdev, R8169_RX_RING_BYTES,
-					       &tp->RxPhyAddr);
+	tp->RxDescArray = dma_alloc_coherent(&pdev->dev, R8169_RX_RING_BYTES,
+					     &tp->RxPhyAddr, GFP_KERNEL);
 	if (!tp->RxDescArray)
 		goto err_free_tx_0;
 
@@ -3340,12 +3342,12 @@ out:
 err_release_ring_2:
 	rtl8169_rx_clear(tp);
 err_free_rx_1:
-	pci_free_consistent(pdev, R8169_RX_RING_BYTES, tp->RxDescArray,
-			    tp->RxPhyAddr);
+	dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
+			  tp->RxPhyAddr);
 	tp->RxDescArray = NULL;
 err_free_tx_0:
-	pci_free_consistent(pdev, R8169_TX_RING_BYTES, tp->TxDescArray,
-			    tp->TxPhyAddr);
+	dma_free_coherent(&pdev->dev, R8169_TX_RING_BYTES, tp->TxDescArray,
+			  tp->TxPhyAddr);
 	tp->TxDescArray = NULL;
 err_pm_runtime_put:
 	pm_runtime_put_noidle(&pdev->dev);
@@ -3981,7 +3983,7 @@ static void rtl8169_free_rx_skb(struct rtl8169_private *tp,
 {
 	struct pci_dev *pdev = tp->pci_dev;
 
-	pci_unmap_single(pdev, le64_to_cpu(desc->addr), tp->rx_buf_sz,
+	dma_unmap_single(&pdev->dev, le64_to_cpu(desc->addr), tp->rx_buf_sz,
 			 PCI_DMA_FROMDEVICE);
 	dev_kfree_skb(*sk_buff);
 	*sk_buff = NULL;
@@ -4020,7 +4022,7 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev,
 
 	skb_reserve(skb, align ? ((pad - 1) & (unsigned long)skb->data) : pad);
 
-	mapping = pci_map_single(pdev, skb->data, rx_buf_sz,
+	mapping = dma_map_single(&pdev->dev, skb->data, rx_buf_sz,
 				 PCI_DMA_FROMDEVICE);
 
 	rtl8169_map_to_asic(desc, mapping, rx_buf_sz);
@@ -4105,7 +4107,8 @@ static void rtl8169_unmap_tx_skb(struct pci_dev *pdev, struct ring_info *tx_skb,
 {
 	unsigned int len = tx_skb->len;
 
-	pci_unmap_single(pdev, le64_to_cpu(desc->addr), len, PCI_DMA_TODEVICE);
+	dma_unmap_single(&pdev->dev, le64_to_cpu(desc->addr), len,
+			 PCI_DMA_TODEVICE);
 	desc->opts1 = 0x00;
 	desc->opts2 = 0x00;
 	desc->addr = 0x00;
@@ -4249,7 +4252,8 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
 		txd = tp->TxDescArray + entry;
 		len = frag->size;
 		addr = ((void *) page_address(frag->page)) + frag->page_offset;
-		mapping = pci_map_single(tp->pci_dev, addr, len, PCI_DMA_TODEVICE);
+		mapping = dma_map_single(&tp->pci_dev->dev, addr, len,
+					 PCI_DMA_TODEVICE);
 
 		/* anti gcc 2.95.3 bugware (sic) */
 		status = opts1 | len | (RingEnd * !((entry + 1) % NUM_TX_DESC));
@@ -4319,7 +4323,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
 		tp->tx_skb[entry].skb = skb;
 	}
 
-	mapping = pci_map_single(tp->pci_dev, skb->data, len, PCI_DMA_TODEVICE);
+	mapping = dma_map_single(&tp->pci_dev->dev, skb->data, len,
+				 PCI_DMA_TODEVICE);
 
 	tp->tx_skb[entry].len = len;
 	txd->addr = cpu_to_le64(mapping);
@@ -4482,8 +4487,8 @@ static inline bool rtl8169_try_rx_copy(struct sk_buff **sk_buff,
 	if (!skb)
 		goto out;
 
-	pci_dma_sync_single_for_cpu(tp->pci_dev, addr, pkt_size,
-				    PCI_DMA_FROMDEVICE);
+	dma_sync_single_for_cpu(&tp->pci_dev->dev, addr, pkt_size,
+				PCI_DMA_FROMDEVICE);
 	skb_copy_from_linear_data(*sk_buff, skb->data, pkt_size);
 	*sk_buff = skb;
 	done = true;
@@ -4552,11 +4557,11 @@ static int rtl8169_rx_interrupt(struct net_device *dev,
 			}
 
 			if (rtl8169_try_rx_copy(&skb, tp, pkt_size, addr)) {
-				pci_dma_sync_single_for_device(pdev, addr,
+				dma_sync_single_for_device(&pdev->dev, addr,
 					pkt_size, PCI_DMA_FROMDEVICE);
 				rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
 			} else {
-				pci_unmap_single(pdev, addr, tp->rx_buf_sz,
+				dma_unmap_single(&pdev->dev, addr, tp->rx_buf_sz,
 						 PCI_DMA_FROMDEVICE);
 				tp->Rx_skbuff[entry] = NULL;
 			}
@@ -4774,10 +4779,10 @@ static int rtl8169_close(struct net_device *dev)
 
 	free_irq(dev->irq, dev);
 
-	pci_free_consistent(pdev, R8169_RX_RING_BYTES, tp->RxDescArray,
-			    tp->RxPhyAddr);
-	pci_free_consistent(pdev, R8169_TX_RING_BYTES, tp->TxDescArray,
-			    tp->TxPhyAddr);
+	dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
+			  tp->RxPhyAddr);
+	dma_free_coherent(&pdev->dev, R8169_TX_RING_BYTES, tp->TxDescArray,
+			  tp->TxPhyAddr);
 	tp->TxDescArray = NULL;
 	tp->RxDescArray = NULL;
 
-- 
1.7.0.1

Re: [PATCH 2/2 -next] r8169: use device model DMA API

[Denis Kirjanov]

On Thu, Sep 23, 2010 at 4:01 PM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> Use DMA API as PCI equivalents will be deprecated. This change also
> allow to allocate with GFP_KERNEL where possible.
>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---

dma_map_single and friend can fail. Thus, we should check for a return value.
Yes, this is not directly related to current patch, but it would be
great to fix this.

>  drivers/net/r8169.c |   53 +++++++++++++++++++++++++++-----------------------
>  1 files changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
> index 4f94073..51dd9ac 100644
> --- a/drivers/net/r8169.c
> +++ b/drivers/net/r8169.c
> @@ -1217,7 +1217,8 @@ static void rtl8169_update_counters(struct net_device *dev)
>        if ((RTL_R8(ChipCmd) & CmdRxEnb) == 0)
>                return;
>
> -       counters = pci_alloc_consistent(tp->pci_dev, sizeof(*counters), &paddr);
> +       counters = dma_alloc_coherent(&tp->pci_dev->dev, sizeof(*counters),
> +                                     &paddr, GFP_KERNEL);
>        if (!counters)
>                return;
>
> @@ -1238,7 +1239,8 @@ static void rtl8169_update_counters(struct net_device *dev)
>        RTL_W32(CounterAddrLow, 0);
>        RTL_W32(CounterAddrHigh, 0);
>
> -       pci_free_consistent(tp->pci_dev, sizeof(*counters), counters, paddr);
> +       dma_free_coherent(&tp->pci_dev->dev, sizeof(*counters), counters,
> +                         paddr);
>  }
>
>  static void rtl8169_get_ethtool_stats(struct net_device *dev,
> @@ -3298,15 +3300,15 @@ static int rtl8169_open(struct net_device *dev)
>
>        /*
>         * Rx and Tx desscriptors needs 256 bytes alignment.
> -        * pci_alloc_consistent provides more.
> +        * dma_alloc_coherent provides more.
>         */
> -       tp->TxDescArray = pci_alloc_consistent(pdev, R8169_TX_RING_BYTES,
> -                                              &tp->TxPhyAddr);
> +       tp->TxDescArray = dma_alloc_coherent(&pdev->dev, R8169_TX_RING_BYTES,
> +                                            &tp->TxPhyAddr, GFP_KERNEL);
>        if (!tp->TxDescArray)
>                goto err_pm_runtime_put;
>
> -       tp->RxDescArray = pci_alloc_consistent(pdev, R8169_RX_RING_BYTES,
> -                                              &tp->RxPhyAddr);
> +       tp->RxDescArray = dma_alloc_coherent(&pdev->dev, R8169_RX_RING_BYTES,
> +                                            &tp->RxPhyAddr, GFP_KERNEL);
>        if (!tp->RxDescArray)
>                goto err_free_tx_0;
>
> @@ -3340,12 +3342,12 @@ out:
>  err_release_ring_2:
>        rtl8169_rx_clear(tp);
>  err_free_rx_1:
> -       pci_free_consistent(pdev, R8169_RX_RING_BYTES, tp->RxDescArray,
> -                           tp->RxPhyAddr);
> +       dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
> +                         tp->RxPhyAddr);
>        tp->RxDescArray = NULL;
>  err_free_tx_0:
> -       pci_free_consistent(pdev, R8169_TX_RING_BYTES, tp->TxDescArray,
> -                           tp->TxPhyAddr);
> +       dma_free_coherent(&pdev->dev, R8169_TX_RING_BYTES, tp->TxDescArray,
> +                         tp->TxPhyAddr);
>        tp->TxDescArray = NULL;
>  err_pm_runtime_put:
>        pm_runtime_put_noidle(&pdev->dev);
> @@ -3981,7 +3983,7 @@ static void rtl8169_free_rx_skb(struct rtl8169_private *tp,
>  {
>        struct pci_dev *pdev = tp->pci_dev;
>
> -       pci_unmap_single(pdev, le64_to_cpu(desc->addr), tp->rx_buf_sz,
> +       dma_unmap_single(&pdev->dev, le64_to_cpu(desc->addr), tp->rx_buf_sz,
>                         PCI_DMA_FROMDEVICE);
>        dev_kfree_skb(*sk_buff);
>        *sk_buff = NULL;
> @@ -4020,7 +4022,7 @@ static struct sk_buff *rtl8169_alloc_rx_skb(struct pci_dev *pdev,
>
>        skb_reserve(skb, align ? ((pad - 1) & (unsigned long)skb->data) : pad);
>
> -       mapping = pci_map_single(pdev, skb->data, rx_buf_sz,
> +       mapping = dma_map_single(&pdev->dev, skb->data, rx_buf_sz,
>                                 PCI_DMA_FROMDEVICE);
>
>        rtl8169_map_to_asic(desc, mapping, rx_buf_sz);
> @@ -4105,7 +4107,8 @@ static void rtl8169_unmap_tx_skb(struct pci_dev *pdev, struct ring_info *tx_skb,
>  {
>        unsigned int len = tx_skb->len;
>
> -       pci_unmap_single(pdev, le64_to_cpu(desc->addr), len, PCI_DMA_TODEVICE);
> +       dma_unmap_single(&pdev->dev, le64_to_cpu(desc->addr), len,
> +                        PCI_DMA_TODEVICE);
>        desc->opts1 = 0x00;
>        desc->opts2 = 0x00;
>        desc->addr = 0x00;
> @@ -4249,7 +4252,8 @@ static int rtl8169_xmit_frags(struct rtl8169_private *tp, struct sk_buff *skb,
>                txd = tp->TxDescArray + entry;
>                len = frag->size;
>                addr = ((void *) page_address(frag->page)) + frag->page_offset;
> -               mapping = pci_map_single(tp->pci_dev, addr, len, PCI_DMA_TODEVICE);
> +               mapping = dma_map_single(&tp->pci_dev->dev, addr, len,
> +                                        PCI_DMA_TODEVICE);
>
>                /* anti gcc 2.95.3 bugware (sic) */
>                status = opts1 | len | (RingEnd * !((entry + 1) % NUM_TX_DESC));
> @@ -4319,7 +4323,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
>                tp->tx_skb[entry].skb = skb;
>        }
>
> -       mapping = pci_map_single(tp->pci_dev, skb->data, len, PCI_DMA_TODEVICE);
> +       mapping = dma_map_single(&tp->pci_dev->dev, skb->data, len,
> +                                PCI_DMA_TODEVICE);
>
>        tp->tx_skb[entry].len = len;
>        txd->addr = cpu_to_le64(mapping);
> @@ -4482,8 +4487,8 @@ static inline bool rtl8169_try_rx_copy(struct sk_buff **sk_buff,
>        if (!skb)
>                goto out;
>
> -       pci_dma_sync_single_for_cpu(tp->pci_dev, addr, pkt_size,
> -                                   PCI_DMA_FROMDEVICE);
> +       dma_sync_single_for_cpu(&tp->pci_dev->dev, addr, pkt_size,
> +                               PCI_DMA_FROMDEVICE);
>        skb_copy_from_linear_data(*sk_buff, skb->data, pkt_size);
>        *sk_buff = skb;
>        done = true;
> @@ -4552,11 +4557,11 @@ static int rtl8169_rx_interrupt(struct net_device *dev,
>                        }
>
>                        if (rtl8169_try_rx_copy(&skb, tp, pkt_size, addr)) {
> -                               pci_dma_sync_single_for_device(pdev, addr,
> +                               dma_sync_single_for_device(&pdev->dev, addr,
>                                        pkt_size, PCI_DMA_FROMDEVICE);
>                                rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
>                        } else {
> -                               pci_unmap_single(pdev, addr, tp->rx_buf_sz,
> +                               dma_unmap_single(&pdev->dev, addr, tp->rx_buf_sz,
>                                                 PCI_DMA_FROMDEVICE);
>                                tp->Rx_skbuff[entry] = NULL;
>                        }
> @@ -4774,10 +4779,10 @@ static int rtl8169_close(struct net_device *dev)
>
>        free_irq(dev->irq, dev);
>
> -       pci_free_consistent(pdev, R8169_RX_RING_BYTES, tp->RxDescArray,
> -                           tp->RxPhyAddr);
> -       pci_free_consistent(pdev, R8169_TX_RING_BYTES, tp->TxDescArray,
> -                           tp->TxPhyAddr);
> +       dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
> +                         tp->RxPhyAddr);
> +       dma_free_coherent(&pdev->dev, R8169_TX_RING_BYTES, tp->TxDescArray,
> +                         tp->TxPhyAddr);
>        tp->TxDescArray = NULL;
>        tp->RxDescArray = NULL;
>
> --
> 1.7.0.1
>
> --
> 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
>



-- 
Regards,
Denis

Re: [PATCH 2/2 -next] r8169: use device model DMA API

[Stanislaw Gruszka]

On Thu, 23 Sep 2010 18:59:10 +0400
Denis Kirjanov <kirjanov@gmail.com> wrote:

> On Thu, Sep 23, 2010 at 4:01 PM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> > Use DMA API as PCI equivalents will be deprecated. This change also
> > allow to allocate with GFP_KERNEL where possible.
> >
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > ---
> 
> dma_map_single and friend can fail. Thus, we should check for a return value.
> Yes, this is not directly related to current patch, but it would be
> great to fix this.

Sure, I will another patch with fix soon.

Stanislaw

Re: [PATCH 1/2 -next] r8169: allocate with GFP_KERNEL flag when able to sleep

[Francois Romieu]

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).

-- 
Ueimor

Re: [PATCH 1/2 -next] r8169: allocate with GFP_KERNEL flag when able to sleep

[Stanislaw Gruszka]

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

Re: [PATCH 1/2 -next] r8169: allocate with GFP_KERNEL flag when able to sleep

[Francois Romieu]

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.

-- 
Ueimor

Re: [PATCH 1/2 -next] r8169: allocate with GFP_KERNEL flag when able to sleep

[David Miller]

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.

Re: [PATCH 1/2 -next] r8169: allocate with GFP_KERNEL flag when able to sleep

[Eric Dumazet]

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...

Re: [PATCH 1/2 -next] r8169: allocate with GFP_KERNEL flag when able to sleep

[David Miller]

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.

Re: [PATCH 1/2 -next] r8169: allocate with GFP_KERNEL flag when able to sleep

[Eric Dumazet]

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...)

Re: [PATCH 1/2 -next] r8169: allocate with GFP_KERNEL flag when able to sleep

[Ben Hutchings]

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

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.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/2 -next] r8169: allocate with GFP_KERNEL flag when able to sleep

[Eric Dumazet]

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

Re: [PATCH 1/2 -next] r8169: allocate with GFP_KERNEL flag when able to sleep

[Ben Hutchings]

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

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.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]