[PATCH RFC net-next 0/2] Fix pfmemalloc in __netdev_alloc_skb

[PATCH RFC net-next 0/2] Fix pfmemalloc in __netdev_alloc_skb

[Govindarajulu Varadarajan]

Hi

After the introduction of skb->pfmemalloc in
c93bdd0e0:(allow skb allocation to use PFMEMALLOC reserves), skb->pfmemalloc
should be set to true if buff is allocated from reserve memory.

But in a1c7fff7e(netdev_alloc_skb() use build_skb()), netdev_alloc_skb() does
not set skb->pfmemalloc if the page is allocated from reserve memory.

Also if the page in netdev_alloc_cache->frag is allocated from reserve memory,
and if the next call to __netdev_alloc_frag does not have __GFP_MEMALLOC in
flags, we should not return a reserved memory. Because the reserve memory could
be sitting in receive queue waiting for packet to arrive. We should release
the reserve memory as soon as possible.

Is my understanding correct?

Govindarajulu Varadarajan (2):
  net: skbuff: propagate pfmemalloc to skb
  net: skbuff: do not allocate emergency memory if flags is not
    __GFP_MEMALLOC

 net/core/skbuff.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

-- 
2.1.0

[PATCH RFC net-next 1/2] net: skbuff: propagate pfmemalloc to skb

[Govindarajulu Varadarajan]

In __netdev_alloc_skb, __netdev_alloc_frag might have allocated page with
flag __GFP_MEMALLOC. But we do not propagate it to the skb.

Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
 net/core/skbuff.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 163b673..51a3328 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -425,8 +425,14 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
 
 		if (likely(data)) {
 			skb = build_skb(data, fragsz);
-			if (unlikely(!skb))
+			if (unlikely(!skb)) {
 				put_page(virt_to_head_page(data));
+			} else {
+				struct page *page;
+
+				page = virt_to_head_page(data);
+				skb_propagate_pfmemalloc(page, skb);
+			}
 		}
 	} else {
 		skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask,
-- 
2.1.0

[PATCH RFC net-next 2/2] net: skbuff: do not allocate emergency memory if flags is not __GFP_MEMALLOC

[Govindarajulu Varadarajan]

It is possible that nc->frag.page is previously allocated from emergency memory.
For new alloc call, if the flags does not contain __GFP_MEMALLOC, do not
return pfmemalloc memory.

Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
 net/core/skbuff.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 51a3328..792ce89 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -364,6 +364,13 @@ recycle:
 		atomic_set(&nc->frag.page->_count, NETDEV_PAGECNT_MAX_BIAS);
 		nc->pagecnt_bias = NETDEV_PAGECNT_MAX_BIAS;
 		nc->frag.offset = 0;
+	} else if (nc->frag.page->pfmemalloc && !(gfp_mask & __GFP_MEMALLOC)) {
+		if (atomic_sub_and_test(nc->pagecnt_bias,
+					&nc->frag.page->_count)) {
+			atomic_set(&nc->frag.page->_count, 1);
+			kfree(page_address(nc->frag.page));
+		}
+		goto refill;
 	}
 
 	if (nc->frag.offset + fragsz > nc->frag.size) {
-- 
2.1.0

Re: [PATCH RFC net-next 1/2] net: skbuff: propagate pfmemalloc to skb

[Eric Dumazet]

On Sun, 2014-08-24 at 03:07 +0530, Govindarajulu Varadarajan wrote:
> In __netdev_alloc_skb, __netdev_alloc_frag might have allocated page with
> flag __GFP_MEMALLOC. But we do not propagate it to the skb.
> 
> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
> ---
>  net/core/skbuff.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 163b673..51a3328 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -425,8 +425,14 @@ struct sk_buff *__netdev_alloc_skb(struct net_device *dev,
>  
>  		if (likely(data)) {
>  			skb = build_skb(data, fragsz);
> -			if (unlikely(!skb))
> +			if (unlikely(!skb)) {
>  				put_page(virt_to_head_page(data));
> +			} else {
> +				struct page *page;
> +
> +				page = virt_to_head_page(data);
> +				skb_propagate_pfmemalloc(page, skb);
> +			}
>  		}
>  	} else {
>  		skb = __alloc_skb(length + NET_SKB_PAD, gfp_mask,

Oh well...

We made everything we could to avoid touching the page head for every
frag allocation.

Just cache page->pfmemalloc into nc->frag.pfmemalloc

Think of how often asymmetric alloc/free happen in networking stack and
why we use NETDEV_PAGECNT_MAX_BIAS trick.

Thanks

Re: [PATCH RFC net-next 2/2] net: skbuff: do not allocate emergency memory if flags is not __GFP_MEMALLOC

[Eric Dumazet]

On Sun, 2014-08-24 at 03:07 +0530, Govindarajulu Varadarajan wrote:
> It is possible that nc->frag.page is previously allocated from emergency memory.
> For new alloc call, if the flags does not contain __GFP_MEMALLOC, do not
> return pfmemalloc memory.
> 
> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
> ---
>  net/core/skbuff.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 51a3328..792ce89 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -364,6 +364,13 @@ recycle:
>  		atomic_set(&nc->frag.page->_count, NETDEV_PAGECNT_MAX_BIAS);
>  		nc->pagecnt_bias = NETDEV_PAGECNT_MAX_BIAS;
>  		nc->frag.offset = 0;
> +	} else if (nc->frag.page->pfmemalloc && !(gfp_mask & __GFP_MEMALLOC)) {
> +		if (atomic_sub_and_test(nc->pagecnt_bias,
> +					&nc->frag.page->_count)) {
> +			atomic_set(&nc->frag.page->_count, 1);
> +			kfree(page_address(nc->frag.page));

No idea of what you think you do.

How have you tested this patch ?

> +		}
> +		goto refill;
>  	}
>  
>  	if (nc->frag.offset + fragsz > nc->frag.size) {