[net-next,2/2] net: page_pool: don't use page->private to store dma_addr_t

Message ID 154990121192.24530.11128024662816211563.stgit@firesoul
State Superseded
Headers show
Series
  • Untitled series #18531
Related show

Commit Message

Jesper Dangaard Brouer Feb. 11, 2019, 4:06 p.m.
From: Ilias Apalodimas <ilias.apalodimas@linaro.org>


As pointed out by David Miller the current page_pool implementation
stores dma_addr_t in page->private.
This won't work on 32-bit platforms with 64-bit DMA addresses since the
page->private is an unsigned long and the dma_addr_t a u64.

A previous patch is adding dma_addr_t on struct page to accommodate this.
This patch adapts the page_pool related functions to use the newly added
struct for storing and retrieving DMA addresses from network drivers.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

---
 net/core/page_pool.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Alexander Duyck Feb. 11, 2019, 7:31 p.m. | #1
On Mon, Feb 11, 2019 at 8:07 AM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>

> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>

>

> As pointed out by David Miller the current page_pool implementation

> stores dma_addr_t in page->private.

> This won't work on 32-bit platforms with 64-bit DMA addresses since the

> page->private is an unsigned long and the dma_addr_t a u64.

>

> A previous patch is adding dma_addr_t on struct page to accommodate this.

> This patch adapts the page_pool related functions to use the newly added

> struct for storing and retrieving DMA addresses from network drivers.

>

> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

> ---

>  net/core/page_pool.c |   13 +++++++++----

>  1 file changed, 9 insertions(+), 4 deletions(-)

>

> diff --git a/net/core/page_pool.c b/net/core/page_pool.c

> index 43a932cb609b..897a69a1477e 100644

> --- a/net/core/page_pool.c

> +++ b/net/core/page_pool.c

> @@ -136,7 +136,9 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,

>         if (!(pool->p.flags & PP_FLAG_DMA_MAP))

>                 goto skip_dma_map;

>

> -       /* Setup DMA mapping: use page->private for DMA-addr

> +       /* Setup DMA mapping: use 'struct page' area for storing DMA-addr

> +        * since dma_addr_t can be either 32 or 64 bits and does not always fit

> +        * into page private data (i.e 32bit cpu with 64bit DMA caps)

>          * This mapping is kept for lifetime of page, until leaving pool.

>          */

>         dma = dma_map_page(pool->p.dev, page, 0,

> @@ -146,7 +148,7 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,

>                 put_page(page);

>                 return NULL;

>         }

> -       set_page_private(page, dma); /* page->private = dma; */

> +       page->dma_addr = dma;

>

>  skip_dma_map:

>         /* When page just alloc'ed is should/must have refcnt 1. */

> @@ -175,13 +177,16 @@ EXPORT_SYMBOL(page_pool_alloc_pages);

>  static void __page_pool_clean_page(struct page_pool *pool,

>                                    struct page *page)

>  {

> +       dma_addr_t dma;

> +

>         if (!(pool->p.flags & PP_FLAG_DMA_MAP))

>                 return;

>

> +       dma = page->dma_addr;

>         /* DMA unmap */

> -       dma_unmap_page(pool->p.dev, page_private(page),

> +       dma_unmap_page(pool->p.dev, dma,

>                        PAGE_SIZE << pool->p.order, pool->p.dma_dir);

> -       set_page_private(page, 0);

> +       page->dma_addr = 0;

>  }

>

>  /* Return a page to the page allocator, cleaning up our state */


This comment is unrelated to this patch specifically, but applies more
generally to the page_pool use of dma_unmap_page.

So just looking at this I am pretty sure the use of just
dma_unmap_page isn't correct here. You should probably be using
dma_unmap_page_attrs and specifically be passing the attribute
DMA_ATTR_SKIP_CPU_SYNC so that you can tear down the mapping without
invalidating the contents of the page.

This is something that will work for most cases but if you run into a
case where this is used with SWIOTLB in bounce buffer mode you would
end up potentially corrupting data on the unmap call.
Jesper Dangaard Brouer Feb. 12, 2019, 8:23 a.m. | #2
On Mon, 11 Feb 2019 11:31:13 -0800
Alexander Duyck <alexander.duyck@gmail.com> wrote:

> On Mon, Feb 11, 2019 at 8:07 AM Jesper Dangaard Brouer

> <brouer@redhat.com> wrote:

> >

> > From: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> >

> > As pointed out by David Miller the current page_pool implementation

> > stores dma_addr_t in page->private.

> > This won't work on 32-bit platforms with 64-bit DMA addresses since the

> > page->private is an unsigned long and the dma_addr_t a u64.

> >

> > A previous patch is adding dma_addr_t on struct page to accommodate this.

> > This patch adapts the page_pool related functions to use the newly added

> > struct for storing and retrieving DMA addresses from network drivers.

> >

> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

> > ---

> >  net/core/page_pool.c |   13 +++++++++----

> >  1 file changed, 9 insertions(+), 4 deletions(-)

> >

> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c

> > index 43a932cb609b..897a69a1477e 100644

> > --- a/net/core/page_pool.c

> > +++ b/net/core/page_pool.c

> > @@ -136,7 +136,9 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,

> >         if (!(pool->p.flags & PP_FLAG_DMA_MAP))

> >                 goto skip_dma_map;

> >

> > -       /* Setup DMA mapping: use page->private for DMA-addr

> > +       /* Setup DMA mapping: use 'struct page' area for storing DMA-addr

> > +        * since dma_addr_t can be either 32 or 64 bits and does not always fit

> > +        * into page private data (i.e 32bit cpu with 64bit DMA caps)

> >          * This mapping is kept for lifetime of page, until leaving pool.

> >          */

> >         dma = dma_map_page(pool->p.dev, page, 0,

> > @@ -146,7 +148,7 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,

> >                 put_page(page);

> >                 return NULL;

> >         }

> > -       set_page_private(page, dma); /* page->private = dma; */

> > +       page->dma_addr = dma;

> >

> >  skip_dma_map:

> >         /* When page just alloc'ed is should/must have refcnt 1. */

> > @@ -175,13 +177,16 @@ EXPORT_SYMBOL(page_pool_alloc_pages);

> >  static void __page_pool_clean_page(struct page_pool *pool,

> >                                    struct page *page)

> >  {

> > +       dma_addr_t dma;

> > +

> >         if (!(pool->p.flags & PP_FLAG_DMA_MAP))

> >                 return;

> >

> > +       dma = page->dma_addr;

> >         /* DMA unmap */

> > -       dma_unmap_page(pool->p.dev, page_private(page),

> > +       dma_unmap_page(pool->p.dev, dma,

> >                        PAGE_SIZE << pool->p.order, pool->p.dma_dir);

> > -       set_page_private(page, 0);

> > +       page->dma_addr = 0;

> >  }

> >

> >  /* Return a page to the page allocator, cleaning up our state */  

> 

> This comment is unrelated to this patch specifically, but applies more

> generally to the page_pool use of dma_unmap_page.

> 

> So just looking at this I am pretty sure the use of just

> dma_unmap_page isn't correct here. You should probably be using

> dma_unmap_page_attrs and specifically be passing the attribute

> DMA_ATTR_SKIP_CPU_SYNC so that you can tear down the mapping without

> invalidating the contents of the page.


It is unrelated to this patch, but YES you are right.  I was aware of
this, but it slipped my mind.  You were the one that taught me the
principle page_pool is based on, that we keep the DMA mapping, but
instead let the driver perform the DMA-sync operations.

Thanks for catching this!  I actually think that the current small
ARM64 board we are playing with at the moment (Espressobin) will have a
performance benefit from doing this.


> This is something that will work for most cases but if you run into a

> case where this is used with SWIOTLB in bounce buffer mode you would

> end up potentially corrupting data on the unmap call.


I do have a board Machiattobin, that operate with SWIOTLB bounce
buffers, which it is not suppose to, and something that I'll hopefully
get a round to fix soon.  But we have not implemented use of page_pool
on that board yet. So, thanks for catching this.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Patch

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 43a932cb609b..897a69a1477e 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -136,7 +136,9 @@  static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
 	if (!(pool->p.flags & PP_FLAG_DMA_MAP))
 		goto skip_dma_map;
 
-	/* Setup DMA mapping: use page->private for DMA-addr
+	/* Setup DMA mapping: use 'struct page' area for storing DMA-addr
+	 * since dma_addr_t can be either 32 or 64 bits and does not always fit
+	 * into page private data (i.e 32bit cpu with 64bit DMA caps)
 	 * This mapping is kept for lifetime of page, until leaving pool.
 	 */
 	dma = dma_map_page(pool->p.dev, page, 0,
@@ -146,7 +148,7 @@  static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
 		put_page(page);
 		return NULL;
 	}
-	set_page_private(page, dma); /* page->private = dma; */
+	page->dma_addr = dma;
 
 skip_dma_map:
 	/* When page just alloc'ed is should/must have refcnt 1. */
@@ -175,13 +177,16 @@  EXPORT_SYMBOL(page_pool_alloc_pages);
 static void __page_pool_clean_page(struct page_pool *pool,
 				   struct page *page)
 {
+	dma_addr_t dma;
+
 	if (!(pool->p.flags & PP_FLAG_DMA_MAP))
 		return;
 
+	dma = page->dma_addr;
 	/* DMA unmap */
-	dma_unmap_page(pool->p.dev, page_private(page),
+	dma_unmap_page(pool->p.dev, dma,
 		       PAGE_SIZE << pool->p.order, pool->p.dma_dir);
-	set_page_private(page, 0);
+	page->dma_addr = 0;
 }
 
 /* Return a page to the page allocator, cleaning up our state */