[all-commits] [llvm/llvm-project] 95ebf2: [libc++] Refactor the predicate taking variant of ...

[Jan Kokemüller via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 95ebf2be0e6600465a4d0f4e7d81c7ded0559fba
      https://github.com/llvm/llvm-project/commit/95ebf2be0e6600465a4d0f4e7d81c7ded0559fba
  Author: Jan Kokemüller <jan.kokemueller at gmail.com>
  Date:   2024-02-19 (Mon, 19 Feb 2024)

  Changed paths:
    M libcxx/include/__atomic/atomic_sync.h
    M libcxx/include/latch
    M libcxx/include/semaphore
    M libcxx/src/atomic.cpp

  Log Message:
  -----------
  [libc++] Refactor the predicate taking variant of `__cxx_atomic_wait` (#80596)

This is a follow-up PR to
<https://github.com/llvm/llvm-project/pull/79265>. It aims to be a
gentle refactoring of the `__cxx_atomic_wait` function that takes a
predicate.

The key idea here is that this function's signature is changed to look
like this (`std::function` used just for clarity):

```c++
__cxx_atomic_wait_fn(Atp*, std::function<bool(Tp &)> poll, memory_order __order);
```

...where `Tp` is the corresponding `value_type` to the atomic variable
type `Atp`. The function's semantics are similar to `atomic`s `.wait()`,
but instead of having a hardcoded predicate (is the loaded value unequal
to `old`?) the predicate is specified explicitly.

The `poll` function may change its argument, and it is very important
that if it returns `false`, it leaves its current understanding of the
atomic's value in the argument. Internally, `__cxx_atomic_wait_fn`
dispatches to two waiting mechanisms, depending on the type of the
atomic variable:

1. If the atomic variable can be waited on directly (for example,
Linux's futex mechanism only supports waiting on 32 bit long variables),
the value of the atomic variable (which `poll` made its decision on) is
then given to the underlying system wait function (e.g. futex).
2. If the atomic variable can not be waited on directly, there is a
global pool of atomics that are used for this task. The ["eventcount"
pattern](<https://gist.github.com/mratsim/04a29bdd98d6295acda4d0677c4d0041>)
is employed to make this possible.

The eventcount pattern needs a "monitor" variable which is read before
the condition is checked another time. libcxx has the
`__libcpp_atomic_monitor` function for this. However, this function only
has to be called in case "2", i.e. when the eventcount is actually used.
In case "1", the futex is used directly, so the monitor must be the
value of the atomic variable that the `poll` function made its decision
on to continue blocking. Previously, `__libcpp_atomic_monitor` was
_also_ used in case "1". This was the source of the ABA style bug that
PR#79265 fixed.

However, the solution in PR#79265 has some disadvantages:

- It exposes internals such as `cxx_contention_t` or the fact that
`__libcpp_thread_poll_with_backoff` needs two functions to higher level
constructs such as `semaphore`.
- It doesn't prevent consumers calling `__cxx_atomic_wait` in an error
prone way, i.e. by providing to it a predicate that doesn't take an
argument. This makes ABA style issues more likely to appear.

Now, `__cxx_atomic_wait_fn` takes just _one_ function, which is then
transformed into the `poll` and `backoff` callables needed by
`__libcpp_thread_poll_with_backoff`.

Aside from the `__cxx_atomic_wait` changes, the only other change is the
weakening of the initial atomic load of `semaphore`'s `try_acquire` into
`memory_order_relaxed` and the CAS inside the loop is changed from
`strong` to `weak`. Both weakenings should be fine, since the CAS is
called in a loop, and the "acquire" semantics of `try_acquire` come from
the CAS, not from the initial load.



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications