Re: [PATCH v2 7/9] cpus: Introduce qemu_mutex_unlock_iothread_prepare()

[David Hildenbrand <david@redhat.com>]

On 27.07.21 18:08, Peter Xu wrote:
> On Tue, Jul 27, 2021 at 02:59:26PM +0200, David Hildenbrand wrote:
>> On 23.07.21 21:34, Peter Xu wrote:
>>> The prepare function before unlocking BQL.  There're only three places that can
>>> release the BQL: unlock(), cond_wait() or cond_timedwait().
>>>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>    softmmu/cpus.c | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
>>> index 9131f77f87..6085f8edbe 100644
>>> --- a/softmmu/cpus.c
>>> +++ b/softmmu/cpus.c
>>> @@ -66,6 +66,10 @@
>>>    static QemuMutex qemu_global_mutex;
>>> +static void qemu_mutex_unlock_iothread_prepare(void)
>>> +{
>>> +}
>>> +
>>>    bool cpu_is_stopped(CPUState *cpu)
>>>    {
>>>        return cpu->stopped || !runstate_is_running();
>>> @@ -523,16 +527,19 @@ void qemu_mutex_unlock_iothread(void)
>>>    {
>>>        g_assert(qemu_mutex_iothread_locked());
>>>        iothread_locked = false;
>>> +    qemu_mutex_unlock_iothread_prepare();
>>>        qemu_mutex_unlock(&qemu_global_mutex);
>>>    }
>>>    void qemu_cond_wait_iothread(QemuCond *cond)
>>>    {
>>> +    qemu_mutex_unlock_iothread_prepare();
>>>        qemu_cond_wait(cond, &qemu_global_mutex);
>>>    }
>>>    void qemu_cond_timedwait_iothread(QemuCond *cond, int ms)
>>>    {
>>> +    qemu_mutex_unlock_iothread_prepare();
>>>        qemu_cond_timedwait(cond, &qemu_global_mutex, ms);
>>>    }
>>>
>>
>> I'd squash this patch into the next one.
>>
>> I don't quite like the function name, but don't really have a better
>> suggestion .... maybe qemu_mutex_might_unlock_iothread(), similar to
>> might_sleep() or might_fault() in the kernel. (although here it's pretty
>> clear and not a "might"; could be useful in other context where we might
>> conditionally unlock the BQL at some point in the future, though)
> 
> Yes, IMHO "might" describes a capability of doing something, here it's not
> (this one should only be called right before releasing bql, not within any
> context of having some capability).  The other option I thought was "pre" but
> it will be just a short version of "prepare".
> 
> Let me know if you have a better suggestion on naming. :) Otherwise I'll keep
> the naming, squash this patch into the next and keep your r-b for that.

Fine with me :)


-- 
Thanks,

David / dhildenb