Nov. 23, 2022
2:05 p.m.
On Wed Nov 23 00:51:16 2022 +0000, Zebediah Figura wrote:
> Having actually looked at 5/5, I guess I see what you're doing, but it
> seems an awkward way to do it. What you really want is to protect some
> state (mostly just "rate") with a more restricted mutex, but temporarily
> dropping that mutex then feels wrong in various ways. The fact that e.g.
> GetCharacteristics() accesses "state", which is sort of treated like
> it's protected by shutdown_cs, doesn't seem great either.
> I wonder if the right solution here is an rwlock for "state" and
> associated fields ("wg_parser", "stream_count", etc.), and I guess a
> separate lock for "rate".
No, `state` is not protected by `shutdown_cs`, but by `cs`, at least in my intentions. It is illegal to read or write `state` while you're not holding `cs`. What `shutdown_cs` protects is rather the WG parser, and precisely it excludes the simultaneous call of `wg_parser_disconnect()` and `wg_parser_stream_get_buffer()` (which are racy). Then, as a little optimization, `shutdown_cs` is released before `wg_parser_disconnect()` is called, because at that point you already know that no thread will ever be able to call `wg_parser_stream_get_buffer()`, since:
* once `state` is set to `SOURCE_SHUTDOWN`, it is never changed any more;
* each time `shutdown_cs` is entered, `state` is always checked to be different from `SOURCE_SHUTDOWN`.
In a sense, you can say that the WG parser is protected by the condition "`shutdown_cs` is loked OR `state == SOURCE_SHUTDOWN`". The idea is that in this way you can immediately drop the locks in `Shutdown()` and let all the waiters return immediately, instead of waiting for the cleanup to finish.
However, I realize that a significant additional correctness burden for probably rather limited benefit, so I will try to push a variation in which `shutdown_cs` is more literally directly protecting access to the WG parser. The alternative would be to avoid `shutdown_cs` at all and modify the WG parser so that `wg_parser_disconnect()` and `wg_parser_stream_get_buffer()` are not racy. However that requires putting more locking on that side, and I am not sure it's a good idea (it is what my original patch would do, except that that was an incomplete implementation).
I don't really see how rwlocks would help with this problem.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/1278#note_17176