On Mon, May 19, 2014 at 05:34:38PM +0800, Jason Wang wrote: > On 05/18/2014 05:04 PM, Michael S. Tsirkin wrote: > > On Fri, May 16, 2014 at 01:02:51PM +0800, Jason Wang wrote: > >> On 05/15/2014 05:45 PM, Michael S. Tsirkin wrote: > >>> On Thu, May 15, 2014 at 05:22:28PM +0800, Jason Wang wrote: > >>>> On 05/15/2014 04:28 PM, Michael S. Tsirkin wrote: > >>>>> Thanks, looks good. > >>>>> Some minor comments below, > >>>>> > >>>>> On Thu, May 15, 2014 at 03:16:47PM +0800, Jason Wang wrote: > >>>>>> It's hard to track all mac addresses and their configurations (e.g > >>>>>> vlan or ipv6) in qemu. Without those informations, it's impossible to > >>>>> s/those informations/this information/ > >>>>> > >>>>>> build proper garp packet after migration. The only possible solution > >>>>>> to this is let guest (who knew all configurations) to do this. > >>>>> s/knew/knows/ > >>>>> > >>>>>> So, this patch introduces a new readonly config status bit of > >>>>>> virtio-net, > >>>>>> VIRTIO_NET_S_ANNOUNCE which is used to notify guest to announce > >>>>>> presence of its link through config update interrupt.When guest has > >>>>>> done the announcement, it should ack the notification through > >>>>>> VIRTIO_NET_CTRL_ANNOUNCE_ACK cmd. This feature is negotiated by a new > >>>>>> feature bit VIRTIO_NET_F_ANNOUNCE (which has already been supported by > >>>>>> Linux guest). > >>>>>> > >>>>>> During load, a counter of announcing rounds were set so that the after > >>>>> s/were/is/ > >>>>> s/the after/after/ > >>>> Will correct those typos. > >>>>>> the vm is running it can trigger rounds of config interrupts to notify > >>>>>> the guest to build and send the correct garps. > >>>>>> > >>>>>> Tested with ping to guest with vlan during migration. Without the > >>>>>> patch, lots of the packets were lost after migration. With the patch, > >>>>>> could not notice packet loss after migration. > >>>>> below changelog should go after ---, until the ack list. > >>>>> > >>>> Ok. > >>>>>> Reference: > >>>>>> RFC v2: > >>>>>> https://lists.gnu.org/archive/html/qemu-devel/2014-04/msg01750.html > >>>>>> RFC v1: > >>>>>> https://lists.gnu.org/archive/html/qemu-devel/2014-03/msg02648.html > >>>>>> V7: > >>>>>> https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg01127.html > >>>>>> > >>>>>> Changes from RFC v2: > >>>>>> - use QEMU_CLOCK_VIRTUAL instead of QEMU_CLOCK_REALTIME > >>>>>> - compat self announce for 2.0 machine type > >>>>>> > >>>>>> Changes from RFC v1: > >>>>>> - clean VIRTIO_NET_S_ANNOUNCE bit during reset > >>>>>> - free announce timer during clean > >>>>>> - make announce work for non-vhost case > >>>>>> > >>>>>> Changes from V7: > >>>>>> - Instead of introducing a global method for each kind of nic, this > >>>>>> version limits the changes to virtio-net itself. > >>>>>> > >>>>>> Cc: Liuyongan <liuyon...@huawei.com> > >>>>>> Cc: Amos Kong <ak...@redhat.com> > >>>>>> Signed-off-by: Jason Wang <jasow...@redhat.com> > >>>>>> --- > >>>>>> hw/net/virtio-net.c | 48 > >>>>>> ++++++++++++++++++++++++++++++++++++++++ > >>>>>> include/hw/i386/pc.h | 5 ++++ > >>>>>> include/hw/virtio/virtio-net.h | 16 +++++++++++++ > >>>>>> 3 files changed, 69 insertions(+), 0 deletions(-) > >>>>>> > >>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >>>>>> index 940a7cf..98d59e9 100644 > >>>>>> --- a/hw/net/virtio-net.c > >>>>>> +++ b/hw/net/virtio-net.c > >>>>>> @@ -25,6 +25,7 @@ > >>>>>> #include "monitor/monitor.h" > >>>>>> > >>>>>> #define VIRTIO_NET_VM_VERSION 11 > >>>>>> +#define VIRTIO_NET_ANNOUNCE_ROUNDS 3 > >>>>>> > >>>>>> #define MAC_TABLE_ENTRIES 64 > >>>>>> #define MAX_VLAN (1 << 12) /* Per 802.1Q definition */ > >>>>> I would make it 5 to be consistent with SELF_ANNOUNCE_ROUNDS > >>>>> in savevm.c > >>>>> > >>>>> in fact, let's move SELF_ANNOUNCE_ROUNDS to include/migration/vmstate.h > >>>>> and reuse it. > >>>> Ok. > >>>>>> @@ -99,6 +100,25 @@ static bool virtio_net_started(VirtIONet *n, > >>>>>> uint8_t status) > >>>>>> (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running; > >>>>>> } > >>>>>> > >>>>>> +static void virtio_net_announce_timer(void *opaque) > >>>>>> +{ > >>>>>> + VirtIONet *n = opaque; > >>>>>> + VirtIODevice *vdev = VIRTIO_DEVICE(n); > >>>>>> + > >>>>>> + if (n->announce && > >>>>> I would make it > 0 here, just in case it becomes negative as a result > >>>>> of some bug. > >>>> Sure. > >>>>>> + virtio_net_started(n, vdev->status) && > >>>>>> + vdev->guest_features & (0x1 << VIRTIO_NET_F_GUEST_ANNOUNCE) && > >>>>>> + vdev->guest_features & (0x1 << VIRTIO_NET_F_CTRL_VQ)) { > >>>>>> + > >>>>>> + n->announce--; > >>>>>> + n->status |= VIRTIO_NET_S_ANNOUNCE; > >>>>>> + virtio_notify_config(vdev); > >>>>>> + } else { > >>>>>> + timer_del(n->announce_timer); > >>>>> why do this here? > >>>>> > >>>>>> + n->announce = 0; > >>>>> why is this here? > >>>>> > >>>> If guest shuts down the device, there's no need to do the announcing. > >>> It's still weird. > >>> Either announce is 0 and then timer was not running > >>> or it's > 0 and then this won't trigger. > >> Right, the logic here is for QEMU_CLOCK_REALTIME. But there's another > >> question, we use QEMU_CLOCK_VIRTUAL while qemu is using > >> QEMU_CLOCK_REALTIME for its announcing. This looks fine but not sure > >> whether this is safe. > > Meaning QEMU_CLOCK_REALTIME that qemu uses? > > Not sure either but it doesn't modify guest state so it seems safe. > > > > > >> And if the link was down, it's better for us to > >> stop the announcing? > > I think that it doesn't matter: guest won't announce when > > link is down, anyway. > > Not worth it to write extra logic here in the host. > > Ok. > >>>>>> + } > >>>>>> +} > >>>>>> + > >>>>>> static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > >>>>>> { > >>>>>> VirtIODevice *vdev = VIRTIO_DEVICE(n); > >>>>>> @@ -147,6 +167,8 @@ static void virtio_net_set_status(struct > >>>>>> VirtIODevice *vdev, uint8_t status) > >>>>>> > >>>>>> virtio_net_vhost_status(n, status); > >>>>>> > >>>>>> + virtio_net_announce_timer(n); > >>>>>> + > >>>>> why do this here? why not right after we set announce counter? > >>>> The reasons are: > >>>> > >>>> - The counters were set in load, but the device is not running so we > >>>> could not inject the interrupt at that time. > >>> I see. This makes sense - but this isn't intuitive. > >>> Why don't we simply start timer with current time? > >>> Need to make sure it runs fine if time passes, but > >>> I think it's fine. > >> Not sure I get the point, I didn't see any differences except for an > >> extra timer fire. > > The only reason you call virtio_net_announce_timer from set_status > > is because it gets run on vm start/stop. > > It's true but not intuitive. > > Just run timer always from timer, it's clearer this way :) > > > > Sure. > >>>> - We can stop the progress when guest is shutting down the device. > >>> On shut down guest will reset device stopping timer - this seems enough. > >> Yes, I see. > >>>>>> for (i = 0; i < n->max_queues; i++) { > >>>>>> q = &n->vqs[i]; > >>>>>> > >>>>>> @@ -322,6 +344,9 @@ 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 = 0; > >>>>>> + n->status &= ~VIRTIO_NET_S_ANNOUNCE; > >>>>>> > >>>>>> /* Flush any MAC and VLAN filter table state */ > >>>>>> n->mac_table.in_use = 0; > >>>>>> @@ -731,6 +756,22 @@ static int virtio_net_handle_vlan_table(VirtIONet > >>>>>> *n, uint8_t cmd, > >>>>>> return VIRTIO_NET_OK; > >>>>>> } > >>>>>> > >>>>>> +static int virtio_net_handle_announce(VirtIONet *n, uint8_t cmd, > >>>>>> + struct iovec *iov, unsigned int > >>>>>> iov_cnt) > >>>>>> +{ > >>>>>> + if (cmd == VIRTIO_NET_CTRL_ANNOUNCE_ACK) { > >>>>>> + n->status &= ~VIRTIO_NET_S_ANNOUNCE; > >>>>>> + if (n->announce) { > >>>>> I would make it > 0 here, just in case it becomes negative as a result > >>>>> of some bug. > >>>> Ok. > >>>>>> + timer_mod(n->announce_timer, > >>>>>> + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 50 + > >>>>>> + (VIRTIO_NET_ANNOUNCE_ROUNDS - n->announce - 1) > >>>>>> * 100); > >>>>> savevm.c has this code, and a comment: > >>>>> /* delay 50ms, 150ms, 250ms, ... */ > >>>>> 50 + (SELF_ANNOUNCE_ROUNDS - count - 1) * 100 > >>>>> > >>>>> how about an API in include/migration/vmstate.h > >>>>> > >>>>> static inline > >>>>> int64_t self_announce_delay(int round) > >>>>> { > >>>>> assert(round < SELF_ANNOUNCE_ROUNDS && round > 0); > >>>>> /* delay 50ms, 150ms, 250ms, ... */ > >>>>> return 50 + (SELF_ANNOUNCE_ROUNDS - round - 1) * 100; > >>>>> } > >>>>> > >>>>> or something to this end. > >>>>> > >>>> Good idea, will do this. > >>>>>> + } > >>>>>> + return VIRTIO_NET_OK; > >>>>>> + } else { > >>>>>> + return VIRTIO_NET_ERR; > >>>>>> + } > >>>>>> +} > >>>>>> + > >>>>>> static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd, > >>>>>> struct iovec *iov, unsigned int > >>>>>> iov_cnt) > >>>>>> { > >>>>>> @@ -794,6 +835,8 @@ static void virtio_net_handle_ctrl(VirtIODevice > >>>>>> *vdev, VirtQueue *vq) > >>>>>> status = virtio_net_handle_mac(n, ctrl.cmd, iov, iov_cnt); > >>>>>> } else if (ctrl.class == VIRTIO_NET_CTRL_VLAN) { > >>>>>> status = virtio_net_handle_vlan_table(n, ctrl.cmd, iov, > >>>>>> iov_cnt); > >>>>>> + } else if (ctrl.class == VIRTIO_NET_CTRL_ANNOUNCE) { > >>>>>> + status = virtio_net_handle_announce(n, ctrl.cmd, iov, > >>>>>> iov_cnt); > >>>>>> } else if (ctrl.class == VIRTIO_NET_CTRL_MQ) { > >>>>>> status = virtio_net_handle_mq(n, ctrl.cmd, iov, iov_cnt); > >>>>>> } else if (ctrl.class == VIRTIO_NET_CTRL_GUEST_OFFLOADS) { > >>>>>> @@ -1451,6 +1494,7 @@ static int virtio_net_load(QEMUFile *f, void > >>>>>> *opaque, int version_id) > >>>>>> qemu_get_subqueue(n->nic, i)->link_down = link_down; > >>>>>> } > >>>>>> > >>>>>> + n->announce = VIRTIO_NET_ANNOUNCE_ROUNDS; > >>>>> Well if virtio_net_handle_announce runs now it will start timer > >>>>> in the past, right? > >>>>> This seems wrong. > >>>> Not sure I get the case. When in virtio_net_load() the vm is not even > >>>> running so looks like virtio_net_handle_announce() could not run in the > >>>> same time. > >>> I see, this works because you decrement it when VM starts running. > >>> I think it's not a good idea to rely on this though, > >>> better do everything from timer, and handle > >>> the case of command arriving too early. > >>> > >> Right, if QEMU_CLOCK_VIRTUAL is fine, we can do everything in a timer. > >> > >> For the case of command arriving too early. Is there anything else we > >> need to do? Since we only start the next timer when we get ack command. > >> > >> Thanks > > I think we need to make sure we don't set the timer in the past or > > very far in the future. > > n->announce is between 0 and VIRTIO_NET_ANNOUNCE_ROUNDS - 1, when doing > timer_mod(), so it looks ok.
All I am saying, make sure it's within the range even if handle announce is called before timer_mod. Can not happen with your current patch, but must make sure it's still correct after you make changes :) Also, maybe add an assert there. > There's another case that if vm was stopped > for a long time, the timer will also be delayed, but it's still safe in > this case.
