[core] Converted GlobControlLock to a read-write lock.

[maxsharabayko]

CUDTUnited::m_GlobControlLock is now a read-write lock (srt::sync::SharedMutex).

CUDTUnited::m_GlobControlLock guards all member containers of the CUDTUnited.

• CUDTUnited::m_Groups - add/remove under exclusive lock, access - under non-exclusive lock.

• CUDTUnited::m_Sockets - add/remove under exclusive lock, access - under non-exclusive lock.

• CUDTUnited::m_PeerRec – record sockets from peers to avoid repeated connection request. Add/remove under exclusive lock, access - under non-exclusive lock.

• CUDTUnited::m_ClosedSockets - add/remove under exclusive lock, access - under non-exclusive lock.

• CUDTUnited::m_mMultiplexer - - add/remove and access under exclusive lock. ❗

Addresses #2393.

[ethouris]

I think we need to decide something about these calls. With the current locking implementation these are not needed and have been left here to allow implementation of the mutex tracker, which is still hanging around in one of the PRs. So, either add an appropriate empty function with that name to cover the stub, or we'll need to remove them all.

[maxsharabayko]

There was a problem with an initialization of a conditional variable. There is a difference in POSIX on how you initialize the one in a global / static scope, and the one in runtime. That's what setupCond do on POSIX builds. And there was a crash if that initialization was done in the constructor.
setupMutex is indeed not needed except for the mutex tracker.

void Condition::init()
{
    pthread_condattr_t* attr = NULL;
#if SRT_SYNC_CLOCK == SRT_SYNC_CLOCK_GETTIME_MONOTONIC
    pthread_condattr_t  CondAttribs;
    pthread_condattr_init(&CondAttribs);
    pthread_condattr_setclock(&CondAttribs, CLOCK_MONOTONIC);
    attr = &CondAttribs;
#endif
    const int res = pthread_cond_init(&m_cv, attr);
    if (res != 0)
        throw std::runtime_error("pthread_cond_init monotonic failed");
}

[ethouris]

That was here to fix the problem of locking m_ConnectionLock after m_GlobControlLock. Why is that no longer necessary?

[maxsharabayko]

The GlobControl "resource" is now held in a shared manner protecting containers from modification, but there is no mutex being locked. Hence no need for this unlock-lock trick.

[ethouris]

UDT???

[maxsharabayko]

Suggested change

	/// Guards all member containers of `CUDTUnited`. Protect UDT API from data races.
	/// Guards all member containers of `CUDTUnited`. Protect SRT API from data races.

[ethouris]

"Shared lock" would be not unconfusing.

[maxsharabayko]

Suggested change

	/// Non-exclusive lock prohibits changes of containers (insert/remove), but allows modifications
	/// A shared (non-exclusive) lock prohibits changes of containers (insert/remove), but allows modifications

[ethouris]

Yes, that's likely a good idea. And also the shared lock suffices for that, while acquireSocketsGroup could be used here (that is, the constructor overload of GroupKeeper that uses this call).

[ethouris]

In this place you can consider using GroupKeeper instead of SharedLock on m_GlobControlLock - if possible.

[ethouris]

One comment describes a serious problem here, please fix. Others just to consider.

[ethouris]

Shouldn't this be unlock_shared()?

[maxsharabayko]

Agh, right.

Suggested change

	CUDT::uglobal().m_GlobControlLock.lock_shared();
	CUDT::uglobal().m_GlobControlLock.unlock_shared();

[isotherm]

Greetings,

I arrived here due to a contention problem with many sockets (330+ receive sockets on a 48-core Xeon running Linux). This creates 1300+ threads: network receive, TsbPd, RcvQ, SndQ. According to perf, this global lock was the main problem, with CPU usage at 100%, mostly spent in futex spinlocks.

Therefore I cherry picked this change onto 1.5.4 to verify the results. However, performance actually dropped. perf showed even more futex contention. I was able to resolve by replacing the SharedMutex implementation with pthread_rwlock_* (or std::shared_mutex). These implementations do not require a futex once a reader already holds the lock, since it can be done with atomics. Using either of these library shared mutexes, CPU usage drops to ~45%, and we could receive on all sockets without loss.

I am trying to fix this problem now with an eye on the future, so I have some questions about this changeset:

1. The SharedMutex API appears to mirror std::shared_mutex. Will there be a way to enable the more performant implementation (pthread or C++17) at compile time? Would you welcome a pull request to that effect? (Note, SharedMutex::getReaderCount, used only in tests, wouldn't be supported with either library implementation.)
2. Is the global mutex needed to serialize more than access to the global maps, or can we further improve, for example:
   • Using concurrent data structures, for example a map that has locks per tree branch?
   • Using wait-free, lock-free structures like a wait-free, lock-free hash map?
   • Using read-copy-update, since the reads happen thousands of times per second per socket but opening and closing sockets, and other socket events, are quite seldom?
3. Has there been any general consideration of how to address performance scaling? Is there anything on the roadmap that I can work toward and/or submit pull requests toward? Since I will be generating patches, I would prefer them to align with future goals, where possible.

Note, all the threads are still taking significant CPU and frequent wake ups (see e.g. #3094), but that is mostly not related to the global lock.

[ethouris]

Hi @isotherm, sorry that it took that long to review this.

This PR is going to be abandoned because it can be no longer processed, however changes provided here are currently merged to the dev branch, which is going to become the future 1.6.0 release.

The std::shared_mutex is used as an implementation for srt::sync::SharedMutex if you --use-stdc++-sync and --use-c++-std=c++17, which isn't default, but it's supported. This will be not merged to 1.5.5 yet, but is in this development branch for the future 1.6.0 version.

We probably will have some lock-free containers in the future, but m_GlobControlLock isn't that easy like just to change the implementation for the sockets container. This mutex keeps consistency of the whole system, so for example removal of a socket from m_Sockets and putting it into m_ClosedSockets must be done non-preemptively, and there ain't no way to do this using lock-free algorithms.

I will be all the way interested how the performance has changed in the dev branch towards 1.5.5 candidate. My last measurements were optimistic, but the perspective of the others is all too welcome.