diff mbox series

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

Message ID 20240903122208.3379182-1-linyunsheng@huawei.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series [RFC] page_pool: add debugging to catch concurrent access to pool->alloc | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 16 this patch: 18
netdev/checkpatch warning CHECK: Macro argument 'warn_on_destry' may be better as '(warn_on_destry)' to avoid precedence issues WARNING: Argument 'allow_direct' is not used in function-like macro WARNING: Argument 'pool' is not used in function-like macro WARNING: Argument 'warn_on_destry' is not used in function-like macro WARNING: Where possible, use lockdep_assert_held instead of assertions based on spin_is_locked WARNING: line length of 89 exceeds 80 columns WARNING: please write a help paragraph that fully describes the config symbol
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 3 this patch: 3
netdev/source_inline success Was 0 now: 0

Commit Message

Yunsheng Lin Sept. 3, 2024, 12:22 p.m. UTC 
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@huawei.com>
---
 net/Kconfig.debug    |  8 +++++
 net/core/page_pool.c | 70 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 74 insertions(+), 4 deletions(-)
diff mbox series

Patch

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);