Message ID | 1374396185-10870-5-git-send-email-pingfank@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On 2013-07-21 10:43, Liu Ping Fan wrote: > In kvm mode, vm_clock may be read on AioContexts outside BQL(next > patch). This will make timers_state --the foundation of vm_clock > exposed to race condition. Using private lock to protect it. > Note in tcg mode, vm_clock still read inside BQL, so icount is > left without change. > > Lock rule: private lock innermost, ie BQL->"this lock" > > Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> > --- > cpus.c | 27 +++++++++++++++++++++++---- > 1 file changed, 23 insertions(+), 4 deletions(-) > > diff --git a/cpus.c b/cpus.c > index 4254ca9..22df5fb 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -113,6 +113,8 @@ typedef struct TimersState { > } TimersState; > > static TimersState timers_state; > +/* lock rule: innermost */ > +static QemuMutex timers_state_lock; > > /* Return the virtual CPU time, based on the instruction counter. */ > int64_t cpu_get_icount(void) > @@ -134,11 +136,15 @@ int64_t cpu_get_icount(void) > /* return the host CPU cycle counter and handle stop/restart */ > int64_t cpu_get_ticks(void) > { > + int64_t ret; > + > if (use_icount) { [ Some day we should introduce something like assert_bql_held() and add it here, among other places. ] > return cpu_get_icount(); > } > + qemu_mutex_lock(&timers_state_lock); > if (!timers_state.cpu_ticks_enabled) { > - return timers_state.cpu_ticks_offset; > + ret = timers_state.cpu_ticks_offset; > + goto out; No need for goto here and below. > } else { > int64_t ticks; > ticks = cpu_get_real_ticks(); > @@ -148,41 +154,53 @@ int64_t cpu_get_ticks(void) > timers_state.cpu_ticks_offset += timers_state.cpu_ticks_prev - ticks; > } > timers_state.cpu_ticks_prev = ticks; > - return ticks + timers_state.cpu_ticks_offset; > + ret = ticks + timers_state.cpu_ticks_offset; > + goto out; > } > +out: > + qemu_mutex_lock(&timers_state_lock); > + return ret; > } > > /* return the host CPU monotonic timer and handle stop/restart */ > int64_t cpu_get_clock(void) > { > int64_t ti; > + > + qemu_mutex_lock(&timers_state_lock); > if (!timers_state.cpu_ticks_enabled) { > - return timers_state.cpu_clock_offset; > + ti = timers_state.cpu_clock_offset; > } else { > ti = get_clock(); > - return ti + timers_state.cpu_clock_offset; > + ti += timers_state.cpu_clock_offset; > } > + qemu_mutex_unlock(&timers_state_lock); > + return ti; > } > > /* enable cpu_get_ticks() */ > void cpu_enable_ticks(void) > { > + qemu_mutex_lock(&timers_state_lock); > if (!timers_state.cpu_ticks_enabled) { > timers_state.cpu_ticks_offset -= cpu_get_real_ticks(); > timers_state.cpu_clock_offset -= get_clock(); > timers_state.cpu_ticks_enabled = 1; > } > + qemu_mutex_unlock(&timers_state_lock); > } > > /* disable cpu_get_ticks() : the clock is stopped. You must not call > cpu_get_ticks() after that. */ > void cpu_disable_ticks(void) > { > + qemu_mutex_lock(&timers_state_lock); > if (timers_state.cpu_ticks_enabled) { > timers_state.cpu_ticks_offset = cpu_get_ticks(); > timers_state.cpu_clock_offset = cpu_get_clock(); > timers_state.cpu_ticks_enabled = 0; > } > + qemu_mutex_unlock(&timers_state_lock); > } > > /* Correlation between real and virtual time is always going to be > @@ -353,6 +371,7 @@ static const VMStateDescription vmstate_timers = { > > void configure_icount(const char *option) [ Misnamed function, it's not only about icount stuff - but not an issue of this patch. ] > { > + qemu_mutex_init(&timers_state_lock); > vmstate_register(NULL, 0, &vmstate_timers, &timers_state); > if (!option) { > return; > Looks good except for the goto. Jan
diff --git a/cpus.c b/cpus.c index 4254ca9..22df5fb 100644 --- a/cpus.c +++ b/cpus.c @@ -113,6 +113,8 @@ typedef struct TimersState { } TimersState; static TimersState timers_state; +/* lock rule: innermost */ +static QemuMutex timers_state_lock; /* Return the virtual CPU time, based on the instruction counter. */ int64_t cpu_get_icount(void) @@ -134,11 +136,15 @@ int64_t cpu_get_icount(void) /* return the host CPU cycle counter and handle stop/restart */ int64_t cpu_get_ticks(void) { + int64_t ret; + if (use_icount) { return cpu_get_icount(); } + qemu_mutex_lock(&timers_state_lock); if (!timers_state.cpu_ticks_enabled) { - return timers_state.cpu_ticks_offset; + ret = timers_state.cpu_ticks_offset; + goto out; } else { int64_t ticks; ticks = cpu_get_real_ticks(); @@ -148,41 +154,53 @@ int64_t cpu_get_ticks(void) timers_state.cpu_ticks_offset += timers_state.cpu_ticks_prev - ticks; } timers_state.cpu_ticks_prev = ticks; - return ticks + timers_state.cpu_ticks_offset; + ret = ticks + timers_state.cpu_ticks_offset; + goto out; } +out: + qemu_mutex_lock(&timers_state_lock); + return ret; } /* return the host CPU monotonic timer and handle stop/restart */ int64_t cpu_get_clock(void) { int64_t ti; + + qemu_mutex_lock(&timers_state_lock); if (!timers_state.cpu_ticks_enabled) { - return timers_state.cpu_clock_offset; + ti = timers_state.cpu_clock_offset; } else { ti = get_clock(); - return ti + timers_state.cpu_clock_offset; + ti += timers_state.cpu_clock_offset; } + qemu_mutex_unlock(&timers_state_lock); + return ti; } /* enable cpu_get_ticks() */ void cpu_enable_ticks(void) { + qemu_mutex_lock(&timers_state_lock); if (!timers_state.cpu_ticks_enabled) { timers_state.cpu_ticks_offset -= cpu_get_real_ticks(); timers_state.cpu_clock_offset -= get_clock(); timers_state.cpu_ticks_enabled = 1; } + qemu_mutex_unlock(&timers_state_lock); } /* disable cpu_get_ticks() : the clock is stopped. You must not call cpu_get_ticks() after that. */ void cpu_disable_ticks(void) { + qemu_mutex_lock(&timers_state_lock); if (timers_state.cpu_ticks_enabled) { timers_state.cpu_ticks_offset = cpu_get_ticks(); timers_state.cpu_clock_offset = cpu_get_clock(); timers_state.cpu_ticks_enabled = 0; } + qemu_mutex_unlock(&timers_state_lock); } /* Correlation between real and virtual time is always going to be @@ -353,6 +371,7 @@ static const VMStateDescription vmstate_timers = { void configure_icount(const char *option) { + qemu_mutex_init(&timers_state_lock); vmstate_register(NULL, 0, &vmstate_timers, &timers_state); if (!option) { return;
In kvm mode, vm_clock may be read on AioContexts outside BQL(next patch). This will make timers_state --the foundation of vm_clock exposed to race condition. Using private lock to protect it. Note in tcg mode, vm_clock still read inside BQL, so icount is left without change. Lock rule: private lock innermost, ie BQL->"this lock" Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com> --- cpus.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)