[v2,00/20] Enable lock annotations on most libraries and drivers

  This is a followup of the series that introduced lock annotations.
I reworked and made annotations work in what seemed the easier cases.
In most cases, I chose to convert inline wrappers around the EAL lock
API to simple macro: I did not see much value in those wrappers and this
is way simpler than adding __rte_*lock_function tags everywhere.

A list of libraries and drivers still need more work as their code have
non obvious locks handling. For those components, the check is opted
out.
I leave it to their respective maintainers to enable the checks later.

FreeBSD libc pthread API has lock annotations while Linux glibc has
none.
We could simply disable the check on FreeBSD, but having this check,
a few issues got raised in drivers that are built with FreeBSD.
For now, I went with a simple #ifdef FreeBSD for pthread mutex related
annotations in our code.

Maintainers, please review.
  

On Fri, Feb 24, 2023, at 16:11, David Marchand wrote:
> This is a followup of the series that introduced lock annotations.
> I reworked and made annotations work in what seemed the easier cases.
> In most cases, I chose to convert inline wrappers around the EAL lock
> API to simple macro: I did not see much value in those wrappers and this
> is way simpler than adding __rte_*lock_function tags everywhere.
>
> A list of libraries and drivers still need more work as their code have
> non obvious locks handling. For those components, the check is opted
> out.
> I leave it to their respective maintainers to enable the checks later.
>
> FreeBSD libc pthread API has lock annotations while Linux glibc has
> none.
> We could simply disable the check on FreeBSD, but having this check,
> a few issues got raised in drivers that are built with FreeBSD.
> For now, I went with a simple #ifdef FreeBSD for pthread mutex related
> annotations in our code.
>

Hi David,

This is a great change, thanks for doing it.
However I am not sure I understand the logic regarding the '#ifdef FREEBSD'.

These annotations provide static hints to clang's thread safety analysis.
What is the dependency on FreeBSD glibc?
  

Salut Gaëtan,

On Fri, Feb 24, 2023 at 4:59 PM Gaëtan Rivet <grive@u256.net> wrote:
> > FreeBSD libc pthread API has lock annotations while Linux glibc has
> > none.
> > We could simply disable the check on FreeBSD, but having this check,
> > a few issues got raised in drivers that are built with FreeBSD.
> > For now, I went with a simple #ifdef FreeBSD for pthread mutex related
> > annotations in our code.
> >
>
> Hi David,
>
> This is a great change, thanks for doing it.
> However I am not sure I understand the logic regarding the '#ifdef FREEBSD'.
>
> These annotations provide static hints to clang's thread safety analysis.
> What is the dependency on FreeBSD glibc?

FreeBSD libc added clang annotations, while glibc did not.

FreeBSD 13.1 libc:
int             pthread_mutex_lock(pthread_mutex_t * __mutex)
                    __locks_exclusive(*__mutex);

With:

#if __has_extension(c_thread_safety_attributes)
#define __lock_annotate(x)      __attribute__((x))
#else
#define __lock_annotate(x)
#endif

/* Function acquires an exclusive or shared lock. */
#define __locks_exclusive(...) \
        __lock_annotate(exclusive_lock_function(__VA_ARGS__))


glibc:
extern int pthread_mutex_lock (pthread_mutex_t *__mutex)
     __THROWNL __nonnull ((1));



Since glibc does not declare that pthread_mutex_t is lockable, adding
an annotation triggers an error for Linux (taking eal_common_proc.c
patch 14 as an example):

../lib/eal/common/eal_common_proc.c:911:2: error:
'exclusive_locks_required' attribute requires arguments whose type is
annotated with 'capability' attribute; type here is 'pthread_mutex_t'
[-Werror,-Wthread-safety-attributes]
        __rte_exclusive_locks_required(pending_requests.lock)
        ^
../lib/eal/include/rte_lock_annotations.h:23:17: note: expanded from
macro '__rte_exclusive_locks_required'
        __attribute__((exclusive_locks_required(__VA_ARGS__)))
                       ^

We could wrap this annotation in a new construct (which would require
some build time check wrt pthread API state).
Not sure it is worth the effort though, for now.
  

On Sat, Feb 25, 2023, at 11:16, David Marchand wrote:
> Salut Gaëtan,
>
> On Fri, Feb 24, 2023 at 4:59 PM Gaëtan Rivet <grive@u256.net> wrote:
>> > FreeBSD libc pthread API has lock annotations while Linux glibc has
>> > none.
>> > We could simply disable the check on FreeBSD, but having this check,
>> > a few issues got raised in drivers that are built with FreeBSD.
>> > For now, I went with a simple #ifdef FreeBSD for pthread mutex related
>> > annotations in our code.
>> >
>>
>> Hi David,
>>
>> This is a great change, thanks for doing it.
>> However I am not sure I understand the logic regarding the '#ifdef FREEBSD'.
>>
>> These annotations provide static hints to clang's thread safety analysis.
>> What is the dependency on FreeBSD glibc?
>
> FreeBSD libc added clang annotations, while glibc did not.
>
> FreeBSD 13.1 libc:
> int             pthread_mutex_lock(pthread_mutex_t * __mutex)
>                     __locks_exclusive(*__mutex);
>
> With:
>
> #if __has_extension(c_thread_safety_attributes)
> #define __lock_annotate(x)      __attribute__((x))
> #else
> #define __lock_annotate(x)
> #endif
>
> /* Function acquires an exclusive or shared lock. */
> #define __locks_exclusive(...) \
>         __lock_annotate(exclusive_lock_function(__VA_ARGS__))
>
>
> glibc:
> extern int pthread_mutex_lock (pthread_mutex_t *__mutex)
>      __THROWNL __nonnull ((1));
>
>
>
> Since glibc does not declare that pthread_mutex_t is lockable, adding
> an annotation triggers an error for Linux (taking eal_common_proc.c
> patch 14 as an example):
>
> ../lib/eal/common/eal_common_proc.c:911:2: error:
> 'exclusive_locks_required' attribute requires arguments whose type is
> annotated with 'capability' attribute; type here is 'pthread_mutex_t'
> [-Werror,-Wthread-safety-attributes]
>         __rte_exclusive_locks_required(pending_requests.lock)
>         ^
> ../lib/eal/include/rte_lock_annotations.h:23:17: note: expanded from
> macro '__rte_exclusive_locks_required'
>         __attribute__((exclusive_locks_required(__VA_ARGS__)))
>                        ^
>
> We could wrap this annotation in a new construct (which would require
> some build time check wrt pthread API state).
> Not sure it is worth the effort though, for now.
>
>
> -- 
> David Marchand

Ah ok, so if I understand correctly, DPDK would need to declare some
'__rte_lockable rte_mutex' and associated functions for transparent support,
to wrap above the pthread API.

Unless it happens, would it be possible to condition the thread-safety annotations
to FREEBSD as well?

Maybe have two versions of the annotations:

  __rte_exclusive_locks_required() /* Conditioned on clang */
  __pthread_exclusive_locks_required() /* Conditioned on glibc/pthread support */

the first one to use on RTE types that can be declared with __rte_lockable,
the second for pthread objects that we don't want to replace.
I know the namespace is not great so maybe named another way.

The '#ifdef FREEBSD' ossifies the annotation support at the function level,
when this is a matter of types. This impedance mismatch would mean large
changes if someone was to make this support evolve at some point.
  

On Mon, Feb 27, 2023 at 5:13 PM Gaëtan Rivet <grive@u256.net> wrote:
> Ah ok, so if I understand correctly, DPDK would need to declare some
> '__rte_lockable rte_mutex' and associated functions for transparent support,
> to wrap above the pthread API.

Yes, this is what I had in mind for the mid/long term but it was too
late for 23.03 after -rc1.

The Windows porting effort will probably need this abstraction too as
we are trying to stop relying on the pthread API.
I don't see this item in Microsoft roadmap, though.


>
> Unless it happens, would it be possible to condition the thread-safety annotations
> to FREEBSD as well?
>
> Maybe have two versions of the annotations:
>
>   __rte_exclusive_locks_required() /* Conditioned on clang */
>   __pthread_exclusive_locks_required() /* Conditioned on glibc/pthread support */
>
> the first one to use on RTE types that can be declared with __rte_lockable,
> the second for pthread objects that we don't want to replace.
> I know the namespace is not great so maybe named another way.
>
> The '#ifdef FREEBSD' ossifies the annotation support at the function level,
> when this is a matter of types. This impedance mismatch would mean large
> changes if someone was to make this support evolve at some point.

I don't see how it requires large changes with the current approach.
The patches in this series are a matter of lines.
If we come up with the right abstraction later, this uglyness will be removed.

In any case, I am running short of time for 23.03.
I am missing reviews from driver maintainers, which is a pity.
Thomas raised an eyebrow on the malloc: and mem: patches too.

I will probably defer this series to the next release.
  

Hello Tyler,

On Thu, Mar 2, 2023 at 9:52 AM David Marchand <david.marchand@redhat.com> wrote:
> On Mon, Feb 27, 2023 at 5:13 PM Gaëtan Rivet <grive@u256.net> wrote:
> > Ah ok, so if I understand correctly, DPDK would need to declare some
> > '__rte_lockable rte_mutex' and associated functions for transparent support,
> > to wrap above the pthread API.
>
> Yes, this is what I had in mind for the mid/long term but it was too
> late for 23.03 after -rc1.
>
> The Windows porting effort will probably need this abstraction too as
> we are trying to stop relying on the pthread API.
> I don't see this item in Microsoft roadmap, though.

Do you have an opinion on this topic?

Thanks.
  

On Mon, Apr 03, 2023 at 12:52:06PM +0200, David Marchand wrote:
> Hello Tyler,
> 
> On Thu, Mar 2, 2023 at 9:52 AM David Marchand <david.marchand@redhat.com> wrote:
> > On Mon, Feb 27, 2023 at 5:13 PM Gaëtan Rivet <grive@u256.net> wrote:
> > > Ah ok, so if I understand correctly, DPDK would need to declare some
> > > '__rte_lockable rte_mutex' and associated functions for transparent support,
> > > to wrap above the pthread API.
> >
> > Yes, this is what I had in mind for the mid/long term but it was too
> > late for 23.03 after -rc1.
> >
> > The Windows porting effort will probably need this abstraction too as
> > we are trying to stop relying on the pthread API.
> > I don't see this item in Microsoft roadmap, though.
> 
> Do you have an opinion on this topic?

Sorry David I got distracted I'll review the thread again.  I think with
Windows toolsets we left off with expanding empty for now?

Anyway, I'll take another pass today if I get a chance.

> 
> Thanks.
> 
> -- 
> David Marchand
  

On Mon, Apr 03, 2023 at 12:52:06PM +0200, David Marchand wrote:
> Hello Tyler,
> 
> On Thu, Mar 2, 2023 at 9:52 AM David Marchand <david.marchand@redhat.com> wrote:
> > On Mon, Feb 27, 2023 at 5:13 PM Gaëtan Rivet <grive@u256.net> wrote:
> > > Ah ok, so if I understand correctly, DPDK would need to declare some
> > > '__rte_lockable rte_mutex' and associated functions for transparent support,
> > > to wrap above the pthread API.
> >
> > Yes, this is what I had in mind for the mid/long term but it was too
> > late for 23.03 after -rc1.
> >
> > The Windows porting effort will probably need this abstraction too as
> > we are trying to stop relying on the pthread API.
> > I don't see this item in Microsoft roadmap, though.
> 
> Do you have an opinion on this topic?

Okay, trying to grok the question here. If the question is do we want to
introduce a mutex/condition variable and lock/unlock signal/wait
abstraction?

I would certainly like to see reduced conditional compilation that
applications have to perform for the platform features. I also really
would like to purge the remaining pthread_{mutex,condvar} shim since it
is unsightly.

With msvc I think we could probably achieve the same with C11 threads but
I haven't investigated feasability and said with no investigation older
glibc may not provide the optional feature.

In the absence of C11 threads we can provide an rte_ abstraction but I
don't think I can put it on the roadmap until basic msvc support is
stood up (a question of resource prioritization as always).

I could commit to looking at it once msvc and atomics changes are merged
the earliest possible time frame for that is the start of the 23.11
cycle. If that happens at decent velocity I could even see adding it to
23.11 roadmap.

For now if someone else decides to introduce an abstraction I would just
caution strongly not to remove __rte_experimental from any API added
until I get a chance to focus on it.

ty

> 
> Thanks.
> 
> -- 
> David Marchand
  

On Mon, Apr 3, 2023 at 5:37 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> On Mon, Apr 03, 2023 at 12:52:06PM +0200, David Marchand wrote:
> > Hello Tyler,
> >
> > On Thu, Mar 2, 2023 at 9:52 AM David Marchand <david.marchand@redhat.com> wrote:
> > > On Mon, Feb 27, 2023 at 5:13 PM Gaëtan Rivet <grive@u256.net> wrote:
> > > > Ah ok, so if I understand correctly, DPDK would need to declare some
> > > > '__rte_lockable rte_mutex' and associated functions for transparent support,
> > > > to wrap above the pthread API.
> > >
> > > Yes, this is what I had in mind for the mid/long term but it was too
> > > late for 23.03 after -rc1.
> > >
> > > The Windows porting effort will probably need this abstraction too as
> > > we are trying to stop relying on the pthread API.
> > > I don't see this item in Microsoft roadmap, though.
> >
> > Do you have an opinion on this topic?
>
> Okay, trying to grok the question here. If the question is do we want to
> introduce a mutex/condition variable and lock/unlock signal/wait
> abstraction?
>
> I would certainly like to see reduced conditional compilation that
> applications have to perform for the platform features. I also really
> would like to purge the remaining pthread_{mutex,condvar} shim since it
> is unsightly.
>
> With msvc I think we could probably achieve the same with C11 threads but
> I haven't investigated feasability and said with no investigation older
> glibc may not provide the optional feature.
>
> In the absence of C11 threads we can provide an rte_ abstraction but I
> don't think I can put it on the roadmap until basic msvc support is
> stood up (a question of resource prioritization as always).
>
> I could commit to looking at it once msvc and atomics changes are merged
> the earliest possible time frame for that is the start of the 23.11
> cycle. If that happens at decent velocity I could even see adding it to
> 23.11 roadmap.

Ok for me.


>
> For now if someone else decides to introduce an abstraction I would just
> caution strongly not to remove __rte_experimental from any API added
> until I get a chance to focus on it.

We *could* introduce an internal API.
But I prefer we wait for your input on this topic rather than
introduce some artificial API.

I'll simply disable those checks on FreeBSD.