[PATCH v4] libcxgb: use raw_cpu_ptr() in ppm_get_cpu_entries()

[Haoran Wang <haoranwangsec@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4308 bytes --]

ppm_get_cpu_entries() must not acquire a sleeping lock while preemption
is disabled: get_cpu() raises preempt_count and spin_lock_bh() may sleep
on PREEMPT_RT kernels.  Read the current CPU with raw_cpu_ptr() instead,
which keeps the per-CPU accessor stable without disabling preemption.

Using the pool of a CPU the thread has since migrated away from is
harmless: the per-CPU pools are shards of the global ppod index space,
and the free path ppm_unmark_entries() derives the owning pool from the
index instead of from the running CPU, so any thread can service any
shard.  Derive the global index base from pool->base so it always
matches the pool that was locked.

Signed-off-by: Haoran Wang <haoranwangsec@gmail.com>

---
Hi David,

thanks for the comments.  v4 below:

- The commit message now explains why using the pool of another CPU is
  harmless, and the PREEMPT_RT problem statement is trimmed to two
  sentences.
- pr_debug() now labels the pool's owner as "pool cpu" and also prints
  the running CPU, so a thread servicing a foreign shard is visible in
  the log.
- I also stress-tested the patched allocator itself: the unmodified
  libcxgb_ppm.c with this patch applied, compiled into an out-of-tree
  module and driven through the real cxgbi_ppm_ppods_reserve() /
  cxgbi_ppm_ppod_release() API by 8 unbound kernel threads on a 2-CPU
  QEMU x86_64 guest, with about half of the releases executed by
  another thread (and so, usually, another CPU).  Across 7 runs
  totalling 1.12M allocations there was no double-issued index, no bad
  free, and every pool drained back to empty each run; 4 allocations
  were naturally served by another CPU's pool because the caller
  migrated inside the allocator, and the accounting stayed exact.  The
  test harness is AI-generated; module and logs are attached.
- On the sharding granularity: agreed, the per-pool split only spreads
  lock contention and cache-line bouncing; on systems with thousands of
  CPUs a coarser sharding would serve the same purpose.  That is a
  separate optimisation and does not affect this fix, so I left it
  untouched here.

Changes since v3:
- Explain in the commit message why a foreign CPU's pool is harmless;
  trim the PREEMPT_RT problem description (review).
- pr_debug(): print the running CPU and the pool-owning CPU separately
  and label them accordingly (review).
- pr_debug(): the owning CPU keeps being derived from pool->base, so it
  is always consistent with the locked pool.
- Attach stress-test evidence for the foreign-pool argument.

Changes since v2:
- Keep the owning CPU in the pr_debug() output, derived from
  pool->base / ppm->pool_index_max (suggested by Sebastian).
- Reword the commit message to describe what the patch does and move
  the review discussion into this changelog; spell PREEMPT_RT.

Changes since v1:
- Drop the cpu variable: read the global-index base from pool->base
  (initialised at pool allocation) instead of a cpu captured before
  the lock, so the base is always consistent with the pool that was
  locked.

 drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

--- a/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c
+++ b/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c
@@ -119,16 +119,13 @@
 			       unsigned long caller_data)
 {
 	struct cxgbi_ppm_pool *pool;
-	unsigned int cpu;
 	int i;
 
 	if (!ppm->pool)
 		return -EINVAL;
 
-	cpu = get_cpu();
-	pool = per_cpu_ptr(ppm->pool, cpu);
+	pool = raw_cpu_ptr(ppm->pool);
 	spin_lock_bh(&pool->lock);
-	put_cpu();
 
 	i = ppm_find_unused_entries(pool->bmap, ppm->pool_index_max,
 				    pool->next, count, 0);
@@ -144,11 +141,12 @@
 
 	spin_unlock_bh(&pool->lock);
 
-	pr_debug("%s: cpu %u, idx %d + %d (%d), next %u.\n",
-		 __func__, cpu, i, count, i + cpu * ppm->pool_index_max,
-		pool->next);
+	pr_debug("%s: cpu %u, pool cpu %u, idx %d + %d (%d), next %u.\n",
+		 __func__, raw_smp_processor_id(),
+		 pool->base / ppm->pool_index_max, i, count, i + pool->base,
+		 pool->next);
 
-	i += cpu * ppm->pool_index_max;
+	i += pool->base;
 	ppm_mark_entries(ppm, i, count, caller_data);
 
 	return i;
@@ -375,6 +373,7 @@
 
 		memset(ppool, 0, alloc_sz);
 		spin_lock_init(&ppool->lock);
+		ppool->base = count;
 		count += ppmax;
 	}
 

[-- Attachment #2: ppm_v4_stresstest.txt --]
[-- Type: text/plain, Size: 14776 bytes --]

libcxgb ppm v4 -- stress test for the "wrong CPU's pool" question
=================================================================

DISCLOSURE
----------
The test harness below (pooltest_real.c, Makefile and the run recipe)
is AI-generated code (Claude).  The code under test is the unmodified
drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c (Linux 7.3), with
the proposed v4 patch applied verbatim via `patch -p1`, compiled into
the test module.

WHAT IT TESTS
-------------
The question from review: why is it harmless when ppm_get_cpu_entries()
ends up using the pool of a CPU the caller has migrated away from?

The module includes the patched libcxgb_ppm.c and exercises the real
API only:

  - cxgbi_ppm_init()          real ppm setup (ppmax 16384, reserve
                              factor 2: 4096 ppods per CPU pool plus
                              8192 in the general pool),
  - cxgbi_ppm_ppods_reserve() allocates via ppm_get_cpu_entries() --
                              the function this patch touches -- with
                              the general pool as fallback, exactly as
                              the drivers use it,
  - cxgbi_ppm_ppod_release()  frees via ppm_unmark_entries(), which
                              derives the owning pool from the index.

8 unbound kernel threads over-subscribe 2 CPUs so the scheduler
migrates them, and about half of the ppods are handed to another
thread (and so, usually, another CPU) for release.  A global bitmap
oracle flags any index that is issued twice or freed twice / never
issued; at the end every pool bitmap must be empty and issued must
equal freed.  Each allocation also records whether the pool that
served it belongs to the CPU the caller is on when the call returns
("foreign pool observation").

RESULTS (Linux 7.3, x86_64, QEMU q35, 2 CPUs, pristine kernel; the
module is the only out-of-tree artifact)
-----------------------------------------------------------------
Last boot, 3 rounds of 8 threads x 20000 reservations each:

libcxgb: ippm lo: ppm 0x000000001a2cc2f8, 0x000000000787b5ae, base 0/16384, pg 4096,0, rsvd 8192,4096.
pooltest_real: real ppm: ppmax 16384, pool_rsvd 8192, pool_index_max 4096, bmap_index_max 8192, possible cpus 2
pooltest_real: cpu0 pool base 0 (set by v4 init hunk)
pooltest_real: cpu1 pool base 4096 (set by v4 init hunk)
pooltest_real: RESULT: allocs=160000 (own-free=79897 handoff-free=79894) migrations=0 foreign_pool_obs=0 enospc=0
pooltest_real: ORACLE: double_alloc=0 bad_free=0 issued=399501 freed=399501 left_issued=0 pool_leak=0 general_leak=0
pooltest_real: REAL CODE (v4-patched) HOLDS ALL INVARIANTS
libcxgb: ippm lo: ppm 0x000000001a2cc2f8, 0x000000000787b5ae, base 0/16384, pg 4096,0, rsvd 8192,4096.
pooltest_real: real ppm: ppmax 16384, pool_rsvd 8192, pool_index_max 4096, bmap_index_max 8192, possible cpus 2
pooltest_real: cpu0 pool base 0 (set by v4 init hunk)
pooltest_real: cpu1 pool base 4096 (set by v4 init hunk)
pooltest_real: RESULT: allocs=160000 (own-free=80407 handoff-free=79488) migrations=1 foreign_pool_obs=0 enospc=0
pooltest_real: ORACLE: double_alloc=0 bad_free=0 issued=399483 freed=399483 left_issued=0 pool_leak=0 general_leak=0
pooltest_real: REAL CODE (v4-patched) HOLDS ALL INVARIANTS
libcxgb: ippm lo: ppm 0x000000001a2cc2f8, 0x000000000787b5ae, base 0/16384, pg 4096,0, rsvd 8192,4096.
pooltest_real: real ppm: ppmax 16384, pool_rsvd 8192, pool_index_max 4096, bmap_index_max 8192, possible cpus 2
pooltest_real: cpu0 pool base 0 (set by v4 init hunk)
pooltest_real: cpu1 pool base 4096 (set by v4 init hunk)
pooltest_real: RESULT: allocs=160000 (own-free=80175 handoff-free=79621) migrations=0 foreign_pool_obs=0 enospc=0
pooltest_real: ORACLE: double_alloc=0 bad_free=0 issued=400107 freed=400107 left_issued=0 pool_leak=0 general_leak=0
pooltest_real: REAL CODE (v4-patched) HOLDS ALL INVARIANTS

Cumulative over 7 runs (rounds) of the same load:

  allocations                    1.12M  (7 x 160k)
  thread migrations observed        14
  allocations served by a foreign
  CPU's pool (naturally occurring)   4
  double-issued index                0
  bad free                           0
  leaked / lost entries              0   (all pools drained to empty
                                          every run, issued == freed)

So the "wrong CPU's pool" case does occur on the real, patched code,
and the accounting stays exact.  This matches the argument in the
commit message: the pool that is locked always owns the index range it
hands out, and the free path derives the pool from the index itself,
never from the running CPU.

HOW TO REPRODUCE
----------------
    # 1. patched copy of the real source (single source of truth is
    #    the v4 patch itself)
    mkdir -p r/drivers/net/ethernet/chelsio/libcxgb
    cp <kernel>/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.{c,h} \
       r/drivers/net/ethernet/chelsio/libcxgb/
    ( cd r && patch -p1 < libcxgb_v4.patch )
    sed '/^MODULE_AUTHOR/,$d' \
        r/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c \
        > libcxgb_ppm_patched.c
    cp r/drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.h .

    # 2. build as an out-of-tree module (kernel stays pristine)
    make -C <kernel-src> O=<build-dir> M=$PWD KBUILD_MODPOST_WARN=1 modules

    # 3. run on any x86_64 guest with >= 2 CPUs
    insmod pooltest_real.ko threads=8 ops=20000
    rmmod pooltest_real.ko        # results in dmesg

======================================================================
pooltest_real.c
======================================================================
// pooltest_real.c — stress the REAL libcxgb_ppm.c code, v4 patch applied,
// compiled into this out-of-tree module (kernel stays pristine).
//
// Everything below the API line is the untouched driver code path:
//   cxgbi_ppm_init()          — real init: creates the per-cpu pools
//                               (ppm_alloc_cpu_pool, incl. v4's base init)
//   cxgbi_ppm_ppods_reserve() — real alloc: per-cpu pool first — the very
//                               function the patch touches — then the
//                               general pool as fallback
//   cxgbi_ppm_ppod_release()  — real free: ppm_unmark_entries() derives the
//                               owning pool from the index, never the CPU
//
// 8 UNBOUND kernel threads over-subscribe 2 CPUs, so the scheduler migrates
// them constantly, and tickets are handed between threads, so frees land on
// foreign CPUs at a high rate.  Oracles:
//   - global issued bitmap: an index handed out twice (double_alloc) or
//     freed twice / never allocated (bad_free) is caught regardless of
//     which pool/CPU was involved;
//   - every per-cpu pool and the general pool must drain to empty;
//   - issued == freed;
//   - each per-cpu-pool allocation is checked: owning pool vs running CPU
//     at return (foreign-pool observation).
#include "libcxgb_ppm_patched.c"

#undef pr_fmt
#define pr_fmt(fmt) "pooltest_real: " fmt

#include <linux/moduleparam.h>
#include <linux/kthread.h>
#include <linux/completion.h>
#include <linux/random.h>
#include <linux/sched.h>
#include <linux/smp.h>

static struct cxgbi_ppm *g_ppm;
static void *g_ppm_pp;
static struct net_device *g_ndev;
static unsigned long *issued;

static atomic_t n_issued   = ATOMIC_INIT(0);
static atomic_t n_freed    = ATOMIC_INIT(0);
static atomic_t n_double   = ATOMIC_INIT(0);
static atomic_t n_badclear = ATOMIC_INIT(0);
static atomic_t n_enospc   = ATOMIC_INIT(0);
static atomic_t n_foreign  = ATOMIC_INIT(0);

static int ops = 20000;
static int threads = 8;
module_param(ops, int, 0444);
module_param(threads, int, 0444);

struct tick {
	u32 idx;
	unsigned int npods;
};

#define RHOLD 16

/* FIFO ring: tickets handed to OTHER threads, freed on whatever CPU */
struct hq {
	spinlock_t lock;
	int head, tail, cap;
	struct tick *ring;
};
static struct hq handoff;

static int handoff_push(u32 idx, unsigned int npods)
{
	unsigned long flags;
	int r = -1;

	spin_lock_irqsave(&handoff.lock, flags);
	if (handoff.tail - handoff.head < handoff.cap) {
		handoff.ring[handoff.tail % handoff.cap].idx = idx;
		handoff.ring[handoff.tail % handoff.cap].npods = npods;
		handoff.tail++;
		r = 0;
	}
	spin_unlock_irqrestore(&handoff.lock, flags);
	return r;
}

static int handoff_pop(struct tick *t)
{
	unsigned long flags;
	int r = -1;

	spin_lock_irqsave(&handoff.lock, flags);
	if (handoff.tail > handoff.head) {
		*t = handoff.ring[handoff.head % handoff.cap];
		handoff.head++;
		r = 0;
	}
	spin_unlock_irqrestore(&handoff.lock, flags);
	return r;
}

/* free through the REAL release path + oracle bookkeeping.
 * Oracle bits must be cleared BEFORE the real release: after the real
 * free another thread may legitimately re-allocate the same range, and
 * its test_and_set would collide with our not-yet-cleared bit (false
 * double).  While our bit is cleared but the real allocator still owns
 * the range, nobody can obtain it, so there is no window in this order.
 */
static void release_ticket(struct tick *t)
{
	unsigned int k;

	for (k = 0; k < t->npods; k++) {
		if (!test_and_clear_bit(t->idx + k, issued))
			atomic_inc(&n_badclear);
		else
			atomic_inc(&n_freed);
	}
	cxgbi_ppm_ppod_release(g_ppm, t->idx);
}

struct rarg {
	struct completion done;
	struct tick hold[RHOLD];
	long n_alloc, n_hofree, n_minefree, migrations, foreign;
};

static int rworker(void *data)
{
	struct rarg *a = data;
	int i, last = raw_smp_processor_id();

	for (i = 0; i < ops; i++) {
		int slot = get_random_u32() % RHOLD;
		int c = raw_smp_processor_id();
		u32 idx, tag;
		int rc;

		if (c != last) {
			a->migrations++;
			last = c;
		}
		if ((i & 0xf) == 0)
			cond_resched();	/* let the scheduler move us around */

		/* make room: give the ticket away, or free it right here */
		if (a->hold[slot].npods) {
			if ((get_random_u32() & 1) &&
			    !handoff_push(a->hold[slot].idx, a->hold[slot].npods)) {
				a->hold[slot].npods = 0;
			} else {
				release_ticket(&a->hold[slot]);
				a->hold[slot].npods = 0;
				a->n_minefree++;
			}
		}
		/* free a ticket produced on some other CPU/thread */
		if (get_random_u32() & 1) {
			struct tick t;

			if (!handoff_pop(&t)) {
				release_ticket(&t);
				a->n_hofree++;
			}
		}

		/* the real alloc path */
		rc = cxgbi_ppm_ppods_reserve(g_ppm, 1 + get_random_u32() % 16,
					     0, &idx, &tag, 0);
		if (rc > 0) {
			unsigned int k;

			for (k = 0; k < rc; k++) {
				if (test_and_set_bit(idx + k, issued))
					atomic_inc(&n_double);
				atomic_inc(&n_issued);
			}
			/* foreign-pool observation: pool that served us vs
			 * CPU we are on when the call returns */
			if (idx < g_ppm->pool_rsvd &&
			    idx / g_ppm->pool_index_max !=
			    raw_smp_processor_id()) {
				atomic_inc(&n_foreign);
				a->foreign++;
			}
			a->hold[slot].idx = idx;
			a->hold[slot].npods = rc;
			a->n_alloc++;
		} else {
			int s2 = get_random_u32() % RHOLD;

			atomic_inc(&n_enospc);
			if (a->hold[s2].npods) {
				release_ticket(&a->hold[s2]);
				a->hold[s2].npods = 0;
			}
		}
	}
	for (i = 0; i < RHOLD; i++) {
		if (a->hold[i].npods) {
			release_ticket(&a->hold[i]);
			a->hold[i].npods = 0;
			a->n_minefree++;
		}
	}
	complete(&a->done);
	return 0;
}

static int __init pooltest_real_init(void)
{
	struct cxgbi_tag_format tformat = {0};
	struct task_struct **tsk;
	struct rarg *wa;
	long n_alloc = 0, n_hofree = 0, n_minefree = 0, migr = 0, foreign = 0;
	int cpu, i, rc, leak_pool = 0, leak_gen, issued_left;
	struct tick t;

	g_ndev = dev_get_by_name(&init_net, "lo");
	if (!g_ndev) {
		pr_err("no loopback netdev\n");
		return -ENODEV;
	}

	tformat.pgsz_order[0] = 0;
	tformat.pgsz_order[1] = 1;
	tformat.pgsz_order[2] = 2;
	tformat.pgsz_order[3] = 3;

	/* ppmax 16384, reserve_factor 2: half in per-cpu pools, half general */
	rc = cxgbi_ppm_init(&g_ppm_pp, g_ndev, NULL, NULL, &tformat,
			    16384 << PPOD_SIZE_SHIFT, 0, 0, 2, 0, 0);
	if (rc < 0) {
		pr_err("cxgbi_ppm_init failed: %d\n", rc);
		dev_put(g_ndev);
		return rc;
	}
	g_ppm = g_ppm_pp;

	pr_info("real ppm: ppmax %u, pool_rsvd %u, pool_index_max %u, bmap_index_max %u, possible cpus %d\n",
		g_ppm->ppmax, g_ppm->pool_rsvd, g_ppm->pool_index_max,
		g_ppm->bmap_index_max, num_possible_cpus());
	for_each_possible_cpu(cpu) {
		struct cxgbi_ppm_pool *pp = per_cpu_ptr(g_ppm->pool, cpu);

		pr_info("cpu%d pool base %u (set by v4 init hunk)\n",
			cpu, pp->base);
	}

	issued = bitmap_zalloc(g_ppm->ppmax, GFP_KERNEL);
	handoff.cap = 8192;
	handoff.ring = kcalloc(handoff.cap, sizeof(*handoff.ring), GFP_KERNEL);
	spin_lock_init(&handoff.lock);

	wa = kcalloc(threads, sizeof(*wa), GFP_KERNEL);
	tsk = kcalloc(threads, sizeof(*tsk), GFP_KERNEL);
	for (i = 0; i < threads; i++) {
		init_completion(&wa[i].done);
		tsk[i] = kthread_run(rworker, &wa[i], "ppmr%d", i);
		if (IS_ERR(tsk[i]))
			complete(&wa[i].done);
	}
	for (i = 0; i < threads; i++) {
		wait_for_completion(&wa[i].done);
		n_alloc += wa[i].n_alloc;
		n_hofree += wa[i].n_hofree;
		n_minefree += wa[i].n_minefree;
		migr += wa[i].migrations;
		foreign += wa[i].foreign;
	}
	while (!handoff_pop(&t))
		release_ticket(&t);

	for_each_possible_cpu(cpu)
		leak_pool += bitmap_weight(
			per_cpu_ptr(g_ppm->pool, cpu)->bmap,
			g_ppm->pool_index_max);
	leak_gen = bitmap_weight(g_ppm->ppod_bmap, g_ppm->bmap_index_max);
	issued_left = bitmap_weight(issued, g_ppm->ppmax);

	pr_info("RESULT: allocs=%ld (own-free=%ld handoff-free=%ld) migrations=%ld foreign_pool_obs=%d enospc=%d\n",
		n_alloc, n_minefree, n_hofree, migr,
		atomic_read(&n_foreign), atomic_read(&n_enospc));
	pr_info("ORACLE: double_alloc=%d bad_free=%d issued=%d freed=%d left_issued=%d pool_leak=%d general_leak=%d\n",
		atomic_read(&n_double), atomic_read(&n_badclear),
		atomic_read(&n_issued), atomic_read(&n_freed), issued_left,
		leak_pool, leak_gen);
	if (!atomic_read(&n_double) && !atomic_read(&n_badclear) &&
	    !issued_left && !leak_pool && !leak_gen &&
	    atomic_read(&n_issued) == atomic_read(&n_freed))
		pr_info("REAL CODE (v4-patched) HOLDS ALL INVARIANTS\n");
	else
		pr_info("INVARIANT VIOLATION in real code run\n");

	bitmap_free(issued);
	kfree(handoff.ring);
	kfree(wa);
	kfree(tsk);
	cxgbi_ppm_release(g_ppm);
	dev_put(g_ndev);
	return 0;
}

static void __exit pooltest_real_exit(void)
{
}

module_init(pooltest_real_init);
module_exit(pooltest_real_exit);
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("stress the real (v4-patched) libcxgb_ppm.c alloc paths");

======================================================================
Makefile
======================================================================
obj-m := pooltest.o pooltest_real.o

KDIR  := /root/linux-7.3
BUILD := /root/lkbuild73

all:
	make -C $(KDIR) O=$(BUILD) M=$(CURDIR) KBUILD_MODPOST_WARN=1 modules