use upgradable lock in SHM mappings

[yellowhatter]

No description provided.

[Mallets]

I'm not sure about this change. According to the documentation:

Upgradable read lock reserves the right to be upgraded to a write lock, which means there can be at most one upgradable read lock at a time. Note that attempts to acquire an upgradable read lock will block if there are concurrent attempts to acquire another upgradable read lock or a write lock.

That means, if I have understood it correctly, we can't have parallel reads in this case. As a consequence, only one message at the time can be verified for shm/net conversion even if it does not require it. Is there any particular reason to move to an upgradable_read?

[yellowhatter]

@Mallets Let's check an example: in the original codebase two concurrent threads will use concurrent read access, I agree, BUT when releasing read locks and concurrently acquiring write locks we have 50% probability of one unnecessary context switch!

The worst-case:
thread0: acqure R lock
thread1: acqure R lock
thread0: R access
thread1: R access
thread0: release R lock
thread0: acquire W lock, block because thread1 still holds R lock <- unnecessary context switch, 50% probability
thread1: release R lock
thread0: wake and acquire W lock
thread1: acquire W lock, block because thread0 still holds W lock
thread0: W access
thread0: release W lock
thread1: wake and acquire W lock
thread1: W access
thread1: release W lock

[Mallets]

@Mallets Let's check an example: in the original codebase two concurrent threads will use concurrent read access, I agree, BUT when releasing read locks and concurrently acquiring write locks we have 50% probability of one unnecessary context switch!

The worst-case: thread0: acqure R lock thread1: acqure R lock thread0: R access thread1: R access thread0: release R lock thread0: acquire W lock, block because thread1 still holds R lock <- unnecessary context switch, 50% probability thread1: release R lock thread0: wake and acquire W lock thread1: acquire W lock, block because thread0 still holds W lock thread0: W access thread0: release W lock thread1: wake and acquire W lock thread1: W access thread1: release W lock

Understood. But that context switch would happen iff the try_read_shmbuf() fails. The reason of try_read_shmbuf() failing is linked to the fact that the shared memory segment has not been flinked yet. But once the segment is flinked, the try_read_shmbuf() would succeed more often than not. If we consider the case of a peer connected with N other peers operating over shm, it would mean that only one message translation at the time can be done at reading side if we use the upgradable read vs parallel read in case of a normal RwLockGuard. IMHO parallel reads are more important than avoiding one context switch during flink initial phase that is done only once.

[yellowhatter]

@Mallets Let's check an example: in the original codebase two concurrent threads will use concurrent read access, I agree, BUT when releasing read locks and concurrently acquiring write locks we have 50% probability of one unnecessary context switch!
The worst-case: thread0: acqure R lock thread1: acqure R lock thread0: R access thread1: R access thread0: release R lock thread0: acquire W lock, block because thread1 still holds R lock <- unnecessary context switch, 50% probability thread1: release R lock thread0: wake and acquire W lock thread1: acquire W lock, block because thread0 still holds W lock thread0: W access thread0: release W lock thread1: wake and acquire W lock thread1: W access thread1: release W lock

Understood. But that context switch would happen iff the try_read_shmbuf() fails. The reason of try_read_shmbuf() failing is linked to the fact that the shared memory segment has not been flinked yet. But once the segment is flinked, the try_read_shmbuf() would succeed more often than not. If we consider the case of a peer connected with N other peers operating over shm, it would mean that only one message translation at the time can be done at reading side if we use the upgradable read vs parallel read in case of a normal RwLockGuard. IMHO parallel reads are more important than avoiding one context switch during flink initial phase that is done only once.

I missed that point! Understood and agree!

[Mallets]

Great, then I'll close this PR without merging since it may not bring the expected benefits.