| Message ID | 1495649128-10529-9-git-send-email-vyasevic@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, nother? > 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 ) { extra space
* 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. 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
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 >
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); + 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); }
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> --- include/migration/vmstate.h | 1 + migration/savevm.c | 41 +++++++++++++++++++++++++++++++---------- 2 files changed, 32 insertions(+), 10 deletions(-)