[PATCH 1/2] net: Export __netdev_alloc_frag() to allow gfp_mask flags

[PATCH 1/2] net: Export __netdev_alloc_frag() to allow gfp_mask flags

[WingMan Kwok]

This patch makes the function __netdev_alloc_frag() non-static and
exports it so that drivers that need to specify additional flags,
such as __GFP_DMA, can use it. The currently exported function,
netdev_alloc_frag() doesn't allow passing in gfp_mask flags.

Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
Signed-off-by: Reece R. Pollack <x0183204@ti.com>
---
 include/linux/skbuff.h |    1 +
 net/core/skbuff.c      |    3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index d6cdd6e..596deb3 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2139,6 +2139,7 @@ static inline void __skb_queue_purge(struct sk_buff_head *list)
 		kfree_skb(skb);
 }
 
+void *__netdev_alloc_frag(unsigned int fragsz, gfp_t gfp_mask);
 void *netdev_alloc_frag(unsigned int fragsz);
 
 struct sk_buff *__netdev_alloc_skb(struct net_device *dev, unsigned int length,
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index b6a19ca..6f3451f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -350,7 +350,7 @@ EXPORT_SYMBOL(build_skb);
 static DEFINE_PER_CPU(struct page_frag_cache, netdev_alloc_cache);
 static DEFINE_PER_CPU(struct page_frag_cache, napi_alloc_cache);
 
-static void *__netdev_alloc_frag(unsigned int fragsz, gfp_t gfp_mask)
+void *__netdev_alloc_frag(unsigned int fragsz, gfp_t gfp_mask)
 {
 	struct page_frag_cache *nc;
 	unsigned long flags;
@@ -362,6 +362,7 @@ static void *__netdev_alloc_frag(unsigned int fragsz, gfp_t gfp_mask)
 	local_irq_restore(flags);
 	return data;
 }
+EXPORT_SYMBOL(__netdev_alloc_frag);
 
 /**
  * netdev_alloc_frag - allocate a page fragment
-- 
1.7.9.5

[PATCH 2/2] net: netcp: Allocate RX packet buffers using __GFP_DMA

[WingMan Kwok]

The Keystone II DMA hardware can only access addresses in the
lower 2 GiB of SDRAM. This patch makes sure the RX buffers are
allocated using the __GFP_DMA flag so they meet this requirement.

Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
Signed-off-by: Reece R. Pollack <x0183204@ti.com>
---
 drivers/net/ethernet/ti/netcp_core.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 2d07701..39c68df 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -820,11 +820,13 @@ static void netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 				SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 
 		if (primary_buf_len <= PAGE_SIZE) {
-			bufptr = netdev_alloc_frag(primary_buf_len);
+			bufptr = __netdev_alloc_frag(primary_buf_len,
+						     GFP_ATOMIC | __GFP_COLD |
+						     __GFP_DMA);
 			pad[1] = primary_buf_len;
 		} else {
 			bufptr = kmalloc(primary_buf_len, GFP_ATOMIC |
-					 GFP_DMA32 | __GFP_COLD);
+					 __GFP_COLD | __GFP_DMA);
 			pad[1] = 0;
 		}
 
@@ -838,9 +840,10 @@ static void netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 
 	} else {
 		/* Allocate a secondary receive queue entry */
-		page = alloc_page(GFP_ATOMIC | GFP_DMA32 | __GFP_COLD);
+		page = alloc_page(GFP_ATOMIC | __GFP_COLD | __GFP_DMA);
 		if (unlikely(!page)) {
-			dev_warn_ratelimited(netcp->ndev_dev, "Secondary page alloc failed\n");
+			dev_warn_ratelimited(netcp->ndev_dev,
+					     "Secondary page alloc failed\n");
 			goto fail;
 		}
 		buf_len = PAGE_SIZE;
-- 
1.7.9.5

Re: [PATCH 2/2] net: netcp: Allocate RX packet buffers using __GFP_DMA

[Eric Dumazet]

On Wed, 2015-07-29 at 11:10 -0400, WingMan Kwok wrote:
> The Keystone II DMA hardware can only access addresses in the
> lower 2 GiB of SDRAM. This patch makes sure the RX buffers are
> allocated using the __GFP_DMA flag so they meet this requirement.
> 
> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
> Signed-off-by: Reece R. Pollack <x0183204@ti.com>
> ---
>  drivers/net/ethernet/ti/netcp_core.c |   11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

Following patch gets rid of netdev_alloc_frag() which is not GFP_DMA
ready (and wont be)

diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 3ca87f26582a..0971c46d6cd5 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -808,14 +808,8 @@ static void netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 		primary_buf_len = SKB_DATA_ALIGN(buf_len) +
 				SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
 
-		if (primary_buf_len <= PAGE_SIZE) {
-			bufptr = netdev_alloc_frag(primary_buf_len);
-			pad[1] = primary_buf_len;
-		} else {
-			bufptr = kmalloc(primary_buf_len, GFP_ATOMIC |
-					 GFP_DMA32 | __GFP_COLD);
-			pad[1] = 0;
-		}
+		bufptr = kmalloc(primary_buf_len, GFP_ATOMIC | GFP_DMA);
+		pad[1] = 0;
 
 		if (unlikely(!bufptr)) {
 			dev_warn_ratelimited(netcp->ndev_dev, "Primary RX buffer alloc failed\n");
@@ -827,7 +821,7 @@ static void netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
 
 	} else {
 		/* Allocate a secondary receive queue entry */
-		page = alloc_page(GFP_ATOMIC | GFP_DMA32 | __GFP_COLD);
+		page = alloc_page(GFP_ATOMIC | GFP_DMA | __GFP_COLD);
 		if (unlikely(!page)) {
 			dev_warn_ratelimited(netcp->ndev_dev, "Secondary page alloc failed\n");
 			goto fail;

Re: [PATCH 2/2] net: netcp: Allocate RX packet buffers using __GFP_DMA

[Murali Karicheri]

On 07/29/2015 04:59 PM, Eric Dumazet wrote:
> On Wed, 2015-07-29 at 11:10 -0400, WingMan Kwok wrote:
>> The Keystone II DMA hardware can only access addresses in the
>> lower 2 GiB of SDRAM. This patch makes sure the RX buffers are
>> allocated using the __GFP_DMA flag so they meet this requirement.
>>
>> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
>> Signed-off-by: Reece R. Pollack <x0183204@ti.com>
>> ---
>>   drivers/net/ethernet/ti/netcp_core.c |   11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> Following patch gets rid of netdev_alloc_frag() which is not GFP_DMA
> ready (and wont be)
>
Eric,

Thanks for the suggestion.

I think the original assumption was that kmalloc() always allocate a 
minimum of 1 page for regular or small sized packets. The usage of 
netdev_alloc_frag() is to allocate small sized buffers. However 
kmalloc() can return different size buffers (/proc/slabinfo) and your 
below code should work fine for our driver. We will rework the patch as 
per your suggestion.

Murali

> diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
> index 3ca87f26582a..0971c46d6cd5 100644
> --- a/drivers/net/ethernet/ti/netcp_core.c
> +++ b/drivers/net/ethernet/ti/netcp_core.c
> @@ -808,14 +808,8 @@ static void netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>   		primary_buf_len = SKB_DATA_ALIGN(buf_len) +
>   				SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>
> -		if (primary_buf_len <= PAGE_SIZE) {
> -			bufptr = netdev_alloc_frag(primary_buf_len);
> -			pad[1] = primary_buf_len;
> -		} else {
> -			bufptr = kmalloc(primary_buf_len, GFP_ATOMIC |
> -					 GFP_DMA32 | __GFP_COLD);
> -			pad[1] = 0;
> -		}
> +		bufptr = kmalloc(primary_buf_len, GFP_ATOMIC | GFP_DMA);
> +		pad[1] = 0;
>
>   		if (unlikely(!bufptr)) {
>   			dev_warn_ratelimited(netcp->ndev_dev, "Primary RX buffer alloc failed\n");
> @@ -827,7 +821,7 @@ static void netcp_allocate_rx_buf(struct netcp_intf *netcp, int fdq)
>
>   	} else {
>   		/* Allocate a secondary receive queue entry */
> -		page = alloc_page(GFP_ATOMIC | GFP_DMA32 | __GFP_COLD);
> +		page = alloc_page(GFP_ATOMIC | GFP_DMA | __GFP_COLD);
>   		if (unlikely(!page)) {
>   			dev_warn_ratelimited(netcp->ndev_dev, "Secondary page alloc failed\n");
>   			goto fail;
>
>
>
>


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: [PATCH 1/2] net: Export __netdev_alloc_frag() to allow gfp_mask flags

[Eric Dumazet]

On Wed, 2015-07-29 at 11:10 -0400, WingMan Kwok wrote:
> This patch makes the function __netdev_alloc_frag() non-static and
> exports it so that drivers that need to specify additional flags,
> such as __GFP_DMA, can use it. The currently exported function,
> netdev_alloc_frag() doesn't allow passing in gfp_mask flags.
> 
> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
> Signed-off-by: Reece R. Pollack <x0183204@ti.com>
> ---
>  include/linux/skbuff.h |    1 +
>  net/core/skbuff.c      |    3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)

You can not do this.

__napi_alloc_frag() uses __alloc_page_frag() using a per cpu reserve.

This per cpu reserve would be shared by regular GFP_ATOMIC and your
__GFP_DMA allocations.

Re: [PATCH 1/2] net: Export __netdev_alloc_frag() to allow gfp_mask flags

[Murali Karicheri]

Eric,

On 07/29/2015 12:31 PM, Eric Dumazet wrote:
> On Wed, 2015-07-29 at 11:10 -0400, WingMan Kwok wrote:
>> This patch makes the function __netdev_alloc_frag() non-static and
>> exports it so that drivers that need to specify additional flags,
>> such as __GFP_DMA, can use it. The currently exported function,
>> netdev_alloc_frag() doesn't allow passing in gfp_mask flags.
>>
>> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
>> Signed-off-by: Reece R. Pollack <x0183204@ti.com>
>> ---
>>   include/linux/skbuff.h |    1 +
>>   net/core/skbuff.c      |    3 ++-
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>
> You can not do this.
>
> __napi_alloc_frag() uses __alloc_page_frag() using a per cpu reserve.
>
Thanks for your response.

I assume you mean to say __netdev_alloc_frag() which is what the patch 
affects. Right?

> This per cpu reserve would be shared by regular GFP_ATOMIC and your
> __GFP_DMA allocations.

I am trying to understand the issue here. Is there any issue in sharing 
this per CPU reserve between DMA and ATOMIC allocations. Without this 
flag, the assumption is this function can return memory which is not 
DMA-able and this flag assures it is allocated from DMA zone.

Murali
>
>
>
>
>


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: [PATCH 1/2] net: Export __netdev_alloc_frag() to allow gfp_mask flags

[Eric Dumazet]

On Wed, 2015-07-29 at 16:22 -0400, Murali Karicheri wrote:
> Eric,
> 
> On 07/29/2015 12:31 PM, Eric Dumazet wrote:
> > On Wed, 2015-07-29 at 11:10 -0400, WingMan Kwok wrote:
> >> This patch makes the function __netdev_alloc_frag() non-static and
> >> exports it so that drivers that need to specify additional flags,
> >> such as __GFP_DMA, can use it. The currently exported function,
> >> netdev_alloc_frag() doesn't allow passing in gfp_mask flags.
> >>
> >> Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
> >> Signed-off-by: Reece R. Pollack <x0183204@ti.com>
> >> ---
> >>   include/linux/skbuff.h |    1 +
> >>   net/core/skbuff.c      |    3 ++-
> >>   2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > You can not do this.
> >
> > __napi_alloc_frag() uses __alloc_page_frag() using a per cpu reserve.
> >
> Thanks for your response.
> 
> I assume you mean to say __netdev_alloc_frag() which is what the patch 
> affects. Right?
> 
> > This per cpu reserve would be shared by regular GFP_ATOMIC and your
> > __GFP_DMA allocations.
> 
> I am trying to understand the issue here. Is there any issue in sharing 
> this per CPU reserve between DMA and ATOMIC allocations. Without this 
> flag, the assumption is this function can return memory which is not 
> DMA-able and this flag assures it is allocated from DMA zone.

First caller __netdev_alloc_frag() uses GFP_ATOMIC.

A big page (32 KB) is allocated and stored into cache. Part of it given
to caller. (like 1536 bytes or so)

Then your driver calls with __GFP_DMA.

We find a prior page on percpu cache with enough room in it to allocate
a fragment.

Your driver getd a fragment from the prior GFP_ATOMIC allocation, with
no DMA guarantee.

Therefore, your patch is not working in all cases.