On 05/30/2017 03:19 PM, Dr. David Alan Gilbert wrote: > * Vladislav Yasevich (vyase...@redhat.com) wrote: >> Switch qemu_announce_self and virtio annoucements to use >> the announcement timer framework. This makes sure that both >> timers use the same timeouts and number of annoucement attempts >> >> Based on work by Dr. David Alan Gilbert <dgilb...@redhat.com> >> >> Signed-off-by: Vladislav Yasevich <vyase...@redhat.com> >> --- >> hw/net/virtio-net.c | 28 ++++++++++++++++------------ >> include/hw/virtio/virtio-net.h | 3 +-- >> include/migration/vmstate.h | 17 +++++++++++------ >> include/sysemu/sysemu.h | 2 +- >> migration/migration.c | 2 +- >> migration/savevm.c | 28 ++++++++++++++-------------- >> 6 files changed, 44 insertions(+), 36 deletions(-) >> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >> index 7d091c9..1c65825 100644 >> --- a/hw/net/virtio-net.c >> +++ b/hw/net/virtio-net.c >> @@ -25,6 +25,7 @@ >> #include "qapi/qmp/qjson.h" >> #include "qapi-event.h" >> #include "hw/virtio/virtio-access.h" >> +#include "migration/migration.h" >> >> #define VIRTIO_NET_VM_VERSION 11 >> >> @@ -115,7 +116,7 @@ static void virtio_net_announce_timer(void *opaque) >> VirtIONet *n = opaque; >> VirtIODevice *vdev = VIRTIO_DEVICE(n); >> >> - n->announce_counter--; >> + n->announce_timer->round--; >> n->status |= VIRTIO_NET_S_ANNOUNCE; >> virtio_notify_config(vdev); >> } >> @@ -427,8 +428,8 @@ static void virtio_net_reset(VirtIODevice *vdev) >> n->nobcast = 0; >> /* multiqueue is disabled by default */ >> n->curr_queues = 1; >> - timer_del(n->announce_timer); >> - n->announce_counter = 0; >> + timer_del(n->announce_timer->tm); >> + n->announce_timer->round = 0; >> n->status &= ~VIRTIO_NET_S_ANNOUNCE; >> >> /* Flush any MAC and VLAN filter table state */ >> @@ -872,10 +873,10 @@ static int virtio_net_handle_announce(VirtIONet *n, >> uint8_t cmd, >> if (cmd == VIRTIO_NET_CTRL_ANNOUNCE_ACK && >> n->status & VIRTIO_NET_S_ANNOUNCE) { >> n->status &= ~VIRTIO_NET_S_ANNOUNCE; >> - if (n->announce_counter) { >> - timer_mod(n->announce_timer, >> + if (n->announce_timer->round) { >> + timer_mod(n->announce_timer->tm, >> qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + >> - self_announce_delay(n->announce_counter)); >> + self_announce_delay(n->announce_timer)); >> } >> return VIRTIO_NET_OK; >> } else { >> @@ -1615,8 +1616,8 @@ static int virtio_net_post_load_device(void *opaque, >> int version_id) >> >> if (virtio_vdev_has_feature(vdev, VIRTIO_NET_F_GUEST_ANNOUNCE) && >> virtio_vdev_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) { >> - n->announce_counter = SELF_ANNOUNCE_ROUNDS; >> - timer_mod(n->announce_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL)); >> + n->announce_timer->round = n->announce_timer->params.rounds; >> + timer_mod(n->announce_timer->tm, >> qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL)); > > Do you think it's worth having a > qemu_announce_timer_init(AnnounceTimer *, QEMU_CLOCK_*) > that would initialise the round and any other values and > do the initial timer_mod ?
I had that initially, but ended up with just 1 user (virtio). So removed. I think I might put it back. It's really a announce_timer_start() type thing. May be it'll convert both places to use that api to make it cleaner. > >> } >> >> return 0; >> @@ -1938,8 +1939,10 @@ static void virtio_net_device_realize(DeviceState >> *dev, Error **errp) >> qemu_macaddr_default_if_unset(&n->nic_conf.macaddr); >> memcpy(&n->mac[0], &n->nic_conf.macaddr, sizeof(n->mac)); >> n->status = VIRTIO_NET_S_LINK_UP; >> - n->announce_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, >> - virtio_net_announce_timer, n); >> + n->announce_timer = qemu_announce_timer_new(qemu_get_announce_params(), >> + QEMU_CLOCK_VIRTUAL); >> + n->announce_timer->tm = timer_new_ms(QEMU_CLOCK_VIRTUAL, >> + virtio_net_announce_timer, n); >> >> if (n->netclient_type) { >> /* >> @@ -2001,8 +2004,9 @@ static void virtio_net_device_unrealize(DeviceState >> *dev, Error **errp) >> virtio_net_del_queue(n, i); >> } >> >> - timer_del(n->announce_timer); >> - timer_free(n->announce_timer); >> + timer_del(n->announce_timer->tm); >> + timer_free(n->announce_timer->tm); >> + g_free(n->announce_timer); > > Hmm, why is this all safe - I guess this is in the BQL? Well, this is virito's explicit timer. I didn't check, but it really no different then destroying the virtio-specific QEMU timer created for the devices. > >> g_free(n->vqs); >> qemu_del_nic(n->nic); >> virtio_cleanup(vdev); >> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h >> index 1eec9a2..51dd4c3 100644 >> --- a/include/hw/virtio/virtio-net.h >> +++ b/include/hw/virtio/virtio-net.h >> @@ -94,8 +94,7 @@ typedef struct VirtIONet { >> char *netclient_name; >> char *netclient_type; >> uint64_t curr_guest_offloads; >> - QEMUTimer *announce_timer; >> - int announce_counter; >> + AnnounceTimer *announce_timer; >> bool needs_vnet_hdr_swap; >> } VirtIONet; >> >> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h >> index a6bf84d..f8aed9b 100644 >> --- a/include/migration/vmstate.h >> +++ b/include/migration/vmstate.h >> @@ -1022,8 +1022,6 @@ extern const VMStateInfo vmstate_info_qtailq; >> #define VMSTATE_END_OF_LIST() \ >> {} >> >> -#define SELF_ANNOUNCE_ROUNDS 5 >> - >> void loadvm_free_handlers(MigrationIncomingState *mis); >> >> int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd, >> @@ -1071,11 +1069,18 @@ AnnounceTimer >> *qemu_announce_timer_create(AnnounceParameters *params, >> QEMUTimerCB *cb); >> >> static inline >> -int64_t self_announce_delay(int round) >> +int64_t self_announce_delay(AnnounceTimer *timer) >> { >> - assert(round < SELF_ANNOUNCE_ROUNDS && round > 0); >> - /* delay 50ms, 150ms, 250ms, ... */ >> - return 50 + (SELF_ANNOUNCE_ROUNDS - round - 1) * 100; >> + int64_t ret; >> + >> + ret = timer->params.initial + >> + (timer->params.rounds - timer->round - 1) * >> + timer->params.step; >> + >> + if (ret < 0 || ret > timer->params.max) { >> + ret = timer->params.max; >> + } >> + return ret; >> } > > This feels like it should be with the rest of your code somewhere? You had it moved into the vmstate.c with your patches. I left it in vmstate.h, but as Juan mentioned, this should all be moved together under net somewhere. I think I'll do that for the next series. > >> void dump_vmstate_json_to_file(FILE *out_fp); >> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h >> index 7fd49c4..2ef1687 100644 >> --- a/include/sysemu/sysemu.h >> +++ b/include/sysemu/sysemu.h >> @@ -85,7 +85,7 @@ bool qemu_validate_announce_parameters(AnnounceParameters >> *params, >> Error **errp); >> void qemu_set_announce_parameters(AnnounceParameters *announce_params, >> AnnounceParameters *params); >> -void qemu_announce_self(void); >> +void qemu_announce_self(AnnounceParameters *params); >> >> /* Subcommands for QEMU_VM_COMMAND */ >> enum qemu_vm_cmd { >> diff --git a/migration/migration.c b/migration/migration.c >> index 0304c01..987c1cf 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -345,7 +345,7 @@ static void process_incoming_migration_bh(void *opaque) >> * This must happen after all error conditions are dealt with and >> * we're sure the VM is going to be running on this host. >> */ >> - qemu_announce_self(); >> + qemu_announce_self(qemu_get_announce_params()); >> >> /* If global state section was not received or we are in running >> state, we need to obey autostart. Any other state is set with >> diff --git a/migration/savevm.c b/migration/savevm.c >> index 607b090..555157a 100644 >> --- a/migration/savevm.c >> +++ b/migration/savevm.c >> @@ -212,21 +212,19 @@ static void qemu_announce_self_iter(NICState *nic, >> void *opaque) >> qemu_send_packet_raw(qemu_get_queue(nic), buf, len); >> } >> >> - >> static void qemu_announce_self_once(void *opaque) >> { >> - static int count = SELF_ANNOUNCE_ROUNDS; >> - QEMUTimer *timer = *(QEMUTimer **)opaque; >> + AnnounceTimer *timer = (AnnounceTimer *)opaque; >> >> qemu_foreach_nic(qemu_announce_self_iter, NULL); >> >> - if (--count) { >> - /* delay 50ms, 150ms, 250ms, ... */ >> - timer_mod(timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + >> - self_announce_delay(count)); >> + if (--timer->round) { >> + timer_mod(timer->tm, qemu_clock_get_ms(timer->type) + >> + self_announce_delay(timer)); >> } else { >> - timer_del(timer); >> - timer_free(timer); >> + timer_del(timer->tm); >> + timer_free(timer->tm); >> + g_free(timer); > > That's kind of odd in that it doesn't cause the thing the opaque > pointed to, to be zero'd, so problem if someone free'd it > in an exit path? But probably OK in this case. > In this case, the opaque is the AnnounceTimer itself. So we end up: +-> Announce_timer: +----> QemuTimer: | tm -----------+ opaque --+ | | +----------------------------------------+ zero-ing opaque is a really qemu-timers job, but it doesn't do so, and in this case, it's OK (sort of), since we call timer_free(). Thanks -vlad > Dave > >> } >> } >> >> @@ -252,11 +250,13 @@ AnnounceTimer >> *qemu_announce_timer_create(AnnounceParameters *params, >> return timer; >> } >> >> -void qemu_announce_self(void) >> +void qemu_announce_self(AnnounceParameters *params) >> { >> - static QEMUTimer *timer; >> - timer = timer_new_ms(QEMU_CLOCK_REALTIME, qemu_announce_self_once, >> &timer); >> - qemu_announce_self_once(&timer); >> + AnnounceTimer *timer; >> + >> + timer = qemu_announce_timer_create(params, QEMU_CLOCK_REALTIME, >> + qemu_announce_self_once); >> + qemu_announce_self_once(timer); >> } >> >> /***********************************************************/ >> @@ -1730,7 +1730,7 @@ static void loadvm_postcopy_handle_run_bh(void *opaque) >> */ >> cpu_synchronize_all_post_init(); >> >> - qemu_announce_self(); >> + qemu_announce_self(qemu_get_announce_params()); >> >> /* Make sure all file formats flush their mutable metadata. >> * If we get an error here, just don't restart the VM yet. */ >> -- >> 2.7.4 >> > -- > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK >