Re: [PATCH v4 1/9] eal: annotate spinlock, rwlock and seqlock

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 1/19/23 19:46, David Marchand wrote:
> clang offers some thread safety checks, statically verifying that locks
> are taken and released in the code.
> To use those checks, the full code leading to taking or releasing locks
> must be annotated with some attributes.
> 
> Wrap those attributes into our own set of macros.
> 
> rwlock, seqlock and the "normal" spinlock are instrumented.
> 
> Those checks might be of interest out of DPDK, but it requires that the
> including application locks are annotated.
> On the other hand, applications out there might have been using
> those same checks.
> To be on the safe side, keep this instrumentation under a
> RTE_ANNOTATE_LOCKS internal build flag.
> 
> A component may en/disable this check by setting
> annotate_locks = true/false in its meson.build.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since RFC v3:
> - rebased,
> - added some documentation,
> - added assert attribute,
> - instrumented seqlock,
> - cleaned annotations in arch-specific headers,
> 
> Changes since RFC v2:
> - fixed rwlock trylock,
> - instrumented _tm spinlocks,
> - aligned attribute names to clang,
> 
> ---
>   .../prog_guide/env_abstraction_layer.rst      | 24 ++++++
>   doc/guides/rel_notes/release_23_03.rst        |  5 ++
>   drivers/meson.build                           |  5 ++
>   lib/eal/include/generic/rte_rwlock.h          | 27 +++++--
>   lib/eal/include/generic/rte_spinlock.h        | 31 +++++---
>   lib/eal/include/meson.build                   |  1 +
>   lib/eal/include/rte_lock_annotations.h        | 73 +++++++++++++++++++
>   lib/eal/include/rte_seqlock.h                 |  6 ++
>   lib/eal/ppc/include/rte_spinlock.h            |  3 +
>   lib/eal/x86/include/rte_rwlock.h              |  4 +
>   lib/eal/x86/include/rte_spinlock.h            |  9 +++
>   lib/meson.build                               |  5 ++
>   12 files changed, 179 insertions(+), 14 deletions(-)
>   create mode 100644 lib/eal/include/rte_lock_annotations.h
> 
> diff --git a/doc/guides/prog_guide/env_abstraction_layer.rst b/doc/guides/prog_guide/env_abstraction_layer.rst
> index 35fbebe1be..6c1e8ba985 100644
> --- a/doc/guides/prog_guide/env_abstraction_layer.rst
> +++ b/doc/guides/prog_guide/env_abstraction_layer.rst
> @@ -529,6 +529,30 @@ Misc Functions
>   
>   Locks and atomic operations are per-architecture (i686 and x86_64).
>   
> +Lock annotations
> +~~~~~~~~~~~~~~~~
> +
> +R/W locks, seq locks and spinlocks have been instrumented to help developers in
> +catching issues in DPDK.
> +
> +This instrumentation relies on
> +`clang Thread Safety checks <https://clang.llvm.org/docs/ThreadSafetyAnalysis.html>`_.
> +All attributes are prefixed with __rte and are fully described in the clang
> +documentation.
> +
> +Some general comments:
> +
> +- it is important that lock requirements are expressed at the function
> +  declaration level in headers so that other code units can be inspected,
> +- when some global lock is necessary to some user-exposed API, it is preferred
> +  to expose it via an internal helper rather than expose the global variable,
> +- there are a list of known limitations with clang instrumentation, but before
> +  waiving checks with ``__rte_no_thread_safety_analysis`` in your code, please
> +  discuss it on the mailing list,
> +
> +A DPDK library/driver can enabled/disable the checks by setting

s/enabled/enable/

> +``annotate_locks`` accordingly in its ``meson.build`` file.
> +
>   IOVA Mode Detection
>   ~~~~~~~~~~~~~~~~~~~
>   
> diff --git a/doc/guides/rel_notes/release_23_03.rst b/doc/guides/rel_notes/release_23_03.rst
> index c15f6fbb9f..5425e59c65 100644
> --- a/doc/guides/rel_notes/release_23_03.rst
> +++ b/doc/guides/rel_notes/release_23_03.rst
> @@ -55,6 +55,11 @@ New Features
>        Also, make sure to start the actual text at the margin.
>        =======================================================
>   
> +* **Introduced lock annotations.**
> +
> +  Added lock annotations attributes to that clang can statically analyze lock

s/to/so/

> +  correctness.
> +

With above typos fixed:

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime