Re: [Qemu-devel] [RFC v2 10/11] tcg: drop global lock during TCG code execution

[Alex Bennée]

Paolo Bonzini <address@hidden> writes:

> On 24/05/2016 23:28, Sergey Fedorov wrote:
>> On 05/04/16 18:32, Alex Bennée wrote:
>>> diff --git a/cpu-exec.c b/cpu-exec.c
>>> index bd50fef..f558508 100644
>>> --- a/cpu-exec.c
>>> +++ b/cpu-exec.c
>>> @@ -28,6 +28,7 @@
>>>  #include "qemu/rcu.h"
>>>  #include "exec/tb-hash.h"
>>>  #include "exec/log.h"
>>> +#include "qemu/main-loop.h"
>>>  #if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
>>>  #include "hw/i386/apic.h"
>>>  #endif
>>> @@ -452,6 +453,8 @@ int cpu_exec(CPUState *cpu)
>>>              for(;;) {
>>>                  interrupt_request = cpu->interrupt_request;
>>>                  if (unlikely(interrupt_request)) {
>>> +                    qemu_mutex_lock_iothread();
>>> +
>>>                      if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) {
>>>                          /* Mask out external interrupts for this step. */
>>>                          interrupt_request &= ~CPU_INTERRUPT_SSTEP_MASK;
>>> @@ -504,6 +507,11 @@ int cpu_exec(CPUState *cpu)
>>>                             the program flow was changed */
>>>                          next_tb = 0;
>>>                      }
>>> +
>>> +                    if (qemu_mutex_iothread_locked()) {
>>> +                        qemu_mutex_unlock_iothread();
>>> +                    }
>>> +
>>
>> Why do we need to check for "qemu_mutex_iothread_locked()" here?
>> iothread lock is always held here, isn't it?
>
> Correct.

I'll fix that for the next iteration. The main else leg still needs to
check though as it doesn't know if the mutex was grabbed before a
siglongjmp.


>
>>>                  }
>>>                  if (unlikely(cpu->exit_request
>>>                               || replay_has_interrupt())) {
>> (snip)
>>> diff --git a/cputlb.c b/cputlb.c
>>> index 466663b..1412049 100644
>>> --- a/cputlb.c
>>> +++ b/cputlb.c
>>> @@ -18,6 +18,7 @@
>>>   */
>>>
>>>  #include "qemu/osdep.h"
>>> +#include "qemu/main-loop.h"
>>
>> No need in this include.
>>
>>>  #include "cpu.h"
>>>  #include "exec/exec-all.h"
>>>  #include "exec/memory.h"
>>> diff --git a/exec.c b/exec.c
>>> index c46c123..9e83673 100644
>>> --- a/exec.c
>>> +++ b/exec.c
>> (snip)
>>> @@ -2347,8 +2353,14 @@ static void io_mem_init(void)
>>>      memory_region_init_io(&io_mem_rom, NULL, &unassigned_mem_ops, NULL, 
>>> NULL, UINT64_MAX);
>>>      memory_region_init_io(&io_mem_unassigned, NULL, &unassigned_mem_ops, 
>>> NULL,
>>>                            NULL, UINT64_MAX);
>>> +
>>> +    /* io_mem_notdirty calls tb_invalidate_phys_page_fast,
>>> +     * which must be called without the iothread mutex.
>>> +     */
>>
>> notdirty_mem_write() operates on page dirty flags. Is it safe to do so
>> out of iothread lock?
>
> Yes, see commit 5f2cb94 ("memory: make
> cpu_physical_memory_sync_dirty_bitmap() fully atomic", 2014-12-02) and
> those before.
>
>>>      memory_region_init_io(&io_mem_notdirty, NULL, &notdirty_mem_ops, NULL,
>>>                            NULL, UINT64_MAX);
>>> +    memory_region_clear_global_locking(&io_mem_notdirty);
>>> +
>>>      memory_region_init_io(&io_mem_watch, NULL, &watch_mem_ops, NULL,
>>>                            NULL, UINT64_MAX);
>>>  }
>> (snip)
>>> diff --git a/translate-all.c b/translate-all.c
>>> index 935d24c..0c377bb 100644
>>> --- a/translate-all.c
>>> +++ b/translate-all.c
>>> @@ -1318,6 +1318,7 @@ void tb_invalidate_phys_range(tb_page_addr_t start, 
>>> tb_page_addr_t end)
>>>   * this TB.
>>>   *
>>>   * Called with mmap_lock held for user-mode emulation
>>> + * If called from generated code, iothread mutex must not be held.
>>
>> What does that mean? If iothread mutex is not required by the function
>> then why mention it here at all?
>
> If this function can take the iothread mutex itself, this would cause a
> deadlock.  I'm not sure if it could though.
>
> Another possibility is that this function can longjmp out into cpu_exec,
> and then the iothread mutex would not be released.  I think this is more
> likely.

Any longjmp should:

            tb_lock_reset();
            if (qemu_mutex_iothread_locked()) {
                qemu_mutex_unlock_iothread();
            }

To reset the locks.

>
> Thanks,
>
> Paolo


--
Alex Bennée