| Message ID | 1470314858-32105-1-git-send-email-ilyal@mellanox.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2016-08-04 at 15:47 +0300, Ilya Lesokhin wrote: > The code currently always references the first page in the > frag therefore there is no need to pay the extra overhead > of making the frag page compound > > Signed-off-by: Ilya Lesokhin <ilyal@mellanox.com> > --- > net/core/sock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/sock.c b/net/core/sock.c > index 08bf97e..77432ba 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -1993,7 +1993,7 @@ bool skb_page_frag_refill(unsigned int sz, struct page_frag *pfrag, gfp_t gfp) > if (SKB_FRAG_PAGE_ORDER) { > /* Avoid direct reclaim but allow kswapd to wake */ > pfrag->page = alloc_pages((gfp & ~__GFP_DIRECT_RECLAIM) | > - __GFP_COMP | __GFP_NOWARN | > + __GFP_NOWARN | > __GFP_NORETRY, > SKB_FRAG_PAGE_ORDER); > if (likely(pfrag->page)) { Looks very risky to me. What numbers can you show us exactly ? splice() is known to play games on pages, make sure it still works ?
You are right I just realized that this patch causes a memory leak, so sorry for the spam. I just had problem understanding when compound pages are needed and when It is enough to use an high order pages. I thought that that high order pages are enough for this use case, tested it and saw no problems. Unfortunately it seems you must use compound pages to keep track of the page order. put_page assumes that the order of a non-compound page is always 0. It would be nice to document it somewhere but I'm not sure where. Thanks, Ilya > -----Original Message----- > From: Eric Dumazet [mailto:eric.dumazet@gmail.com] > Sent: Thursday, August 04, 2016 4:25 PM > To: Ilya Lesokhin <ilyal@mellanox.com> > Cc: netdev@vger.kernel.org; edumazet@google.com > Subject: Re: [PATCH] net: use non-compound pages in frag allocator > > On Thu, 2016-08-04 at 15:47 +0300, Ilya Lesokhin wrote: > > The code currently always references the first page in the frag > > therefore there is no need to pay the extra overhead of making the > > frag page compound > > > > Signed-off-by: Ilya Lesokhin <ilyal@mellanox.com> > > --- > > net/core/sock.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/core/sock.c b/net/core/sock.c index 08bf97e..77432ba > > 100644 > > --- a/net/core/sock.c > > +++ b/net/core/sock.c > > @@ -1993,7 +1993,7 @@ bool skb_page_frag_refill(unsigned int sz, struct > page_frag *pfrag, gfp_t gfp) > > if (SKB_FRAG_PAGE_ORDER) { > > /* Avoid direct reclaim but allow kswapd to wake */ > > pfrag->page = alloc_pages((gfp & ~__GFP_DIRECT_RECLAIM) > | > > - __GFP_COMP | __GFP_NOWARN | > > + __GFP_NOWARN | > > __GFP_NORETRY, > > SKB_FRAG_PAGE_ORDER); > > if (likely(pfrag->page)) { > > > Looks very risky to me. > > What numbers can you show us exactly ? > > splice() is known to play games on pages, make sure it still works ? > >
On Thu, 2016-08-04 at 13:38 +0000, Ilya Lesokhin wrote: > You are right I just realized that this patch causes a memory leak, so sorry for the spam. > > I just had problem understanding when compound pages are needed and when It is enough to use an high order pages. > I thought that that high order pages are enough for this use case, tested it and saw no problems. > > Unfortunately it seems you must use compound pages to keep track of the page order. > put_page assumes that the order of a non-compound page is always 0. > It would be nice to document it somewhere but I'm not sure where. Please do not top-post on netdev mailing list. ( Documentation/development-process/2.Process : line 422 ) AFAIK, only when we can call free_pages(p, order) and provide @order, we can avoid using __GFP_COMP at alloc time. But if some path has to use put_page(), then we are forced to use __GFP_COMP for high order pages. Thanks.
diff --git a/net/core/sock.c b/net/core/sock.c index 08bf97e..77432ba 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1993,7 +1993,7 @@ bool skb_page_frag_refill(unsigned int sz, struct page_frag *pfrag, gfp_t gfp) if (SKB_FRAG_PAGE_ORDER) { /* Avoid direct reclaim but allow kswapd to wake */ pfrag->page = alloc_pages((gfp & ~__GFP_DIRECT_RECLAIM) | - __GFP_COMP | __GFP_NOWARN | + __GFP_NOWARN | __GFP_NORETRY, SKB_FRAG_PAGE_ORDER); if (likely(pfrag->page)) {
The code currently always references the first page in the frag therefore there is no need to pay the extra overhead of making the frag page compound Signed-off-by: Ilya Lesokhin <ilyal@mellanox.com> --- net/core/sock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)