Re: [Qemu-devel] [PATCH 08/12] announce_timer: Add ability to reset an existing

[Vlad Yasevich <vyasevic@redhat.com>]

On 05/30/2017 03:35 PM, Dr. David Alan Gilbert wrote:
> * Vladislav Yasevich (vyasevic@redhat.com) wrote:
>> It is now potentially possible to issue annouce-self command in a tight
>> loop.  Instead of doing nother, we can reset the timeout pararameters,
>> especially since each instance may provide it's own values.  This
>> allows the user to  extend or cut short currently runnig timer.
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> 
> ah ok, you can ignore my comment on the previous patch then!
> 
>> ---
>>  include/migration/vmstate.h |  1 +
>>  migration/savevm.c          | 41 +++++++++++++++++++++++++++++++----------
>>  2 files changed, 32 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> index 689b685..6dfdac3 100644
>> --- a/include/migration/vmstate.h
>> +++ b/include/migration/vmstate.h
>> @@ -1057,6 +1057,7 @@ void vmstate_register_ram_global(struct MemoryRegion *memory);
>>  
>>  typedef struct AnnounceTimer {
>>      QEMUTimer *tm;
>> +    QemuMutex active_lock;
>>      struct AnnounceTimer **entry;
>>      AnnounceParameters params;
>>      QEMUClockType type;
>> diff --git a/migration/savevm.c b/migration/savevm.c
>> index dcba8bd..e43658f 100644
>> --- a/migration/savevm.c
>> +++ b/migration/savevm.c
>> @@ -220,20 +220,29 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque)
>>  
>>  AnnounceTimer *announce_timers[QEMU_ANNOUNCE__MAX];
>>  
>> +static void qemu_announce_timer_destroy(AnnounceTimer *timer)
>> +{
>> +    timer_del(timer->tm);
>> +    timer_free(timer->tm);
>> +    qemu_mutex_destroy(&timer->active_lock);
> 
> Can you explain what makes this safe; we're not
> holding the lock at this point are we? Are we guaranteed
> that no one else will try and take it?
> Either way it should be commented to say why it's safe.

Looking at this again, it doesn't look safe.
The problem is the lookup code.  There is needs to be a lock on the
on the global array that needs to be held during creation/modification.

Thanks
-vlad

> 
> Dave
> 
>> +    g_free(timer);
>> +}
>> +
>>  static void qemu_announce_self_once(void *opaque)
>>  {
>>      AnnounceTimer *timer = (AnnounceTimer *)opaque;
>>  
>> +    qemu_mutex_lock(&timer->active_lock);
>>      qemu_foreach_nic(qemu_announce_self_iter, NULL);
>>  
>> -    if (--timer->round) {
>> +    if (--timer->round ) {
>>          timer_mod(timer->tm, qemu_clock_get_ms(timer->type) +
>>                    self_announce_delay(timer));
>> +        qemu_mutex_unlock(&timer->active_lock);
>>      } else {
>> -            *(timer->entry) = NULL;
>> -            timer_del(timer->tm);
>> -            timer_free(timer->tm);
>> -            g_free(timer);
>> +        *(timer->entry) = NULL;
>> +        qemu_mutex_unlock(&timer->active_lock);
>> +        qemu_announce_timer_destroy(timer);
>>      }
>>  }
>>  
>> @@ -242,6 +251,7 @@ AnnounceTimer *qemu_announce_timer_new(AnnounceParameters *params,
>>  {
>>      AnnounceTimer *timer = g_new(AnnounceTimer, 1);
>>  
>> +    qemu_mutex_init(&timer->active_lock);
>>      timer->params = *params;
>>      timer->round = params->rounds;
>>      timer->type = type;
>> @@ -259,6 +269,21 @@ AnnounceTimer *qemu_announce_timer_create(AnnounceParameters *params,
>>      return timer;
>>  }
>>  
>> +static void qemu_announce_timer_update(AnnounceTimer *timer,
>> +                                       AnnounceParameters *params)
>> +{
>> +    qemu_mutex_lock(&timer->active_lock);
>> +
>> +    /* Update timer paramenter with any new values.
>> +     * Reset the number of rounds to run, and stop the current timer.
>> +     */
>> +    timer->params = *params;
>> +    timer->round = params->rounds;
>> +    timer_del(timer->tm);
>> +
>> +    qemu_mutex_unlock(&timer->active_lock);
>> +}
>> +
>>  void qemu_announce_self(AnnounceParameters *params, AnnounceType type)
>>  {
>>      AnnounceTimer *timer;
>> @@ -270,11 +295,7 @@ void qemu_announce_self(AnnounceParameters *params, AnnounceType type)
>>          announce_timers[type] = timer;
>>          timer->entry = &announce_timers[type];
>>      } else {
>> -        /* For now, don't do anything.  If we want to reset the timer,
>> -         * we'll need to add locking to each announce timer to prevent
>> -         * races between timeout handling and a reset.
>> -         */
>> -        return;
>> +        qemu_announce_timer_update(timer, params);
>>      }
>>      qemu_announce_self_once(timer);
>>  }
>> -- 
>> 2.7.4
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>