llvm dev - Jan 2021 - [clang-tidy][RFC] checks concurrency-async-{blocking, fs, no-new-threads}

Hi,

With official C++20 inclusion of coroutines it gets important to 
identify synchronous code usage inside of coroutines. Volunteer 
preemption of a system thread in asynchronous code (e.g. in
coroutines/fibers/green threads) is a bug that prevents the current 
thread from executing other coroutines/etc. and negatively affects 
overall process performance.

I've tried to address it in a series of clang-tidy checks that
try to find such synchronous code based on blacklists from
POSIX/C++ std/C11/Boost/Linux syscalls. Basically, one should enable 
concurrency-*
checks for the strictest mode. It finds usages of synchronous primitives 
(e.g. std::mutex), fs access (e.g. open(3)), implicit system threads 
creation (e.g. via std::execution::par). In some cases one may want to 
relax the check, e.g.:
- if new threads creation is not an issue, I don't force coroutine-only 
code, just don't want to block coroutine threads => disable 
concurrency-async-no-new-threads
- if I have no means to delegate fs access to a thread pool and don't 
want to use aio, so I have to make synchronous fs access from coroutines 
=> disable concurrency-async-fs

Nathan James in a review (https://reviews.llvm.org/D94621#2497859) 
questions whether such check based on functions/types blacklist worth 
implementing. I think it does as every coroutine/fiber-based C++ program 
may suffer from the same problem - you may not use a lot of std stuff 
unless you're OK with ineffective code, so having such "do not use part
of std/boost" checks is handy.

The checks:

concurrency-async-blocking: https://reviews.llvm.org/D93940
concurrency-async-fs: https://reviews.llvm.org/D94621
concurrency-async-no-new-threads: https://reviews.llvm.org/D94622

The patch series are based on a much more simple check used in 
Yandex.Taxi. I think it worth something to the community, so I've 
separated a big list of functions/types apart to address e.g. 
environments without fs threadpool, and added possibility to extend the 
types/functions lists via option.

I'd be happy to hear any comments how the checks can be improved, or 
maybe organized in some other way, or whether they confront any implicit 
clang-tidy policy.

Hi,

I think this is definitely a real problem and it would be great to have 
a solution but I am somewhat sceptical that clang-tidy is the right tool 
to address it.  The problem is that it's not just blocking system calls 
that are unsafe, it's anything that transitively calls those functions.

To have a reliable tool, you'd want something (possibly built on the 
static analyser? I don't know what the status of cross-module analysis) 
that would build a call graph and warn when a code path might end up in 
a blocking call (ideally by analysing which code paths are guarded on 
specific argument values).

You could probably approximate this with an incremental clang-tidy pass 
that would report functions that called functions in a list and updated 
the list with additional things that it had found.

It is possible that coroutine code in the wild isn't doing many deep 
function calls as more general code, in which case it's quite plausible 
that a simple clang-tidy check would be sufficient.  I worry that a 
deny-list approach is going to be far less maintainable than an 
allow-list though: in POSIX there are a handful of system calls that can 
block, but in win32 there are vastly more and higher-level APIs such as 
Qt have a massive API surface.

A list of C++ standard-library functions that are safe to call in 
coroutines seems plausible to maintain.  I wonder if the right approach 
is a [[clang::coroutine_safe]] attribute that we can put on libc++ 
functions, methods, and classes (if all methods are safe to call, or 
with a [[clang::coroutine_unsafe]] variant for methods where the default 
has been defined).  We could then have a checker that warns if you call 
a function that isn't marked as coroutine-safe from a coroutine or from 
a function that is marked as coroutine-safe.

David

On 27/01/2021 11:23, Vasily Kulikov via llvm-dev wrote:
> Hi,
> 
> With official C++20 inclusion of coroutines it gets important to 
> identify synchronous code usage inside of coroutines. Volunteer 
> preemption of a system thread in asynchronous code (e.g. in
> coroutines/fibers/green threads) is a bug that prevents the current 
> thread from executing other coroutines/etc. and negatively affects 
> overall process performance.
> 
> I've tried to address it in a series of clang-tidy checks that
> try to find such synchronous code based on blacklists from
> POSIX/C++ std/C11/Boost/Linux syscalls. Basically, one should enable 
> concurrency-*
> checks for the strictest mode. It finds usages of synchronous primitives 
> (e.g. std::mutex), fs access (e.g. open(3)), implicit system threads 
> creation (e.g. via std::execution::par). In some cases one may want to 
> relax the check, e.g.:
> - if new threads creation is not an issue, I don't force coroutine-only
> code, just don't want to block coroutine threads => disable 
> concurrency-async-no-new-threads
> - if I have no means to delegate fs access to a thread pool and don't 
> want to use aio, so I have to make synchronous fs access from coroutines 
> => disable concurrency-async-fs
> 
> Nathan James in a review (https://reviews.llvm.org/D94621#2497859) 
> questions whether such check based on functions/types blacklist worth 
> implementing. I think it does as every coroutine/fiber-based C++ program 
> may suffer from the same problem - you may not use a lot of std stuff 
> unless you're OK with ineffective code, so having such "do not use
part
> of std/boost" checks is handy.
> 
> The checks:
> 
> concurrency-async-blocking: https://reviews.llvm.org/D93940
> concurrency-async-fs: https://reviews.llvm.org/D94621
> concurrency-async-no-new-threads: https://reviews.llvm.org/D94622
> 
> The patch series are based on a much more simple check used in 
> Yandex.Taxi. I think it worth something to the community, so I've 
> separated a big list of functions/types apart to address e.g. 
> environments without fs threadpool, and added possibility to extend the 
> types/functions lists via option.
> 
> I'd be happy to hear any comments how the checks can be improved, or 
> maybe organized in some other way, or whether they confront any implicit 
> clang-tidy policy.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>