[RFC, PATCH] net: page_pool: Don't use page->private to store dma_addr_t

* [RFC, PATCH] net: page_pool: Don't use page->private to store dma_addr_t
@ 2019-02-07 14:36 Ilias Apalodimas
  2019-02-07 15:07 ` Matthew Wilcox
  0 siblings, 1 reply; 19+ messages in thread
From: Ilias Apalodimas @ 2019-02-07 14:36 UTC (permalink / raw)
  To: brouer, tariqt, toke; +Cc: davem, netdev, mgorman, linux-mm, Ilias Apalodimas

As pointed out in
https://www.mail-archive.com/netdev@vger.kernel.org/msg257926.html
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.

Since no driver is yet using the DMA mapping capabilities of the API let's
try and fix this by shadowing struct_page and use 'struct list_head lru'
to store and retrieve DMA addresses from network drivers.
As long as the addresses returned from dma_map_page() are aligned the
first bit, used by the compound pages code should not be set.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 include/net/page_pool.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
 net/core/page_pool.c    | 18 ++++++++++++----
 2 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 694d055..618f2e5 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -98,6 +98,52 @@ struct page_pool {
 	struct ptr_ring ring;
 };
 
+/* Until we can update struct-page, have a shadow struct-page, that
+ * include our use-case
+ * Used to store retrieve dma addresses from network drivers.
+ * Never access this directly, use helper functions provided
+ * page_pool_get_dma_addr()
+ */
+struct page_shadow {
+	unsigned long flags;		/* Atomic flags, some possibly
+					 * updated asynchronously
+					 */
+	/*
+	 * Five words (20/40 bytes) are available in this union.
+	 * WARNING: bit 0 of the first word is used for PageTail(). That
+	 * means the other users of this union MUST NOT use the bit to
+	 * avoid collision and false-positive PageTail().
+	 */
+	union {
+		struct {	/* Page cache and anonymous pages */
+			/**
+			 * @lru: Pageout list, eg. active_list protected by
+			 * zone_lru_lock.  Sometimes used as a generic list
+			 * by the page owner.
+			 */
+			struct list_head lru;
+			/* See page-flags.h for PAGE_MAPPING_FLAGS */
+			struct address_space *mapping;
+			pgoff_t index;		/* Our offset within mapping. */
+			/**
+			 * @private: Mapping-private opaque data.
+			 * Usually used for buffer_heads if PagePrivate.
+			 * Used for swp_entry_t if PageSwapCache.
+			 * Indicates order in the buddy system if PageBuddy.
+			 */
+			unsigned long private;
+		};
+		struct {	/* page_pool used by netstack */
+			/**
+			 * @dma_addr: Page_pool need to store DMA-addr, and
+			 * cannot use @private, as DMA-mappings can be 64bit
+			 * even on 32-bit Architectures.
+			 */
+			dma_addr_t dma_addr; /* Shares area with @lru */
+		};
+	};
+};
+
 struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
 
 static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool)
@@ -141,4 +187,13 @@ static inline bool is_page_pool_compiled_in(void)
 #endif
 }
 
+static inline dma_addr_t page_pool_get_dma_addr(struct page *page)
+{
+	struct page_shadow *_page;
+
+	_page = (struct page_shadow *)page;
+
+	return _page->dma_addr;
+}
+
 #endif /* _NET_PAGE_POOL_H */
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 43a932c..1a956a6 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -111,6 +111,7 @@ noinline
 static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool,
 						 gfp_t _gfp)
 {
+	struct page_shadow *_page;
 	struct page *page;
 	gfp_t gfp = _gfp;
 	dma_addr_t dma;
@@ -136,7 +137,7 @@ 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
 	 * This mapping is kept for lifetime of page, until leaving pool.
 	 */
 	dma = dma_map_page(pool->p.dev, page, 0,
@@ -146,7 +147,8 @@ 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 = (struct page_shadow *)page;
+	_page->dma_addr = dma;
 
 skip_dma_map:
 	/* When page just alloc'ed is should/must have refcnt 1. */
@@ -175,13 +177,21 @@ EXPORT_SYMBOL(page_pool_alloc_pages);
 static void __page_pool_clean_page(struct page_pool *pool,
 				   struct page *page)
 {
+	struct page_shadow *_page = (struct page_shadow *)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;
+	/* 1. Make sure we don't need to list-init page->lru.
+	 * 2. What does it mean: bit 0 of LRU first word is used for PageTail()
+	 */
 }
 
 /* Return a page to the page allocator, cleaning up our state */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 19+ messages in thread