[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=both
  

> -----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
  

Hello,

On Thu, Feb 9, 2023 at 8:59 AM Xia, Chenbo <chenbo.xia@intel.com> wrote:
> > 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,
> >

[snip]

> >
> >
> > 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.

We discovered recently that Intel CI filters out doc/ updates in patches (?!).
https://inbox.dpdk.org/dev/20220328121758.26632-1-david.marchand@redhat.com/T/#mb42fa6342204dd01c923339ec0b1587bc0b5ac0a

So yes, it is "related" to the series, but you can ignore Intel CI
report because the reported issue is fixed since the v4 revision.


Btw, thanks for the review Chenbo!
  

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, February 9, 2023 4:08 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; maxime.coquelin@redhat.com; stephen@networkplumber.org;
> Hu, Jiayu <jiayu.hu@intel.com>; Wang, YuanX <yuanx.wang@intel.com>; Ding,
> Xuan <xuan.ding@intel.com>; mb@smartsharesystems.com
> Subject: Re: [PATCH v6 0/9] Lock annotations
> 
> Hello,
> 
> On Thu, Feb 9, 2023 at 8:59 AM Xia, Chenbo <chenbo.xia@intel.com> wrote:
> > > 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,
> > >
> 
> [snip]
> 
> > >
> > >
> > > 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.
> 
> We discovered recently that Intel CI filters out doc/ updates in patches
> (?!).

Oh, I remember you reported some CI issue to us but didn't realize this is
the one. Good to know it's resolved :)

Thanks,
Chenbo

> https://inbox.dpdk.org/dev/20220328121758.26632-1-
> david.marchand@redhat.com/T/#mb42fa6342204dd01c923339ec0b1587bc0b5ac0a
> 
> So yes, it is "related" to the series, but you can ignore Intel CI
> report because the reported issue is fixed since the v4 revision.
> 
> 
> Btw, thanks for the review Chenbo!
> 
> 
> --
> David Marchand
  

On Tue, Feb 7, 2023 at 11:45 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> 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=both
>

We have been discussing the annotations naming convention in another
thread (I copied involved people).
I saw no objection to the approach of simply prefixing with __rte the
clang attributes.
I'll stick to this de facto convention we have in dpdk.

If any objection arises, I will do the necessary adjustments before -rc2.

Series applied, thanks to all reviewers!