On Mon, 11 Feb 2019 11:31:13 -0800
Alexander Duyck <alexander.du...@gmail.com> wrote:
> On Mon, Feb 11, 2019 at 8:07 AM Jesper Dangaard Brouer
> <bro...@redhat.com> wrote:
> >
> > From: Ilias Apalodimas <ilias.apalodi...@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.apalodi...@linaro.org>
> > Signed-off-by: Jesper Dangaard Brouer <bro...@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
