On Mon, Aug 5, 2013 at 9:29 PM, Paolo Bonzini <pbonz...@redhat.com> wrote:
>
>> In kvm mode, vm_clock may be read outside BQL.
>
> Not just in KVM mode (we will be able to use dataplane with TCG sooner
> or later), actually.
>
Oh. But this patch does not fix cpu_get_icount()'s thread-safe issue.
So currently, could I just change the commit log instead of fixing it?
Regards,
Pingfan
> Otherwise looks good!
>
> Paolo
>
>> 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. As for cpu_ticks in timers_state, it
>> is still protected by BQL.
>>
>> Lock rule: private lock innermost, ie BQL->"this lock"
>>
>> Signed-off-by: Liu Ping Fan <pingf...@linux.vnet.ibm.com>
>> ---
>> cpus.c | 36 +++++++++++++++++++++++++++++-------
>> 1 file changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 85e743d..ab92db9 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -107,12 +107,17 @@ static int64_t qemu_icount;
>> typedef struct TimersState {
>> int64_t cpu_ticks_prev;
>> int64_t cpu_ticks_offset;
>> + /* QemuClock will be read out of BQL, so protect is with private lock.
>> + * As for cpu_ticks, no requirement to read it outside BQL.
>> + * Lock rule: innermost
>> + */
>> + QemuSeqLock clock_seqlock;
>> int64_t cpu_clock_offset;
>> int32_t cpu_ticks_enabled;
>> int64_t dummy;
>> } TimersState;
>>
>> -TimersState timers_state;
>> +static TimersState timers_state;
>>
>> /* Return the virtual CPU time, based on the instruction counter. */
>> int64_t cpu_get_icount(void)
>> @@ -132,6 +137,7 @@ int64_t cpu_get_icount(void)
>> }
>>
>> /* return the host CPU cycle counter and handle stop/restart */
>> +/* cpu_ticks is safely if holding BQL */
>> int64_t cpu_get_ticks(void)
>> {
>> if (use_icount) {
>> @@ -156,33 +162,46 @@ int64_t cpu_get_ticks(void)
>> int64_t cpu_get_clock(void)
>> {
>> int64_t ti;
>> - if (!timers_state.cpu_ticks_enabled) {
>> - return timers_state.cpu_clock_offset;
>> - } else {
>> - ti = get_clock();
>> - return ti + timers_state.cpu_clock_offset;
>> - }
>> + unsigned start;
>> +
>> + do {
>> + start = seqlock_read_begin(&timers_state.clock_seqlock);
>> + if (!timers_state.cpu_ticks_enabled) {
>> + ti = timers_state.cpu_clock_offset;
>> + } else {
>> + ti = get_clock();
>> + ti += timers_state.cpu_clock_offset;
>> + }
>> + } while (seqlock_read_check(&timers_state.clock_seqlock, start);
>> +
>> + return ti;
>> }
>>
>> /* enable cpu_get_ticks() */
>> void cpu_enable_ticks(void)
>> {
>> + /* Here, the really thing protected by seqlock is cpu_clock. */
>> + seqlock_write_lock(&timers_state.clock_seqlock);
>> 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;
>> }
>> + seqlock_write_unlock(&timers_state.clock_seqlock);
>> }
>>
>> /* disable cpu_get_ticks() : the clock is stopped. You must not call
>> cpu_get_ticks() after that. */
>> void cpu_disable_ticks(void)
>> {
>> + /* Here, the really thing protected by seqlock is cpu_clock. */
>> + seqlock_write_lock(&timers_state.clock_seqlock);
>> 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;
>> }
>> + seqlock_write_unlock(&timers_state.clock_seqlock);
>> }
>>
>> /* Correlation between real and virtual time is always going to be
>> @@ -364,6 +383,9 @@ static const VMStateDescription vmstate_timers = {
>>
>> void configure_icount(const char *option)
>> {
>> + QemuMutex *mutex = g_malloc0(sizeof(QemuMutex));
>> + qemu_mutex_init(mutex);
>> + seqlock_init(&timers_state.clock_seqlock, mutex);
>> vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
>> if (!option) {
>> return;
>> --
>> 1.8.1.4
>>
>>
>
