[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] CPUArchState: introduce per-cpu lock
From: |
Andreas Färber |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] CPUArchState: introduce per-cpu lock |
Date: |
Mon, 25 Jun 2012 12:16:16 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120421 Thunderbird/12.0 |
Am 24.06.2012 16:02, schrieb liu ping fan:
> On Fri, Jun 22, 2012 at 8:55 PM, Andreas Färber <address@hidden> wrote:
>> Am 21.06.2012 17:06, schrieb Liu Ping Fan:
>>> introduce a lock for per-cpu to protect agaist accesing from
>>> other vcpu thread.
>>>
>>> Signed-off-by: Liu Ping Fan <address@hidden>
>>> ---
>>> cpu-defs.h | 2 ++
>>> cpus.c | 17 +++++++++++++++++
>>> main-loop.h | 3 +++
>>> 3 files changed, 22 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/cpu-defs.h b/cpu-defs.h
>>> index f49e950..7305822 100644
>>> --- a/cpu-defs.h
>>> +++ b/cpu-defs.h
>>> @@ -30,6 +30,7 @@
>>> #include "osdep.h"
>>> #include "qemu-queue.h"
>>> #include "targphys.h"
>>> +#include "qemu-thread-posix.h"
>>>
>>> #ifndef TARGET_LONG_BITS
>>> #error TARGET_LONG_BITS must be defined before including this header
>>> @@ -220,6 +221,7 @@ typedef struct CPUWatchpoint {
>>> CPU_COMMON_THREAD \
>>> struct QemuCond *halt_cond; \
>>> int thread_kicked; \
>>> + struct QemuMutex *cpu_lock; \
>>> struct qemu_work_item *queued_work_first, *queued_work_last; \
>>> const char *cpu_model_str; \
>>> struct KVMState *kvm_state; \
>>
>> Please don't add stuff to CPU_COMMON. Instead add to CPUState in
>> qom/cpu.c. The QOM CPUState part 4 series moves many of those fields there.
My mistake: The struct to add to is in include/qemu/cpu.h obviously.
>>
> Ok, thanks.
>>> diff --git a/cpus.c b/cpus.c
>>> index b182b3d..554f7bc 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -735,6 +735,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>>> env->thread_id = qemu_get_thread_id();
>>> cpu_single_env = env;
>>>
>>> +
>>
>> Stray whitespace addition.
>>
>>> r = kvm_init_vcpu(env);
>>> if (r < 0) {
>>> fprintf(stderr, "kvm_init_vcpu failed: %s\n", strerror(-r));
>>> @@ -891,6 +892,20 @@ int qemu_cpu_is_self(void *_env)
>>> return qemu_thread_is_self(env->thread);
>>> }
>>>
>>> +void qemu_mutex_lock_cpu(void *_env)
>>> +{
>>> + CPUArchState *env = _env;
>>> +
>>> + qemu_mutex_lock(env->cpu_lock);
>>> +}
>>> +
>>> +void qemu_mutex_unlock_cpu(void *_env)
>>> +{
>>> + CPUArchState *env = _env;
>>> +
>>> + qemu_mutex_unlock(env->cpu_lock);
>>> +}
>>> +
>>
>> I don't like these helpers. For one, you are using void * arguments and
>> casting them, for another you are using CPUArchState at all. With my
>> suggestion above these can be CPUState *cpu.
>>
> For using it in apic.c, we need to hide the CPUArchState structure
CPUState serves to hide CPUArchState, with added type safety. :)
Regards,
Andreas
>>> void qemu_mutex_lock_iothread(void)
>>> {
>>> if (!tcg_enabled()) {
>>> @@ -1027,6 +1042,8 @@ void qemu_init_vcpu(void *_env)
>>> env->nr_cores = smp_cores;
>>> env->nr_threads = smp_threads;
>>> env->stopped = 1;
>>> + env->cpu_lock = g_malloc0(sizeof(QemuMutex));
>>> + qemu_mutex_init(env->cpu_lock);
>>
>> Are you sure this is not needed for linux-user/bsd-user? If not needed,
>> then the field should be #ifdef'ed in the struct to assure that.
>> Otherwise this function is never called and you need to move the
>> initialization to the initfn in qom/cpu.c and then should also clean it
>> up in a finalizer.
>>
> It is not needed for linux-user/bsd-user. I will use the macro,
>
> Thanks and regards,
> pingfan
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
[Qemu-devel] [PATCH 2/2] kvm: use per-cpu lock to free vcpu thread out of the big lock, Liu Ping Fan, 2012/06/21
- Re: [Qemu-devel] [RFC] use little granularity lock to substitue qemu_mutex_lock_iothread, Jan Kiszka, 2012/06/21
- Re: [Qemu-devel] [RFC] use little granularity lock to substitue qemu_mutex_lock_iothread, liu ping fan, 2012/06/22
- Re: [Qemu-devel] [RFC] use little granularity lock to substitue qemu_mutex_lock_iothread, Jan Kiszka, 2012/06/22
- Re: [Qemu-devel] [RFC] use little granularity lock to substitue qemu_mutex_lock_iothread, Anthony Liguori, 2012/06/22
- Re: [Qemu-devel] [RFC] use little granularity lock to substitue qemu_mutex_lock_iothread, Jan Kiszka, 2012/06/22
- Re: [Qemu-devel] [RFC] use little granularity lock to substitue qemu_mutex_lock_iothread, Anthony Liguori, 2012/06/22
- Re: [Qemu-devel] [RFC] use little granularity lock to substitue qemu_mutex_lock_iothread, Jan Kiszka, 2012/06/22