Messages in this thread | | | Date | Fri, 20 Jan 2023 09:25:00 +0100 | From | Sebastian Andrzej Siewior <> | Subject | Re: [PATCH v2] locking/rwbase: Prevent indefinite writer starvation |
| |
On 2023-01-19 17:41:01 [+0000], Mel Gorman wrote: > > Yes, it makes your concern much clearer but I'm not sure it actually matters > in terms of preventing write starvation or in terms of correctness. At > worst, a writer is blocked that could have acquired the lock during a tiny > race but that's a timing issue rather than a correctness issue.
Correct. My concern is that one reader may need to wait 4ms+ for the lock while a following reader (that one that sees the timeout) does not. This can lead to confusion later on.
> Lets say the race hits > > reader sees waiter_timeout == 0 > writer acquires wait_lock > __rwbase_write_trylock fails > update waiter_timeout > rwbase_schedule > > Each reader that hits the race goes ahead at a point in time but anything > readers after that observe the timeout and eventually the writer goes ahead. > > If the waiter_timeout was updated before atomic_sub(READER_BIAS), > it doesn't close the race as atomic_sub is unordered so barriers would > also be needed and clearing of waiter_timeout moves to out_unlock in case > __rwbase_write_trylock succeeds. That's possible but the need for barriers > makes it more complicated than is necessary.
yes...
> The race could be closed by moving wait_lock acquisition before the > atomic_sub in rwbase_write_lock() but it expands the scope of the wait_lock > and I'm not sure that's necessary for either correctness or preventing > writer starvation. It's a more straight-forward fix but expanding the > scope of a lock unnecessarily has been unpopular in the past. > > I think we can close the race that concerns you but I'm not convinced we > need to and changing the scope of wait_lock would need a big comment and > probably deserves a separate patch.
would it work to check the timeout vs 0 before and only apply the timeout check if it is != zero? The writer would need to unconditionally or the lowest bit. That should close gaps at a low price. The timeout variable is always read within the lock so there shouldn't be need for any additional barriers.
> Sorry if I'm still missing something stupid and thanks for your patience > reviewing this. thank that it is patience and not pain in the ass ;)
Sebastian
| |