From: Alexander Duyck <alexander.duyck@gmail.com>
To: Jesper Dangaard Brouer <brouer@redhat.com>
Cc: Netdev <netdev@vger.kernel.org>, linux-mm <linux-mm@kvack.org>,
"Toke Høiland-Jørgensen" <toke@toke.dk>,
"Ilias Apalodimas" <ilias.apalodimas@linaro.org>,
"Matthew Wilcox" <willy@infradead.org>,
"Saeed Mahameed" <saeedm@mellanox.com>,
"Andrew Morton" <akpm@linux-foundation.org>,
"Mel Gorman" <mgorman@techsingularity.net>,
"David S. Miller" <davem@davemloft.net>,
"Tariq Toukan" <tariqt@mellanox.com>
Subject: Re: [net-next PATCH 2/2] net: page_pool: don't use page->private to store dma_addr_t
Date: Mon, 11 Feb 2019 11:31:13 -0800 [thread overview]
Message-ID: <CAKgT0Ucw_HGaice7cjM7e_nYuvjU_TKVd54Yc_fHen1pZRkUJw@mail.gmail.com> (raw)
In-Reply-To: <154990121192.24530.11128024662816211563.stgit@firesoul>
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.
next prev parent reply other threads:[~2019-02-11 19:31 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-11 16:06 [net-next PATCH 0/2] Fix page_pool API and dma address storage Jesper Dangaard Brouer
2019-02-11 16:06 ` [net-next PATCH 1/2] mm: add dma_addr_t to struct page Jesper Dangaard Brouer
2019-02-11 16:55 ` Matthew Wilcox
2019-02-12 10:06 ` Jesper Dangaard Brouer
2019-02-11 20:16 ` Andrew Morton
2019-02-12 8:28 ` Jesper Dangaard Brouer
2019-02-11 16:06 ` [net-next PATCH 2/2] net: page_pool: don't use page->private to store dma_addr_t Jesper Dangaard Brouer
2019-02-11 19:31 ` Alexander Duyck [this message]
2019-02-12 8:23 ` Jesper Dangaard Brouer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKgT0Ucw_HGaice7cjM7e_nYuvjU_TKVd54Yc_fHen1pZRkUJw@mail.gmail.com \
--to=alexander.duyck@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=brouer@redhat.com \
--cc=davem@davemloft.net \
--cc=ilias.apalodimas@linaro.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@techsingularity.net \
--cc=netdev@vger.kernel.org \
--cc=saeedm@mellanox.com \
--cc=tariqt@mellanox.com \
--cc=toke@toke.dk \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).