RE: [PATCH v6 0/9] Lock annotations

["Xia, Chenbo" <chenbo.xia@intel.com>]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, February 7, 2023 6:45 PM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; stephen@networkplumber.org; Xia, Chenbo
> <chenbo.xia@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>; Wang, YuanX
> <yuanx.wang@intel.com>; Ding, Xuan <xuan.ding@intel.com>;
> mb@smartsharesystems.com
> Subject: [PATCH v6 0/9] Lock annotations
> 
> vhost internals involves multiple locks to protect data access by
> multiple threads.
> 
> This series uses clang thread safety checks [1] to catch issues during
> compilation: EAL spinlock, seqlock and rwlock are annotated and vhost
> code is instrumented so that clang can statically check correctness.
> 
> Those annotations are quite heavy to maintain because the full path of
> code must be annotated (as can be seen in the vhost datapath code),
> but I think it is worth using.
> 
> This has been tested against the whole tree and some fixes are already
> flying on the mailing list (see [2] for a list).
> 
> If this first series is merged, I will prepare a followup series for EAL
> and other libraries.
> 
> 
> 1: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
> 2:
> https://patchwork.dpdk.org/bundle/dmarchand/lock_fixes/?state=*&archive=bo
> th
> 
> --
> David Marchand
> 
> Changes since v5:
> - rebased after lib/vhost updates (patches 5 and 7),
> 
> Changes since v4:
> - masked annotations from Doxygen as it seems confused with some
>   constructs,
> - fixed typos,
> 
> Changes since RFC v3:
> - sorry Maxime, it has been too long since RFC v3 and the code evolved,
>   so I dropped all your review tags,
> - rebased,
> - added documentation,
> - dropped/fixed annotations in arch-specific and EAL headers,
> - rewrote need reply handling so that we don't have to waive the check
>   on the associated functions,
> - separated IOTLB lock unconditional acquire from the annotation patch,
> - rewrote runtime checks for "unsafe" functions using a panicking assert
>   helper,
> 
> Changes since RFC v2:
> - fixed trylock annotations for rwlock,
> - annotated _tm flavors of spinlock and rwlock,
> - removed Maxime vhost fix from series (since Mimecast does not like
>   me sending Maxime patch...), added a dependency on original fix
>   as a hint for reviewers,
> - renamed attributes,
> 
> Changes since RFC v1:
> - Cc'd people who have pending patches for vhost,
> - moved annotations to EAL and removed wrappers in vhost,
> - as a result of moving to EAL, this series will be tested against
>   the main repo, so patch 1 has been kept as part of the series
>   even if already applied to next-virtio,
> - refined/split patches and annotated all spinlocks in vhost,
> 
> 
> David Marchand (9):
>   eal: annotate spinlock, rwlock and seqlock
>   vhost: simplify need reply handling
>   vhost: terminate when access lock is not taken
>   vhost: annotate virtqueue access lock
>   vhost: annotate async accesses
>   vhost: always take IOTLB lock
>   vhost: annotate IOTLB lock
>   vhost: annotate vDPA device list accesses
>   vhost: enable lock check
> 
>  doc/api/doxy-api.conf.in                      |  11 ++
>  .../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                 |   2 +
>  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 +
>  lib/vhost/iotlb.h                             |   4 +
>  lib/vhost/meson.build                         |   2 +
>  lib/vhost/vdpa.c                              |  20 +--
>  lib/vhost/vhost.c                             |  38 ++---
>  lib/vhost/vhost.h                             |  34 ++++-
>  lib/vhost/vhost_crypto.c                      |   8 ++
>  lib/vhost/vhost_user.c                        | 131 ++++++++----------
>  lib/vhost/virtio_net.c                        | 118 ++++++++++++----
>  21 files changed, 405 insertions(+), 150 deletions(-)
>  create mode 100644 lib/eal/include/rte_lock_annotations.h
> 
> --
> 2.39.1

Seems one compilation error reported? Not sure it's related or not.

Thanks,
Chenbo