On 20/08/20 14:00, Christian Schoenebeck wrote: > 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.
Yes, more assertions are always fine. We were using errorcheck mutexes until a few years ago, unfortunately we couldn't because they are broken with respect to fork (commit 24fa90499, "qemu-thread: do not use PTHREAD_MUTEX_ERRORCHECK", 2015-03-05). > 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. Yes, on the other hand that is not a problem if the BH is idempotent. For example something like this is okay: bh_body_locked() { free(foo); foo = NULL; } bh_body() { qemu_mutex_lock(&foo_lock); bh_body_locked(); qemu_mutex_unlock(&foo_lock); } ... qemu_mutex_lock(&foo_lock); qemu_bh_delete(foo_bh); // also calls qemu_bh_cancel bh_body_locked(); qemu_mutex_unlock(&foo_lock); Paolo