On Donnerstag, 20. August 2020 12:54:49 CEST Paolo Bonzini wrote: > More on the practical side, recursive mutex are an easy way to get a > deadlock. It's a common idiom to do > > /* Need to take foo->lock outside bar->lock. */ > mutex_unlock(&bar->lock); > mutex_lock(&foo->lock); > mutex_lock(&bar->lock);
The general theoretical implications about recursive locks was clear to me. AFAICS your point is that a recursive lock could mislead poeple taking things easy and running into a deadlock scenario like outlined by you. My point was if it happens for whatever reason that a main IO mutex lock was accidentally introduced, i.e. without knowing it was already locked on a higher level, wouldn't it make sense to deal with this in some kind of defensive way? One way would be a recursive type and logging a warning, which you obviously don't like; another option would be an assertion fault instead to make developers immediately aware about the double lock on early testing. Because on a large scale project like this, it is almost impossible for all developers to be aware about all implied locks. Don't you think so? At least IMO the worst case would be a double unlock on a non-recursive main thread mutex and running silently into undefined behaviour. > My suggestion is to work towards protecting the audio code with its own > mutex(es) and ignore the existence of the BQL for subsystems that can do > so (audio is a prime candidate). Also please add comments to > audio_int.h about which functions are called from other threads than the > QEMU main thread. That main thread lock came up here because I noticed this API comment on qemu_bh_cancel(): "While cancellation itself is also wait-free and thread-safe, it can of course race with the loop that executes bottom halves unless you are holding the iothread mutex. This makes it mostly useless if you are not holding the mutex." So this lock was not about driver internal data protection, but rather about dealing with the BH API correctly. Best regards, Christian Schoenebeck