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

[Jan Kiszka <jan.kiszka@siemens.com>]

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 <pingfank@linux.vnet.ibm.com>
> ---
>  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
also protect cpu_ticks_enabled, no? Then you should adjust the comment.

>      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