Re: [Qemu-devel] [PATCH v4 2/4] timer: protect timers_state's clock with seqlock

[liu ping fan]

On Mon, Sep 23, 2013 at 2:21 PM, Jan Kiszka <address@hidden> wrote:
> On 2013-09-22 10:11, Liu Ping Fan wrote:
>> QEMU_CLOCK_VIRTUAL may be read outside BQL. This will make its
>> foundation, i.e. timers_state exposed to race condition.
>> Using private lock to protect it.
>>
>> After this patch, reading QEMU_CLOCK_VIRTUAL is thread safe
>> unless use_icount is true, in which case the existing callers
>> still rely on the BQL
>>
>> Lock rule: private lock innermost, ie BQL->"this lock"
>>
>> Signed-off-by: Liu Ping Fan <address@hidden>
>> ---
>>  cpus.c | 36 ++++++++++++++++++++++++++++++------
>>  1 file changed, 30 insertions(+), 6 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index e566297..870a832 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -37,6 +37,7 @@
>>  #include "sysemu/qtest.h"
>>  #include "qemu/main-loop.h"
>>  #include "qemu/bitmap.h"
>> +#include "qemu/seqlock.h"
>>
>>  #ifndef _WIN32
>>  #include "qemu/compatfd.h"
>> @@ -112,6 +113,13 @@ static int64_t qemu_icount;
>>  typedef struct TimersState {
>>      int64_t cpu_ticks_prev;
>>      int64_t cpu_ticks_offset;
>> +    /* cpu_clock_offset will be read out of BQL, so protect it with private
>> +     * lock. As for cpu_ticks_*, no requirement to read it outside BQL yet.
>> +     * Lock rule: innermost
>> +     */
>> +    QemuSeqLock clock_seqlock;
>> +    /* mutex for seqlock */
>> +    QemuMutex mutex;
>
> If these locks only protect cpu_clock_offset, name them accordingly
> (cpu_clock_offset_seqlock, cpu_clock_offset_mutex). But I think they

The mutex is internal for seqlock, not exported outside. So I think
the name is fine. But, I think we can drop it since the
cpu_enable_ticks/cpu_disable_ticks are always inside BQL.  So I will
rename clock_seqlock as cpu_clock_offset.
> also protect cpu_ticks_enabled, no? Then you should adjust the comment.
>
No. cpu_get_tsc()->cpu_get_ticks() is one reader of cpu_ticks_enabled,
which is protected against writers by BQL, not by this lock.

Thanks and regards,
Pingfan
>>      int64_t cpu_clock_offset;
>>      int32_t cpu_ticks_enabled;
>>      int64_t dummy;
>> @@ -137,6 +145,7 @@ int64_t cpu_get_icount(void)
>>  }
>>
>>  /* return the host CPU cycle counter and handle stop/restart */
>> +/* cpu_ticks is safely if holding BQL */
>
> "Caller must hold the BQL."
>
>>  int64_t cpu_get_ticks(void)
>>  {
>>      if (use_icount) {
>> @@ -161,33 +170,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_retry(&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_offset. */
>> +    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_offset. */
>> +    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
>> @@ -371,6 +393,8 @@ static const VMStateDescription vmstate_timers = {
>>
>>  void configure_icount(const char *option)
>>  {
>> +    qemu_mutex_init(&timers_state.mutex);
>> +    seqlock_init(&timers_state.clock_seqlock, &timers_state.mutex);
>>      vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
>>      if (!option) {
>>          return;
>>
>
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SES-DE
> Corporate Competence Center Embedded Linux