Re: [PATCH v14 net-next 5/9] octeontx2-af: npc: cn20k: add subbank search order control

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-05-14 at 11:55:33, Ratheesh Kannoth (rkannoth@marvell.com) wrote:
> CN20K NPC MCAM is split into 32 subbanks that are searched in a
> predefined order during allocation. Lower-numbered subbanks have
> higher priority than higher-numbered ones.
>
> Add a runtime "srch_order" to control the order in which
> subbanks are searched during MCAM allocation.
>

>
>> +int npc_cn20k_search_order_set(struct rvu *rvu,
>> +			       u64 narr[MAX_NUM_SUB_BANKS], int cnt)
>> +{
>> +	struct npc_mcam *mcam = &rvu->hw->mcam;
>> +	struct npc_subbank *sb;
>> +	struct xarray *xa;
>> +	int prio, rc;
>> +	int sb_idx;
>> +
>> +	if (cnt != npc_priv.num_subbanks) {
>> +		dev_err(rvu->dev, "Number of entries(%u) != %u\n",
>> +			cnt, npc_priv.num_subbanks);
>> +		return -EINVAL;
>> +	}
>> +
>> +	mutex_lock(&mcam->lock);
>> +	npc_lock_all_subbank();
>Does exposing npc_lock_all_subbank() to a user-triggerable devlink callback
>cause a lockdep violation?
>npc_lock_all_subbank() acquires mutexes for all subbanks sequentially.
>Since these mutexes share the same lock class, lockdep might detect this
>as a possible recursive lock. With up to 32 subbanks, this also exceeds
>the maximum lockdep nesting limit of 8 subclasses.

The 32 subbank mutexes are physically distinct locks protecting independent
hardware queues, meaning no actual runtime deadlock path exists. To satisfy
lockdep's verification engine and prevent tracking limit exhaustion during
sequential allocation, the driver uses a singular, overarching top-level
domain lock (mcam->lock) to serialize the entire configuration sequence.

Because mcam->lock acts as the primary synchronization barrier for the
devlink context, concurrent cross-subbank inversion paths are structurally
impossible. To permanently silence the tracking constraint warning, i can post a patch
(post merging this series) to either mutex_lock_nested() with an explicit index state
or disables lockdep tracking entirely for this specialized hardware control
loop using the lockdep_set_novalidate_class() macro

>> +	restrict_valid = false;
>> +
>> +	for (prio = 0; prio < cnt; prio++) {
>> +		sb_idx = narr[prio];
>> +		sb = &npc_priv.sb[sb_idx];
>> +		sb->arr_idx = prio;
>> +
>> +		/* erase entry if exist */
>> +		xa_erase(&npc_priv.xa_sb_free, sb->arr_idx);
>> +		xa_erase(&npc_priv.xa_sb_used, sb->arr_idx);
>> +
>> +		xa = &npc_priv.xa_sb_free;
>> +		if (sb->flags & NPC_SUBBANK_FLAG_USED)
>> +			xa = &npc_priv.xa_sb_used;
>> +
>> +		/* if xa_store() fails due to memory pressure, there is
>> +		 * no point in rolling back, as roll back may also fail
>> +		 */
>> +		rc = xa_err(xa_store(xa, sb->arr_idx,
>> +				     xa_mk_value(sb_idx), GFP_KERNEL));
>> +		if (rc) {
>> +			dev_err(rvu->dev,
>> +				"Setting arr_idx=%d for sb=%d failed\n",
>> +				sb->arr_idx, sb_idx);
>> +			goto fail;
>> +		}
>> +	}
>Does this error path leak subbank resources and leave the xarrays in a
>corrupted state?
>If xa_store() fails, the function jumps to the fail label and aborts
>without restoring the original state. Because the entry at the current
>priority index was already erased via xa_erase(), but the new subbank
>failed to store, that subbank is permanently lost.
>Additionally, the subbank that failed to store is left with a modified
>arr_idx pointing to an empty slot, potentially breaking future allocations.
>Could this function preallocate resources (such as using xa_reserve()) or
>build temporary xarrays to ensure the changes can be applied atomically
>without a destructive intermediate state?
>> +    restrict_valid = false;
>> +
>> +    for (prio = 0; prio < cnt; prio++) {
>> +            sb_idx = narr[prio];
>> +            sb = &npc_priv.sb[sb_idx];
>> +            sb->arr_idx = prio;
>> +
>> +            /* erase entry if exist */
>> +            xa_erase(&npc_priv.xa_sb_free, sb->arr_idx);
>> +            xa_erase(&npc_priv.xa_sb_used, sb->arr_idx);
>> +
>> +            xa = &npc_priv.xa_sb_free;
>> +            if (sb->flags & NPC_SUBBANK_FLAG_USED)
>> +                    xa = &npc_priv.xa_sb_used;
>> +
>> +            /* if xa_store() fails due to memory pressure, there is
>> +             * no point in rolling back, as roll back may also fail
>> +             */
>> +            rc = xa_err(xa_store(xa, sb->arr_idx,
>> +                                 xa_mk_value(sb_idx), GFP_KERNEL));
>> +            if (rc) {
>> +                    dev_err(rvu->dev,
>> +                            "Setting arr_idx=%d for sb=%d failed\n",
>> +                            sb->arr_idx, sb_idx);
>> +                    goto fail;
>> +            }
>> +    }
>Does this error path leak subbank resources and leave the xarrays in a
>corrupted state?
>If xa_store() fails, the function jumps to the fail label and aborts
>without restoring the original state. Because the entry at the current
>priority index was already erased via xa_erase(), but the new subbank
>failed to store, that subbank is permanently lost.
>Additionally, the subbank that failed to store is left with a modified
>arr_idx pointing to an empty slot, potentially breaking future allocations.
>Could this function preallocate resources (such as using xa_reserve()) or
>build temporary xarrays to ensure the changes can be applied atomically
>without a destructive intermediate state?

The omission of a transactional rollback path is intentional due to
the systemic implications of a GFP_KERNEL allocation failure inside
the xarray core. If xa_store() returns an ENOMEM error, the underlying
slab allocator is already exhausted, meaning any programmatic attempt
to erase or restore original indices would also fail, compounding the
unpredictable state.

Because this devlink callback operates under the top-level mcam->lock
mutex, no concurrent allocation or translation worker can read the
partially modified xarrays while the driver is in this degraded state.
An xa_store() failure indicates a fatal, system-wide memory leak or
exhaustion event. At this threshold, the driver prints a critical
error payload to dmesg to allow deterministic post-mortem debugging of
the host's global memory subsystem, rather than attempting an unreliable
in-kernel rollback.