Re: [PATCH v3] libcxgb: use raw_cpu_ptr() in ppm_get_cpu_entries()

[David Laight <david.laight.linux@gmail.com>]

On Mon, 09 Nov 2026 18:07:48 +0800
Haoran Wang <haoranwangsec@gmail.com> wrote:

> ppm_get_cpu_entries() disables preemption with get_cpu(), reads the
> current CPU's pool with per_cpu_ptr(), then acquires the pool's
> spin_lock_bh() while preemption is still disabled, and finally put_cpu().
> 
> On PREEMPT_RT kernels spin_lock_bh() becomes a sleeping lock
> (rt_spin_lock), so the task may sleep with preempt_count elevated:
> scheduling while atomic, reported as a BUG by CONFIG_DEBUG_ATOMIC_SLEEP
> and a deadlock once the lock is contended.  Any iSCSI offload I/O
> through a Chelsio NIC that reaches ppm_get_cpu_entries() triggers it.
> 
> Read the current CPU with raw_cpu_ptr() without raising preempt_count,
> and derive the global index base from pool->base (initialised when the
> pools are allocated) instead of cpu * pool_index_max, so the base
> always matches the pool that was locked.  The owning CPU remains in the
> debug output, now derived from pool->base.

I think you really need to explain why it doesn't matter that the pool
for the 'wrong' cpu is used.

You don't need a big explanation of why you can't call spin_lock_bh()
with preemption disabled, it is probably enough just to say that you
can't (the most reason would be 'can sleep on PREEMPT_RT kernels).

This is separate from any issues of acquiring the wrong lock.
I'm not sure about the trace, if you are tracing the cpu the data belongs
to - then that should be what you call it.
Once you hold the lock I think the cpu is fixed (even under PREEMPT_RT)
so you could trace the actual cpu as well - especially if anything 'funny'
might happen because the data wor the wrong cpu is being used.

Of course, it the code is just trying to avoid lock contention (and maybe
reduce cache line bouncing) it may not matter at all which copy of the
data you get.
In fact one for each cpu may be excessive - think of systems with 1000s
of cpu not single digits.

David


> 
> Signed-off-by: Haoran Wang <haoranwangsec@gmail.com>
> 
> ---
> (Resent with the complete Cc list; the earlier copy reached only you.)
> 
> Hi Sebastian,
> 
> thanks for the review.  v3 below addresses your comments:
> 
> - The commit message now describes what the patch does; the review
>   rationale moved into the changelog below the --- separator.
> - PREEMPT_RT spelled out.
> - pr_debug() keeps printing the owning CPU of the pool, derived as
>   pool->base / ppm->pool_index_max; the global index stays
>   i + pool->base.
> - Tested on: x86_64, compile-tested with CONFIG_PREEMPT_RT=y and
>   CONFIG_DEBUG_ATOMIC_SLEEP=y; no Chelsio hardware at hand, so the
>   change is not exercised on real offload I/O.
> 
> 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.  The per-CPU pools are shards of the global ppod index
>   space and not tied to the running CPU (ppm_unmark_entries() derives
>   the owning pool from the index itself), so locking another CPU's
>   pool after a migration is a cache-locality matter, not a
>   correctness one.
> 
>  drivers/net/ethernet/chelsio/libcxgb/libcxgb_ppm.c |   12 +++++-------
>  1 file changed, 5 insertions(+), 7 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);
> @@ -145,10 +142,10 @@
>  	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);
> +		 __func__, 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 +372,7 @@
>  
>  		memset(ppool, 0, alloc_sz);
>  		spin_lock_init(&ppool->lock);
> +		ppool->base = count;
>  		count += ppmax;
>  	}
>  
>