From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7DAD441B90; Tue, 31 Jan 2023 17:19:07 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6BDEB4067B; Tue, 31 Jan 2023 17:19:07 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 64B9740041 for ; Tue, 31 Jan 2023 17:19:05 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1675181944; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=tLvnCtKb81+pc2/O7xNPL+U9jhPCCCMxXy8X8ZubviU=; b=Cqz3tebAGKzaqlRf9yq4j99dFDH41AfkIrT5g5XgVHJPIfD2m1qWXdhs2OMz5P9nxX6hNS w+f7Cur7ReQGftSX8NYp965uFniS1M2kDCALpD6Y8HBoUhVFKmZ2o1IG1zbhwaqUVbodLK TvsN2NYUxHo9hc/2zuBCNXOW6/LoUz4= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-478-veFQOxaUMCelV3RGvIjjQA-1; Tue, 31 Jan 2023 11:19:03 -0500 X-MC-Unique: veFQOxaUMCelV3RGvIjjQA-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id AC16C2823812; Tue, 31 Jan 2023 16:18:35 +0000 (UTC) Received: from [10.39.208.22] (unknown [10.39.208.22]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 71FF4492B05; Tue, 31 Jan 2023 16:18:33 +0000 (UTC) Message-ID: Date: Tue, 31 Jan 2023 17:18:31 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 To: David Marchand , dev@dpdk.org Cc: stephen@networkplumber.org, chenbo.xia@intel.com, jiayu.hu@intel.com, yuanx.wang@intel.com, xuan.ding@intel.com, Anatoly Burakov , =?UTF-8?Q?Mattias_R=c3=b6nnblom?= , David Christensen , Bruce Richardson , Konstantin Ananyev References: <20220328121758.26632-1-david.marchand@redhat.com> <20230119184620.3195267-1-david.marchand@redhat.com> <20230119184620.3195267-2-david.marchand@redhat.com> From: Maxime Coquelin Subject: Re: [PATCH v4 1/9] eal: annotate spinlock, rwlock and seqlock In-Reply-To: <20230119184620.3195267-2-david.marchand@redhat.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > --- > 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 `_. > +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 Thanks, Maxime