[RFC] page_pool: add debugging to catch concurrent access to pool->alloc

[Yunsheng Lin]

Currently if there is a warning message almost infinity as
below when driver is unloaded, it seems there may be three
reasons as below:
1. Concurrent accese to the pool->alloc related data causing
   incorrect inflight stat.
2. The driver leaks some page.
3. The network stack holds the skb pointing to some page and
   does not seem to release it.

"page_pool_release_retry() stalled pool shutdown: id 949, 98
inflight 1449 sec"

Use the currently unused pool->ring.consumer_lock to catch the
case of concurrent access to pool->alloc.

The binary size is unchanged after this patch when the debug
feature is not enabled.

Signed-off-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
---
 net/Kconfig.debug    |  8 +++++
 net/core/page_pool.c | 70 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 74 insertions(+), 4 deletions(-)

diff --git a/net/Kconfig.debug b/net/Kconfig.debug
index 5e3fffe707dd..5575d63d7a36 100644
--- a/net/Kconfig.debug
+++ b/net/Kconfig.debug
@@ -18,6 +18,14 @@ config NET_NS_REFCNT_TRACKER
 	  Enable debugging feature to track netns references.
 	  This adds memory and cpu costs.
 
+config PAGE_POOL_DEBUG
+	bool "Enable page pool debugging"
+	depends on PAGE_POOL
+	default n
+	help
+	  Enable debugging feature in page pool to catch concurrent
+	  access for pool->alloc cache.
+
 config DEBUG_NET
 	bool "Add generic networking debug"
 	depends on DEBUG_KERNEL && NET
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 2abe6e919224..8ef70d4252fb 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -31,6 +31,36 @@
 
 #define BIAS_MAX	(LONG_MAX >> 1)
 
+#ifdef CONFIG_PAGE_POOL_DEBUG
+#define __page_pool_debug_alloc_lock(pool, allow_direct, warn_on_destry)		\
+	do {										\
+		if (allow_direct) {							\
+			WARN_ON_ONCE(spin_is_locked(&(pool)->ring.consumer_lock));	\
+			spin_lock(&(pool)->ring.consumer_lock);				\
+			WARN_ON_ONCE(warn_on_destry && (pool)->destroy_cnt);		\
+		}									\
+	} while (0)
+
+#define __page_pool_debug_alloc_unlock(pool, allow_direct, warn_on_destry)		\
+	do {										\
+		if (allow_direct) {							\
+			WARN_ON_ONCE(warn_on_destry && (pool)->destroy_cnt);		\
+			spin_unlock(&(pool)->ring.consumer_lock);			\
+		}									\
+	} while (0)
+
+#define page_pool_debug_alloc_lock(pool, allow_direct)					\
+			__page_pool_debug_alloc_lock(pool, allow_direct, true)
+
+#define page_pool_debug_alloc_unlock(pool, allow_direct)				\
+			__page_pool_debug_alloc_unlock(pool, allow_direct, true)
+#else
+#define __page_pool_debug_alloc_lock(pool, allow_direct, warn_on_destry)
+#define __page_pool_debug_alloc_unlock(pool, allow_direct, warn_on_destry)
+#define page_pool_debug_alloc_lock(pool, allow_direct)
+#define page_pool_debug_alloc_unlock(pool, allow_direct)
+#endif
+
 #ifdef CONFIG_PAGE_POOL_STATS
 static DEFINE_PER_CPU(struct page_pool_recycle_stats, pp_system_recycle_stats);
 
@@ -563,7 +593,7 @@ static noinline netmem_ref __page_pool_alloc_pages_slow(struct page_pool *pool,
 /* For using page_pool replace: alloc_pages() API calls, but provide
  * synchronization guarantee for allocation side.
  */
-netmem_ref page_pool_alloc_netmem(struct page_pool *pool, gfp_t gfp)
+static netmem_ref __page_pool_alloc_netmem(struct page_pool *pool, gfp_t gfp)
 {
 	netmem_ref netmem;
 
@@ -576,6 +606,17 @@ netmem_ref page_pool_alloc_netmem(struct page_pool *pool, gfp_t gfp)
 	netmem = __page_pool_alloc_pages_slow(pool, gfp);
 	return netmem;
 }
+
+netmem_ref page_pool_alloc_netmem(struct page_pool *pool, gfp_t gfp)
+{
+	netmem_ref netmem;
+
+	page_pool_debug_alloc_lock(pool, true);
+	netmem = __page_pool_alloc_netmem(pool, gfp);
+	page_pool_debug_alloc_unlock(pool, true);
+
+	return netmem;
+}
 EXPORT_SYMBOL(page_pool_alloc_netmem);
 
 struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp)
@@ -776,6 +817,8 @@ void page_pool_put_unrefed_netmem(struct page_pool *pool, netmem_ref netmem,
 	if (!allow_direct)
 		allow_direct = page_pool_napi_local(pool);
 
+	page_pool_debug_alloc_lock(pool, allow_direct);
+
 	netmem =
 		__page_pool_put_page(pool, netmem, dma_sync_size, allow_direct);
 	if (netmem && !page_pool_recycle_in_ring(pool, netmem)) {
@@ -783,6 +826,8 @@ void page_pool_put_unrefed_netmem(struct page_pool *pool, netmem_ref netmem,
 		recycle_stat_inc(pool, ring_full);
 		page_pool_return_page(pool, netmem);
 	}
+
+	page_pool_debug_alloc_unlock(pool, allow_direct);
 }
 EXPORT_SYMBOL(page_pool_put_unrefed_netmem);
 
@@ -817,6 +862,7 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
 	bool in_softirq;
 
 	allow_direct = page_pool_napi_local(pool);
+	page_pool_debug_alloc_lock(pool, allow_direct);
 
 	for (i = 0; i < count; i++) {
 		netmem_ref netmem = page_to_netmem(virt_to_head_page(data[i]));
@@ -831,6 +877,8 @@ void page_pool_put_page_bulk(struct page_pool *pool, void **data,
 			data[bulk_len++] = (__force void *)netmem;
 	}
 
+	page_pool_debug_alloc_unlock(pool, allow_direct);
+
 	if (!bulk_len)
 		return;
 
@@ -878,10 +926,14 @@ static netmem_ref page_pool_drain_frag(struct page_pool *pool,
 
 static void page_pool_free_frag(struct page_pool *pool)
 {
-	long drain_count = BIAS_MAX - pool->frag_users;
-	netmem_ref netmem = pool->frag_page;
+	netmem_ref netmem;
+	long drain_count;
 
+	page_pool_debug_alloc_lock(pool, true);
+	drain_count = BIAS_MAX - pool->frag_users;
+	netmem = pool->frag_page;
 	pool->frag_page = 0;
+	page_pool_debug_alloc_unlock(pool, true);
 
 	if (!netmem || page_pool_unref_netmem(netmem, drain_count))
 		return;
@@ -899,6 +951,7 @@ netmem_ref page_pool_alloc_frag_netmem(struct page_pool *pool,
 	if (WARN_ON(size > max_size))
 		return 0;
 
+	page_pool_debug_alloc_lock(pool, true);
 	size = ALIGN(size, dma_get_cache_alignment());
 	*offset = pool->frag_offset;
 
@@ -911,9 +964,10 @@ netmem_ref page_pool_alloc_frag_netmem(struct page_pool *pool,
 	}
 
 	if (!netmem) {
-		netmem = page_pool_alloc_netmem(pool, gfp);
+		netmem = __page_pool_alloc_netmem(pool, gfp);
 		if (unlikely(!netmem)) {
 			pool->frag_page = 0;
+			page_pool_debug_alloc_unlock(pool, true);
 			return 0;
 		}
 
@@ -924,12 +978,14 @@ netmem_ref page_pool_alloc_frag_netmem(struct page_pool *pool,
 		*offset = 0;
 		pool->frag_offset = size;
 		page_pool_fragment_netmem(netmem, BIAS_MAX);
+		page_pool_debug_alloc_unlock(pool, true);
 		return netmem;
 	}
 
 	pool->frag_users++;
 	pool->frag_offset = *offset + size;
 	alloc_stat_inc(pool, fast);
+	page_pool_debug_alloc_unlock(pool, true);
 	return netmem;
 }
 EXPORT_SYMBOL(page_pool_alloc_frag_netmem);
@@ -986,8 +1042,10 @@ static void page_pool_empty_alloc_cache_once(struct page_pool *pool)
 
 static void page_pool_scrub(struct page_pool *pool)
 {
+	__page_pool_debug_alloc_lock(pool, true, false);
 	page_pool_empty_alloc_cache_once(pool);
 	pool->destroy_cnt++;
+	__page_pool_debug_alloc_unlock(pool, true, false);
 
 	/* No more consumers should exist, but producers could still
 	 * be in-flight.
@@ -1089,6 +1147,8 @@ void page_pool_update_nid(struct page_pool *pool, int new_nid)
 {
 	netmem_ref netmem;
 
+	page_pool_debug_alloc_lock(pool, true);
+
 	trace_page_pool_update_nid(pool, new_nid);
 	pool->p.nid = new_nid;
 
@@ -1097,5 +1157,7 @@ void page_pool_update_nid(struct page_pool *pool, int new_nid)
 		netmem = pool->alloc.cache[--pool->alloc.count];
 		page_pool_return_page(pool, netmem);
 	}
+
+	page_pool_debug_alloc_unlock(pool, true);
 }
 EXPORT_SYMBOL(page_pool_update_nid);
-- 
2.33.0