|SUCCESS| [v2, 3/3] devtools/cocci: add script to find empty spinloops

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163010

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-14

Based on my review of the provided patch series against DPDK coding standards and contribution guidelines, I have identified several issues:

commit: 20260413164652.33291-2-stephen at networkplumber.org
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: net/cnxk: add pause to spinloops

This patch adds rte_pause() calls to empty spinloop bodies in the cnxk
network driver to reduce CPU consumption on SMT systems.

> diff --git a/drivers/net/cnxk/cn10k_tx.h b/drivers/net/cnxk/cn10k_tx.h
> index 8c912a1f35..7657b06190 100644
> --- a/drivers/net/cnxk/cn10k_tx.h
> +++ b/drivers/net/cnxk/cn10k_tx.h
> @@ -167,7 +167,7 @@ cn10k_nix_vwqe_wait_fc(struct cn10k_eth_txq *txq, uint16_t req)
>  #else
>  	RTE_SET_USED(pkts);
>  	while (rte_atomic_load_explicit(&txq->fc_cache_pkts, rte_memory_order_relaxed) < 0)
> -		;
> +		rte_pause();

Does this spinloop risk becoming an infinite loop if fc_cache_pkts never
becomes non-negative? Should there be a timeout mechanism or at least a
comment explaining the expected duration?

> @@ -402,7 +402,7 @@ cn10k_nix_sec_fc_wait(struct cn10k_eth_txq *txq, uint16_t nb_pkts)
>  #else
>  	/* Wait for primary core to refill FC. */
>  	while (rte_atomic_load_explicit(fc_sw, rte_memory_order_relaxed) < 0)
> -		;
> +		rte_pause();

Similar question: does cn10k_nix_sec_fc_wait() have any guarantee that
the primary core will eventually refill? Could this become a livelock?

[ ... ]

commit: 20260413164652.33291-3-stephen at networkplumber.org
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: event/cnxk: add pause to spinloops

This patch adds rte_pause() calls to empty spinloop bodies in the cnxk
event driver to reduce CPU consumption on SMT systems.

> diff --git a/drivers/event/cnxk/cn10k_worker.c b/drivers/event/cnxk/cn10k_worker.c
> index 80077ec8a1..69ac67115a 100644
> --- a/drivers/event/cnxk/cn10k_worker.c
> +++ b/drivers/event/cnxk/cn10k_worker.c
> @@ -93,7 +93,7 @@ sso_lmt_aw_wait_fc(struct cn10k_sso_hws *ws, int64_t req)
>  
>  retry:
>  	while (rte_atomic_load_explicit(ws->fc_cache_space, rte_memory_order_relaxed) < 0)
> -		;
> +		rte_pause();

Does sso_lmt_aw_wait_fc() have any mechanism to prevent infinite waiting
if fc_cache_space never becomes non-negative?

[ ... ]

> diff --git a/drivers/event/cnxk/cnxk_tim_worker.h b/drivers/event/cnxk/cnxk_tim_worker.h
> index 09f84091ab..887c0800e2 100644
> --- a/drivers/event/cnxk/cnxk_tim_worker.h
> +++ b/drivers/event/cnxk/cnxk_tim_worker.h
> @@ -405,9 +405,9 @@ cnxk_tim_add_entry_mp(struct cnxk_tim_ring *const tim_ring,
>  			     : [crem] "r"(&bkt->w1)
>  			     : "memory");
>  #else
> -		while (rte_atomic_load_explicit((int64_t __rte_atomic *)&bkt->w1,
> +		while (rte_atomic_load_explicit((int64_t __rte_atomic  *)&bkt->w1,

Does this code introduce extra whitespace before the asterisk? There
appear to be two spaces between "__rte_atomic" and "*" when there should
be only one.

> 						rte_memory_order_relaxed) < 0)
> -			;
> +			rte_pause();

[ ... ]

commit: 20260413164652.33291-4-stephen at networkplumber.org
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: devtools/cocci: add script to find empty spinloops

This patch adds a Coccinelle semantic patch to automatically detect and
fix empty spinloops throughout the codebase.

> diff --git a/devtools/cocci/fix_empty_spinloops.cocci b/devtools/cocci/fix_empty_spinloops.cocci
> new file mode 100644
> index 0000000000..ff64b30eac
> --- /dev/null
> +++ b/devtools/cocci/fix_empty_spinloops.cocci
> @@ -0,0 +1,165 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +// Find and fix empty spin loops that should call rte_pause()
> +//
> +// Empty spin loops waste CPU cycles and can cause performance issues.
> +// This script finds various forms of busy-wait loops and adds rte_pause()
> +// to give hints to the CPU and reduce power consumption.
> +
> +// Rule 1: Handle rte_atomic*_read() variants
> + at fix_atomic_read@
> +expression ptr, val;
> +@@
> +
> +(
> +- while (rte_atomic16_read(ptr) == val);
> ++ while (rte_atomic16_read(ptr) == val)
> ++     rte_pause();

Is this script safe to apply automatically? Some spinloops may have
performance-critical timing requirements where adding rte_pause() could
change behavior. Should this be marked as requiring manual review?

Does the script handle cases where the loop body contains only comments?
Those might be intentionally empty for documentation purposes.

[ ... ]

> +// Rule 6: Handle negated conditions
> + at fix_negated@
> +expression ptr, val;
> +@@
> +
> +(
> +- while (!rte_atomic32_read(ptr));
> ++ while (!rte_atomic32_read(ptr))
> ++     rte_pause();
> +|
> +- while (!rte_atomic64_read(ptr));
> ++ while (!rte_atomic64_read(ptr))
> ++     rte_pause();
> +|
> +- while (!rte_atomic_load_explicit(ptr, val));
> ++ while (!rte_atomic_load_explicit(ptr, val))
> ++     rte_pause();
> +)

Does this rule miss the rte_atomic16_read() variant? Rules 1-5 handle
rte_atomic16_read() but this negated rule does not.