This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add simple implementations of mtx_lock and mtx_unlock.
ClosedPublic

Authored by sivachandra on Feb 14 2020, 2:37 PM.

Details

Summary

These functions only support locking and unlocking of plain mutexes.
They will be extended in future changes to handled recursive and timed
mutexes.

Depends on D75380.

Diff Detail

Event Timeline

sivachandra created this revision.Feb 14 2020, 2:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2020, 2:37 PM
sivachandra marked an inline comment as done.Feb 14 2020, 2:52 PM
sivachandra added inline comments.
libc/src/threads/linux/mtx_lock.cpp
17

Though stdatomic.h is not a free standing header, we will use the one provided by the compilers. LLVM-libc will not provide it.

A mutex implementation will need congestion counts. If only a shim is needed, a bi-state mutex is sufficient.

I think pthread_mutex_timedlock and pthread_mutex_unlock can be implemented first, then rebase mtx_lock and mtx_unlock on pthread. A libc eventually has to implement pthread to be usable on a POSIX platform. It can be a shim, with just the basic functionality. Even if you don't want to think of pthread initially, mtx_lock should be based on mtx_timedlock.

We can assume Linux>=2.6.22 and use FUTEX_PRIVATE_FLAG.

A mutex implementation will need congestion counts. If only a shim is needed, a bi-state mutex is sufficient.

A tri-state mutex helps us avoid an unnecessary futext syscall.

I think pthread_mutex_timedlock and pthread_mutex_unlock can be implemented first, then rebase mtx_lock and mtx_unlock on pthread. A libc eventually has to implement pthread to be usable on a POSIX platform. It can be a shim, with just the basic functionality. Even if you don't want to think of pthread initially, mtx_lock should be based on mtx_timedlock.

In LLVM libc, we want to take the opposite approach of implementing pthread as an extension over standard C thread library.

Agree mtx_lock could be a specialization of mtx_timedlock. As I said in the description, this patch is only a simple implementation of a plain mutex. It allows me to avoid worrying about he timespec API for now. Having a plain mutex available paves way for adding and testing other parts of the libc system.

We can assume Linux>=2.6.22 and use FUTEX_PRIVATE_FLAG.

Agreed. I was not sure, but since you also bring it up, I can go ahead with that change. I will update in the next round.

I think pthread_mutex_timedlock and pthread_mutex_unlock can be implemented first, then rebase mtx_lock and mtx_unlock on pthread. A libc eventually has to implement pthread to be usable on a POSIX platform. It can be a shim, with just the basic functionality. Even if you don't want to think of pthread initially, mtx_lock should be based on mtx_timedlock.

In LLVM libc, we want to take the opposite approach of implementing pthread as an extension over standard C thread library.

My intuition (I've never really used <threads.h>) is that it will be more difficult than the opposite because pthread provides many more options than C11's thread library. I think this patch is good and it'll be useful to have mutex's but my guess is when we implement pthread it will become untenable to use what we currently have.

libc/include/threads.h.def
9–10 ↗(On Diff #244775)

LIBC_THREADS_H

libc/lib/CMakeLists.txt
15

You could add a comment here like the rest of them have, but I don't think its needed, its just more cohesive.

libc/src/threads/linux/mtx_lock.cpp
28

I think it would be more readable if this looked like

if (atomic_compare_exchange_strong(futex_data, &mutex_status, MS_Locked))
  return thrd_success;
switch(mutex_status) {
...
}
libc/src/threads/linux/mtx_unlock.cpp
28

The cast should no longer be necessary.

33

no body -> nobody

libc/src/threads/linux/mutex.h
17 ↗(On Diff #244775)

We could make this enum class MutexStatus and drop the MS_ prefix and refer to these as MutexStatus::Free perhaps.

19 ↗(On Diff #244775)

Prefer using.

sivachandra marked 6 inline comments as done.

Address comments.

sivachandra marked an inline comment as done.Feb 19 2020, 10:50 AM
sivachandra added inline comments.
libc/src/threads/linux/mutex.h
17 ↗(On Diff #244775)

That would be nice, but then we will need casts to integer types as stdatomic.h API wants integer types.

Would it be possible to land the enumeration support separately? These are separate changes and I think it'd be cleaner to land them as two commits.

libc/config/linux/api.td
156

Do you plan to graduate these eventually to a common file (and layer Linux on top)? These API aren't Linux specific and we would like to provide these definitions on other platforms like Fuchsia.

libc/test/src/threads/linux/mtx_test.cpp
17 ↗(On Diff #245466)

Can you make these constexpr?

sivachandra marked 2 inline comments as done.
  • Seperate enum generation to a different patch
  • Use threads from LLVM libc to construct mutex tests
  • Remove use of sleep from tests

Would it be possible to land the enumeration support separately? These are separate changes and I think it'd be cleaner to land them as two commits.

Done now: https://reviews.llvm.org/D75379

libc/config/linux/api.td
156

Yes. When we have another platform file, we will put the common parts in a common file.

phosek accepted this revision.Mar 5 2020, 11:38 AM

LGTM

This revision is now accepted and ready to land.Mar 5 2020, 11:38 AM
This revision was automatically updated to reflect the committed changes.