[net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Neil Horman]

atl1c uses netdev_alloc_skb to refill its rx dma ring, but that call makes no
guarantees about the suitability of the memory for use in DMA.  As a result
we've gotten reports of atl1c drivers occasionally hanging and needing to be
reset:
https://bugzilla.kernel.org/show_bug.cgi?id=54021

Fix this by modifying the call to use the internal version __netdev_alloc_skb,
where you can set the gfp_mask explicitly to include GFP_DMA.

Tested by two reporters in the above bug, who have the hardware to validate it.
Both report immediate cessation of the problem with this patch

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Jay Cliburn <jcliburn@gmail.com>
CC: "David S. Miller" <davem@davemloft.net>
CC: stable@vger.kernel.org
Tested-by: Luis Henrix <luis.henrix@gmail.com>
Tested-by: Vincent Alquier <vincent.alquier@gmail.com>
---
 drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index 786a874..d5e38d1 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -1660,7 +1660,7 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
 	while (next_info->flags & ATL1C_BUFFER_FREE) {
 		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
 
-		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
+		skb = __netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len, GFP_ATOMIC|GFP_DMA);
 		if (unlikely(!skb)) {
 			if (netif_msg_rx_err(adapter))
 				dev_warn(&pdev->dev, "alloc rx buffer failed\n");
-- 
1.8.1.4

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Luis Henriques]

Neil Horman <nhorman@tuxdriver.com> writes:

> atl1c uses netdev_alloc_skb to refill its rx dma ring, but that call makes no
> guarantees about the suitability of the memory for use in DMA.  As a result
> we've gotten reports of atl1c drivers occasionally hanging and needing to be
> reset:
> https://bugzilla.kernel.org/show_bug.cgi?id=54021
>
> Fix this by modifying the call to use the internal version __netdev_alloc_skb,
> where you can set the gfp_mask explicitly to include GFP_DMA.
>
> Tested by two reporters in the above bug, who have the hardware to validate it.
> Both report immediate cessation of the problem with this patch
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Jay Cliburn <jcliburn@gmail.com>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: stable@vger.kernel.org
> Tested-by: Luis Henrix <luis.henrix@gmail.com>

Thanks Neil!

Would it be possible for you to update my name and email?  (I've just
updated my bz account to remove my gmail account address -- I thought
I had done this ages ago.)

Tested-by: Luis Henriques <luis.henriques@canonical.com>

Cheers,
-- 
Luis


> Tested-by: Vincent Alquier <vincent.alquier@gmail.com> ---
> drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 2 +- 1 file
> changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index 786a874..d5e38d1 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -1660,7 +1660,7 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
>  	while (next_info->flags & ATL1C_BUFFER_FREE) {
>  		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
>  
> -		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
> +		skb = __netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len, GFP_ATOMIC|GFP_DMA);
>  		if (unlikely(!skb)) {
>  			if (netif_msg_rx_err(adapter))
>  				dev_warn(&pdev->dev, "alloc rx buffer failed\n");

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Neil Horman]

On Fri, Jul 26, 2013 at 05:56:16PM +0100, Luis Henriques wrote:
> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > atl1c uses netdev_alloc_skb to refill its rx dma ring, but that call makes no
> > guarantees about the suitability of the memory for use in DMA.  As a result
> > we've gotten reports of atl1c drivers occasionally hanging and needing to be
> > reset:
> > https://bugzilla.kernel.org/show_bug.cgi?id=54021
> >
> > Fix this by modifying the call to use the internal version __netdev_alloc_skb,
> > where you can set the gfp_mask explicitly to include GFP_DMA.
> >
> > Tested by two reporters in the above bug, who have the hardware to validate it.
> > Both report immediate cessation of the problem with this patch
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: Jay Cliburn <jcliburn@gmail.com>
> > CC: "David S. Miller" <davem@davemloft.net>
> > CC: stable@vger.kernel.org
> > Tested-by: Luis Henrix <luis.henrix@gmail.com>
> 
> Thanks Neil!
> 
> Would it be possible for you to update my name and email?  (I've just
> updated my bz account to remove my gmail account address -- I thought
> I had done this ages ago.)
> 
> Tested-by: Luis Henriques <luis.henriques@canonical.com>
> 
I'm sure if Dave has time, he will square that up.
Neil

> Cheers,
> -- 
> Luis
> 
> 
> > Tested-by: Vincent Alquier <vincent.alquier@gmail.com> ---
> > drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 2 +- 1 file
> > changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> > index 786a874..d5e38d1 100644
> > --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> > +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> > @@ -1660,7 +1660,7 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
> >  	while (next_info->flags & ATL1C_BUFFER_FREE) {
> >  		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
> >  
> > -		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
> > +		skb = __netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len, GFP_ATOMIC|GFP_DMA);
> >  		if (unlikely(!skb)) {
> >  			if (netif_msg_rx_err(adapter))
> >  				dev_warn(&pdev->dev, "alloc rx buffer failed\n");
> 
> 

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 26 Jul 2013 13:02:51 -0400

> On Fri, Jul 26, 2013 at 05:56:16PM +0100, Luis Henriques wrote:
>> Neil Horman <nhorman@tuxdriver.com> writes:
>> 
>> > atl1c uses netdev_alloc_skb to refill its rx dma ring, but that call makes no
>> > guarantees about the suitability of the memory for use in DMA.  As a result
>> > we've gotten reports of atl1c drivers occasionally hanging and needing to be
>> > reset:
>> > https://bugzilla.kernel.org/show_bug.cgi?id=54021
>> >
>> > Fix this by modifying the call to use the internal version __netdev_alloc_skb,
>> > where you can set the gfp_mask explicitly to include GFP_DMA.
>> >
>> > Tested by two reporters in the above bug, who have the hardware to validate it.
>> > Both report immediate cessation of the problem with this patch
>> >
>> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>> > CC: Jay Cliburn <jcliburn@gmail.com>
>> > CC: "David S. Miller" <davem@davemloft.net>
>> > CC: stable@vger.kernel.org
>> > Tested-by: Luis Henrix <luis.henrix@gmail.com>
>> 
>> Thanks Neil!
>> 
>> Would it be possible for you to update my name and email?  (I've just
>> updated my bz account to remove my gmail account address -- I thought
>> I had done this ages ago.)
>> 
>> Tested-by: Luis Henriques <luis.henriques@canonical.com>
>> 
> I'm sure if Dave has time, he will square that up.

I took care of this while applying Neil's patch, thanks.

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Luis Henriques]

David Miller <davem@davemloft.net> writes:

> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Fri, 26 Jul 2013 13:02:51 -0400
>
>> On Fri, Jul 26, 2013 at 05:56:16PM +0100, Luis Henriques wrote:
>>> Neil Horman <nhorman@tuxdriver.com> writes:
>>> 
>>> > atl1c uses netdev_alloc_skb to refill its rx dma ring, but that call makes no
>>> > guarantees about the suitability of the memory for use in DMA.  As a result
>>> > we've gotten reports of atl1c drivers occasionally hanging and needing to be
>>> > reset:
>>> > https://bugzilla.kernel.org/show_bug.cgi?id=54021
>>> >
>>> > Fix this by modifying the call to use the internal version __netdev_alloc_skb,
>>> > where you can set the gfp_mask explicitly to include GFP_DMA.
>>> >
>>> > Tested by two reporters in the above bug, who have the hardware to validate it.
>>> > Both report immediate cessation of the problem with this patch
>>> >
>>> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>>> > CC: Jay Cliburn <jcliburn@gmail.com>
>>> > CC: "David S. Miller" <davem@davemloft.net>
>>> > CC: stable@vger.kernel.org
>>> > Tested-by: Luis Henrix <luis.henrix@gmail.com>
>>> 
>>> Thanks Neil!
>>> 
>>> Would it be possible for you to update my name and email?  (I've just
>>> updated my bz account to remove my gmail account address -- I thought
>>> I had done this ages ago.)
>>> 
>>> Tested-by: Luis Henriques <luis.henriques@canonical.com>
>>> 
>> I'm sure if Dave has time, he will square that up.
>
> I took care of this while applying Neil's patch, thanks.

Thanks!

Cheers,
-- 
Luis

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Ben Hutchings]

On Fri, 2013-07-26 at 12:47 -0400, Neil Horman wrote:
> atl1c uses netdev_alloc_skb to refill its rx dma ring, but that call makes no
> guarantees about the suitability of the memory for use in DMA.  As a result
> we've gotten reports of atl1c drivers occasionally hanging and needing to be
> reset:
> https://bugzilla.kernel.org/show_bug.cgi?id=54021
> 
> Fix this by modifying the call to use the internal version __netdev_alloc_skb,
> where you can set the gfp_mask explicitly to include GFP_DMA.

This is a really bad idea.  GFP_DMA means allocation from the ISA DMA
region (< 16 MB).  pci_map_single() takes care of allocating a bounce
buffer if necessary.

Ben.

> Tested by two reporters in the above bug, who have the hardware to validate it.
> Both report immediate cessation of the problem with this patch
> 
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Jay Cliburn <jcliburn@gmail.com>
> CC: "David S. Miller" <davem@davemloft.net>
> CC: stable@vger.kernel.org
> Tested-by: Luis Henrix <luis.henrix@gmail.com>
> Tested-by: Vincent Alquier <vincent.alquier@gmail.com>
> ---
>  drivers/net/ethernet/atheros/atl1c/atl1c_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index 786a874..d5e38d1 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -1660,7 +1660,7 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
>  	while (next_info->flags & ATL1C_BUFFER_FREE) {
>  		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
>  
> -		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
> +		skb = __netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len, GFP_ATOMIC|GFP_DMA);
>  		if (unlikely(!skb)) {
>  			if (netif_msg_rx_err(adapter))
>  				dev_warn(&pdev->dev, "alloc rx buffer failed\n");

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Ben Hutchings]

On Sat, 2013-07-27 at 01:02 +0100, Ben Hutchings wrote:
> On Fri, 2013-07-26 at 12:47 -0400, Neil Horman wrote:
> > atl1c uses netdev_alloc_skb to refill its rx dma ring, but that call makes no
> > guarantees about the suitability of the memory for use in DMA.  As a result
> > we've gotten reports of atl1c drivers occasionally hanging and needing to be
> > reset:
> > https://bugzilla.kernel.org/show_bug.cgi?id=54021
> > 
> > Fix this by modifying the call to use the internal version __netdev_alloc_skb,
> > where you can set the gfp_mask explicitly to include GFP_DMA.
> 
> This is a really bad idea.  GFP_DMA means allocation from the ISA DMA
> region (< 16 MB).  pci_map_single() takes care of allocating a bounce
> buffer if necessary.
> 
> Ben.
> 
> > Tested by two reporters in the above bug, who have the hardware to validate it.
> > Both report immediate cessation of the problem with this patch
[...]

So perhaps the chip somehow fails to support a full 32-bit address
(which is the current DMA mask), though given that there are 64 address
bits in RX descriptors this seems unlikely.  And the most likely result
of that would be memory corruption, not a stall.

Alternately, perhaps more likely, there's something wrong with the
driver's error handling.  If atl1_alloc_rx_buffer() fails then the RX
queue could run dry.  Depending on how the hardware is designed, that
could result in a complete RX stall (no RX buffers available => no RX
completions => no attempt to allocate more RX buffers).

Maybe your change makes it less likely for atl1_alloc_rx_buffer() to
fail.  On a modern PC the (ISA) DMA zone is basically unused whereas
bounce buffers might be more contended.  Did you try adding some logging
for failure of pci_map_single()?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Luis Henriques]

Ben Hutchings <bhutchings@solarflare.com> writes:

> On Sat, 2013-07-27 at 01:02 +0100, Ben Hutchings wrote:
>> On Fri, 2013-07-26 at 12:47 -0400, Neil Horman wrote:
>> > atl1c uses netdev_alloc_skb to refill its rx dma ring, but that call makes no
>> > guarantees about the suitability of the memory for use in DMA.  As a result
>> > we've gotten reports of atl1c drivers occasionally hanging and needing to be
>> > reset:
>> > https://bugzilla.kernel.org/show_bug.cgi?id=54021
>> > 
>> > Fix this by modifying the call to use the internal version __netdev_alloc_skb,
>> > where you can set the gfp_mask explicitly to include GFP_DMA.
>> 
>> This is a really bad idea.  GFP_DMA means allocation from the ISA DMA
>> region (< 16 MB).  pci_map_single() takes care of allocating a bounce
>> buffer if necessary.
>> 
>> Ben.
>> 
>> > Tested by two reporters in the above bug, who have the hardware to validate it.
>> > Both report immediate cessation of the problem with this patch
> [...]
>
> So perhaps the chip somehow fails to support a full 32-bit address
> (which is the current DMA mask), though given that there are 64 address
> bits in RX descriptors this seems unlikely.  And the most likely result
> of that would be memory corruption, not a stall.
>
> Alternately, perhaps more likely, there's something wrong with the
> driver's error handling.  If atl1_alloc_rx_buffer() fails then the RX
> queue could run dry.  Depending on how the hardware is designed, that
> could result in a complete RX stall (no RX buffers available => no RX
> completions => no attempt to allocate more RX buffers).
>
> Maybe your change makes it less likely for atl1_alloc_rx_buffer() to
> fail.  On a modern PC the (ISA) DMA zone is basically unused whereas
> bounce buffers might be more contended.  Did you try adding some logging
> for failure of pci_map_single()?
>
> Ben.

Just to add a little bit more context (and hopefully not noise), I
started seeing this issue on 3.7.  Bisection resulted on the following
first bad commit:

69b08f6 net: use bigger pages in __netdev_alloc_frag

Reverting this commit (and e5e6730 "skbuff: Move definition of
NETDEV_FRAG_PAGE_MAX_SIZE") solved the problem.

Note also that I'm seeing this issue on a 32 bits system (64 bits
isn't supported).  This initially made me think the problem could be
related with this as 69b08f6 log explicitly refers to 32/64 bit
archs.  But I failed to find any obvious issue with the patch.

Cheers,
-- 
Luis

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

On Sat, 2013-07-27 at 20:30 +0100, Luis Henriques wrote:
> Ben Hutchings <bhutchings@solarflare.com> writes:
> 
> > On Sat, 2013-07-27 at 01:02 +0100, Ben Hutchings wrote:
> >> On Fri, 2013-07-26 at 12:47 -0400, Neil Horman wrote:
> >> > atl1c uses netdev_alloc_skb to refill its rx dma ring, but that call makes no
> >> > guarantees about the suitability of the memory for use in DMA.  As a result
> >> > we've gotten reports of atl1c drivers occasionally hanging and needing to be
> >> > reset:
> >> > https://bugzilla.kernel.org/show_bug.cgi?id=54021
> >> > 
> >> > Fix this by modifying the call to use the internal version __netdev_alloc_skb,
> >> > where you can set the gfp_mask explicitly to include GFP_DMA.
> >> 
> >> This is a really bad idea.  GFP_DMA means allocation from the ISA DMA
> >> region (< 16 MB).  pci_map_single() takes care of allocating a bounce
> >> buffer if necessary.
> >> 
> >> Ben.
> >> 
> >> > Tested by two reporters in the above bug, who have the hardware to validate it.
> >> > Both report immediate cessation of the problem with this patch
> > [...]
> >
> > So perhaps the chip somehow fails to support a full 32-bit address
> > (which is the current DMA mask), though given that there are 64 address
> > bits in RX descriptors this seems unlikely.  And the most likely result
> > of that would be memory corruption, not a stall.
> >
> > Alternately, perhaps more likely, there's something wrong with the
> > driver's error handling.  If atl1_alloc_rx_buffer() fails then the RX
> > queue could run dry.  Depending on how the hardware is designed, that
> > could result in a complete RX stall (no RX buffers available => no RX
> > completions => no attempt to allocate more RX buffers).
> >
> > Maybe your change makes it less likely for atl1_alloc_rx_buffer() to
> > fail.  On a modern PC the (ISA) DMA zone is basically unused whereas
> > bounce buffers might be more contended.  Did you try adding some logging
> > for failure of pci_map_single()?
> >
> > Ben.
> 
> Just to add a little bit more context (and hopefully not noise), I
> started seeing this issue on 3.7.  Bisection resulted on the following
> first bad commit:
> 
> 69b08f6 net: use bigger pages in __netdev_alloc_frag
> 
> Reverting this commit (and e5e6730 "skbuff: Move definition of
> NETDEV_FRAG_PAGE_MAX_SIZE") solved the problem.
> 
> Note also that I'm seeing this issue on a 32 bits system (64 bits
> isn't supported).  This initially made me think the problem could be
> related with this as 69b08f6 log explicitly refers to 32/64 bit
> archs.  But I failed to find any obvious issue with the patch.
> 
> Cheers,

Unfortunately, nothing makes sense here. It looks like a possible
hardware defect on some memory ranges, as only atl1c is impacted.

Restricting allocations to DMA zone would probably avoid this bug
completely, but it adds obvious problems as DMA zone is so small.

Say you have some tcp flows with application not reading received queue
fast enough, DMA zone will be depleted and network will just hang.

This driver never used GFP_DMA, so adding them now seems quite strange.

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Ben Hutchings]

On Sat, 2013-07-27 at 20:30 +0100, Luis Henriques wrote:
> Ben Hutchings <bhutchings@solarflare.com> writes:
> 
> > On Sat, 2013-07-27 at 01:02 +0100, Ben Hutchings wrote:
> >> On Fri, 2013-07-26 at 12:47 -0400, Neil Horman wrote:
> >> > atl1c uses netdev_alloc_skb to refill its rx dma ring, but that call makes no
> >> > guarantees about the suitability of the memory for use in DMA.  As a result
> >> > we've gotten reports of atl1c drivers occasionally hanging and needing to be
> >> > reset:
> >> > https://bugzilla.kernel.org/show_bug.cgi?id=54021
> >> > 
> >> > Fix this by modifying the call to use the internal version __netdev_alloc_skb,
> >> > where you can set the gfp_mask explicitly to include GFP_DMA.
> >> 
> >> This is a really bad idea.  GFP_DMA means allocation from the ISA DMA
> >> region (< 16 MB).  pci_map_single() takes care of allocating a bounce
> >> buffer if necessary.
[...]
> Just to add a little bit more context (and hopefully not noise), I
> started seeing this issue on 3.7.  Bisection resulted on the following
> first bad commit:
> 
> 69b08f6 net: use bigger pages in __netdev_alloc_frag
> 
> Reverting this commit (and e5e6730 "skbuff: Move definition of
> NETDEV_FRAG_PAGE_MAX_SIZE") solved the problem.
> 
> Note also that I'm seeing this issue on a 32 bits system (64 bits
> isn't supported).  This initially made me think the problem could be
> related with this as 69b08f6 log explicitly refers to 32/64 bit
> archs.  But I failed to find any obvious issue with the patch.

Then it seems like this patch works because passing the GFP_DMA flag to
__netdev_alloc_skb() disables the use of __netdev_alloc_frag() and
results in it calling __alloc_skb().  A better workaround would be for
atl1c to call __alloc_skb() directly.

Perhaps the controller doesn't split RX DMA across PCIe page boundaries
(4K), or some other boundaries at smaller intervals than
NETDEV_FRAG_PAGE_MAX_SIZE.

But I think that perhaps the use of __netdev_alloc_frag() should be made
opt-in.  I doubt this is the only driver whose DMA requirements have
been broken.  Since the Linux DMA API lacks any way for devices to
specify boundaries which would then be observed by pci_map_single(), I
don't think this can be considered simply a driver bug.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

On Sat, 2013-07-27 at 22:30 +0100, Ben Hutchings wrote:
> On Sat, 2013-07-27 at 20:30 +0100, Luis Henriques wrote:
> > Ben Hutchings <bhutchings@solarflare.com> writes:
> > 
> > > On Sat, 2013-07-27 at 01:02 +0100, Ben Hutchings wrote:
> > >> On Fri, 2013-07-26 at 12:47 -0400, Neil Horman wrote:
> > >> > atl1c uses netdev_alloc_skb to refill its rx dma ring, but that call makes no
> > >> > guarantees about the suitability of the memory for use in DMA.  As a result
> > >> > we've gotten reports of atl1c drivers occasionally hanging and needing to be
> > >> > reset:
> > >> > https://bugzilla.kernel.org/show_bug.cgi?id=54021
> > >> > 
> > >> > Fix this by modifying the call to use the internal version __netdev_alloc_skb,
> > >> > where you can set the gfp_mask explicitly to include GFP_DMA.
> > >> 
> > >> This is a really bad idea.  GFP_DMA means allocation from the ISA DMA
> > >> region (< 16 MB).  pci_map_single() takes care of allocating a bounce
> > >> buffer if necessary.
> [...]
> > Just to add a little bit more context (and hopefully not noise), I
> > started seeing this issue on 3.7.  Bisection resulted on the following
> > first bad commit:
> > 
> > 69b08f6 net: use bigger pages in __netdev_alloc_frag
> > 
> > Reverting this commit (and e5e6730 "skbuff: Move definition of
> > NETDEV_FRAG_PAGE_MAX_SIZE") solved the problem.
> > 
> > Note also that I'm seeing this issue on a 32 bits system (64 bits
> > isn't supported).  This initially made me think the problem could be
> > related with this as 69b08f6 log explicitly refers to 32/64 bit
> > archs.  But I failed to find any obvious issue with the patch.
> 
> Then it seems like this patch works because passing the GFP_DMA flag to
> __netdev_alloc_skb() disables the use of __netdev_alloc_frag() and
> results in it calling __alloc_skb().  A better workaround would be for
> atl1c to call __alloc_skb() directly.
> 
> Perhaps the controller doesn't split RX DMA across PCIe page boundaries
> (4K), or some other boundaries at smaller intervals than
> NETDEV_FRAG_PAGE_MAX_SIZE.
> 
> But I think that perhaps the use of __netdev_alloc_frag() should be made
> opt-in.  I doubt this is the only driver whose DMA requirements have
> been broken.  Since the Linux DMA API lacks any way for devices to
> specify boundaries which would then be observed by pci_map_single(), I
> don't think this can be considered simply a driver bug.

It is a driver bug to assume anything about alloc_skb(), as there is no
specification about skb->head being aligned to whatever boundary.

The only guarantee is the one provided by kmalloc(), that is 8 bytes.

I specifically asked this exact question in 

https://bugzilla.kernel.org/show_bug.cgi?id=54021#c19

And the Qualcom guys checked and said it was ok. Who should we trust ?

I hope you understand kmalloc(~2000 bytes) never made the assumption the
area fit a single page. SLOB or even SLUB/SLAB with some debugging
features...

If a hardware needs frame being in a single 4K page, its driver must do
its own allocation, or add appropriate aligning logic.

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 27 Jul 2013 16:59:43 -0700

> If a hardware needs frame being in a single 4K page, its driver must do
> its own allocation, or add appropriate aligning logic.

Whatever the cause the original patch in this thread is not the
correct fix and I've thus reverted it.

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Neil Horman]

On Sat, Jul 27, 2013 at 08:02:05PM -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Sat, 27 Jul 2013 16:59:43 -0700
> 
> > If a hardware needs frame being in a single 4K page, its driver must do
> > its own allocation, or add appropriate aligning logic.
> 
> Whatever the cause the original patch in this thread is not the
> correct fix and I've thus reverted it.
> 

So what exactly is the consensus here?  We can certainly modify the patch to
ensure allocation on a 4k boundary, and within a unique 4k page, but thats just
a guess at the problem, and the Qualcomm people have been silent on the issue
for weeks now.

Neil

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

On Sun, 2013-07-28 at 06:44 -0400, Neil Horman wrote:
> On Sat, Jul 27, 2013 at 08:02:05PM -0700, David Miller wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Sat, 27 Jul 2013 16:59:43 -0700
> > 
> > > If a hardware needs frame being in a single 4K page, its driver must do
> > > its own allocation, or add appropriate aligning logic.
> > 
> > Whatever the cause the original patch in this thread is not the
> > correct fix and I've thus reverted it.
> > 
> 
> So what exactly is the consensus here?  We can certainly modify the patch to
> ensure allocation on a 4k boundary, and within a unique 4k page, but thats just
> a guess at the problem, and the Qualcomm people have been silent on the issue
> for weeks now.
> 

Thats the guess yes, but since MTU can be more than 4096 bytes, it could
be something else, some kind of DMA problem.

I wonder why rx_buffer_len includes VLAN_HLEN as this nic provides
accelerated vlan.

drivers/net/ethernet/atheros/atl1c/atl1c_main.c uses :
 hw->dmar_block = atl1c_dma_req_1024;

while drivers/net/ethernet/atheros/atlx/atl1.c uses :
 hw->dmar_block = atl1_dma_req_256;

drivers/net/ethernet/atheros/atlx/atl1.c also has this code around line
1962 :

                        /* rrd seems to be bad */
                        if (unlikely(i-- > 0)) {
                                /* rrd may not be DMAed completely */
                                udelay(1);
                                goto chk_rrd;
                        }


I would add the following debugging aid just to make sure...

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index 786a874..4c794f9 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -1794,6 +1794,7 @@ rrs_checked:
 				buffer_info->length, PCI_DMA_FROMDEVICE);
 			skb = buffer_info->skb;
 		} else {
+			WARN_ON_ONCE(1);
 			/* TODO */
 			if (netif_msg_rx_err(adapter))
 				dev_warn(&pdev->dev,

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Neil Horman]

On Sun, Jul 28, 2013 at 09:15:54AM -0700, Eric Dumazet wrote:
> On Sun, 2013-07-28 at 06:44 -0400, Neil Horman wrote:
> > On Sat, Jul 27, 2013 at 08:02:05PM -0700, David Miller wrote:
> > > From: Eric Dumazet <eric.dumazet@gmail.com>
> > > Date: Sat, 27 Jul 2013 16:59:43 -0700
> > > 
> > > > If a hardware needs frame being in a single 4K page, its driver must do
> > > > its own allocation, or add appropriate aligning logic.
> > > 
> > > Whatever the cause the original patch in this thread is not the
> > > correct fix and I've thus reverted it.
> > > 
> > 
> > So what exactly is the consensus here?  We can certainly modify the patch to
> > ensure allocation on a 4k boundary, and within a unique 4k page, but thats just
> > a guess at the problem, and the Qualcomm people have been silent on the issue
> > for weeks now.
> > 
> 
> Thats the guess yes, but since MTU can be more than 4096 bytes, it could
> be something else, some kind of DMA problem.
> 
Right, thats exactly my concern, its a guess, one which infers problems of its
own.  I'll certainly code up a patch to test, sure, given what we've discussed
here, but I'm not sure it leaves us in any better position than we were in with
my origional patch.

> I wonder why rx_buffer_len includes VLAN_HLEN as this nic provides
> accelerated vlan.
> 
I expect its an overestimation of sizing requirements in the rx ring.  The
set_features method for the driver doesn't re-allocate the rx ring buffers if
the vlan tag accleration feature is disabled, so I suspect that they keep that
extra space in place in the event that its ever turned off.

> drivers/net/ethernet/atheros/atl1c/atl1c_main.c uses :
>  hw->dmar_block = atl1c_dma_req_1024;
> 
> while drivers/net/ethernet/atheros/atlx/atl1.c uses :
>  hw->dmar_block = atl1_dma_req_256;
> 
Another question for the Atheros people I suppose.  Although, atl1c takes this
value through several other contortions in atl1c_configure_tx, in which it seems
to allow the value to be either 2, or 3 (capped by the min_t call, and floored
by the check against DEVICE_CTRL_MAXRRS_MIN).  By all rights the setting in the
atlx driver would be considered invalid in the atl1c driver (if such
comparisons can be drawn).

> drivers/net/ethernet/atheros/atlx/atl1.c also has this code around line
> 1962 :
> 
>                         /* rrd seems to be bad */
>                         if (unlikely(i-- > 0)) {
>                                 /* rrd may not be DMAed completely */
>                                 udelay(1);
>                                 goto chk_rrd;
>                         }
> 
> 
Hmm, that is interesting, expecially given that atl1c has this code:
rfd_num = (rrs->word0 >> RRS_RX_RFD_CNT_SHIFT) &
                                RRS_RX_RFD_CNT_MASK;
                        if (unlikely(rfd_num != 1))
                                /* TODO support mul rfd*/
                                if (netif_msg_rx_err(adapter))
                                        dev_warn(&pdev->dev,
                                                "Multi rfd not support yet!\n");
                        goto rrs_checked;

It appears that atl1c does a simmilar check to what atlx does, but atl1c just
ignores the possibility that rfd_num may be 0.  I wonder if I shouldn't add a
loop simmilar to what alx does there.

> I would add the following debugging aid just to make sure...
> 
> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index 786a874..4c794f9 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -1794,6 +1794,7 @@ rrs_checked:
>  				buffer_info->length, PCI_DMA_FROMDEVICE);
>  			skb = buffer_info->skb;
>  		} else {
> +			WARN_ON_ONCE(1);
>  			/* TODO */
>  			if (netif_msg_rx_err(adapter))
>  				dev_warn(&pdev->dev,
> 
> 
> 
I agree, I think perhaps a loop higher up prior to the rrs_checked label to
ensure that rfd_num is at least 1 prior to going through the rest of the rx
path.  I'll have something for the reporters to test tomorrow.

Thanks!
Neil

> 

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

On Sun, 2013-07-28 at 14:53 -0400, Neil Horman wrote:
>  
> I agree, I think perhaps a loop higher up prior to the rrs_checked label to
> ensure that rfd_num is at least 1 prior to going through the rest of the rx
> path.  I'll have something for the reporters to test tomorrow.
> 

btw, this driver leaks skb horribly in this error path :

                if (rrs->word3 & (RRS_RX_ERR_SUM | RRS_802_3_LEN_ERR)) {
                        atl1c_clean_rfd(rfd_ring, rrs, rfd_num);
                                if (netif_msg_rx_err(adapter))
                                        dev_warn(&pdev->dev,
                                                "wrong packet! rrs word3 is %x\n",
                                                rrs->word3);
                        continue;
                }

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

On Sun, 2013-07-28 at 12:21 -0700, Eric Dumazet wrote:

> 
> btw, this driver leaks skb horribly in this error path :
> 
>                 if (rrs->word3 & (RRS_RX_ERR_SUM | RRS_802_3_LEN_ERR)) {
>                         atl1c_clean_rfd(rfd_ring, rrs, rfd_num);
>                                 if (netif_msg_rx_err(adapter))
>                                         dev_warn(&pdev->dev,
>                                                 "wrong packet! rrs word3 is %x\n",
>                                                 rrs->word3);
>                         continue;
>                 }

Possible fix would be :

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index 786a874..e815c23 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -1729,11 +1729,16 @@ static void atl1c_clean_rfd(struct atl1c_rfd_ring *rfd_ring,
 	u16 i;
 	u16 rfd_index;
 	struct atl1c_buffer *buffer_info = rfd_ring->buffer_info;
+	struct sk_buff *skb;
 
 	rfd_index = (rrs->word0 >> RRS_RX_RFD_INDEX_SHIFT) &
 			RRS_RX_RFD_INDEX_MASK;
 	for (i = 0; i < num; i++) {
-		buffer_info[rfd_index].skb = NULL;
+		skb = buffer_info[rfd_index].skb;
+		if (skb) {
+			kfree_skb(skb);
+			buffer_info[rfd_index].skb = NULL;
+		}
 		ATL1C_SET_BUFFER_STATE(&buffer_info[rfd_index],
 					ATL1C_BUFFER_FREE);
 		if (++rfd_index == rfd_ring->count)
@@ -1793,6 +1798,7 @@ rrs_checked:
 			pci_unmap_single(pdev, buffer_info->dma,
 				buffer_info->length, PCI_DMA_FROMDEVICE);
 			skb = buffer_info->skb;
+			buffer_info->skb = NULL;
 		} else {
 			/* TODO */
 			if (netif_msg_rx_err(adapter))

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Ben Hutchings]

On Sun, 2013-07-28 at 14:53 -0400, Neil Horman wrote:
> On Sun, Jul 28, 2013 at 09:15:54AM -0700, Eric Dumazet wrote:
> > On Sun, 2013-07-28 at 06:44 -0400, Neil Horman wrote:
> > > On Sat, Jul 27, 2013 at 08:02:05PM -0700, David Miller wrote:
> > > > From: Eric Dumazet <eric.dumazet@gmail.com>
> > > > Date: Sat, 27 Jul 2013 16:59:43 -0700
> > > > 
> > > > > If a hardware needs frame being in a single 4K page, its driver must do
> > > > > its own allocation, or add appropriate aligning logic.
> > > > 
> > > > Whatever the cause the original patch in this thread is not the
> > > > correct fix and I've thus reverted it.
> > > > 
> > > 
> > > So what exactly is the consensus here?  We can certainly modify the patch to
> > > ensure allocation on a 4k boundary, and within a unique 4k page, but thats just
> > > a guess at the problem, and the Qualcomm people have been silent on the issue
> > > for weeks now.
> > > 
> > 
> > Thats the guess yes, but since MTU can be more than 4096 bytes, it could
> > be something else, some kind of DMA problem.

Oh well, there goes that theory.

> Right, thats exactly my concern, its a guess, one which infers problems of its
> own.  I'll certainly code up a patch to test, sure, given what we've discussed
> here, but I'm not sure it leaves us in any better position than we were in with
> my origional patch.
> 
> > I wonder why rx_buffer_len includes VLAN_HLEN as this nic provides
> > accelerated vlan.
> > 
> I expect its an overestimation of sizing requirements in the rx ring.  The
> set_features method for the driver doesn't re-allocate the rx ring buffers if
> the vlan tag accleration feature is disabled, so I suspect that they keep that
> extra space in place in the event that its ever turned off.
> 
> > drivers/net/ethernet/atheros/atl1c/atl1c_main.c uses :
> >  hw->dmar_block = atl1c_dma_req_1024;
> > 
> > while drivers/net/ethernet/atheros/atlx/atl1.c uses :
> >  hw->dmar_block = atl1_dma_req_256;
> > 
> Another question for the Atheros people I suppose.  Although, atl1c takes this
> value through several other contortions in atl1c_configure_tx, in which it seems
> to allow the value to be either 2, or 3 (capped by the min_t call, and floored
> by the check against DEVICE_CTRL_MAXRRS_MIN).  By all rights the setting in the
> atlx driver would be considered invalid in the atl1c driver (if such
> comparisons can be drawn).
[...]

The value programmed as the Max Read Request Size should be irrelevant
to memory writes (i.e. RX DMA).  Memory write transactions must be
limited by the Max Payload Size and must not cross 4K boundaries.  I
would expect PCIe devices to split linear DMA transfers into suitably
sized transactions automatically, modulo bugs of course.

Since we know lengths > 4K work, perhaps it would be worth testing with
the fragment cache size reduced to 16K?  The driver would never
previously have used RX buffers crossing 16K boundaries, except if SLOB
was used (and that's an unlikely combination).

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

On Sun, 2013-07-28 at 21:22 +0100, Ben Hutchings wrote:

> 
> Since we know lengths > 4K work, perhaps it would be worth testing with
> the fragment cache size reduced to 16K?  The driver would never
> previously have used RX buffers crossing 16K boundaries, except if SLOB
> was used (and that's an unlikely combination).

Sure, please note the following maths :

NET_SKB_PAD + 1536 + sizeof(struct skb_shared_info) = 1920

16384/1920 = 8

32768/1920 = 17

I don't think atl1c is used in any critical host (given it doesn't even
provide RX checksums and GRO ...), so I will provide a patch doing mere
page allocations.

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

On Sun, 2013-07-28 at 16:01 -0700, Eric Dumazet wrote:
> On Sun, 2013-07-28 at 21:22 +0100, Ben Hutchings wrote:
> 
> > 
> > Since we know lengths > 4K work, perhaps it would be worth testing with
> > the fragment cache size reduced to 16K?  The driver would never
> > previously have used RX buffers crossing 16K boundaries, except if SLOB
> > was used (and that's an unlikely combination).
> 
> Sure, please note the following maths :
> 
> NET_SKB_PAD + 1536 + sizeof(struct skb_shared_info) = 1920
> 
> 16384/1920 = 8
> 
> 32768/1920 = 17
> 
> I don't think atl1c is used in any critical host (given it doesn't even
> provide RX checksums and GRO ...), so I will provide a patch doing mere
> page allocations.
> 

Oh well, look at code around line 2530

        * The atl1c chip can DMA to 64-bit addresses, but it uses a single
         * shared register for the high 32 bits, so only a single, aligned,
         * 4 GB physical address range can be used at a time.
         *
         * Supporting 64-bit DMA on this hardware is more trouble than it's
         * worth.  It is far easier to limit to 32-bit DMA than update
         * various kernel subsystems to support the mechanics required by a
         * fixed-high-32-bit system.
         */
        if ((pci_set_dma_mask(pdev, DMA_BIT_MASK(32)) != 0) ||
            (pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)) != 0)) {
                dev_err(&pdev->dev, "No usable DMA configuration,aborting\n");
                goto err_dma;
        }

It looks like we have a winner !

This $@!? really needs DMA32 allocations.

Currently only tested on TX patch, it needs same care on RX

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index 786a874..e2ee962 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -1660,7 +1660,8 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
 	while (next_info->flags & ATL1C_BUFFER_FREE) {
 		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
 
-		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
+		skb = __netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len,
+					 GFP_ATOMIC | GFP_DMA32);
 		if (unlikely(!skb)) {
 			if (netif_msg_rx_err(adapter))
 				dev_warn(&pdev->dev, "alloc rx buffer failed\n");

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

On Sun, 2013-07-28 at 16:20 -0700, Eric Dumazet wrote:
> On Sun, 2013-07-28 at 16:01 -0700, Eric Dumazet wrote:
> > On Sun, 2013-07-28 at 21:22 +0100, Ben Hutchings wrote:
> > 
> > > 
> > > Since we know lengths > 4K work, perhaps it would be worth testing with
> > > the fragment cache size reduced to 16K?  The driver would never
> > > previously have used RX buffers crossing 16K boundaries, except if SLOB
> > > was used (and that's an unlikely combination).
> > 
> > Sure, please note the following maths :
> > 
> > NET_SKB_PAD + 1536 + sizeof(struct skb_shared_info) = 1920
> > 
> > 16384/1920 = 8
> > 
> > 32768/1920 = 17
> > 
> > I don't think atl1c is used in any critical host (given it doesn't even
> > provide RX checksums and GRO ...), so I will provide a patch doing mere
> > page allocations.
> > 
> 
> Oh well, look at code around line 2530
> 
>         * The atl1c chip can DMA to 64-bit addresses, but it uses a single
>          * shared register for the high 32 bits, so only a single, aligned,
>          * 4 GB physical address range can be used at a time.
>          *
>          * Supporting 64-bit DMA on this hardware is more trouble than it's
>          * worth.  It is far easier to limit to 32-bit DMA than update
>          * various kernel subsystems to support the mechanics required by a
>          * fixed-high-32-bit system.
>          */
>         if ((pci_set_dma_mask(pdev, DMA_BIT_MASK(32)) != 0) ||
>             (pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)) != 0)) {
>                 dev_err(&pdev->dev, "No usable DMA configuration,aborting\n");
>                 goto err_dma;
>         }
> 
> It looks like we have a winner !
> 
> This $@!? really needs DMA32 allocations.
> 
> Currently only tested on TX patch, it needs same care on RX
> 
> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index 786a874..e2ee962 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -1660,7 +1660,8 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
>  	while (next_info->flags & ATL1C_BUFFER_FREE) {
>  		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
>  
> -		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
> +		skb = __netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len,
> +					 GFP_ATOMIC | GFP_DMA32);
>  		if (unlikely(!skb)) {
>  			if (netif_msg_rx_err(adapter))
>  				dev_warn(&pdev->dev, "alloc rx buffer failed\n");
> 

And this needs following patch as well :

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3df4d4c..1871ed8 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -441,7 +441,7 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
 	unsigned int fragsz = SKB_DATA_ALIGN(length + NET_SKB_PAD) +
 			      SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 
-	if (fragsz <= PAGE_SIZE && !(gfp_mask & (__GFP_WAIT | GFP_DMA))) {
+	if (fragsz <= PAGE_SIZE && !(gfp_mask & (__GFP_WAIT | GFP_DMA | GFP_DMA32))) {
 		void *data;
 
 		if (sk_memalloc_socks())

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Neil Horman]

On Sun, Jul 28, 2013 at 04:20:54PM -0700, Eric Dumazet wrote:
> On Sun, 2013-07-28 at 16:01 -0700, Eric Dumazet wrote:
> > On Sun, 2013-07-28 at 21:22 +0100, Ben Hutchings wrote:
> > 
> > > 
> > > Since we know lengths > 4K work, perhaps it would be worth testing with
> > > the fragment cache size reduced to 16K?  The driver would never
> > > previously have used RX buffers crossing 16K boundaries, except if SLOB
> > > was used (and that's an unlikely combination).
> > 
> > Sure, please note the following maths :
> > 
> > NET_SKB_PAD + 1536 + sizeof(struct skb_shared_info) = 1920
> > 
> > 16384/1920 = 8
> > 
> > 32768/1920 = 17
> > 
> > I don't think atl1c is used in any critical host (given it doesn't even
> > provide RX checksums and GRO ...), so I will provide a patch doing mere
> > page allocations.
> > 
> 
> Oh well, look at code around line 2530
> 
>         * The atl1c chip can DMA to 64-bit addresses, but it uses a single
>          * shared register for the high 32 bits, so only a single, aligned,
>          * 4 GB physical address range can be used at a time.
>          *
>          * Supporting 64-bit DMA on this hardware is more trouble than it's
>          * worth.  It is far easier to limit to 32-bit DMA than update
>          * various kernel subsystems to support the mechanics required by a
>          * fixed-high-32-bit system.
>          */
>         if ((pci_set_dma_mask(pdev, DMA_BIT_MASK(32)) != 0) ||
>             (pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)) != 0)) {
>                 dev_err(&pdev->dev, "No usable DMA configuration,aborting\n");
>                 goto err_dma;
>         }
> 
> It looks like we have a winner !
> 
> This $@!? really needs DMA32 allocations.
> 
> Currently only tested on TX patch, it needs same care on RX
> 
> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index 786a874..e2ee962 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -1660,7 +1660,8 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
>  	while (next_info->flags & ATL1C_BUFFER_FREE) {
>  		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
>  
> -		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
> +		skb = __netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len,
> +					 GFP_ATOMIC | GFP_DMA32);
>  		if (unlikely(!skb)) {
>  			if (netif_msg_rx_err(adapter))
>  				dev_warn(&pdev->dev, "alloc rx buffer failed\n");
> 
> 
> 
HA!  I was 99% right on my first patch! :)  Luis, Can you test this patch out
and confirm that it fixes the issue.
Neil

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Ben Hutchings]

On Sun, 2013-07-28 at 16:20 -0700, Eric Dumazet wrote:
> On Sun, 2013-07-28 at 16:01 -0700, Eric Dumazet wrote:
> > On Sun, 2013-07-28 at 21:22 +0100, Ben Hutchings wrote:
> > 
> > > 
> > > Since we know lengths > 4K work, perhaps it would be worth testing with
> > > the fragment cache size reduced to 16K?  The driver would never
> > > previously have used RX buffers crossing 16K boundaries, except if SLOB
> > > was used (and that's an unlikely combination).
> > 
> > Sure, please note the following maths :
> > 
> > NET_SKB_PAD + 1536 + sizeof(struct skb_shared_info) = 1920
> > 
> > 16384/1920 = 8
> > 
> > 32768/1920 = 17
> > 
> > I don't think atl1c is used in any critical host (given it doesn't even
> > provide RX checksums and GRO ...), so I will provide a patch doing mere
> > page allocations.
> > 
> 
> Oh well, look at code around line 2530
> 
>         * The atl1c chip can DMA to 64-bit addresses, but it uses a single
>          * shared register for the high 32 bits, so only a single, aligned,
>          * 4 GB physical address range can be used at a time.
>          *
>          * Supporting 64-bit DMA on this hardware is more trouble than it's
>          * worth.  It is far easier to limit to 32-bit DMA than update
>          * various kernel subsystems to support the mechanics required by a
>          * fixed-high-32-bit system.
>          */
>         if ((pci_set_dma_mask(pdev, DMA_BIT_MASK(32)) != 0) ||
>             (pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)) != 0)) {
>                 dev_err(&pdev->dev, "No usable DMA configuration,aborting\n");
>                 goto err_dma;
>         }
> 
> It looks like we have a winner !
> 
> This $@!? really needs DMA32 allocations.
[...]

Just like many older PCI/PCIe devices.  So what?  pci_map_single() takes
care of that, just so long as the driver sets the DMA mask correctly.
In fact, PCI devices have a 32-bit DMA mask by default.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Mon, 29 Jul 2013 01:07:18 +0100

> Just like many older PCI/PCIe devices.  So what?  pci_map_single() takes
> care of that, just so long as the driver sets the DMA mask correctly.
> In fact, PCI devices have a 32-bit DMA mask by default.

Right, the DMA API should be taking care of this.

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

On Mon, 2013-07-29 at 01:07 +0100, Ben Hutchings wrote:

> Just like many older PCI/PCIe devices.  So what?  pci_map_single() takes
> care of that, just so long as the driver sets the DMA mask correctly.
> In fact, PCI devices have a 32-bit DMA mask by default.

I don't know, maybe the remapping doesn't work if area splits several
pages ?

I mean, why bouncing memory if we can allocate in the right zone in the
first place ?

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Luis Henriques]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On Sun, 2013-07-28 at 16:01 -0700, Eric Dumazet wrote:
>> On Sun, 2013-07-28 at 21:22 +0100, Ben Hutchings wrote:
>> 
>> > 
>> > Since we know lengths > 4K work, perhaps it would be worth testing with
>> > the fragment cache size reduced to 16K?  The driver would never
>> > previously have used RX buffers crossing 16K boundaries, except if SLOB
>> > was used (and that's an unlikely combination).
>> 
>> Sure, please note the following maths :
>> 
>> NET_SKB_PAD + 1536 + sizeof(struct skb_shared_info) = 1920
>> 
>> 16384/1920 = 8
>> 
>> 32768/1920 = 17
>> 
>> I don't think atl1c is used in any critical host (given it doesn't even
>> provide RX checksums and GRO ...), so I will provide a patch doing mere
>> page allocations.
>> 
>
> Oh well, look at code around line 2530
>
>         * The atl1c chip can DMA to 64-bit addresses, but it uses a single
>          * shared register for the high 32 bits, so only a single, aligned,
>          * 4 GB physical address range can be used at a time.
>          *
>          * Supporting 64-bit DMA on this hardware is more trouble than it's
>          * worth.  It is far easier to limit to 32-bit DMA than update
>          * various kernel subsystems to support the mechanics required by a
>          * fixed-high-32-bit system.
>          */
>         if ((pci_set_dma_mask(pdev, DMA_BIT_MASK(32)) != 0) ||
>             (pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)) != 0)) {
>                 dev_err(&pdev->dev, "No usable DMA configuration,aborting\n");
>                 goto err_dma;
>         }
>
> It looks like we have a winner !
>
> This $@!? really needs DMA32 allocations.
>
> Currently only tested on TX patch, it needs same care on RX
>
> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index 786a874..e2ee962 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -1660,7 +1660,8 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
>  	while (next_info->flags & ATL1C_BUFFER_FREE) {
>  		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
>  
> -		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
> +		skb = __netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len,
> +					 GFP_ATOMIC | GFP_DMA32);
>  		if (unlikely(!skb)) {
>  			if (netif_msg_rx_err(adapter))
>  				dev_warn(&pdev->dev, "alloc rx buffer failed\n");
>
>
> --
> 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

Using both patches from Eric (to the atl1c driver and to
net/core/skbuff.c) , I got the following:

[   25.176311] ------------[ cut here ]------------
[   25.179857] kernel BUG at mm/slub.c:1360!
[   25.183495] invalid opcode: 0000 [#1] SMP 
[   25.186919] CPU: 3 PID: 1705 Comm: ip Not tainted 3.11.0-rc2+ #15
[   25.190319] Hardware name: ASUSTeK COMPUTER INC. X101CH/X101CH, BIOS X101CH.1203 07/30/2012
[   25.193828] task: f504f8c0 ti: f514e000 task.ti: f514e000
[   25.197348] EIP: 0060:[<c1135e27>] EFLAGS: 00010002 CPU: 3
[   25.200896] EIP is at new_slab+0x1c7/0x200
[   25.204391] EAX: f6801a00 EBX: f6801a00 ECX: ffffffff EDX: 00010224
[   25.207942] ESI: f6800ea0 EDI: f6801a00 EBP: f514f91c ESP: f514f904
[   25.211541]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
[   25.215113] CR0: 80050033 CR2: bff28afc CR3: 35fcd000 CR4: 000007f0
[   25.218695] Stack:
[   25.222259]  f514f954 c107acc7 00000003 00000000 f6800ea0 f6801a00 f514f984 c179d9e6
[   25.226156]  80100010 00000000 00100010 c162fb40 00010224 f6800ea0 8015000b 00000000
[   25.226168]  00000286 c162fb1c 00000024 f514f964 00000000 f5d963c0 00000296 f5d963c0
[   25.226170] Call Trace:
[   25.226184]  [<c107acc7>] ? enqueue_task_fair+0x5c7/0x7d0
[   25.226197]  [<c179d9e6>] __slab_alloc.constprop.71+0x248/0x409
[   25.226205]  [<c162fb40>] ? __alloc_skb+0x60/0x270
[   25.226211]  [<c162fb1c>] ? __alloc_skb+0x3c/0x270
[   25.226218]  [<c106f688>] ? ttwu_do_wakeup+0x18/0x100
[   25.226226]  [<c1139440>] __kmalloc_track_caller+0x100/0x150
[   25.226232]  [<c10718a9>] ? try_to_wake_up+0x149/0x230
[   25.226238]  [<c162fb40>] ? __alloc_skb+0x60/0x270
[   25.226244]  [<c162f482>] __kmalloc_reserve.isra.30+0x22/0x70
[   25.226250]  [<c162fb40>] __alloc_skb+0x60/0x270
[   25.226257]  [<c162ff71>] __netdev_alloc_skb+0x41/0xc0
[   25.226265]  [<c145cbe5>] atl1c_alloc_rx_buffer+0x125/0x290
[   25.226272]  [<c145ce79>] atl1c_configure+0x129/0x420
[   25.226279]  [<c164d48f>] ? linkwatch_fire_event+0x2f/0x90
[   25.226286]  [<c1006a10>] ? via_no_dac+0x40/0x40
[   25.226292]  [<c145dcb3>] atl1c_up+0x23/0x1e0
[   25.226298]  [<c1006a10>] ? via_no_dac+0x40/0x40
[   25.226305]  [<c145e3c9>] atl1c_open+0x269/0x310
[   25.226311]  [<c1006a10>] ? via_no_dac+0x40/0x40
[   25.226317]  [<c163dd43>] __dev_open+0x83/0xf0
[   25.226325]  [<c17a7224>] ? _raw_spin_unlock_bh+0x14/0x20
[   25.226331]  [<c163e021>] __dev_change_flags+0x81/0x160
[   25.226337]  [<c163e1a8>] dev_change_flags+0x18/0x50
[   25.226343]  [<c164b200>] do_setlink+0x2e0/0x810
[   25.226350]  [<c10fc110>] ? find_get_page+0x20/0xa0
[   25.226357]  [<c12ccff2>] ? nla_parse+0x22/0xa0
[   25.226364]  [<c116b413>] ? __find_get_block_slow+0xd3/0x180
[   25.226370]  [<c164bd92>] rtnl_newlink+0x282/0x510
[   25.226378]  [<c12735cc>] ? security_capable+0x1c/0x30
[   25.226384]  [<c1648c68>] rtnetlink_rcv_msg+0x88/0x1f0
[   25.226391]  [<c1139386>] ? __kmalloc_track_caller+0x46/0x150
[   25.226397]  [<c162fb40>] ? __alloc_skb+0x60/0x270
[   25.226403]  [<c1648be0>] ? rtnetlink_rcv+0x30/0x30
[   25.226410]  [<c165f016>] netlink_rcv_skb+0x86/0xa0
[   25.226416]  [<c1648bd1>] rtnetlink_rcv+0x21/0x30
[   25.226422]  [<c165db38>] netlink_unicast+0x118/0x1b0
[   25.226428]  [<c165e17f>] netlink_sendmsg+0x23f/0x3f0
[   25.226435]  [<c162926b>] sock_sendmsg+0x7b/0xb0
[   25.226443]  [<c1102dc9>] ? __alloc_pages_nodemask+0x119/0x7a0
[   25.226450]  [<c1629661>] ___sys_sendmsg+0x291/0x2a0
[   25.226457]  [<c10fbe56>] ? unlock_page+0x46/0x50
[   25.226464]  [<c111a348>] ? __do_fault+0x388/0x4a0
[   25.226471]  [<c1107196>] ? lru_cache_add+0x16/0x20
[   25.226477]  [<c1125174>] ? page_add_new_anon_rmap+0x74/0x100
[   25.226483]  [<c1631dd5>] ? skb_dequeue+0x45/0x60
[   25.226491]  [<c111da1a>] ? handle_mm_fault+0x1ca/0x2b0
[   25.226497]  [<c11545d1>] ? __d_free+0x31/0x50
[   25.226504]  [<c162a4e8>] __sys_sendmsg+0x38/0x70
[   25.226510]  [<c162a536>] SyS_sendmsg+0x16/0x20
[   25.226517]  [<c162ac1b>] SyS_socketcall+0x29b/0x2f0
[   25.226524]  [<c11440bd>] ? ____fput+0xd/0x10
[   25.226531]  [<c1035b80>] ? vmalloc_sync_all+0x10/0x10
[   25.226537]  [<c17a7fbb>] sysenter_do_call+0x12/0x22
[   25.226600] Code: e9 4a ff ff ff 8b 7d f0 eb b5 31 c0 eb dc 89 f9 b8 00 10 00 00 d3 e0 ba 5a 00 00 00 89 c1 8b 45 f0 e8 ae 42 18 00 e9 38 ff ff ff <0f> 0b 8b 7b 24 b9 00 0f af c1 8b 45 f0 c7 04 24 00 00 00 00 89
[   25.226609] EIP: [<c1135e27>] new_slab+0x1c7/0x200 SS:ESP 0068:f514f904
[   25.226614] ---[ end trace 6188393b9e234ab1 ]---
[   26.757161] input: ACPI Virtual Keyboard Device as /devices/virtual/input/input13

Reverting the skbuff.c patch and using only the atl1c_main.c one, I
see again the failures with the driver.

So far the only options that seem to get the driver working for me are
either Neil Horman's patch or reverting 69b08f6 ("net: use bigger
pages in __netdev_alloc_frag").

Cheers
-- 
Luis

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

On Mon, 2013-07-29 at 10:55 +0100, Luis Henriques wrote:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
> 
> > On Sun, 2013-07-28 at 16:01 -0700, Eric Dumazet wrote:
> >> On Sun, 2013-07-28 at 21:22 +0100, Ben Hutchings wrote:
> >> 
> >> > 
> >> > Since we know lengths > 4K work, perhaps it would be worth testing with
> >> > the fragment cache size reduced to 16K?  The driver would never
> >> > previously have used RX buffers crossing 16K boundaries, except if SLOB
> >> > was used (and that's an unlikely combination).
> >> 
> >> Sure, please note the following maths :
> >> 
> >> NET_SKB_PAD + 1536 + sizeof(struct skb_shared_info) = 1920
> >> 
> >> 16384/1920 = 8
> >> 
> >> 32768/1920 = 17
> >> 
> >> I don't think atl1c is used in any critical host (given it doesn't even
> >> provide RX checksums and GRO ...), so I will provide a patch doing mere
> >> page allocations.
> >> 
> >
> > Oh well, look at code around line 2530
> >
> >         * The atl1c chip can DMA to 64-bit addresses, but it uses a single
> >          * shared register for the high 32 bits, so only a single, aligned,
> >          * 4 GB physical address range can be used at a time.
> >          *
> >          * Supporting 64-bit DMA on this hardware is more trouble than it's
> >          * worth.  It is far easier to limit to 32-bit DMA than update
> >          * various kernel subsystems to support the mechanics required by a
> >          * fixed-high-32-bit system.
> >          */
> >         if ((pci_set_dma_mask(pdev, DMA_BIT_MASK(32)) != 0) ||
> >             (pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)) != 0)) {
> >                 dev_err(&pdev->dev, "No usable DMA configuration,aborting\n");
> >                 goto err_dma;
> >         }
> >
> > It looks like we have a winner !
> >
> > This $@!? really needs DMA32 allocations.
> >
> > Currently only tested on TX patch, it needs same care on RX
> >
> > diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> > index 786a874..e2ee962 100644
> > --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> > +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> > @@ -1660,7 +1660,8 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
> >  	while (next_info->flags & ATL1C_BUFFER_FREE) {
> >  		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
> >  
> > -		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
> > +		skb = __netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len,
> > +					 GFP_ATOMIC | GFP_DMA32);
> >  		if (unlikely(!skb)) {
> >  			if (netif_msg_rx_err(adapter))
> >  				dev_warn(&pdev->dev, "alloc rx buffer failed\n");
> >
> >
> > --
> > 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
> 
> Using both patches from Eric (to the atl1c driver and to
> net/core/skbuff.c) , I got the following:
> 
> [   25.176311] ------------[ cut here ]------------
> [   25.179857] kernel BUG at mm/slub.c:1360!
> [   25.183495] invalid opcode: 0000 [#1] SMP 
> [   25.186919] CPU: 3 PID: 1705 Comm: ip Not tainted 3.11.0-rc2+ #15
> [   25.190319] Hardware name: ASUSTeK COMPUTER INC. X101CH/X101CH, BIOS X101CH.1203 07/30/2012
> [   25.193828] task: f504f8c0 ti: f514e000 task.ti: f514e000
> [   25.197348] EIP: 0060:[<c1135e27>] EFLAGS: 00010002 CPU: 3
> [   25.200896] EIP is at new_slab+0x1c7/0x200
> [   25.204391] EAX: f6801a00 EBX: f6801a00 ECX: ffffffff EDX: 00010224
> [   25.207942] ESI: f6800ea0 EDI: f6801a00 EBP: f514f91c ESP: f514f904
> [   25.211541]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
> [   25.215113] CR0: 80050033 CR2: bff28afc CR3: 35fcd000 CR4: 000007f0
> [   25.218695] Stack:
> [   25.222259]  f514f954 c107acc7 00000003 00000000 f6800ea0 f6801a00 f514f984 c179d9e6
> [   25.226156]  80100010 00000000 00100010 c162fb40 00010224 f6800ea0 8015000b 00000000
> [   25.226168]  00000286 c162fb1c 00000024 f514f964 00000000 f5d963c0 00000296 f5d963c0
> [   25.226170] Call Trace:
> [   25.226184]  [<c107acc7>] ? enqueue_task_fair+0x5c7/0x7d0
> [   25.226197]  [<c179d9e6>] __slab_alloc.constprop.71+0x248/0x409
> [   25.226205]  [<c162fb40>] ? __alloc_skb+0x60/0x270
> [   25.226211]  [<c162fb1c>] ? __alloc_skb+0x3c/0x270
> [   25.226218]  [<c106f688>] ? ttwu_do_wakeup+0x18/0x100
> [   25.226226]  [<c1139440>] __kmalloc_track_caller+0x100/0x150
> [   25.226232]  [<c10718a9>] ? try_to_wake_up+0x149/0x230
> [   25.226238]  [<c162fb40>] ? __alloc_skb+0x60/0x270
> [   25.226244]  [<c162f482>] __kmalloc_reserve.isra.30+0x22/0x70
> [   25.226250]  [<c162fb40>] __alloc_skb+0x60/0x270
> [   25.226257]  [<c162ff71>] __netdev_alloc_skb+0x41/0xc0
> [   25.226265]  [<c145cbe5>] atl1c_alloc_rx_buffer+0x125/0x290
> [   25.226272]  [<c145ce79>] atl1c_configure+0x129/0x420
> [   25.226279]  [<c164d48f>] ? linkwatch_fire_event+0x2f/0x90
> [   25.226286]  [<c1006a10>] ? via_no_dac+0x40/0x40
> [   25.226292]  [<c145dcb3>] atl1c_up+0x23/0x1e0
> [   25.226298]  [<c1006a10>] ? via_no_dac+0x40/0x40
> [   25.226305]  [<c145e3c9>] atl1c_open+0x269/0x310
> [   25.226311]  [<c1006a10>] ? via_no_dac+0x40/0x40
> [   25.226317]  [<c163dd43>] __dev_open+0x83/0xf0
> [   25.226325]  [<c17a7224>] ? _raw_spin_unlock_bh+0x14/0x20
> [   25.226331]  [<c163e021>] __dev_change_flags+0x81/0x160
> [   25.226337]  [<c163e1a8>] dev_change_flags+0x18/0x50
> [   25.226343]  [<c164b200>] do_setlink+0x2e0/0x810
> [   25.226350]  [<c10fc110>] ? find_get_page+0x20/0xa0
> [   25.226357]  [<c12ccff2>] ? nla_parse+0x22/0xa0
> [   25.226364]  [<c116b413>] ? __find_get_block_slow+0xd3/0x180
> [   25.226370]  [<c164bd92>] rtnl_newlink+0x282/0x510
> [   25.226378]  [<c12735cc>] ? security_capable+0x1c/0x30
> [   25.226384]  [<c1648c68>] rtnetlink_rcv_msg+0x88/0x1f0
> [   25.226391]  [<c1139386>] ? __kmalloc_track_caller+0x46/0x150
> [   25.226397]  [<c162fb40>] ? __alloc_skb+0x60/0x270
> [   25.226403]  [<c1648be0>] ? rtnetlink_rcv+0x30/0x30
> [   25.226410]  [<c165f016>] netlink_rcv_skb+0x86/0xa0
> [   25.226416]  [<c1648bd1>] rtnetlink_rcv+0x21/0x30
> [   25.226422]  [<c165db38>] netlink_unicast+0x118/0x1b0
> [   25.226428]  [<c165e17f>] netlink_sendmsg+0x23f/0x3f0
> [   25.226435]  [<c162926b>] sock_sendmsg+0x7b/0xb0
> [   25.226443]  [<c1102dc9>] ? __alloc_pages_nodemask+0x119/0x7a0
> [   25.226450]  [<c1629661>] ___sys_sendmsg+0x291/0x2a0
> [   25.226457]  [<c10fbe56>] ? unlock_page+0x46/0x50
> [   25.226464]  [<c111a348>] ? __do_fault+0x388/0x4a0
> [   25.226471]  [<c1107196>] ? lru_cache_add+0x16/0x20
> [   25.226477]  [<c1125174>] ? page_add_new_anon_rmap+0x74/0x100
> [   25.226483]  [<c1631dd5>] ? skb_dequeue+0x45/0x60
> [   25.226491]  [<c111da1a>] ? handle_mm_fault+0x1ca/0x2b0
> [   25.226497]  [<c11545d1>] ? __d_free+0x31/0x50
> [   25.226504]  [<c162a4e8>] __sys_sendmsg+0x38/0x70
> [   25.226510]  [<c162a536>] SyS_sendmsg+0x16/0x20
> [   25.226517]  [<c162ac1b>] SyS_socketcall+0x29b/0x2f0
> [   25.226524]  [<c11440bd>] ? ____fput+0xd/0x10
> [   25.226531]  [<c1035b80>] ? vmalloc_sync_all+0x10/0x10
> [   25.226537]  [<c17a7fbb>] sysenter_do_call+0x12/0x22
> [   25.226600] Code: e9 4a ff ff ff 8b 7d f0 eb b5 31 c0 eb dc 89 f9 b8 00 10 00 00 d3 e0 ba 5a 00 00 00 89 c1 8b 45 f0 e8 ae 42 18 00 e9 38 ff ff ff <0f> 0b 8b 7b 24 b9 00 0f af c1 8b 45 f0 c7 04 24 00 00 00 00 89
> [   25.226609] EIP: [<c1135e27>] new_slab+0x1c7/0x200 SS:ESP 0068:f514f904
> [   25.226614] ---[ end trace 6188393b9e234ab1 ]---
> [   26.757161] input: ACPI Virtual Keyboard Device as /devices/virtual/input/input13
> 
> Reverting the skbuff.c patch and using only the atl1c_main.c one, I
> see again the failures with the driver.
> 
> So far the only options that seem to get the driver working for me are
> either Neil Horman's patch or reverting 69b08f6 ("net: use bigger
> pages in __netdev_alloc_frag").
> 
> Cheers

Yes, forget about kmalloc() use GFP_DMA32, its not supported.

Lets try following patch ?

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index 786a874..f32189b 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -1639,6 +1639,28 @@ static inline void atl1c_rx_checksum(struct atl1c_adapter *adapter,
 	skb_checksum_none_assert(skb);
 }
 
+
+static struct sk_buff *atl1c_alloc_skb(unsigned int len)
+{
+	unsigned int head_size;
+	void *data;
+
+	head_size = SKB_DATA_ALIGN(len + NET_SKB_PAD) +
+		    SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+
+	if (head_size <= PAGE_SIZE) {
+		struct page *page = alloc_page(GFP_ATOMIC);
+
+		data = page ? page_address(page) : NULL;
+	} else {
+		data = kmalloc(head_size, GFP_ATOMIC);
+		head_size = 0;
+	}
+	if (data)
+		return build_skb(data, head_size);
+	return NULL;
+}
+
 static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
 {
 	struct atl1c_rfd_ring *rfd_ring = &adapter->rfd_ring;
@@ -1660,7 +1682,7 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
 	while (next_info->flags & ATL1C_BUFFER_FREE) {
 		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
 
-		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
+		skb = atl1c_alloc_skb(adapter->rx_buffer_len);
 		if (unlikely(!skb)) {
 			if (netif_msg_rx_err(adapter))
 				dev_warn(&pdev->dev, "alloc rx buffer failed\n");

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Luis Henriques]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On Mon, 2013-07-29 at 10:55 +0100, Luis Henriques wrote:
>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>> 
>> > On Sun, 2013-07-28 at 16:01 -0700, Eric Dumazet wrote:
>> >> On Sun, 2013-07-28 at 21:22 +0100, Ben Hutchings wrote:
>> >> 
>> >> > 
>> >> > Since we know lengths > 4K work, perhaps it would be worth testing with
>> >> > the fragment cache size reduced to 16K?  The driver would never
>> >> > previously have used RX buffers crossing 16K boundaries, except if SLOB
>> >> > was used (and that's an unlikely combination).
>> >> 
>> >> Sure, please note the following maths :
>> >> 
>> >> NET_SKB_PAD + 1536 + sizeof(struct skb_shared_info) = 1920
>> >> 
>> >> 16384/1920 = 8
>> >> 
>> >> 32768/1920 = 17
>> >> 
>> >> I don't think atl1c is used in any critical host (given it doesn't even
>> >> provide RX checksums and GRO ...), so I will provide a patch doing mere
>> >> page allocations.
>> >> 
>> >
>> > Oh well, look at code around line 2530
>> >
>> >         * The atl1c chip can DMA to 64-bit addresses, but it uses a single
>> >          * shared register for the high 32 bits, so only a single, aligned,
>> >          * 4 GB physical address range can be used at a time.
>> >          *
>> >          * Supporting 64-bit DMA on this hardware is more trouble than it's
>> >          * worth.  It is far easier to limit to 32-bit DMA than update
>> >          * various kernel subsystems to support the mechanics required by a
>> >          * fixed-high-32-bit system.
>> >          */
>> >         if ((pci_set_dma_mask(pdev, DMA_BIT_MASK(32)) != 0) ||
>> >             (pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)) != 0)) {
>> >                 dev_err(&pdev->dev, "No usable DMA configuration,aborting\n");
>> >                 goto err_dma;
>> >         }
>> >
>> > It looks like we have a winner !
>> >
>> > This $@!? really needs DMA32 allocations.
>> >
>> > Currently only tested on TX patch, it needs same care on RX
>> >
>> > diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
>> > index 786a874..e2ee962 100644
>> > --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
>> > +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
>> > @@ -1660,7 +1660,8 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
>> >  	while (next_info->flags & ATL1C_BUFFER_FREE) {
>> >  		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
>> >  
>> > -		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
>> > +		skb = __netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len,
>> > +					 GFP_ATOMIC | GFP_DMA32);
>> >  		if (unlikely(!skb)) {
>> >  			if (netif_msg_rx_err(adapter))
>> >  				dev_warn(&pdev->dev, "alloc rx buffer failed\n");
>> >
>> >
>> > --
>> > 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
>> 
>> Using both patches from Eric (to the atl1c driver and to
>> net/core/skbuff.c) , I got the following:
>> 
>> [   25.176311] ------------[ cut here ]------------
>> [   25.179857] kernel BUG at mm/slub.c:1360!
>> [   25.183495] invalid opcode: 0000 [#1] SMP 
>> [   25.186919] CPU: 3 PID: 1705 Comm: ip Not tainted 3.11.0-rc2+ #15
>> [   25.190319] Hardware name: ASUSTeK COMPUTER INC. X101CH/X101CH, BIOS X101CH.1203 07/30/2012
>> [   25.193828] task: f504f8c0 ti: f514e000 task.ti: f514e000
>> [   25.197348] EIP: 0060:[<c1135e27>] EFLAGS: 00010002 CPU: 3
>> [   25.200896] EIP is at new_slab+0x1c7/0x200
>> [   25.204391] EAX: f6801a00 EBX: f6801a00 ECX: ffffffff EDX: 00010224
>> [   25.207942] ESI: f6800ea0 EDI: f6801a00 EBP: f514f91c ESP: f514f904
>> [   25.211541]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
>> [   25.215113] CR0: 80050033 CR2: bff28afc CR3: 35fcd000 CR4: 000007f0
>> [   25.218695] Stack:
>> [   25.222259]  f514f954 c107acc7 00000003 00000000 f6800ea0 f6801a00 f514f984 c179d9e6
>> [   25.226156]  80100010 00000000 00100010 c162fb40 00010224 f6800ea0 8015000b 00000000
>> [   25.226168]  00000286 c162fb1c 00000024 f514f964 00000000 f5d963c0 00000296 f5d963c0
>> [   25.226170] Call Trace:
>> [   25.226184]  [<c107acc7>] ? enqueue_task_fair+0x5c7/0x7d0
>> [   25.226197]  [<c179d9e6>] __slab_alloc.constprop.71+0x248/0x409
>> [   25.226205]  [<c162fb40>] ? __alloc_skb+0x60/0x270
>> [   25.226211]  [<c162fb1c>] ? __alloc_skb+0x3c/0x270
>> [   25.226218]  [<c106f688>] ? ttwu_do_wakeup+0x18/0x100
>> [   25.226226]  [<c1139440>] __kmalloc_track_caller+0x100/0x150
>> [   25.226232]  [<c10718a9>] ? try_to_wake_up+0x149/0x230
>> [   25.226238]  [<c162fb40>] ? __alloc_skb+0x60/0x270
>> [   25.226244]  [<c162f482>] __kmalloc_reserve.isra.30+0x22/0x70
>> [   25.226250]  [<c162fb40>] __alloc_skb+0x60/0x270
>> [   25.226257]  [<c162ff71>] __netdev_alloc_skb+0x41/0xc0
>> [   25.226265]  [<c145cbe5>] atl1c_alloc_rx_buffer+0x125/0x290
>> [   25.226272]  [<c145ce79>] atl1c_configure+0x129/0x420
>> [   25.226279]  [<c164d48f>] ? linkwatch_fire_event+0x2f/0x90
>> [   25.226286]  [<c1006a10>] ? via_no_dac+0x40/0x40
>> [   25.226292]  [<c145dcb3>] atl1c_up+0x23/0x1e0
>> [   25.226298]  [<c1006a10>] ? via_no_dac+0x40/0x40
>> [   25.226305]  [<c145e3c9>] atl1c_open+0x269/0x310
>> [   25.226311]  [<c1006a10>] ? via_no_dac+0x40/0x40
>> [   25.226317]  [<c163dd43>] __dev_open+0x83/0xf0
>> [   25.226325]  [<c17a7224>] ? _raw_spin_unlock_bh+0x14/0x20
>> [   25.226331]  [<c163e021>] __dev_change_flags+0x81/0x160
>> [   25.226337]  [<c163e1a8>] dev_change_flags+0x18/0x50
>> [   25.226343]  [<c164b200>] do_setlink+0x2e0/0x810
>> [   25.226350]  [<c10fc110>] ? find_get_page+0x20/0xa0
>> [   25.226357]  [<c12ccff2>] ? nla_parse+0x22/0xa0
>> [   25.226364]  [<c116b413>] ? __find_get_block_slow+0xd3/0x180
>> [   25.226370]  [<c164bd92>] rtnl_newlink+0x282/0x510
>> [   25.226378]  [<c12735cc>] ? security_capable+0x1c/0x30
>> [   25.226384]  [<c1648c68>] rtnetlink_rcv_msg+0x88/0x1f0
>> [   25.226391]  [<c1139386>] ? __kmalloc_track_caller+0x46/0x150
>> [   25.226397]  [<c162fb40>] ? __alloc_skb+0x60/0x270
>> [   25.226403]  [<c1648be0>] ? rtnetlink_rcv+0x30/0x30
>> [   25.226410]  [<c165f016>] netlink_rcv_skb+0x86/0xa0
>> [   25.226416]  [<c1648bd1>] rtnetlink_rcv+0x21/0x30
>> [   25.226422]  [<c165db38>] netlink_unicast+0x118/0x1b0
>> [   25.226428]  [<c165e17f>] netlink_sendmsg+0x23f/0x3f0
>> [   25.226435]  [<c162926b>] sock_sendmsg+0x7b/0xb0
>> [   25.226443]  [<c1102dc9>] ? __alloc_pages_nodemask+0x119/0x7a0
>> [   25.226450]  [<c1629661>] ___sys_sendmsg+0x291/0x2a0
>> [   25.226457]  [<c10fbe56>] ? unlock_page+0x46/0x50
>> [   25.226464]  [<c111a348>] ? __do_fault+0x388/0x4a0
>> [   25.226471]  [<c1107196>] ? lru_cache_add+0x16/0x20
>> [   25.226477]  [<c1125174>] ? page_add_new_anon_rmap+0x74/0x100
>> [   25.226483]  [<c1631dd5>] ? skb_dequeue+0x45/0x60
>> [   25.226491]  [<c111da1a>] ? handle_mm_fault+0x1ca/0x2b0
>> [   25.226497]  [<c11545d1>] ? __d_free+0x31/0x50
>> [   25.226504]  [<c162a4e8>] __sys_sendmsg+0x38/0x70
>> [   25.226510]  [<c162a536>] SyS_sendmsg+0x16/0x20
>> [   25.226517]  [<c162ac1b>] SyS_socketcall+0x29b/0x2f0
>> [   25.226524]  [<c11440bd>] ? ____fput+0xd/0x10
>> [   25.226531]  [<c1035b80>] ? vmalloc_sync_all+0x10/0x10
>> [   25.226537]  [<c17a7fbb>] sysenter_do_call+0x12/0x22
>> [ 25.226600] Code: e9 4a ff ff ff 8b 7d f0 eb b5 31 c0 eb dc 89 f9 b8 00 10 00
>> 00 d3 e0 ba 5a 00 00 00 89 c1 8b 45 f0 e8 ae 42 18 00 e9 38 ff ff ff <0f> 0b
>> 8b 7b 24 b9 00 0f af c1 8b 45 f0 c7 04 24 00 00 00 00 89
>> [   25.226609] EIP: [<c1135e27>] new_slab+0x1c7/0x200 SS:ESP 0068:f514f904
>> [   25.226614] ---[ end trace 6188393b9e234ab1 ]---
>> [   26.757161] input: ACPI Virtual Keyboard Device as /devices/virtual/input/input13
>> 
>> Reverting the skbuff.c patch and using only the atl1c_main.c one, I
>> see again the failures with the driver.
>> 
>> So far the only options that seem to get the driver working for me are
>> either Neil Horman's patch or reverting 69b08f6 ("net: use bigger
>> pages in __netdev_alloc_frag").
>> 
>> Cheers
>
> Yes, forget about kmalloc() use GFP_DMA32, its not supported.
>
> Lets try following patch ?
>
> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index 786a874..f32189b 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -1639,6 +1639,28 @@ static inline void atl1c_rx_checksum(struct atl1c_adapter *adapter,
>  	skb_checksum_none_assert(skb);
>  }
>  
> +
> +static struct sk_buff *atl1c_alloc_skb(unsigned int len)
> +{
> +	unsigned int head_size;
> +	void *data;
> +
> +	head_size = SKB_DATA_ALIGN(len + NET_SKB_PAD) +
> +		    SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +
> +	if (head_size <= PAGE_SIZE) {
> +		struct page *page = alloc_page(GFP_ATOMIC);
> +
> +		data = page ? page_address(page) : NULL;
> +	} else {
> +		data = kmalloc(head_size, GFP_ATOMIC);
> +		head_size = 0;
> +	}
> +	if (data)
> +		return build_skb(data, head_size);
> +	return NULL;
> +}
> +
>  static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
>  {
>  	struct atl1c_rfd_ring *rfd_ring = &adapter->rfd_ring;
> @@ -1660,7 +1682,7 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
>  	while (next_info->flags & ATL1C_BUFFER_FREE) {
>  		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
>  
> -		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
> +		skb = atl1c_alloc_skb(adapter->rx_buffer_len);
>  		if (unlikely(!skb)) {
>  			if (netif_msg_rx_err(adapter))
>  				dev_warn(&pdev->dev, "alloc rx buffer failed\n");
>

I confirm that I can't reproduce the issue using this patch.

Cheers,
-- 
Luis

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

On Mon, 2013-07-29 at 13:09 +0100, Luis Henriques wrote:

> 
> I confirm that I can't reproduce the issue using this patch.
> 

Thanks, I'll send a polished patch, as this one had an error if
build_skb() returns NULL (in case sk_buff allocation fails)

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

On Mon, 2013-07-29 at 08:30 -0700, Eric Dumazet wrote:
> On Mon, 2013-07-29 at 13:09 +0100, Luis Henriques wrote:
> 
> > 
> > I confirm that I can't reproduce the issue using this patch.
> > 
> 
> Thanks, I'll send a polished patch, as this one had an error if
> build_skb() returns NULL (in case sk_buff allocation fails)

Please try the following patch : It should use 2K frags instead of 4K
for normal 1500 mtu

Thanks !

[PATCH] atl1c: use custom skb allocator

We had reports ( https://bugzilla.kernel.org/show_bug.cgi?id=54021 )
that using high order pages for skb allocations is problematic for atl1c

We do not know exactly what the problem is, but we suspect that crossing
4K pages is not well supported by this hardware.

Use a custom allocator, using page allocator and 2K fragments for
optimal stack behavior. We might make this allocator generic
in future kernels.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Luis Henriques <luis.henriques@canonical.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
---
 drivers/net/ethernet/atheros/atl1c/atl1c.h      |    3 +
 drivers/net/ethernet/atheros/atl1c/atl1c_main.c |   40 +++++++++++++-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c.h b/drivers/net/ethernet/atheros/atl1c/atl1c.h
index b2bf324..0f05565 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c.h
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c.h
@@ -520,6 +520,9 @@ struct atl1c_adapter {
 	struct net_device   *netdev;
 	struct pci_dev      *pdev;
 	struct napi_struct  napi;
+	struct page         *rx_page;
+	unsigned int	    rx_page_offset;
+	unsigned int	    rx_frag_size;
 	struct atl1c_hw        hw;
 	struct atl1c_hw_stats  hw_stats;
 	struct mii_if_info  mii;    /* MII interface info */
diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index 786a874..a36a760 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -481,10 +481,15 @@ static int atl1c_set_mac_addr(struct net_device *netdev, void *p)
 static void atl1c_set_rxbufsize(struct atl1c_adapter *adapter,
 				struct net_device *dev)
 {
+	unsigned int head_size;
 	int mtu = dev->mtu;
 
 	adapter->rx_buffer_len = mtu > AT_RX_BUF_SIZE ?
 		roundup(mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN, 8) : AT_RX_BUF_SIZE;
+
+	head_size = SKB_DATA_ALIGN(adapter->rx_buffer_len + NET_SKB_PAD) +
+		    SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+	adapter->rx_frag_size = roundup_pow_of_two(head_size);
 }
 
 static netdev_features_t atl1c_fix_features(struct net_device *netdev,
@@ -952,6 +957,10 @@ static void atl1c_free_ring_resources(struct atl1c_adapter *adapter)
 		kfree(adapter->tpd_ring[0].buffer_info);
 		adapter->tpd_ring[0].buffer_info = NULL;
 	}
+	if (adapter->rx_page) {
+		put_page(adapter->rx_page);
+		adapter->rx_page = NULL;
+	}
 }
 
 /**
@@ -1639,6 +1648,35 @@ static inline void atl1c_rx_checksum(struct atl1c_adapter *adapter,
 	skb_checksum_none_assert(skb);
 }
 
+static struct sk_buff *atl1c_alloc_skb(struct atl1c_adapter *adapter)
+{
+	struct sk_buff *skb;
+	struct page *page;
+
+	if (adapter->rx_frag_size > PAGE_SIZE)
+		return netdev_alloc_skb(adapter->netdev,
+					adapter->rx_buffer_len);
+
+	page = adapter->rx_page;
+	if (!page) {
+		adapter->rx_page = page = alloc_page(GFP_ATOMIC);
+		if (unlikely(!page))
+			return NULL;
+		adapter->rx_page_offset = 0;
+	}
+
+	skb = build_skb(page_address(page) + adapter->rx_page_offset,
+			adapter->rx_frag_size);
+	if (likely(skb)) {
+		adapter->rx_page_offset += adapter->rx_frag_size;
+		if (adapter->rx_page_offset >= PAGE_SIZE)
+			adapter->rx_page = NULL;
+		else
+			get_page(page);
+	}
+	return skb;
+}
+
 static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
 {
 	struct atl1c_rfd_ring *rfd_ring = &adapter->rfd_ring;
@@ -1660,7 +1698,7 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
 	while (next_info->flags & ATL1C_BUFFER_FREE) {
 		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
 
-		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
+		skb = atl1c_alloc_skb(adapter);
 		if (unlikely(!skb)) {
 			if (netif_msg_rx_err(adapter))
 				dev_warn(&pdev->dev, "alloc rx buffer failed\n");

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Luis Henriques]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> From: Eric Dumazet <edumazet@google.com>
>
> On Mon, 2013-07-29 at 08:30 -0700, Eric Dumazet wrote:
>> On Mon, 2013-07-29 at 13:09 +0100, Luis Henriques wrote:
>> 
>> > 
>> > I confirm that I can't reproduce the issue using this patch.
>> > 
>> 
>> Thanks, I'll send a polished patch, as this one had an error if
>> build_skb() returns NULL (in case sk_buff allocation fails)
>
> Please try the following patch : It should use 2K frags instead of 4K
> for normal 1500 mtu

This patch seems to work fine as well -- I'm unable to reproduce the
issue.

Cheers,
-- 
Luis


>
> Thanks !
>
> [PATCH] atl1c: use custom skb allocator
>
> We had reports ( https://bugzilla.kernel.org/show_bug.cgi?id=54021 )
> that using high order pages for skb allocations is problematic for atl1c
>
> We do not know exactly what the problem is, but we suspect that crossing
> 4K pages is not well supported by this hardware.
>
> Use a custom allocator, using page allocator and 2K fragments for
> optimal stack behavior. We might make this allocator generic
> in future kernels.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Luis Henriques <luis.henriques@canonical.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> ---
>  drivers/net/ethernet/atheros/atl1c/atl1c.h      |    3 +
>  drivers/net/ethernet/atheros/atl1c/atl1c_main.c |   40 +++++++++++++-
>  2 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c.h b/drivers/net/ethernet/atheros/atl1c/atl1c.h
> index b2bf324..0f05565 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c.h
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c.h
> @@ -520,6 +520,9 @@ struct atl1c_adapter {
>  	struct net_device   *netdev;
>  	struct pci_dev      *pdev;
>  	struct napi_struct  napi;
> +	struct page         *rx_page;
> +	unsigned int	    rx_page_offset;
> +	unsigned int	    rx_frag_size;
>  	struct atl1c_hw        hw;
>  	struct atl1c_hw_stats  hw_stats;
>  	struct mii_if_info  mii;    /* MII interface info */
> diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> index 786a874..a36a760 100644
> --- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> +++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
> @@ -481,10 +481,15 @@ static int atl1c_set_mac_addr(struct net_device *netdev, void *p)
>  static void atl1c_set_rxbufsize(struct atl1c_adapter *adapter,
>  				struct net_device *dev)
>  {
> +	unsigned int head_size;
>  	int mtu = dev->mtu;
>  
>  	adapter->rx_buffer_len = mtu > AT_RX_BUF_SIZE ?
>  		roundup(mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN, 8) : AT_RX_BUF_SIZE;
> +
> +	head_size = SKB_DATA_ALIGN(adapter->rx_buffer_len + NET_SKB_PAD) +
> +		    SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +	adapter->rx_frag_size = roundup_pow_of_two(head_size);
>  }
>  
>  static netdev_features_t atl1c_fix_features(struct net_device *netdev,
> @@ -952,6 +957,10 @@ static void atl1c_free_ring_resources(struct atl1c_adapter *adapter)
>  		kfree(adapter->tpd_ring[0].buffer_info);
>  		adapter->tpd_ring[0].buffer_info = NULL;
>  	}
> +	if (adapter->rx_page) {
> +		put_page(adapter->rx_page);
> +		adapter->rx_page = NULL;
> +	}
>  }
>  
>  /**
> @@ -1639,6 +1648,35 @@ static inline void atl1c_rx_checksum(struct atl1c_adapter *adapter,
>  	skb_checksum_none_assert(skb);
>  }
>  
> +static struct sk_buff *atl1c_alloc_skb(struct atl1c_adapter *adapter)
> +{
> +	struct sk_buff *skb;
> +	struct page *page;
> +
> +	if (adapter->rx_frag_size > PAGE_SIZE)
> +		return netdev_alloc_skb(adapter->netdev,
> +					adapter->rx_buffer_len);
> +
> +	page = adapter->rx_page;
> +	if (!page) {
> +		adapter->rx_page = page = alloc_page(GFP_ATOMIC);
> +		if (unlikely(!page))
> +			return NULL;
> +		adapter->rx_page_offset = 0;
> +	}
> +
> +	skb = build_skb(page_address(page) + adapter->rx_page_offset,
> +			adapter->rx_frag_size);
> +	if (likely(skb)) {
> +		adapter->rx_page_offset += adapter->rx_frag_size;
> +		if (adapter->rx_page_offset >= PAGE_SIZE)
> +			adapter->rx_page = NULL;
> +		else
> +			get_page(page);
> +	}
> +	return skb;
> +}
> +
>  static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
>  {
>  	struct atl1c_rfd_ring *rfd_ring = &adapter->rfd_ring;
> @@ -1660,7 +1698,7 @@ static int atl1c_alloc_rx_buffer(struct atl1c_adapter *adapter)
>  	while (next_info->flags & ATL1C_BUFFER_FREE) {
>  		rfd_desc = ATL1C_RFD_DESC(rfd_ring, rfd_next_to_use);
>  
> -		skb = netdev_alloc_skb(adapter->netdev, adapter->rx_buffer_len);
> +		skb = atl1c_alloc_skb(adapter);
>  		if (unlikely(!skb)) {
>  			if (netif_msg_rx_err(adapter))
>  				dev_warn(&pdev->dev, "alloc rx buffer failed\n");
>

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 29 Jul 2013 10:24:04 -0700

> [PATCH] atl1c: use custom skb allocator
> 
> We had reports ( https://bugzilla.kernel.org/show_bug.cgi?id=54021 )
> that using high order pages for skb allocations is problematic for atl1c
> 
> We do not know exactly what the problem is, but we suspect that crossing
> 4K pages is not well supported by this hardware.
> 
> Use a custom allocator, using page allocator and 2K fragments for
> optimal stack behavior. We might make this allocator generic
> in future kernels.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Theoretically we'll still potentially hit this with > PAGE_SIZE
MTUs....

But whatever, this is a step in the positive direction so applied
and queued up for -stable, thanks Eric!

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Benjamin Poirier]

On 2013/07/30 19:11, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 29 Jul 2013 10:24:04 -0700
> 
> > [PATCH] atl1c: use custom skb allocator
> > 

BTW, this didn't end up in git with the intended patch subject. The
extra text prepended in the email also made it to the log.

7b70176 atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring


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

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

On Wed, 2013-07-31 at 13:48 -0400, Benjamin Poirier wrote:
> On 2013/07/30 19:11, David Miller wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Mon, 29 Jul 2013 10:24:04 -0700
> > 
> > > [PATCH] atl1c: use custom skb allocator
> > > 
> 
> BTW, this didn't end up in git with the intended patch subject. The
> extra text prepended in the email also made it to the log.
> 
> 7b70176 atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

True, but its not a problem, as long as the bug is fixed ;)

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[David Miller]

From: Benjamin Poirier <benjamin.poirier@gmail.com>
Date: Wed, 31 Jul 2013 13:48:54 -0400

> On 2013/07/30 19:11, David Miller wrote:
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Mon, 29 Jul 2013 10:24:04 -0700
>> 
>> > [PATCH] atl1c: use custom skb allocator
>> > 
> 
> BTW, this didn't end up in git with the intended patch subject. The
> extra text prepended in the email also made it to the log.
> 
> 7b70176 atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

I know, I realized this when I sent the pull request to Linus.

Eric, your patch submission format is extremely error prone for me
sometime.  When you submit patches in email thread, and do things like
you did here, it causes problems sometimes.

Really just send it in the usual way, with the real Subject line
matching the commit message header line you wish to use etc.

Thanks!

Re: [net PATCH] atl1c: Fix misuse of netdev_alloc_skb in refilling rx ring

[Eric Dumazet]

On Wed, 2013-07-31 at 12:01 -0700, David Miller wrote:

> I know, I realized this when I sent the pull request to Linus.
> 
> Eric, your patch submission format is extremely error prone for me
> sometime.  When you submit patches in email thread, and do things like
> you did here, it causes problems sometimes.
> 
> Really just send it in the usual way, with the real Subject line
> matching the commit message header line you wish to use etc.
> 

Sorry for that, I'll do this next times !

Thanks !