Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: sync: Add UnlockToRLock() to RWMutex #38891

Closed
pjebs opened this issue May 6, 2020 · 44 comments
Closed

proposal: sync: Add UnlockToRLock() to RWMutex #38891

pjebs opened this issue May 6, 2020 · 44 comments

Comments

@pjebs
Copy link
Contributor

pjebs commented May 6, 2020

There is a scenario that can't be done efficiently (or at all) from outside sync package.
This is with regards to RWMutex

If you have an operation that has a well delineated "write" portion and "read" portion:

  1. Aquire a Lock
  2. Do something with the lock.
  3. Release the Lock.
  4. Immediately acquire RLock.
  5. Do something with read lock.
  6. Release the RUnlock.

The objective is:

  1. Increase throughput by allowing more goroutines wishing to Read to be able to do so. AND
  2. Prevent another goroutine that wants to Lock from interjecting between steps 3 and 4 above.

My proposal is a function called UnlockToRLock() that is simple to implement from inside sync package that covers the scenario above.

The alternative is:

  1. Reduce throughput by keeping Step 1-6 inside a Lock OR
  2. Accept a goroutine that wants to acquire lock interjecting between Step 3&4 (the scenario assumes an operation where the "read" portion must follow the "write" portion, and it is not correct for another goroutine to potentially interfere before the "read")
@gopherbot gopherbot added this to the Proposal milestone May 6, 2020
@ianlancetaylor
Copy link
Contributor

I think this is a dup of #4026 and #23513.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) May 6, 2020
@navytux
Copy link
Contributor

navytux commented May 11, 2020

For the reference: pygolang settled on UnlockToRLock name:

https://lab.nexedi.com/nexedi/pygolang/commit/a9345a98

@rsc
Copy link
Contributor

rsc commented May 20, 2020

Over on #23513 (comment) I said:

I'm not convinced this is helpful in a real program, and there is a real cost to adding complexity to the API. Please show us benchmarks improving a real workload, not a microbenchmark.

Optimizing the "holding for writing and dropping to holding for reading" makes it sound like "holding for writing" is something that can be done efficiently, but my understanding is that on today's machines, if an RWMutex is held for writing with any appreciable frequency, you don't see performance wins beyond having a plain Mutex. The contention from the writers dominates everything else.

If my understanding is correct, then this new API surface would add complexity without any performance benefit, and so we shouldn't add it.

Does anyone have numbers to address this concern, one way or the other?

@rsc rsc moved this from Incoming to Active in Proposals (old) May 20, 2020
@networkimprov
Copy link

@pjebs, you could get some of the semantic benefit of UnlockAndRLock() with TryRLock(). Any thoughts?

@navytux
Copy link
Contributor

navytux commented May 20, 2020

@rsc, on my side it is not the performance in the first place, but semantic guarantee of not releasing the lock while downgrading it. If there is no such guarantee, the writer that done modification part of its job and no longer needs the lock to be exclusively held, needs to recheck the data for consistency after Unlock->Rlock because the data might have been changed by other mutators while the lock was not held. With atomic downgrade there is no such need.

And contrary to upgrade, which is not possible to implement without deadlocks, atomic downgrade is well-defined operation with well-defined semantic.

Leaving performance topic for others.

@ianlancetaylor
Copy link
Contributor

@navytux The question that @rsc is asking is: if you need to downgrade the lock, would you get better performance overall by always using a sync.Mutex rather than a sync.RWMutex. sync.RWMutex is more complex and harder to understand than sync.Mutex, so the only reason to ever use a sync.RWMutex is performance. If you need to acquire a write lock and downgrade the lock, @rsc is suggesting that you will get a better performance by using sync.Mutex. There is no reason to make sync.RWMutex still more complex if it doesn't give a performance advantage.

@networkimprov
Copy link

There are other reasons to use a RWMutex, including to wait for all potentially-concurrent tasks to cease (and block new ones), e.g. before shutdown/upgrade. Such tasks acquire a long-lived read lock.

If a situation implies a RWMutex, you should use that, even if there's no apparent performance issue. As the app evolves, or its workloads in the field increase, you avoid the need to replace a Mutex with a RWMutex. And you have more meaningful code.

You might also be able to replace a collection of Mutexes covering separate resources with a single RWMutex covering them all, if most of the access to them is read-only.

@ianlancetaylor
Copy link
Contributor

@networkimprov Those are nice examples, but do any of them require a UnlockThenRLock method?

@as
Copy link
Contributor

as commented May 20, 2020

I experimented with this concept here https://github.com/as/lock/blob/master/lock.go and it was called Downgrade.

In practice, it had very little benefit for similar reasons that @rsc mentioned.

@networkimprov
Copy link

My point is that performance is really not the correct focus of this discussion.

@navytux makes a good point re the need to recheck the data for consistency after Unlock->RLock. That operation may not be lightweight.

@as
Copy link
Contributor

as commented May 21, 2020

My point is that performance is really not the correct focus of this discussion.

@networkimprov I disagree, the consistency can only be violated if you release the lock. Why do you release the lock and re-acquire the read lock?

@ianlancetaylor
Copy link
Contributor

Performance really does come in, because the code can simply replace the sync.RWMutex with a sync.Mutex. The resulting code will be simpler, and there will be no need to use UnlockThenRLock because you will always hold a full lock. And it seems that for cases where it is useful to call UnlockThenRLock, using a sync.Mutex may well be faster. So the question is: if it would be both faster and simpler to use a sync.Mutex, then why should we make the implementation of sync.RWMutex more complex?

@as
Copy link
Contributor

as commented May 21, 2020

The temptation that leads to these types of proposals is that there are many readers and few writers. The writer is finished writing in the critical section and wants to unblock the readers, but for some reason, needs read access in the critical section.

However, there is usually a way to avoid reading in the critical section altogether. For example, a writer stores a value into a map, and then loads it and returns it. There is no reason to really load the element since the writer was the one who put it in there.

In more complex cases, this is desired when there are multiple maps, but the result often has negligible performance benefits upon systematic benchmarking. I suspect this is because it is easy to underestimate the complexity of sync.RWMutex and overestimate the penalty of doing extra operations while holding the write side of the lock in the critical section.

@networkimprov
Copy link

I think we should let @pjebs and @navytux explain their use cases before we claim that they're weak :-)

I'm disappointed to see the semantic value of RWMutex brushed off. Switching to a Mutex for this situation may make the code "simpler" in a way, but ultimately less meaningful.

This is also the third (at least) request for this feature, so maybe there's something to it.

@as
Copy link
Contributor

as commented May 21, 2020

@networkimprov I found these, but the first one is talking about upgrading a RWMutex reader to writer. Do you have the issue for the third request?

#4026
#23513 (comment)
#38891

@navytux
Copy link
Contributor

navytux commented May 21, 2020

@navytux The question that @rsc is asking is: if you need to downgrade the lock, would you get better performance overall by always using a sync.Mutex rather than a sync.RWMutex.

@ianlancetaylor thanks for clarifying the context of this question. In short my answer is "no" - RWMutex - also known as shared readers / exclusive writer lock - brings real performance benefit because it allows many readers to be run simultaneously instead of running only a single reader at a time as would be implied by regular sync.Mutex.

Now let me try to explain:

In the filesystem I write, both at server and at client sides, a particular database view is maintained for transactional data snapshot. There are readers of the data which need data snapshot to remain stable throughout their operations and tasks that update database view (e.g. as new transactions are being committed and database sends corresponding notifications). Here it is natural to use shared-readers/single-writer lock - aka sync.RWMutex - to organize synchronization in between readers and snapshot updater. However contrary to usual scenarios with plain mutex the lock is not short-lived because readers perform network round-trip while accessing the database under the lock for data snapshot to remain stable. I think in this case it goes without saying that using sync.Mutex instead of sync.RWMutex brings significant performance degradation because then there would be only 1 allowed simultaneous database user.

The above could work without atomic downgrade if the updater could work with only taking the lock for write and then completely releasing it when done. That is however not the case for me:

  • on client, after first part of the update process, the updater needs to let other "snapshot observer" (pinner thread) to run, because second part of the update have to send requests to filesystem server that trigger messages to the pinner that must be handled and replied, or else the whole thing will get stuck due to design of virtual memory handling.
  • Unlock'ing and then RLock'ing separately can cause deadlocks, because in the presence of multiple shared locks, if it was e.g. originally A.W B.R lock order, without atomic downgrade and doing A.Unlock + A.RLock it will become B.R A.R leading to AB-BA style deadlock wrt another process which tries to take A.R B.R.
  • it is indeed not trivial to recheck potentially changed state if snapshot lock was released and retaken because there can be another simultaneous resync requests (= requests to change database view) initiated by client.

Organization of client locking: https://lab.nexedi.com/kirr/wendelin.core/blob/9f5f8bab/wcfs/client/wcfs.cpp#L117-184
The place where client.resync needs atomic downgrade:
https://lab.nexedi.com/kirr/wendelin.core/blob/9f5f8bab/wcfs/client/wcfs.cpp#L716-756
Organization of locking on filesystem server:
https://lab.nexedi.com/kirr/wendelin.core/blob/9f5f8bab/wcfs/wcfs.go#L428-464

Overall I think for a well-defined and well-understood operation it is better to put complexity into sync.RWMutex instead of exposing complexity to programmers and forcing them to solve this tasks by themselves many times instead of only once, properly and for everyone.

For naming my rationale for UnlockToRLock is that UnlockThenRLock name does not tell a reader that operation is atomic, and it could be perceived as real Unlock then RLock. On the other hand UnlockToRLock clearly tells that the mutex is not reaching "unlocked" state in between.

Kirill

@networkimprov
Copy link

#4026 asked for both downgrade & upgrade, and it was clarified that upgrade isn't possible without a failure mode.

@bcmills
Copy link
Contributor

bcmills commented May 21, 2020

The performance argument is interesting to me, because on the one hand it seems like a RWLock should be mostly harmless if the writer-locked sections are very short, but on the other hand the very fairness of RWLock makes even a very short writer-locked section dangerous: it is possible for a reader lock to block the writer for a long time, which then blocks all other readers behind that writer. So even a RWLock with a very short critical section for the writer can be quite dangerous.

(And, to be honest, most of the uses of RWLock that I've seen would be better-served by some other synchronization pattern anyway...)

@bcmills
Copy link
Contributor

bcmills commented May 21, 2020

@navytux, note that for “snapshot”-style uses in particular, an atomic.Value is almost always a better fit, perhaps in conjunction with a sync.Mutex to restrict concurrent updates. With that approach, the readers never block.

@networkimprov
Copy link

it is possible for a reader lock to block the writer for a long time, which then blocks all other readers

Bryan, how is that worse than a Mutex in the scenario you give, or are you saying that any Mutex is wrong for it?

most ... would be better-served by some other synchronization pattern

For example?

@as
Copy link
Contributor

as commented May 21, 2020

@networkimprov RCU

@cespare
Copy link
Contributor

cespare commented May 21, 2020

(And, to be honest, most of the uses of RWLock that I've seen would be better-served by some other synchronization pattern anyway...)

I endorse this. I think we should avoid adding features to rwmutexes that make them more tempting to use than they already are.

I like @nelhage's blog post as a reference for the problems with read-write locks.

@navytux
Copy link
Contributor

navytux commented May 22, 2020

@navytux, note that for “snapshot”-style uses in particular, an atomic.Value is almost always a better fit, perhaps in conjunction with a sync.Mutex to restrict concurrent updates.

@bcmills, by "snapshot" I was meaning something different than just one value snapshot. In my case it is something like database connection with many objects obtained through it and exposed to the user. When updating we need to update both the connection and already exposed objects in consistent way. Maybe I'm missing something, but it is hard for me to see how atomic.Value helps here. The write path - the updater - is also not "small" as it involves network round-trips too. I still stand on my point that shared/exclusive lock is the best fit to this scenario. Please see details in my original post.

The part of the writer that performs network round-trip:
https://lab.nexedi.com/kirr/wendelin.core/blob/9f5f8bab/wcfs/client/wcfs.cpp#L733-753

@navytux
Copy link
Contributor

navytux commented May 22, 2020

It is true that whether shared-lock is writer-priority, or reader-priority, or of some other kind, makes a difference. Even more: wcfs implements additional custom locking protocol on top of writer-priotity sync.RWMutex to avoid deadlocks due to potential locking cycle through page lock in OS kernel:

https://lab.nexedi.com/kirr/wendelin.core/blob/7dbddf9d/wcfs/notes.txt#L186-205
https://lab.nexedi.com/kirr/wendelin.core/blob/7dbddf9d/wcfs/wcfs.go#L547-565

That does not mean shared locks are useless.

It is true that RWMutex locking patterns can be implemented with just channels (see e.g. here (2) for writer-preference version). It is also however true that just plain sync.Mutex can be implemented with just channel as well.

Does that mean that Mutex and/or RWMutex are useless and should not be there?
My personal answer is no.

@networkimprov
Copy link

So Caleb, Bryan, and "as" suggest that one must roll your own sync scheme with atomics when UnlockToRLock() is required, because a RWMutex isn't optimal in certain (possibly unrelated) scenarios?

Are there any widely used, well-tested third-party packages with alternate sync schemes?

Roll-your-own sync may not turn out well. Isn't safeguarding Go users against incorrect code a higher priority than directing them towards optimal code? (And isn't there a proverb about premature optimization? :-)

@bcmills
Copy link
Contributor

bcmills commented May 22, 2020

@networkimprov, converting a write-lock to a read-lock is never “required”: you can keep holding the write-lock instead. Our argument is that if you need an optimization, UnlockThenRLock is usually not the right fix anyway, because you'll still end up with much of the same latency and contention that you would have had just holding the write lock.

@rsc
Copy link
Contributor

rsc commented May 27, 2020

@bcmills's last comment makes clear why I brought up performance: if there's not a performance win, then just keep holding the write lock.

@navytux's scenario is fascinating. I am honestly a bit surprised that you've gotten as far as you have without wanting control over the read-write contention policy. I'd have expected that a database would care about exactly how to resolve those conflicts and not be at the mercy of whatever the RWMutex implementation does, especially since the RWLock is tuned for in-memory protection (with very different concerns than a database).

Thanks @as for reporting back on past experience with this operation (and that it didn't help much).

Overall, the general sentiment seems to be that we can do without adding this operation, except for @navytux's example. Am I reading that right?

@navytux
Copy link
Contributor

navytux commented Jun 3, 2020

@rsc, thanks for feedback and kind words. Writer-preference shared/exclusive lock semantic fits into my need for contention policy, that why I'm using sync.RWMutex directly.

While I understand that majority of the uses for sync.Mutex and sync.RWMutex are for short-lived critical sections, nothing in sync package documentation says so. There it is only said that those primitives provide mutual exclusion of various kinds. And as e.g. my example shows, mutual exclusion is not always used for short-lived interaction. Talking about sync.Mutex, there is even a sign that it is not always used as a kind of critical section lock because it implements semantic of 1 valued semaphore which can be released by actor different from who originally acquired it. This property is not an overlook - for example it was explicitly preserved when @dvyukov proposed to remove it for sync.Mutex semantic to become more akin to critical section lock and thus more tractable for race/deadlock detector.

What I'm trying to say is: if sync.*Mutex provide "mutual exclusion", not "critical sections", why are we bringing arguments that apply only to "critical sections" cases? In other words: why are we ruling out arguments coming from cases where locked mutex states are long-lived?

From what I've read above, my understanding is that it is very likely for my case to be declared "special", RWMutex.UnlockToRLock rejected - similarly to the fate of #4026, #23513, "Atomically downgrade write lock to read lock?" thread on golang-nuts, etc ...; and that I should go and implement high-level synchronization primitives myself, or maybe even "substitute a custom RWMutex implementation into your own code (copy and modify)".

While certainly doable, I think that would be a pity overall outcome.

Kirill

/cc @nixprime, @dsymonds, @josharian

@pjebs pjebs changed the title proposal: sync: Add UnlockThenRLock() to RWMutex proposal: sync: Add UnlockToRLock() to RWMutex Jun 3, 2020
@bcmills
Copy link
Contributor

bcmills commented Jun 3, 2020

@navytux, I think you're reading more into the phrase “critical section” than at least I intended. I interpret the “critical section” as a contiguous segment of the program's execution trace, not its source code. (That is: the “critical section” is the portion of the execution of the program during which the lock is held.)

If you don't like that term, the same argument applies to “mutual exclusion”: as far as I can tell sync.RWMutex is prone to latency spikes whenever it may be read- or write-locked for a long duration, regardless of how that duration is divided among goroutines.

@rsc
Copy link
Contributor

rsc commented Jun 3, 2020

Yes, in my mind a "critical section" can begin in one goroutine and be finished in another goroutine. That's why sync.Mutex allows unlocking in a different goroutine.

@rsc
Copy link
Contributor

rsc commented Jun 3, 2020

With the understanding that @navytux still disagrees, it does seem like the overall consensus otherwise is in favor of not adding UnlockToRLock. If someone came back with a clear performance win compared to Unlock+RLock then that might be worth revisiting.

But for now, this seems like a likely decline.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jun 3, 2020
@pjebs
Copy link
Contributor Author

pjebs commented Jun 4, 2020

In my past life I use to be a barrister. I found arguing in court so emotionally draining that I became a Go developer instead. I guess I've come full circle:

From what I read above, I see confusion and misunderstandings amongst the key proponents so I will attempt to elucidate why people's arguments are sliding past each other.

But firstly I will restate below the scenario that my proposal was catering for. There was a mistake in my original formulation which I've now corrected above.

========

The scenario is an operation that has a well delineated "write" portion and "read" portion:

  1. Aquire a Lock
  2. Do some "write" with the lock.
  3. Release the Lock.
  4. Immediately acquire RLock.
  5. Do something with read lock.
  6. Release the RUnlock.

The objective is:

  1. Increase throughput by allowing more goroutines wishing to Read to be able to do so. AND
  2. Prevent another goroutine that wants to Write Lock from interjecting between steps 3 and 4 above.

The alternative is:

  1. Reduce throughput by keeping Step 1-6 inside a Lock OR
  2. Accept a goroutine that wants to acquire lock interjecting between Step 3&4 (the scenario assumes an operation where the "read" portion must follow the "write" portion, and it is not correct for another "write" goroutine to potentially interject before the "read")

========

Performance

A lot of the discussion is related to the RWLock not actually increasing performance due to potential contention by the different write locks.

@rsc believes there may be no gain by implementing the proposal, hence it's not worth the team's limited resources implementing it and/or complicating the API surface.

@ianlancetaylor believes there is definitely no gain, hence it should not be implemented. He advocates for Alternative #1 stated above.

@bcmills believes the proposal is probably innocuous so there is no loss, but no practical gain either, hence it should not be implemented.

I'm not aware of any empirical evidence either way for these assertions.

My response

I contend that this proposal does improve performance in terms of overall throughput. In an operation where there is a well delineated "write" and "read" portion, AND there are other (hypothetically many) independent operations that only want to acquire a "read", then quite clearly a write lock will block those independent read operations from running, when that need not be the case.

This proposal allows those independent "read-only" operations from performing their task while the current operation is proceeding (and in the "read" portion).

This desired functionality is the reason why this proposal has come up numerous times.

Now @bcmills and @rsc are knee deep in low-level areas of Go that I and some of the other proponents have no understanding of. Perhaps they see something that we don't from a low-level implementation perspective.

In @bcmills case, he assumes that the proposal won't work because the "read" locks from the independent read-operations will potentially be behind a higher-priority "write" lock so there will not be an increase in throughput. On the other hand, all the proponents of the proposal assume that those independent "read-only" operations can "jump" the queue (bypass the awaiting "write" lock) and thus acquire the "read" once the original operation calls UnlockToRLock and leaves the "write" portion.

That is another source of confusion.

Labelling Angle

There is another angle to this proposal that @navytux and @networkimprov have highlighted. Irrespective of the "contention" performance issues, they believe that such a function is an important addition to RWMutex to properly indicate (and signal to other developers eg doing code review) some further facet of the way they implemented their 'operation'. It signals that their operation mirrors the scenario stated at the start (distinct write and read portion).

From this perspective, it doesn't matter if UnlockToRLock behind the scenes does absolutely nothing and retains the write lock without downgrading it. From this perspective, it's innocuous from a practical implementation perspective but It does provide some semantic benefits which is valuable in its own right.

But they would hope as a bonus that the UnlockToRLock function does in practice implement the "downgrading" feature.

** My response**

I believe that UnlockToRLock should be added also because it is valuable in it's own right. In the original comment of the proposal I wrote that this feature is easy to implement. It was based on a quick study of the sync package as is. Perhaps @bcmills is right and it is not trivial to allow other waiting "read-only" goroutines to jump over a high-priority awaiting "write" lock.

In that case then I would still be in favour of the proposal so that we can write correct code that uses UnlockToRLock function knowing that is is essentially a no-op. However, in the future if this proposal is properly implemented, then a rebuild of our application would magically get a performance boost.

** Better ways to do it **

There is another argument made by some people. They claim that for the scenario listed, the RWMutex is not the correct approach. They say there other better ways of achieving it and this proposal will encourage developers to use an RWMutex when they should not be.

** My response **

I disagree. The RWMutex, as described by the documentation, seems to be exactly the right tool to use for this scenario - especially if UnlockToRLock is added. If its underlying implementation is the bottleneck then it should be rewritten so that developers can use the RWMutex for this scenario. Alternatively, the entire RWMutex should just be wholesale deprecated and a message placed saying it was a mistake to provide it because it's simply not efficient to use and doesn't do a good job at what it was meant to do and suggest the standard mutex to be used. Having said that, that is not factually correct. The RWMutex is in fact better to use when there is lots of readers and not much write contention. (My personal scenario is such a case, but I can't share it due to confidentiality reasons).

The developer can instead be notified of how and when it should be used similar to the instructions/recommendations in the new sync pkg map.

@bcmills
Copy link
Contributor

bcmills commented Jun 4, 2020

@pjebs,

Perhaps @bcmills is right and it is not trivial to allow other waiting "read-only" goroutines to jump over a high-priority awaiting "write" lock.

Implementation-wise, it is not difficult. However, we intentionally do not allow readers to jump because that can lead to writer starvation. (That is: sync.RWMutex is, by design, a write-preferring lock.)

However, in the future if this proposal is properly implemented, then a rebuild of our application would magically get a performance boost.

I don't think there would be any point in writing the application as if sync.RWMutex would one day become a read-preferring lock, because to do so would reintroduce starvation, and would thus be an incompatible change.

On the other hand, a read-preferring lock would be straightforward to implement as a third-party library. (I would not be at all surprised if one already exists somewhere in the open-source world.) And since mutexes should never be part of a package's exported API (concurrency should be an implementation detail!), it is easy to switch any given package to use a third-party alternative.

You could argue, perhaps, that sync.RWMutex should be rewritten to use some much more sophisticated (read: complex and difficult-to-reason-about) heuristic instead of simple reader- or writer-preference, but I don't think that would be worth the implementation cost or usage complexity — in my experience, there are always clearer concurrency patterns available.

@pjebs
Copy link
Contributor Author

pjebs commented Jun 4, 2020

What about if the use of UnlockToRlock allows a jump? That would be backward compatible.

@bcmills
Copy link
Contributor

bcmills commented Jun 4, 2020

Personally, I would classify that as a “more sophisticated (read: complex and difficult-to-reason-about) heuristic”.

@bcmills
Copy link
Contributor

bcmills commented Jun 4, 2020

(It also wouldn't help for readers starved before the first call to UnlockToRlock.)

@pjebs
Copy link
Contributor Author

pjebs commented Jun 5, 2020

I still think implementing UnlockToRlock and keeping it "write-preferring" can still yield throughput gains with regards to all the awaiting read locks ahead of the awaiting write lock. Currently the awaiting read locks are just waiting for the initial operation to end and release the write lock.

@prattmic
Copy link
Member

prattmic commented Jun 5, 2020

I can speak briefly to #23513. The implementation for this is available at as gvisor.dev/gvisor/pkg/sync.RWMutex if anyone would like to play with it.

This lock is used to protect what are (in effect) page tables. The write lock is held to create/delete entries, and the read lock is held during any I/O using those mappings. This function is one of the key uses of DowngradeLock, where the slow path needs to create a mapping and then use it for I/O.

The approach of using DowngradeLock vs Unlock + RLock certainly provided throughput improvements, as the unlock invalidates several lookups we've done at that point, requiring extra lookups. And it is certainly an improvement over holding the write lock during potentially expensive I/O. Unfortunately, I don't have the numbers we found available off-hand.

That said, the writer-priority behavior @bcmills noted is absolutely going to have a limiting factor on our performance.

Today, this lock protects all memory mappings for a single process, so all I/O or memory operations in the process must take the same lock. In all likelihood, gVisor will likely eventually change the design to use different locks for small regions of the address space (the upstream Linux kernel is also considering this). Since that design would significantly reduce contention, DowngradeLock would likely provide significantly less benefit.

@nixprime
Copy link
Contributor

nixprime commented Jun 5, 2020

In all likelihood, gVisor will likely eventually change the design to use different locks for small regions of the address space (the upstream Linux kernel is also considering this). Since that design would significantly reduce contention, DowngradeLock would likely provide significantly less benefit.

I wouldn't take this as a given, for a number of reasons:

  • Linux has been considering making mmap_sem a range lock since at least 2014, without having established consensus, because range locking is slower than an ordinary mutex in the absence of contention.

  • The most recent exploration of range-lock mmap_sem still includes mm_downgrade_write_range_lock, and AFAIK there is no intention to remove it. (Because Linux's locking, and cost of installing page table entries, differs somewhat from gVisor's, their use of downgrading is rarer, but it's still used for e.g. downgrading mmap_sem from exclusive to shared during unmap.)

  • gVisor uses DowngradeLock for both MM.mappingMu (analogous to Linux's mmap_sem) and MM.activeMu (analogous to Linux's page table locks, which in most cases is already partially fine-grained on most architectures). However, Linux's page tables differ from the gVisor's equivalent in several key ways: most significantly, Linux's page table locking is considerably more complex, with (IIUC) mmap_sem serializing alloc/free of page tables, page_table_lock serializing concurrent allocations, and split page table locks serializing mutations of individual entries (for e.g. copy-on-write), so the split lock isn't required to protect the consistency of a data structure like the MM.pmas B-tree in gVisor; and Linux is usually only operating on one page (or hugepage etc.) at a time, so its lock splitting is straightforward and fast (the spinlock in question is usually in the struct page of the given page table).

@networkimprov
Copy link

@prattmic, would you say that DowngradeLock() is valuable/essential in early production releases, at which point an optimal, app-specific concurrency scheme isn't yet apparent, or even necessary?

@prattmic
Copy link
Member

No, it is not "essential in early production releases". We went a long time without DowngradeLock(); using it is an optimization, though one that may be improved with an app-specific mechanism (though not as simply as I said before as @nixprime points out).

@rsc
Copy link
Contributor

rsc commented Jun 10, 2020

@pjebs, you are saying it will lead to an increase in throughput. Do you have concrete numbers?

What we've been saying all along is that if there is a clear performance win then that would be good evidence for doing something. In the absence of any clear evidence for what to do, we typically hold back and don't make changes.

Will leave as likely decline for another week. If it is declined next week and we get a clear performance rationale after that, we can always revisit.

@networkimprov
Copy link

I think we heard two use cases, for gVvisor (where it improved performance) and Wendelin.core (where it's required for data coherence). We also heard the semantic (vs performance) value of RWMutex.

Is that not sufficient evidence to address this? If not, what precisely is required?

@rsc
Copy link
Contributor

rsc commented Jun 17, 2020

I'd be happy to reopen this discussion with specific numbers from gVisor or any other production app. We only have an anecdote, not production numbers we can anaylze (and in particular compare to a plain non-RW Mutex).

For now, it seems there is no change in consensus here, so declined.

@rsc rsc closed this as completed Jun 17, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jun 17, 2020
@golang golang locked and limited conversation to collaborators Jun 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests