Patchwork [11/15] lock: introduce global lock for device tree

login
register
mail settings
Submitter pingfan liu
Date Aug. 8, 2012, 6:25 a.m.
Message ID <1344407156-25562-12-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/175862/
State New
Headers show

Comments

pingfan liu - Aug. 8, 2012, 6:25 a.m.
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 cpus.c      |   12 ++++++++++++
 main-loop.h |    3 +++
 2 files changed, 15 insertions(+), 0 deletions(-)
Paolo Bonzini - Aug. 8, 2012, 9:41 a.m.
Il 08/08/2012 08:25, Liu Ping Fan ha scritto:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  cpus.c      |   12 ++++++++++++
>  main-loop.h |    3 +++
>  2 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index b182b3d..a734b36 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -611,6 +611,7 @@ static void qemu_tcg_init_cpu_signals(void)
>  }
>  #endif /* _WIN32 */
>  
> +QemuMutex qemu_device_tree_mutex;
>  QemuMutex qemu_global_mutex;
>  static QemuCond qemu_io_proceeded_cond;
>  static bool iothread_requesting_mutex;
> @@ -634,6 +635,7 @@ void qemu_init_cpu_loop(void)
>      qemu_cond_init(&qemu_work_cond);
>      qemu_cond_init(&qemu_io_proceeded_cond);
>      qemu_mutex_init(&qemu_global_mutex);
> +    qemu_mutex_init(&qemu_device_tree_mutex);
>  
>      qemu_thread_get_self(&io_thread);
>  }
> @@ -911,6 +913,16 @@ void qemu_mutex_unlock_iothread(void)
>      qemu_mutex_unlock(&qemu_global_mutex);
>  }
>  
> +void qemu_lock_devtree(void)
> +{
> +    qemu_mutex_lock(&qemu_device_tree_mutex);
> +}
> +
> +void qemu_unlock_devtree(void)
> +{
> +    qemu_mutex_unlock(&qemu_device_tree_mutex);
> +}

We don't need the wrappers.  They are there for the big lock just
because TCG needs extra work for iothread_requesting_mutex.

Paolo

>  static int all_vcpus_paused(void)
>  {
>      CPUArchState *penv = first_cpu;
> diff --git a/main-loop.h b/main-loop.h
> index dce1cd9..17e959a 100644
> --- a/main-loop.h
> +++ b/main-loop.h
> @@ -353,6 +353,9 @@ void qemu_mutex_lock_iothread(void);
>   */
>  void qemu_mutex_unlock_iothread(void);
>  
> +void qemu_lock_devtree(void);
> +void qemu_unlock_devtree(void);
> +
>  /* internal interfaces */
>  
>  void qemu_fd_register(int fd);
>
Avi Kivity - Aug. 8, 2012, 9:42 a.m.
On 08/08/2012 09:25 AM, Liu Ping Fan wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 

Please explain the motivation.  AFAICT, the big qemu lock is sufficient.
pingfan liu - Aug. 9, 2012, 7:27 a.m.
On Wed, Aug 8, 2012 at 5:42 PM, Avi Kivity <avi@redhat.com> wrote:
> On 08/08/2012 09:25 AM, Liu Ping Fan wrote:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>
> Please explain the motivation.  AFAICT, the big qemu lock is sufficient.
>
Oh, this is one of the series locks for the removal of big qemu lock.
The degradation of big lock will take several steps, including to
introduce device's private lock. Till then, when the device add path
from iothread and the remove path in io-dispatch is out of the big
qemu lock.  We need this extra lock.

These series is too big, so I send out the 1st phase for review.

Regards,
pingan
>
> --
> error compiling committee.c: too many arguments to function
pingfan liu - Aug. 9, 2012, 7:28 a.m.
On Wed, Aug 8, 2012 at 5:41 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 08/08/2012 08:25, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  cpus.c      |   12 ++++++++++++
>>  main-loop.h |    3 +++
>>  2 files changed, 15 insertions(+), 0 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index b182b3d..a734b36 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -611,6 +611,7 @@ static void qemu_tcg_init_cpu_signals(void)
>>  }
>>  #endif /* _WIN32 */
>>
>> +QemuMutex qemu_device_tree_mutex;
>>  QemuMutex qemu_global_mutex;
>>  static QemuCond qemu_io_proceeded_cond;
>>  static bool iothread_requesting_mutex;
>> @@ -634,6 +635,7 @@ void qemu_init_cpu_loop(void)
>>      qemu_cond_init(&qemu_work_cond);
>>      qemu_cond_init(&qemu_io_proceeded_cond);
>>      qemu_mutex_init(&qemu_global_mutex);
>> +    qemu_mutex_init(&qemu_device_tree_mutex);
>>
>>      qemu_thread_get_self(&io_thread);
>>  }
>> @@ -911,6 +913,16 @@ void qemu_mutex_unlock_iothread(void)
>>      qemu_mutex_unlock(&qemu_global_mutex);
>>  }
>>
>> +void qemu_lock_devtree(void)
>> +{
>> +    qemu_mutex_lock(&qemu_device_tree_mutex);
>> +}
>> +
>> +void qemu_unlock_devtree(void)
>> +{
>> +    qemu_mutex_unlock(&qemu_device_tree_mutex);
>> +}
>
> We don't need the wrappers.  They are there for the big lock just
> because TCG needs extra work for iothread_requesting_mutex.
>
Sorry, could you give more detail about TCG, what is extra work.

Thanks,
pingfan

> Paolo
>
>>  static int all_vcpus_paused(void)
>>  {
>>      CPUArchState *penv = first_cpu;
>> diff --git a/main-loop.h b/main-loop.h
>> index dce1cd9..17e959a 100644
>> --- a/main-loop.h
>> +++ b/main-loop.h
>> @@ -353,6 +353,9 @@ void qemu_mutex_lock_iothread(void);
>>   */
>>  void qemu_mutex_unlock_iothread(void);
>>
>> +void qemu_lock_devtree(void);
>> +void qemu_unlock_devtree(void);
>> +
>>  /* internal interfaces */
>>
>>  void qemu_fd_register(int fd);
>>
>
>
Paolo Bonzini - Aug. 9, 2012, 7:41 a.m.
Il 09/08/2012 09:28, liu ping fan ha scritto:
> On Wed, Aug 8, 2012 at 5:41 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 08/08/2012 08:25, Liu Ping Fan ha scritto:
>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>
>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>> ---
>>>  cpus.c      |   12 ++++++++++++
>>>  main-loop.h |    3 +++
>>>  2 files changed, 15 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index b182b3d..a734b36 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -611,6 +611,7 @@ static void qemu_tcg_init_cpu_signals(void)
>>>  }
>>>  #endif /* _WIN32 */
>>>
>>> +QemuMutex qemu_device_tree_mutex;
>>>  QemuMutex qemu_global_mutex;
>>>  static QemuCond qemu_io_proceeded_cond;
>>>  static bool iothread_requesting_mutex;
>>> @@ -634,6 +635,7 @@ void qemu_init_cpu_loop(void)
>>>      qemu_cond_init(&qemu_work_cond);
>>>      qemu_cond_init(&qemu_io_proceeded_cond);
>>>      qemu_mutex_init(&qemu_global_mutex);
>>> +    qemu_mutex_init(&qemu_device_tree_mutex);
>>>
>>>      qemu_thread_get_self(&io_thread);
>>>  }
>>> @@ -911,6 +913,16 @@ void qemu_mutex_unlock_iothread(void)
>>>      qemu_mutex_unlock(&qemu_global_mutex);
>>>  }
>>>
>>> +void qemu_lock_devtree(void)
>>> +{
>>> +    qemu_mutex_lock(&qemu_device_tree_mutex);
>>> +}
>>> +
>>> +void qemu_unlock_devtree(void)
>>> +{
>>> +    qemu_mutex_unlock(&qemu_device_tree_mutex);
>>> +}
>>
>> We don't need the wrappers.  They are there for the big lock just
>> because TCG needs extra work for iothread_requesting_mutex.
>>
> Sorry, could you give more detail about TCG, what is extra work.

void qemu_mutex_lock_iothread(void)
{
    if (!tcg_enabled()) {
        qemu_mutex_lock(&qemu_global_mutex);
    } else {
        iothread_requesting_mutex = true;
        if (qemu_mutex_trylock(&qemu_global_mutex)) {
            qemu_cpu_kick_thread(first_cpu);
            qemu_mutex_lock(&qemu_global_mutex);
        }
        iothread_requesting_mutex = false;
        qemu_cond_broadcast(&qemu_io_proceeded_cond);
    }
}

You do not need any of the code in the "else" branch for the device tree
mutex, so you do not need wrappers.
Avi Kivity - Aug. 9, 2012, 8:31 a.m.
On 08/09/2012 10:27 AM, liu ping fan wrote:
> On Wed, Aug 8, 2012 at 5:42 PM, Avi Kivity <avi@redhat.com> wrote:
>> On 08/08/2012 09:25 AM, Liu Ping Fan wrote:
>>> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>
>>
>> Please explain the motivation.  AFAICT, the big qemu lock is sufficient.
>>
> Oh, this is one of the series locks for the removal of big qemu lock.

Why do you want to remove the big qemu lock?

Even now it is not heavily contended.  We should focus on fixing the
cases where is it contended, instead of removing it completely, which is
sure to make further development harder and is likely to introduce
locking bugs.

> The degradation of big lock will take several steps, including to
> introduce device's private lock. Till then, when the device add path
> from iothread and the remove path in io-dispatch is out of the big
> qemu lock.  We need this extra lock.
> 
> These series is too big, so I send out the 1st phase for review.

Even the first phase is too big.

Patch

diff --git a/cpus.c b/cpus.c
index b182b3d..a734b36 100644
--- a/cpus.c
+++ b/cpus.c
@@ -611,6 +611,7 @@  static void qemu_tcg_init_cpu_signals(void)
 }
 #endif /* _WIN32 */
 
+QemuMutex qemu_device_tree_mutex;
 QemuMutex qemu_global_mutex;
 static QemuCond qemu_io_proceeded_cond;
 static bool iothread_requesting_mutex;
@@ -634,6 +635,7 @@  void qemu_init_cpu_loop(void)
     qemu_cond_init(&qemu_work_cond);
     qemu_cond_init(&qemu_io_proceeded_cond);
     qemu_mutex_init(&qemu_global_mutex);
+    qemu_mutex_init(&qemu_device_tree_mutex);
 
     qemu_thread_get_self(&io_thread);
 }
@@ -911,6 +913,16 @@  void qemu_mutex_unlock_iothread(void)
     qemu_mutex_unlock(&qemu_global_mutex);
 }
 
+void qemu_lock_devtree(void)
+{
+    qemu_mutex_lock(&qemu_device_tree_mutex);
+}
+
+void qemu_unlock_devtree(void)
+{
+    qemu_mutex_unlock(&qemu_device_tree_mutex);
+}
+
 static int all_vcpus_paused(void)
 {
     CPUArchState *penv = first_cpu;
diff --git a/main-loop.h b/main-loop.h
index dce1cd9..17e959a 100644
--- a/main-loop.h
+++ b/main-loop.h
@@ -353,6 +353,9 @@  void qemu_mutex_lock_iothread(void);
  */
 void qemu_mutex_unlock_iothread(void);
 
+void qemu_lock_devtree(void);
+void qemu_unlock_devtree(void);
+
 /* internal interfaces */
 
 void qemu_fd_register(int fd);