[PATCH 2/9] netconsole: take over skb pool lifecycle from netpoll

[Breno Leitao]

The fallback skb pool fronted by find_skb() is netconsole's only
client: every other netpoll consumer (bonding, team, vlan, bridge,
macvlan, dsa) goes through __netpoll_setup() / netpoll_send_skb()
without ever touching np->skb_pool. Today __netpoll_setup() and
__netpoll_cleanup() create and destroy the pool for everyone, paying
~48 KB of pre-allocated skbs per netpoll instance that almost
nobody uses.

Move the responsibility to netconsole. Add netconsole-side
netconsole_skb_pool_{init,flush}() wrappers that call the (now
exported) refill_skbs(), refill_skbs_work_handler() and
skb_pool_flush() helpers, and wire them at the same three netpoll
setup paths (resume_target, enabled_store, alloc_param_target) and
matching teardowns (netconsole_process_cleanups_core,
drop_netconsole_target, free_param_target).

Init runs *before* netpoll_setup(), and the failure path flushes
before returning. netpoll_setup() makes nt->np.dev visible to
target_list walkers (notably netconsole_netdev_event); if a
NETDEV_UNREGISTER / NETDEV_RELEASE / NETDEV_JOIN raced and moved
the target to target_cleanup_list before init had run,
netconsole_process_cleanups_core() would call
netconsole_skb_pool_flush() on an uninitialised refill_wq /
skb_pool, splatting WARN_ON(!work->func) in flush_work() and
acquiring an uninitialised spinlock in skb_queue_purge_reason().
Doing init first preserves the pre-series invariant that the pool
is valid whenever nt->np.dev is observable.

Drop the corresponding init/flush from __netpoll_setup(),
__netpoll_cleanup() and the netpoll_setup() error path; the now
empty 'flush' label is also removed. The fields and helpers stay
in struct netpoll for now; subsequent patches relocate the helper
functions and then the fields themselves.

For non-netconsole consumers, np->skb_pool / np->refill_wq are
never initialised, never refilled and never flushed by netpoll
itself, but they are also never read by anyone, so the change is a
no-op for them.

Signed-off-by: Breno Leitao <leitao@xxxxxxxxxx>
---
 drivers/net/netconsole.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++--
 net/core/netpoll.c       | 12 +-----------
 2 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index d804d44af87c..b84de3ba44c3 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -283,11 +283,35 @@ static bool bound_by_mac(struct netconsole_target *nt)
 	return is_valid_ether_addr(nt->np.dev_mac);
 }
 
+/* Initialise the per-target skb pool that find_skb() falls back to and
+ * seed it. Pair with netconsole_skb_pool_flush() at the matching
+ * netpoll teardown.
+ */
+static void netconsole_skb_pool_init(struct netconsole_target *nt)
+{
+	skb_queue_head_init(&nt->np.skb_pool);
+	INIT_WORK(&nt->np.refill_wq, refill_skbs_work_handler);
+	refill_skbs(&nt->np);
+}
+
+static void netconsole_skb_pool_flush(struct netconsole_target *nt)
+{
+	skb_pool_flush(&nt->np);
+}
+
 /* Attempts to resume logging to a deactivated target. */
 static void resume_target(struct netconsole_target *nt)
 {
+	/* Initialise the skb pool before netpoll_setup() makes nt->np.dev
+	 * visible to target_list walkers (e.g. netconsole_netdev_event),
+	 * which otherwise may move the target to the cleanup list and
+	 * call netconsole_skb_pool_flush() on uninitialised state.
+	 */
+	netconsole_skb_pool_init(nt);
+
 	if (netpoll_setup(&nt->np)) {
 		/* netpoll fails setup once, do not try again. */
+		netconsole_skb_pool_flush(nt);
 		nt->state = STATE_DISABLED;
 		return;
 	}
@@ -389,6 +413,7 @@ static void netconsole_process_cleanups_core(void)
 	list_for_each_entry_safe(nt, tmp, &target_cleanup_list, list) {
 		/* all entries in the cleanup_list needs to be disabled */
 		WARN_ON_ONCE(nt->state == STATE_ENABLED);
+		netconsole_skb_pool_flush(nt);
 		do_netpoll_cleanup(&nt->np);
 		if (bound_by_mac(nt))
 			memset(&nt->np.dev_name, 0, IFNAMSIZ);
@@ -732,9 +757,19 @@ static ssize_t enabled_store(struct config_item *item,
 		 */
 		netconsole_print_banner(&nt->np);
 
+		/* Initialise the skb pool before netpoll_setup() so the pool
+		 * is valid as soon as nt->np.dev becomes visible to
+		 * target_list walkers (netconsole_netdev_event), which would
+		 * otherwise call netconsole_skb_pool_flush() on uninitialised
+		 * state.
+		 */
+		netconsole_skb_pool_init(nt);
+
 		ret = netpoll_setup(&nt->np);
-		if (ret)
+		if (ret) {
+			netconsole_skb_pool_flush(nt);
 			goto out_unlock;
+		}
 
 		nt->state = STATE_ENABLED;
 		pr_info("network logging started\n");
@@ -1474,8 +1509,10 @@ static void drop_netconsole_target(struct config_group *group,
 	 * The target may have never been enabled, or was manually disabled
 	 * before being removed so netpoll may have already been cleaned up.
 	 */
-	if (nt->state == STATE_ENABLED)
+	if (nt->state == STATE_ENABLED) {
+		netconsole_skb_pool_flush(nt);
 		netpoll_cleanup(&nt->np);
+	}
 
 	config_item_put(&nt->group.cg_item);
 }
@@ -2257,10 +2294,18 @@ static struct netconsole_target *alloc_param_target(char *target_config,
 	if (err)
 		goto fail;
 
+	/* Initialise the skb pool before netpoll_setup() so the pool is
+	 * valid as soon as nt->np.dev becomes visible. The target is not
+	 * yet on target_list, so a netdev event cannot reach it here, but
+	 * mirror the configfs path for symmetry.
+	 */
+	netconsole_skb_pool_init(nt);
+
 	err = netpoll_setup(&nt->np);
 	if (err) {
 		pr_err("Not enabling netconsole for %s%d. Netpoll setup failed\n",
 		       NETCONSOLE_PARAM_TARGET_PREFIX, cmdline_count);
+		netconsole_skb_pool_flush(nt);
 		if (!IS_ENABLED(CONFIG_NETCONSOLE_DYNAMIC))
 			/* only fail if dynamic reconfiguration is set,
 			 * otherwise, keep the target in the list, but disabled.
@@ -2282,6 +2327,8 @@ static struct netconsole_target *alloc_param_target(char *target_config,
 static void free_param_target(struct netconsole_target *nt)
 {
 	cancel_work_sync(&nt->resume_wq);
+	if (nt->state == STATE_ENABLED)
+		netconsole_skb_pool_flush(nt);
 	netpoll_cleanup(&nt->np);
 #ifdef	CONFIG_NETCONSOLE_DYNAMIC
 	kfree(nt->userdata);
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 84cbfa85028a..a4d176ff9376 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -382,9 +382,6 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 	const struct net_device_ops *ops;
 	int err;
 
-	skb_queue_head_init(&np->skb_pool);
-	INIT_WORK(&np->refill_wq, refill_skbs_work_handler);
-
 	if (ndev->priv_flags & IFF_DISABLE_NETPOLL) {
 		np_err(np, "%s doesn't support polling, aborting\n",
 		       ndev->name);
@@ -419,9 +416,6 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev)
 	np->dev = ndev;
 	strscpy(np->dev_name, ndev->name, IFNAMSIZ);
 
-	/* fill up the skb queue */
-	refill_skbs(np);
-
 	/* last thing to do is link it to the net device structure */
 	rcu_assign_pointer(ndev->npinfo, npinfo);
 
@@ -611,7 +605,7 @@ int netpoll_setup(struct netpoll *np)
 
 	err = __netpoll_setup(np, ndev);
 	if (err)
-		goto flush;
+		goto put;
 	rtnl_unlock();
 
 	/* Make sure all NAPI polls which started before dev->npinfo
@@ -622,8 +616,6 @@ int netpoll_setup(struct netpoll *np)
 
 	return 0;
 
-flush:
-	skb_pool_flush(np);
 put:
 	DEBUG_NET_WARN_ON_ONCE(np->dev);
 	if (ip_overwritten)
@@ -674,8 +666,6 @@ static void __netpoll_cleanup(struct netpoll *np)
 		RCU_INIT_POINTER(np->dev->npinfo, NULL);
 		call_rcu(&npinfo->rcu, rcu_cleanup_netpoll_info);
 	}
-
-	skb_pool_flush(np);
 }
 
 void __netpoll_free(struct netpoll *np)

-- 
2.54.0