|SUCCESS| [v2] doc, eal, devtools: discourage new __rte_always_inline

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-01

commit e358be7fcfXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
Author: Stephen Hemminger <stephen at networkplumber.org>

doc, eal, devtools: discourage new __rte_always_inline

This patch updates documentation and tooling to discourage the use of
__rte_always_inline, preferring to let the compiler make inlining
decisions. It adds a checkpatches.sh warning for new uses and expands
comments in rte_common.h regarding inline annotations.

> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> index f5dd77443f..2a3d364178 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -137,6 +137,14 @@ check_forbidden_additions() { # <patch>
>  		-f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
>  		"$1" || res=1
>  
> +	# forbid new use of __rte_always_inline
> +	awk -v FOLDERS="lib drivers app examples" \
> +		-v EXPRESSIONS='\\<__rte_always_inline\\>' \
> +		-v RET_ON_FAIL=1 \
> +		-v MESSAGE='Adding __rte_always_inline; prefer plain inline' \
> +		-f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> +		"$1" || res=1
> +

The MESSAGE string here ends with "inline" but doesn't include
punctuation. Is this consistent with other MESSAGE strings in this
file? Should it end with a period for consistency?

[ ... ]

> diff --git a/doc/guides/contributing/coding_style.rst b/doc/guides/contributing/coding_style.rst
> index 243a3c2959..8f9e77d1c1 100644
> --- a/doc/guides/contributing/coding_style.rst
> +++ b/doc/guides/contributing/coding_style.rst

[ ... ]

> +Use of ``__rte_always_inline``
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The ``__rte_always_inline`` attribute forces the compiler to inline a function regardless of its size or call-graph heuristics.
> +Prefer plain ``inline`` (or no annotation at all for static functions) and let the compiler decide.
> +Modern compilers at ``-O2`` make good inlining decisions for small ``static inline`` functions in headers,
> +and forced inlining can hurt performance by inflating function bodies, increasing register pressure, and overriding profile-guided optimization.
> +
> +``__rte_always_inline`` should only be used when one of the following applies:
> +
> +* The function contains ``__rte_constant()`` checks that gate a constant-folded fast path,
> +  and the optimization is lost if the function is not inlined into the caller.
> +  Examples include byte-order helpers and length-dispatched copy/compare routines.
> +
> +* The function wraps inline assembly or a compiler intrinsic whose correctness depends on being inlined into the caller's register context (for example, intrinsics requiring a compile-time constant argument).
> +
> +* Measurement on a representative workload shows that the annotation is required to retain performance, and the reason is documented in the commit message that introduces it.

Does the second bullet point in this list exceed 79 characters? The
line starting "* The function wraps inline assembly or a compiler
intrinsic whose correctness" appears to be over the limit.

> +
> +Each use must be justified at the point it is introduced. Adding ``__rte_always_inline`` because nearby code uses it is not a justification;
> +if the constant or intrinsic that requires inlining is several call levels up the call chain,
> +restructure the code rather than annotating the entire chain.

Same question about line length here. Does "Adding
``__rte_always_inline`` because nearby code uses it is not a
justification;" exceed 79 characters?

[ ... ]

> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index 71415346cf..e358be7fcf 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h

[ ... ]

> @@ -482,7 +482,22 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>  #endif
>  
>  /**
> - * Force a function to be inlined
> + * Force a function to be inlined, regardless of the compiler's size and
> + * call-graph heuristics.
> + *
> + * Prefer plain @c inline (or no annotation on a static function) and let the compiler decide.
> + * Modern compilers at -O2 inline small static functions well,
> + * and forcing it can hurt by inflating call sites, raising register pressure,
> + * and overriding profile-guided optimization.
> + *
> + * Reserve this attribute for cases where inlining is required for
> + * correctness, or for a documented and measured performance reason, e.g.
> + *  - a constant-folded fast path gated by @c __rte_constant() that is lost
> + *    unless the function is inlined into the caller;
> + *  - a wrapper around inline asm or an intrinsic that needs a
> + *    compile-time-constant argument from the caller's context.
> + *
> + * See the DPDK coding style guide for the full policy.
>   */

Does the comment use the correct doxygen formatting? The '@c'
references like "@c inline" and "@c __rte_constant()" are typically
used in doxygen for code elements, but is this consistent with other
API documentation in rte_common.h?

[ ... ]

> @@ -501,6 +520,12 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>  
>  /**
>   * Hint function in the hot path
> + *
> + * The compiler may optimize the function more aggressively, treat calls
> + * to it as likely for branch prediction, and group it with other hot
> + * functions to improve instruction-cache locality. This affects code
> + * placement and prediction, not inlining; combine with an inline
> + * annotation if both are wanted.
>   */

[ ... ]

> @@ -510,6 +535,14 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
>  
>  /**
>   * Hint function in the cold path
> + *
> + * The compiler optimizes the function for size rather than speed,
> + * marks branches that reach it as unlikely, and may move it to a separate
> + * section to keep it off the hot path and reduce instruction-cache
> + * pressure there.
> + *
> + * Suitable for error handling, logging, and setup/teardown code.
> + * Functions marked @c __rte_noreturn are already treated as cold.
>   */

These new documentation comments are helpful, but do they follow the
existing doxygen style in this file? Should @c references be used
consistently, and is the level of detail appropriate for API
documentation versus internal guidelines?