| Message ID | 154990121192.24530.11128024662816211563.stgit@firesoul (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix page_pool API and dma address storage | expand |
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.
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.
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 */