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

[liu ping fan]

On Tue, Aug 6, 2013 at 5:30 PM, Stefan Hajnoczi <address@hidden> wrote:
> On Mon, Aug 05, 2013 at 03:33:24PM +0800, Liu Ping Fan wrote:
>> 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
>> +     */
>
> Please document exactly which fields the lock protects.
>
The lock protects cpu_clock_offset. Will fix it.

>>  /* enable cpu_get_ticks() */
>>  void cpu_enable_ticks(void)
>>  {
>> +    /* Here, the really thing protected by seqlock is cpu_clock. */
>
> What is cpu_clock?
>
cpu_clock_offset
>> @@ -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);
>
> We always set up this mutex, so it could be a field in timers_state.
> That avoids the g_malloc() without g_free().
>
Will fix in next version

Thx&regards,
Pingfan