| Message ID | 1433510652-13866-1-git-send-email-thibaut.collet@6wind.com |
|---|---|
| State | New |
| Headers | show |
On 06/05/2015 09:24 PM, Thibaut Collet wrote: > Add VIRTIO_NET_F_GUEST_ANNOUNCE capability to vhost-net when netdev backend is > vhost-user. > > For netdev backend using virtio-net NIC the self announce is managed directly > by the virtio-net NIC and not by the netdev backend itself. > To avoid duplication of announces (once from the guest and once from QEMU) a > bitfield is added in the NetClientState structure. > If this bit is set self announce does not send message to the guest to request > gratuitous ARP but let virtio-net NIC set the VIRTIO_NET_S_ANNOUNCE for > gratuitous ARP. > > Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com> > --- > v2: do not discard anymore packets send to vhost-user: it is GARP request after > live migration. > As suggested by S. Hajnoczi qemu_announce_self skips virtio-net NIC that > already send GARP. > > hw/net/vhost_net.c | 2 ++ > include/net/net.h | 1 + > net/vhost-user.c | 2 ++ > savevm.c | 11 ++++++++--- > 4 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > index 426b23e..a745f97 100644 > --- a/hw/net/vhost_net.c > +++ b/hw/net/vhost_net.c > @@ -82,6 +82,8 @@ static const int user_feature_bits[] = { > VIRTIO_NET_F_CTRL_MAC_ADDR, > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, > > + VIRTIO_NET_F_GUEST_ANNOUNCE, > + > VIRTIO_NET_F_MQ, > > VHOST_INVALID_FEATURE_BIT > diff --git a/include/net/net.h b/include/net/net.h > index e66ca03..a78e9df 100644 > --- a/include/net/net.h > +++ b/include/net/net.h > @@ -85,6 +85,7 @@ struct NetClientState { > char *name; > char info_str[256]; > unsigned receive_disabled : 1; > + unsigned self_announce_disabled : 1; > NetClientDestructor *destructor; > unsigned int queue_index; > unsigned rxfilter_notify_enabled:1; > diff --git a/net/vhost-user.c b/net/vhost-user.c > index 8d26728..b345446 100644 > --- a/net/vhost-user.c > +++ b/net/vhost-user.c > @@ -147,6 +147,8 @@ static int net_vhost_user_init(NetClientState *peer, const char *device, > > s = DO_UPCAST(VhostUserState, nc, nc); > > + /* Self announce is managed directly by virtio-net NIC */ > + s->nc.self_announce_disabled = 1; Shouldn't we do this in set_features consider it needs guest support or you want to disable gratuitous packet for vhost-user at all? > /* We don't provide a receive callback */ > s->nc.receive_disabled = 1; > s->chr = chr; > diff --git a/savevm.c b/savevm.c > index 3b0e222..7a134b1 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -80,11 +80,16 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque) > { > uint8_t buf[60]; > int len; > + NetClientState *nc; > > - trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr)); > - len = announce_self_create(buf, nic->conf->macaddr.a); > + nc = qemu_get_queue(nic); > > - qemu_send_packet_raw(qemu_get_queue(nic), buf, len); > + if (!nc->peer->self_announce_disabled) { > + trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr)); > + len = announce_self_create(buf, nic->conf->macaddr.a); > + > + qemu_send_packet_raw(nc, buf, len); > + } > } > >
Hi,
My understanding of gratuitous packet with virtio for any backend (vhost
user or other):
- When the VM is loaded (first start or migration) the virtio net
interfaces are loaded ( virtio_net_load_device function in
hw/net/virtio-net.c)
- If the guest has the VIRTIO_NET_F_GUEST_ANNOUNCE capability, request to
send gratuitous packet is done.
1. To enable gratuitous packet through this mechanism I have added
VIRTIO_NET_F_GUEST_ANNOUNCE
capability to hw/net/vhost_net.c. So host and guest can negotiate this
feature when vhost-user is used.
2. self announce occurs in case of live migration. During a live migration
a GARP is sent to all net backend through a queue dedicated to the net
backend.
But for vhost-user:
- this operation is not possible (vhost-user has no queue)
- it is already done with the previous mechanism.
Rather to define a queue to vhost user and notify twice the guest to
send gratuitous packet I have disable GARP from self announce and use only
the first mechanism for that.
I have tested my modifications with guest that supports
VIRTIO_NET_F_GUEST_ANNOUNCE and vhost-user on the host. After a live
migration I have the GARP from the guest.
Regards.
On Mon, Jun 8, 2015 at 7:55 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 06/05/2015 09:24 PM, Thibaut Collet wrote:
> > Add VIRTIO_NET_F_GUEST_ANNOUNCE capability to vhost-net when netdev
> backend is
> > vhost-user.
> >
> > For netdev backend using virtio-net NIC the self announce is managed
> directly
> > by the virtio-net NIC and not by the netdev backend itself.
> > To avoid duplication of announces (once from the guest and once from
> QEMU) a
> > bitfield is added in the NetClientState structure.
> > If this bit is set self announce does not send message to the guest to
> request
> > gratuitous ARP but let virtio-net NIC set the VIRTIO_NET_S_ANNOUNCE for
> > gratuitous ARP.
> >
> > Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
> > ---
> > v2: do not discard anymore packets send to vhost-user: it is GARP
> request after
> > live migration.
> > As suggested by S. Hajnoczi qemu_announce_self skips virtio-net NIC
> that
> > already send GARP.
> >
> > hw/net/vhost_net.c | 2 ++
> > include/net/net.h | 1 +
> > net/vhost-user.c | 2 ++
> > savevm.c | 11 ++++++++---
> > 4 files changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index 426b23e..a745f97 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -82,6 +82,8 @@ static const int user_feature_bits[] = {
> > VIRTIO_NET_F_CTRL_MAC_ADDR,
> > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS,
> >
> > + VIRTIO_NET_F_GUEST_ANNOUNCE,
> > +
> > VIRTIO_NET_F_MQ,
> >
> > VHOST_INVALID_FEATURE_BIT
> > diff --git a/include/net/net.h b/include/net/net.h
> > index e66ca03..a78e9df 100644
> > --- a/include/net/net.h
> > +++ b/include/net/net.h
> > @@ -85,6 +85,7 @@ struct NetClientState {
> > char *name;
> > char info_str[256];
> > unsigned receive_disabled : 1;
> > + unsigned self_announce_disabled : 1;
> > NetClientDestructor *destructor;
> > unsigned int queue_index;
> > unsigned rxfilter_notify_enabled:1;
> > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > index 8d26728..b345446 100644
> > --- a/net/vhost-user.c
> > +++ b/net/vhost-user.c
> > @@ -147,6 +147,8 @@ static int net_vhost_user_init(NetClientState *peer,
> const char *device,
> >
> > s = DO_UPCAST(VhostUserState, nc, nc);
> >
> > + /* Self announce is managed directly by virtio-net NIC */
> > + s->nc.self_announce_disabled = 1;
>
> Shouldn't we do this in set_features consider it needs guest support or
> you want to disable gratuitous packet for vhost-user at all?
>
> > /* We don't provide a receive callback */
> > s->nc.receive_disabled = 1;
> > s->chr = chr;
> > diff --git a/savevm.c b/savevm.c
> > index 3b0e222..7a134b1 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -80,11 +80,16 @@ static void qemu_announce_self_iter(NICState *nic,
> void *opaque)
> > {
> > uint8_t buf[60];
> > int len;
> > + NetClientState *nc;
> >
> > - trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
> > - len = announce_self_create(buf, nic->conf->macaddr.a);
> > + nc = qemu_get_queue(nic);
> >
> > - qemu_send_packet_raw(qemu_get_queue(nic), buf, len);
> > + if (!nc->peer->self_announce_disabled) {
> > +
> trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr));
> > + len = announce_self_create(buf, nic->conf->macaddr.a);
> > +
> > + qemu_send_packet_raw(nc, buf, len);
> > + }
> > }
> >
> >
>
>
On 06/08/2015 04:21 PM, Thibaut Collet wrote: > Hi, > > My understanding of gratuitous packet with virtio for any backend > (vhost user or other): > - When the VM is loaded (first start or migration) the virtio net > interfaces are loaded ( virtio_net_load_device function in > hw/net/virtio-net.c) > - If the guest has the VIRTIO_NET_F_GUEST_ANNOUNCE capability, request > to send gratuitous packet is done. > > 1. To enable gratuitous packet through this mechanism I have > added VIRTIO_NET_F_GUEST_ANNOUNCE capability to hw/net/vhost_net.c. So > host and guest can negotiate this feature when vhost-user is used. > > 2. self announce occurs in case of live migration. During a live > migration a GARP is sent to all net backend through a queue dedicated > to the net backend. > But for vhost-user: > - this operation is not possible (vhost-user has no queue) > - it is already done with the previous mechanism. > Rather to define a queue to vhost user and notify twice the guest > to send gratuitous packet I have disable GARP from self announce and > use only the first mechanism for that. > > I have tested my modifications with guest that supports > VIRTIO_NET_F_GUEST_ANNOUNCE and vhost-user on the host. After a live > migration I have the GARP from the guest. Yes, your patch works well for recent drivers. But the problem is legacy guest/driver without VIRTIO_NET_F_GUEST_ANNOUNCE. In this case there will be no GARP sent after migration. Thanks
Hi, I agree that my patch is OK only for recent drivers. To have a simple patch with only few modifications I do not implement a solution for old driver but I can be done later. For legacy guest without VIRTIO_NET_F_GUEST_ANNOUNCE the problem is more complex. The RARP must be sent by the vhost client/backend. This component is outside QEMU (the reference implementation is snabbswitch: http://www.virtualopensystems.com/en/solutions/guides/snabbswitch-qemu/). To do that: - a receive function must be defined for the vhost user. - a message must be added between QEMU and vapp. This message will be sent only for old guest driver to avoid GARP duplication. - the added self_announce_disabled must be removed (decision to send or not the RARP is done later by the backend and not by the generic migration method) Do you agree with this solution ? Regards. On Mon, Jun 8, 2015 at 11:14 AM, Jason Wang <jasowang@redhat.com> wrote: > > > On 06/08/2015 04:21 PM, Thibaut Collet wrote: > > Hi, > > > > My understanding of gratuitous packet with virtio for any backend > > (vhost user or other): > > - When the VM is loaded (first start or migration) the virtio net > > interfaces are loaded ( virtio_net_load_device function in > > hw/net/virtio-net.c) > > - If the guest has the VIRTIO_NET_F_GUEST_ANNOUNCE capability, request > > to send gratuitous packet is done. > > > > 1. To enable gratuitous packet through this mechanism I have > > added VIRTIO_NET_F_GUEST_ANNOUNCE capability to hw/net/vhost_net.c. So > > host and guest can negotiate this feature when vhost-user is used. > > > > 2. self announce occurs in case of live migration. During a live > > migration a GARP is sent to all net backend through a queue dedicated > > to the net backend. > > But for vhost-user: > > - this operation is not possible (vhost-user has no queue) > > - it is already done with the previous mechanism. > > Rather to define a queue to vhost user and notify twice the guest > > to send gratuitous packet I have disable GARP from self announce and > > use only the first mechanism for that. > > > > I have tested my modifications with guest that supports > > VIRTIO_NET_F_GUEST_ANNOUNCE and vhost-user on the host. After a live > > migration I have the GARP from the guest. > > Yes, your patch works well for recent drivers. But the problem is legacy > guest/driver without VIRTIO_NET_F_GUEST_ANNOUNCE. In this case there > will be no GARP sent after migration. > > Thanks >
On Mon, Jun 08, 2015 at 12:01:38PM +0200, Thibaut Collet wrote: > Hi, > > I agree that my patch is OK only for recent drivers. To have a simple patch > with only few modifications I do not implement a solution for old driver but I > can be done later. > > For legacy guest without VIRTIO_NET_F_GUEST_ANNOUNCE the problem is more > complex. The RARP must be sent by the vhost client/backend. This component is > outside QEMU (the reference implementation is snabbswitch: http:// > www.virtualopensystems.com/en/solutions/guides/snabbswitch-qemu/). To do that: > - a receive function must be defined for the vhost user. > - a message must be added between QEMU and vapp. This message will be sent only > for old guest driver to avoid GARP duplication. > - the added self_announce_disabled must be removed (decision to send or not the > RARP is done later by the backend and not by the generic migration method) > > Do you agree with this solution ? > > > Regards. I don't get it. Why do you need any extra messages for old drivers? To detect old drivers, simply have backend check whether VIRTIO_NET_F_GUEST_ANNOUNCE is set. But I don't see this as a blocker for this patch, this can be added separately as needed. > On Mon, Jun 8, 2015 at 11:14 AM, Jason Wang <jasowang@redhat.com> wrote: > > > > On 06/08/2015 04:21 PM, Thibaut Collet wrote: > > Hi, > > > > My understanding of gratuitous packet with virtio for any backend > > (vhost user or other): > > - When the VM is loaded (first start or migration) the virtio net > > interfaces are loaded ( virtio_net_load_device function in > > hw/net/virtio-net.c) > > - If the guest has the VIRTIO_NET_F_GUEST_ANNOUNCE capability, request > > to send gratuitous packet is done. > > > > 1. To enable gratuitous packet through this mechanism I have > > added VIRTIO_NET_F_GUEST_ANNOUNCE capability to hw/net/vhost_net.c. So > > host and guest can negotiate this feature when vhost-user is used. > > > > 2. self announce occurs in case of live migration. During a live > > migration a GARP is sent to all net backend through a queue dedicated > > to the net backend. > > But for vhost-user: > > - this operation is not possible (vhost-user has no queue) > > - it is already done with the previous mechanism. > > Rather to define a queue to vhost user and notify twice the guest > > to send gratuitous packet I have disable GARP from self announce and > > use only the first mechanism for that. > > > > I have tested my modifications with guest that supports > > VIRTIO_NET_F_GUEST_ANNOUNCE and vhost-user on the host. After a live > > migration I have the GARP from the guest. > > Yes, your patch works well for recent drivers. But the problem is legacy > guest/driver without VIRTIO_NET_F_GUEST_ANNOUNCE. In this case there > will be no GARP sent after migration. > > Thanks > >
On Fri, Jun 05, 2015 at 03:24:12PM +0200, Thibaut Collet wrote: > Add VIRTIO_NET_F_GUEST_ANNOUNCE capability to vhost-net when netdev backend is > vhost-user. > > For netdev backend using virtio-net NIC the self announce is managed directly > by the virtio-net NIC and not by the netdev backend itself. > To avoid duplication of announces (once from the guest and once from QEMU) a > bitfield is added in the NetClientState structure. > If this bit is set self announce does not send message to the guest to request > gratuitous ARP but let virtio-net NIC set the VIRTIO_NET_S_ANNOUNCE for > gratuitous ARP. > > Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com> > --- > v2: do not discard anymore packets send to vhost-user: it is GARP request after > live migration. > As suggested by S. Hajnoczi qemu_announce_self skips virtio-net NIC that > already send GARP. > > hw/net/vhost_net.c | 2 ++ > include/net/net.h | 1 + > net/vhost-user.c | 2 ++ > savevm.c | 11 ++++++++--- > 4 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > index 426b23e..a745f97 100644 > --- a/hw/net/vhost_net.c > +++ b/hw/net/vhost_net.c > @@ -82,6 +82,8 @@ static const int user_feature_bits[] = { > VIRTIO_NET_F_CTRL_MAC_ADDR, > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, > > + VIRTIO_NET_F_GUEST_ANNOUNCE, > + > VIRTIO_NET_F_MQ, > > VHOST_INVALID_FEATURE_BIT > diff --git a/include/net/net.h b/include/net/net.h > index e66ca03..a78e9df 100644 > --- a/include/net/net.h > +++ b/include/net/net.h > @@ -85,6 +85,7 @@ struct NetClientState { > char *name; > char info_str[256]; > unsigned receive_disabled : 1; > + unsigned self_announce_disabled : 1; > NetClientDestructor *destructor; > unsigned int queue_index; > unsigned rxfilter_notify_enabled:1; > diff --git a/net/vhost-user.c b/net/vhost-user.c > index 8d26728..b345446 100644 > --- a/net/vhost-user.c > +++ b/net/vhost-user.c > @@ -147,6 +147,8 @@ static int net_vhost_user_init(NetClientState *peer, const char *device, > > s = DO_UPCAST(VhostUserState, nc, nc); > > + /* Self announce is managed directly by virtio-net NIC */ > + s->nc.self_announce_disabled = 1; > /* We don't provide a receive callback */ > s->nc.receive_disabled = 1; > s->chr = chr; > diff --git a/savevm.c b/savevm.c > index 3b0e222..7a134b1 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -80,11 +80,16 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque) > { > uint8_t buf[60]; > int len; > + NetClientState *nc; > > - trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr)); > - len = announce_self_create(buf, nic->conf->macaddr.a); > + nc = qemu_get_queue(nic); > > - qemu_send_packet_raw(qemu_get_queue(nic), buf, len); > + if (!nc->peer->self_announce_disabled) { > + trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr)); > + len = announce_self_create(buf, nic->conf->macaddr.a); > + > + qemu_send_packet_raw(nc, buf, len); > + } > } > I don't think qemu_send_packet_raw can ever work for vhost user. What happens if you merely add VIRTIO_NET_F_GUEST_ANNOUNCE to the feature list, and drop the rest of the patch? > -- > 1.7.10.4
On 06/08/2015 06:01 PM, Thibaut Collet wrote: > Hi, > > I agree that my patch is OK only for recent drivers. To have a simple > patch with only few modifications I do not implement a solution for > old driver but I can be done later. > This makes sense. > For legacy guest without VIRTIO_NET_F_GUEST_ANNOUNCE the problem is > more complex. The RARP must be sent by the vhost client/backend. This > component is outside QEMU (the reference implementation is > snabbswitch: http://www.virtualopensystems.com/en/solutions/guides/snabbswitch-qemu/). > To do that: > - a receive function must be defined for the vhost user. > - a message must be added between QEMU and vapp. This message will be > sent only for old guest driver to avoid GARP duplication. > - the added self_announce_disabled must be removed (decision to send > or not the RARP is done later by the backend and not by the generic > migration method) > > Do you agree with this solution ? > > Sounds ok. > Regards. > > On Mon, Jun 8, 2015 at 11:14 AM, Jason Wang <jasowang@redhat.com > <mailto:jasowang@redhat.com>> wrote: > > > > On 06/08/2015 04:21 PM, Thibaut Collet wrote: > > Hi, > > > > My understanding of gratuitous packet with virtio for any backend > > (vhost user or other): > > - When the VM is loaded (first start or migration) the virtio net > > interfaces are loaded ( virtio_net_load_device function in > > hw/net/virtio-net.c) > > - If the guest has the VIRTIO_NET_F_GUEST_ANNOUNCE capability, > request > > to send gratuitous packet is done. > > > > 1. To enable gratuitous packet through this mechanism I have > > added VIRTIO_NET_F_GUEST_ANNOUNCE capability to > hw/net/vhost_net.c. So > > host and guest can negotiate this feature when vhost-user is used. > > > > 2. self announce occurs in case of live migration. During a live > > migration a GARP is sent to all net backend through a queue > dedicated > > to the net backend. > > But for vhost-user: > > - this operation is not possible (vhost-user has no queue) > > - it is already done with the previous mechanism. > > Rather to define a queue to vhost user and notify twice the guest > > to send gratuitous packet I have disable GARP from self announce and > > use only the first mechanism for that. > > > > I have tested my modifications with guest that supports > > VIRTIO_NET_F_GUEST_ANNOUNCE and vhost-user on the host. After a live > > migration I have the GARP from the guest. > > Yes, your patch works well for recent drivers. But the problem is > legacy > guest/driver without VIRTIO_NET_F_GUEST_ANNOUNCE. In this case there > will be no GARP sent after migration. > > Thanks > >
Hi, > I don't think qemu_send_packet_raw can ever work for vhost user. > What happens if you merely add VIRTIO_NET_F_GUEST_ANNOUNCE > to the feature list, and drop the rest of the patch? If you merely add VIRTIO_NET_F_GUEST_ANNOUNCE you have the GARP with recent guest after a live migration. The rest of the patch (can be set in a separate commit) avoid a qemu crashes with live migration when vhost-user is present: - When a live migration migration is complete a RARP is automatically send to any net backend (self announce mechanism) - vhost user does not provide any receive function and RARP request is stored in the vhost-user queue - When a migration back is done all the net backend queues are purged. The stored RARP request for vhost-user is then sent to the register receive callback of vhost-user that is NULL. Support of live migration for vhost user needs the whole patch. (and maore if we want support legacy guest with no support of VIRTIO_NET_F_GUEST_ANNOUNCE) > I don't get it. Why do you need any extra messages for old drivers? To > detect old drivers, simply have backend check whether > VIRTIO_NET_F_GUEST_ANNOUNCE is set. For old driver we can not use the mechanism implemented in virtio-net that manages VIRTIO_NET_F_GUEST_ANNOUNCE. In this case we must send the RARP on the network interface created by vhost-user. My first idea to do that was to add a message between QEMU and vhost client/backend (vapp or other) to let the vhost client/backend send the RARP. But maybe it will be easier to let QEMU send directly the RARP message on the network interface created by vhost user. This point can be done later if it is needed. Regards. On Mon, Jun 8, 2015 at 12:12 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Fri, Jun 05, 2015 at 03:24:12PM +0200, Thibaut Collet wrote: > > Add VIRTIO_NET_F_GUEST_ANNOUNCE capability to vhost-net when netdev > backend is > > vhost-user. > > > > For netdev backend using virtio-net NIC the self announce is managed > directly > > by the virtio-net NIC and not by the netdev backend itself. > > To avoid duplication of announces (once from the guest and once from > QEMU) a > > bitfield is added in the NetClientState structure. > > If this bit is set self announce does not send message to the guest to > request > > gratuitous ARP but let virtio-net NIC set the VIRTIO_NET_S_ANNOUNCE for > > gratuitous ARP. > > > > Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com> > > --- > > v2: do not discard anymore packets send to vhost-user: it is GARP > request after > > live migration. > > As suggested by S. Hajnoczi qemu_announce_self skips virtio-net NIC > that > > already send GARP. > > > > hw/net/vhost_net.c | 2 ++ > > include/net/net.h | 1 + > > net/vhost-user.c | 2 ++ > > savevm.c | 11 ++++++++--- > > 4 files changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > > index 426b23e..a745f97 100644 > > --- a/hw/net/vhost_net.c > > +++ b/hw/net/vhost_net.c > > @@ -82,6 +82,8 @@ static const int user_feature_bits[] = { > > VIRTIO_NET_F_CTRL_MAC_ADDR, > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, > > > > + VIRTIO_NET_F_GUEST_ANNOUNCE, > > + > > VIRTIO_NET_F_MQ, > > > > VHOST_INVALID_FEATURE_BIT > > diff --git a/include/net/net.h b/include/net/net.h > > index e66ca03..a78e9df 100644 > > --- a/include/net/net.h > > +++ b/include/net/net.h > > @@ -85,6 +85,7 @@ struct NetClientState { > > char *name; > > char info_str[256]; > > unsigned receive_disabled : 1; > > + unsigned self_announce_disabled : 1; > > NetClientDestructor *destructor; > > unsigned int queue_index; > > unsigned rxfilter_notify_enabled:1; > > diff --git a/net/vhost-user.c b/net/vhost-user.c > > index 8d26728..b345446 100644 > > --- a/net/vhost-user.c > > +++ b/net/vhost-user.c > > @@ -147,6 +147,8 @@ static int net_vhost_user_init(NetClientState *peer, > const char *device, > > > > s = DO_UPCAST(VhostUserState, nc, nc); > > > > + /* Self announce is managed directly by virtio-net NIC */ > > + s->nc.self_announce_disabled = 1; > > /* We don't provide a receive callback */ > > s->nc.receive_disabled = 1; > > s->chr = chr; > > diff --git a/savevm.c b/savevm.c > > index 3b0e222..7a134b1 100644 > > --- a/savevm.c > > +++ b/savevm.c > > @@ -80,11 +80,16 @@ static void qemu_announce_self_iter(NICState *nic, > void *opaque) > > { > > uint8_t buf[60]; > > int len; > > + NetClientState *nc; > > > > - trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr)); > > - len = announce_self_create(buf, nic->conf->macaddr.a); > > + nc = qemu_get_queue(nic); > > > > - qemu_send_packet_raw(qemu_get_queue(nic), buf, len); > > + if (!nc->peer->self_announce_disabled) { > > + > trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr)); > > + len = announce_self_create(buf, nic->conf->macaddr.a); > > + > > + qemu_send_packet_raw(nc, buf, len); > > + } > > } > > > > I don't think qemu_send_packet_raw can ever work for vhost user. > What happens if you merely add VIRTIO_NET_F_GUEST_ANNOUNCE > to the feature list, and drop the rest of the patch? > > > > -- > > 1.7.10.4 >
On Mon, Jun 08, 2015 at 01:29:39PM +0200, Thibaut Collet wrote: > Hi, > > > I don't think qemu_send_packet_raw can ever work for vhost user. > > What happens if you merely add VIRTIO_NET_F_GUEST_ANNOUNCE > > to the feature list, and drop the rest of the patch? > > If you merely add VIRTIO_NET_F_GUEST_ANNOUNCE you have the GARP with recent > guest after a live migration. > The rest of the patch (can be set in a separate commit) avoid a qemu crashes > with live migration when vhost-user is present: > - When a live migration migration is complete a RARP is automatically send to > any net backend (self announce mechanism) > - vhost user does not provide any receive function and RARP request is stored > in the vhost-user queue > - When a migration back is done all the net backend queues are purged. The > stored RARP request for vhost-user is then sent to the register receive > callback of vhost-user that is NULL. > > Support of live migration for vhost user needs the whole patch. (and maore if > we want support legacy guest with no support of VIRTIO_NET_F_GUEST_ANNOUNCE) How about implementing a receive function that discards all packets then? > > > I don't get it. Why do you need any extra messages for old drivers? To > > detect old drivers, simply have backend check whether > > VIRTIO_NET_F_GUEST_ANNOUNCE is set. > > For old driver we can not use the mechanism implemented in virtio-net that > manages VIRTIO_NET_F_GUEST_ANNOUNCE. In this case we must send the RARP on the > network interface created by vhost-user. > My first idea to do that was to add a message between QEMU and vhost client/ > backend (vapp or other) to let the vhost client/backend send the RARP. > But maybe it will be easier to let QEMU send directly the RARP message on the > network interface created by vhost user. > This point can be done later if it is needed. > > Regards. Can't vhost-user backend sends this automatically? Why do we need to do anything in QEMU? > > On Mon, Jun 8, 2015 at 12:12 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Jun 05, 2015 at 03:24:12PM +0200, Thibaut Collet wrote: > > Add VIRTIO_NET_F_GUEST_ANNOUNCE capability to vhost-net when netdev > backend is > > vhost-user. > > > > For netdev backend using virtio-net NIC the self announce is managed > directly > > by the virtio-net NIC and not by the netdev backend itself. > > To avoid duplication of announces (once from the guest and once from > QEMU) a > > bitfield is added in the NetClientState structure. > > If this bit is set self announce does not send message to the guest to > request > > gratuitous ARP but let virtio-net NIC set the VIRTIO_NET_S_ANNOUNCE for > > gratuitous ARP. > > > > Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com> > > --- > > v2: do not discard anymore packets send to vhost-user: it is GARP request > after > > live migration. > > As suggested by S. Hajnoczi qemu_announce_self skips virtio-net NIC > that > > already send GARP. > > > > hw/net/vhost_net.c | 2 ++ > > include/net/net.h | 1 + > > net/vhost-user.c | 2 ++ > > savevm.c | 11 ++++++++--- > > 4 files changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > > index 426b23e..a745f97 100644 > > --- a/hw/net/vhost_net.c > > +++ b/hw/net/vhost_net.c > > @@ -82,6 +82,8 @@ static const int user_feature_bits[] = { > > VIRTIO_NET_F_CTRL_MAC_ADDR, > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, > > > > + VIRTIO_NET_F_GUEST_ANNOUNCE, > > + > > VIRTIO_NET_F_MQ, > > > > VHOST_INVALID_FEATURE_BIT > > diff --git a/include/net/net.h b/include/net/net.h > > index e66ca03..a78e9df 100644 > > --- a/include/net/net.h > > +++ b/include/net/net.h > > @@ -85,6 +85,7 @@ struct NetClientState { > > char *name; > > char info_str[256]; > > unsigned receive_disabled : 1; > > + unsigned self_announce_disabled : 1; > > NetClientDestructor *destructor; > > unsigned int queue_index; > > unsigned rxfilter_notify_enabled:1; > > diff --git a/net/vhost-user.c b/net/vhost-user.c > > index 8d26728..b345446 100644 > > --- a/net/vhost-user.c > > +++ b/net/vhost-user.c > > @@ -147,6 +147,8 @@ static int net_vhost_user_init(NetClientState *peer, > const char *device, > > > > s = DO_UPCAST(VhostUserState, nc, nc); > > > > + /* Self announce is managed directly by virtio-net NIC */ > > + s->nc.self_announce_disabled = 1; > > /* We don't provide a receive callback */ > > s->nc.receive_disabled = 1; > > s->chr = chr; > > diff --git a/savevm.c b/savevm.c > > index 3b0e222..7a134b1 100644 > > --- a/savevm.c > > +++ b/savevm.c > > @@ -80,11 +80,16 @@ static void qemu_announce_self_iter(NICState *nic, > void *opaque) > > { > > uint8_t buf[60]; > > int len; > > + NetClientState *nc; > > > > - trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr)); > > - len = announce_self_create(buf, nic->conf->macaddr.a); > > + nc = qemu_get_queue(nic); > > > > - qemu_send_packet_raw(qemu_get_queue(nic), buf, len); > > + if (!nc->peer->self_announce_disabled) { > > + trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf-> > macaddr)); > > + len = announce_self_create(buf, nic->conf->macaddr.a); > > + > > + qemu_send_packet_raw(nc, buf, len); > > + } > > } > > > > I don't think qemu_send_packet_raw can ever work for vhost user. > What happens if you merely add VIRTIO_NET_F_GUEST_ANNOUNCE > to the feature list, and drop the rest of the patch? > > > > -- > > 1.7.10.4 > >
> How about implementing a receive function that discards all packets > then? Implementing a receive function that discards all packets is an other solution. I have not chosen this one to avoid to mask other potential issues: Normally vhost-user never receives packets. By keeping a NULL function as received callback we can detect easily case where packets are sent (qemu crashes) and solve this issue. > Can't vhost-user backend sends this automatically? > Why do we need to do anything in QEMU? My explanations are maybe unclear. For old driver we have to send the RARP to the guest through the network interface (and to implement a receive function for vhost-user). My question is: where is the best location to do that: 1. In the receive function of vhost-user (in the file net/vhost-user.c) 2. In a self announce function (called when vhost-user receives a RARP, analysis of the packet content is needed in this case) in the file hw/net/vhost-net.c 3. In the vhost-user backend (file hw/virtio/vhost-user.c). In this case a new message must be defined between hw/net/vhost-net.c and hw/virtio/vhost-user.c. 4. In the vhost client/backend (vapp or other) On Mon, Jun 8, 2015 at 2:45 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Mon, Jun 08, 2015 at 01:29:39PM +0200, Thibaut Collet wrote: > > Hi, > > > > > I don't think qemu_send_packet_raw can ever work for vhost user. > > > What happens if you merely add VIRTIO_NET_F_GUEST_ANNOUNCE > > > to the feature list, and drop the rest of the patch? > > > > If you merely add VIRTIO_NET_F_GUEST_ANNOUNCE you have the GARP with > recent > > guest after a live migration. > > The rest of the patch (can be set in a separate commit) avoid a qemu > crashes > > with live migration when vhost-user is present: > > - When a live migration migration is complete a RARP is automatically > send to > > any net backend (self announce mechanism) > > - vhost user does not provide any receive function and RARP request is > stored > > in the vhost-user queue > > - When a migration back is done all the net backend queues are purged. > The > > stored RARP request for vhost-user is then sent to the register receive > > callback of vhost-user that is NULL. > > > > Support of live migration for vhost user needs the whole patch. (and > maore if > > we want support legacy guest with no support > of VIRTIO_NET_F_GUEST_ANNOUNCE) > > How about implementing a receive function that discards all packets > then? > > > > > > I don't get it. Why do you need any extra messages for old drivers? > To > > > detect old drivers, simply have backend check whether > > > VIRTIO_NET_F_GUEST_ANNOUNCE is set. > > > > For old driver we can not use the mechanism implemented in virtio-net > that > > manages VIRTIO_NET_F_GUEST_ANNOUNCE. In this case we must send the RARP > on the > > network interface created by vhost-user. > > My first idea to do that was to add a message between QEMU and vhost > client/ > > backend (vapp or other) to let the vhost client/backend send the RARP. > > But maybe it will be easier to let QEMU send directly the RARP message > on the > > network interface created by vhost user. > > This point can be done later if it is needed. > > > > Regards. > > Can't vhost-user backend sends this automatically? > Why do we need to do anything in QEMU? > > > > > On Mon, Jun 8, 2015 at 12:12 PM, Michael S. Tsirkin <mst@redhat.com> > wrote: > > > > On Fri, Jun 05, 2015 at 03:24:12PM +0200, Thibaut Collet wrote: > > > Add VIRTIO_NET_F_GUEST_ANNOUNCE capability to vhost-net when netdev > > backend is > > > vhost-user. > > > > > > For netdev backend using virtio-net NIC the self announce is > managed > > directly > > > by the virtio-net NIC and not by the netdev backend itself. > > > To avoid duplication of announces (once from the guest and once > from > > QEMU) a > > > bitfield is added in the NetClientState structure. > > > If this bit is set self announce does not send message to the > guest to > > request > > > gratuitous ARP but let virtio-net NIC set the > VIRTIO_NET_S_ANNOUNCE for > > > gratuitous ARP. > > > > > > Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com> > > > --- > > > v2: do not discard anymore packets send to vhost-user: it is GARP > request > > after > > > live migration. > > > As suggested by S. Hajnoczi qemu_announce_self skips > virtio-net NIC > > that > > > already send GARP. > > > > > > hw/net/vhost_net.c | 2 ++ > > > include/net/net.h | 1 + > > > net/vhost-user.c | 2 ++ > > > savevm.c | 11 ++++++++--- > > > 4 files changed, 13 insertions(+), 3 deletions(-) > > > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > > > index 426b23e..a745f97 100644 > > > --- a/hw/net/vhost_net.c > > > +++ b/hw/net/vhost_net.c > > > @@ -82,6 +82,8 @@ static const int user_feature_bits[] = { > > > VIRTIO_NET_F_CTRL_MAC_ADDR, > > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, > > > > > > + VIRTIO_NET_F_GUEST_ANNOUNCE, > > > + > > > VIRTIO_NET_F_MQ, > > > > > > VHOST_INVALID_FEATURE_BIT > > > diff --git a/include/net/net.h b/include/net/net.h > > > index e66ca03..a78e9df 100644 > > > --- a/include/net/net.h > > > +++ b/include/net/net.h > > > @@ -85,6 +85,7 @@ struct NetClientState { > > > char *name; > > > char info_str[256]; > > > unsigned receive_disabled : 1; > > > + unsigned self_announce_disabled : 1; > > > NetClientDestructor *destructor; > > > unsigned int queue_index; > > > unsigned rxfilter_notify_enabled:1; > > > diff --git a/net/vhost-user.c b/net/vhost-user.c > > > index 8d26728..b345446 100644 > > > --- a/net/vhost-user.c > > > +++ b/net/vhost-user.c > > > @@ -147,6 +147,8 @@ static int net_vhost_user_init(NetClientState > *peer, > > const char *device, > > > > > > s = DO_UPCAST(VhostUserState, nc, nc); > > > > > > + /* Self announce is managed directly by virtio-net NIC */ > > > + s->nc.self_announce_disabled = 1; > > > /* We don't provide a receive callback */ > > > s->nc.receive_disabled = 1; > > > s->chr = chr; > > > diff --git a/savevm.c b/savevm.c > > > index 3b0e222..7a134b1 100644 > > > --- a/savevm.c > > > +++ b/savevm.c > > > @@ -80,11 +80,16 @@ static void qemu_announce_self_iter(NICState > *nic, > > void *opaque) > > > { > > > uint8_t buf[60]; > > > int len; > > > + NetClientState *nc; > > > > > > - > trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr)); > > > - len = announce_self_create(buf, nic->conf->macaddr.a); > > > + nc = qemu_get_queue(nic); > > > > > > - qemu_send_packet_raw(qemu_get_queue(nic), buf, len); > > > + if (!nc->peer->self_announce_disabled) { > > > + trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf-> > > macaddr)); > > > + len = announce_self_create(buf, nic->conf->macaddr.a); > > > + > > > + qemu_send_packet_raw(nc, buf, len); > > > + } > > > } > > > > > > > I don't think qemu_send_packet_raw can ever work for vhost user. > > What happens if you merely add VIRTIO_NET_F_GUEST_ANNOUNCE > > to the feature list, and drop the rest of the patch? > > > > > > > -- > > > 1.7.10.4 > > > > >
On Fri, Jun 05, 2015 at 03:24:12PM +0200, Thibaut Collet wrote: > Add VIRTIO_NET_F_GUEST_ANNOUNCE capability to vhost-net when netdev backend is > vhost-user. > > For netdev backend using virtio-net NIC the self announce is managed directly > by the virtio-net NIC and not by the netdev backend itself. > To avoid duplication of announces (once from the guest and once from QEMU) a > bitfield is added in the NetClientState structure. > If this bit is set self announce does not send message to the guest to request > gratuitous ARP but let virtio-net NIC set the VIRTIO_NET_S_ANNOUNCE for > gratuitous ARP. > > Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com> > --- > v2: do not discard anymore packets send to vhost-user: it is GARP request after > live migration. > As suggested by S. Hajnoczi qemu_announce_self skips virtio-net NIC that > already send GARP. > > hw/net/vhost_net.c | 2 ++ > include/net/net.h | 1 + > net/vhost-user.c | 2 ++ > savevm.c | 11 ++++++++--- > 4 files changed, 13 insertions(+), 3 deletions(-) Please send each patch revision as a new email thread. This makes it easy to see your new patches in threaded email clients. Stefan
On Mon, Jun 08, 2015 at 10:21:38AM +0200, Thibaut Collet wrote: > Hi, > > My understanding of gratuitous packet with virtio for any backend (vhost > user or other): > - When the VM is loaded (first start or migration) the virtio net > interfaces are loaded ( virtio_net_load_device function in > hw/net/virtio-net.c) > - If the guest has the VIRTIO_NET_F_GUEST_ANNOUNCE capability, request to > send gratuitous packet is done. > > 1. To enable gratuitous packet through this mechanism I have added > VIRTIO_NET_F_GUEST_ANNOUNCE > capability to hw/net/vhost_net.c. So host and guest can negotiate this > feature when vhost-user is used. > > 2. self announce occurs in case of live migration. During a live migration > a GARP is sent to all net backend through a queue dedicated to the net > backend. > But for vhost-user: > - this operation is not possible (vhost-user has no queue) > - it is already done with the previous mechanism. > Rather to define a queue to vhost user and notify twice the guest to > send gratuitous packet I have disable GARP from self announce and use only > the first mechanism for that. > > I have tested my modifications with guest that supports > VIRTIO_NET_F_GUEST_ANNOUNCE and vhost-user on the host. After a live > migration I have the GARP from the guest. I think Jason is pointing out that your patch lacks support for guests that do not negotiate VIRTIO_NET_F_GUEST_ANNOUNCE. If the guest does not set the feature bit then packets might continue to get forwarded to the old host. Perhaps the correct place to implement this is in the virtio-net.c device instead of in vhost-user.c. The non-vhost-user case should also skip sending two ARP packets. BUT before we go any further: I've asked several times whether vhost-user support live migration? You didn't answer that question. Fixing this issue only makes sense if vhost-user live migration is supported. Stefan
> I've asked several times whether vhost-user support live migration? vhost-user can support live migration but it needs such fixes of Qemu. I have found the issue and develop my patch by trying live migration with vhost user. > I think Jason is pointing out that your patch lacks support for guests > that do not negotiate VIRTIO_NET_F_GUEST_ANNOUNCE. I have understood the issue with old guest pointed by Jason. I have thinking about the best way to do solve it and any advices are welcome. > Please send each patch revision as a new email thread. This makes it > easy to see your new patches in threaded email clients. I have used the same email thread as it is a reworked of my first patch. I will create a new email thread for the next version. On Mon, Jun 8, 2015 at 3:32 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote: > On Mon, Jun 08, 2015 at 10:21:38AM +0200, Thibaut Collet wrote: > > Hi, > > > > My understanding of gratuitous packet with virtio for any backend (vhost > > user or other): > > - When the VM is loaded (first start or migration) the virtio net > > interfaces are loaded ( virtio_net_load_device function in > > hw/net/virtio-net.c) > > - If the guest has the VIRTIO_NET_F_GUEST_ANNOUNCE capability, request to > > send gratuitous packet is done. > > > > 1. To enable gratuitous packet through this mechanism I have added > > VIRTIO_NET_F_GUEST_ANNOUNCE > > capability to hw/net/vhost_net.c. So host and guest can negotiate this > > feature when vhost-user is used. > > > > 2. self announce occurs in case of live migration. During a live > migration > > a GARP is sent to all net backend through a queue dedicated to the net > > backend. > > But for vhost-user: > > - this operation is not possible (vhost-user has no queue) > > - it is already done with the previous mechanism. > > Rather to define a queue to vhost user and notify twice the guest to > > send gratuitous packet I have disable GARP from self announce and use > only > > the first mechanism for that. > > > > I have tested my modifications with guest that supports > > VIRTIO_NET_F_GUEST_ANNOUNCE and vhost-user on the host. After a live > > migration I have the GARP from the guest. > > I think Jason is pointing out that your patch lacks support for guests > that do not negotiate VIRTIO_NET_F_GUEST_ANNOUNCE. > > If the guest does not set the feature bit then packets might continue to > get forwarded to the old host. > > Perhaps the correct place to implement this is in the virtio-net.c > device instead of in vhost-user.c. The non-vhost-user case should also > skip sending two ARP packets. > > BUT before we go any further: > > I've asked several times whether vhost-user support live migration? > > You didn't answer that question. Fixing this issue only makes sense if > vhost-user live migration is supported. > > Stefan >
On Mon, Jun 8, 2015 at 3:05 PM, Thibaut Collet <thibaut.collet@6wind.com> wrote: >> I think Jason is pointing out that your patch lacks support for guests >> that do not negotiate VIRTIO_NET_F_GUEST_ANNOUNCE. > > I have understood the issue with old guest pointed by Jason. > I have thinking about the best way to do solve it and any advices are > welcome. 1. Add vhost-user virtio-net command to inject packets Add a new VhostUserRequest VHOST_USER_NET_INJECT_PACKET allowing QEMU to hand a packet to the vhost-user process for injection. This command is virtio-net specific and fails if called on a different device type. 2. Fail negotiation when VIRTIO_NET_F_GUEST_ANNOUNCE is not accepted This is only really possible in VIRTIO 1.0 and later. Pre-1.0 does not allow the device to reject due to disabled features. file:///home/stefanha/virtio/trunk/virtio-v1.0-cs02.html#x1-450001 Therefore this is not a great solution. 3. The easiest solution - nop .receive() Just implement a nop .receive() which drops the packet and prints a warning message to stderr. This way VIRTIO_NET_F_GUEST_ANNOUNCE guests work while legacy guests don't send a gratuitous ARP packet. Stefan
> 3. The easiest solution - nop .receive() The solution 3 is similar of my implementation and does not solve issue pointed by Jason: legacy guest do not send a gratuitous ARP. > 2. Fail negotiation when VIRTIO_NET_F_GUEST_ANNOUNCE is not accepted > file:///home/stefanha/virtio/trunk/virtio-v1.0-cs02.html#x1-450001 Could you send me the virtio-v1.0-cs02.html#x1-450001 if you think it can help me ? Nevertheless it seems that only solution 1 can provide a solution to the issue pointed by Jason. On Mon, Jun 8, 2015 at 5:13 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Mon, Jun 8, 2015 at 3:05 PM, Thibaut Collet <thibaut.collet@6wind.com> > wrote: > >> I think Jason is pointing out that your patch lacks support for guests > >> that do not negotiate VIRTIO_NET_F_GUEST_ANNOUNCE. > > > > I have understood the issue with old guest pointed by Jason. > > I have thinking about the best way to do solve it and any advices are > > welcome. > > 1. Add vhost-user virtio-net command to inject packets > > Add a new VhostUserRequest VHOST_USER_NET_INJECT_PACKET allowing QEMU > to hand a packet to the vhost-user process for injection. This > command is virtio-net specific and fails if called on a different > device type. > > 2. Fail negotiation when VIRTIO_NET_F_GUEST_ANNOUNCE is not accepted > > This is only really possible in VIRTIO 1.0 and later. Pre-1.0 does > not allow the device to reject due to disabled features. > > file:///home/stefanha/virtio/trunk/virtio-v1.0-cs02.html#x1-450001 > > Therefore this is not a great solution. > > 3. The easiest solution - nop .receive() > > Just implement a nop .receive() which drops the packet and prints a > warning message to stderr. This way VIRTIO_NET_F_GUEST_ANNOUNCE > guests work while legacy guests don't send a gratuitous ARP packet. > > Stefan >
On Mon, Jun 08, 2015 at 03:20:23PM +0200, Thibaut Collet wrote: > > How about implementing a receive function that discards all packets > > then? > > Implementing a receive function that discards all packets is an other solution. > I have not chosen this one to avoid to mask other potential issues: > Normally vhost-user never receives packets. By keeping a NULL function as > received callback we can detect easily case where packets are sent (qemu > crashes) and solve this issue. What I have an issue with is the fact that you make it depend on guest configuration. vhost user backend can't get packets from qemu at all, it doesn't make sense to disable sending packets only if guest set some flag. In particular, this flag isn't set around guest reset. > > Can't vhost-user backend sends this automatically? > > Why do we need to do anything in QEMU? > > My explanations are maybe unclear. For old driver we have to send the RARP to > the guest through the network interface (and to implement a receive function > for vhost-user). My question is: where is the best location to do that: > 1. In the receive function of vhost-user (in the file net/vhost-user.c) > 2. In a self announce function (called when vhost-user receives a RARP, > analysis of the packet content is needed in this case) in the file hw/net/ > vhost-net.c > 3. In the vhost-user backend (file hw/virtio/vhost-user.c). In this case a new > message must be defined between hw/net/vhost-net.c and hw/virtio/vhost-user.c. > 4. In the vhost client/backend (vapp or other) In the vhost client/backend would be best. See any issues with this?
On Mon, Jun 08, 2015 at 04:13:59PM +0100, Stefan Hajnoczi wrote: > On Mon, Jun 8, 2015 at 3:05 PM, Thibaut Collet <thibaut.collet@6wind.com> wrote: > >> I think Jason is pointing out that your patch lacks support for guests > >> that do not negotiate VIRTIO_NET_F_GUEST_ANNOUNCE. > > > > I have understood the issue with old guest pointed by Jason. > > I have thinking about the best way to do solve it and any advices are > > welcome. > > 1. Add vhost-user virtio-net command to inject packets > > Add a new VhostUserRequest VHOST_USER_NET_INJECT_PACKET allowing QEMU > to hand a packet to the vhost-user process for injection. This > command is virtio-net specific and fails if called on a different > device type. > > 2. Fail negotiation when VIRTIO_NET_F_GUEST_ANNOUNCE is not accepted > > This is only really possible in VIRTIO 1.0 and later. Pre-1.0 does > not allow the device to reject due to disabled features. > > file:///home/stefanha/virtio/trunk/virtio-v1.0-cs02.html#x1-450001 > > Therefore this is not a great solution. > > 3. The easiest solution - nop .receive() > > Just implement a nop .receive() which drops the packet and prints a > warning message to stderr. This way VIRTIO_NET_F_GUEST_ANNOUNCE > guests work while legacy guests don't send a gratuitous ARP packet. > > Stefan 3 sounds good to me.
On 06/09/2015 12:13 AM, Michael S. Tsirkin wrote: > On Mon, Jun 08, 2015 at 04:13:59PM +0100, Stefan Hajnoczi wrote: >> On Mon, Jun 8, 2015 at 3:05 PM, Thibaut Collet <thibaut.collet@6wind.com> wrote: >>>> I think Jason is pointing out that your patch lacks support for guests >>>> that do not negotiate VIRTIO_NET_F_GUEST_ANNOUNCE. >>> I have understood the issue with old guest pointed by Jason. >>> I have thinking about the best way to do solve it and any advices are >>> welcome. >> 1. Add vhost-user virtio-net command to inject packets >> >> Add a new VhostUserRequest VHOST_USER_NET_INJECT_PACKET allowing QEMU >> to hand a packet to the vhost-user process for injection. This >> command is virtio-net specific and fails if called on a different >> device type. >> >> 2. Fail negotiation when VIRTIO_NET_F_GUEST_ANNOUNCE is not accepted >> >> This is only really possible in VIRTIO 1.0 and later. Pre-1.0 does >> not allow the device to reject due to disabled features. >> >> file:///home/stefanha/virtio/trunk/virtio-v1.0-cs02.html#x1-450001 >> >> Therefore this is not a great solution. >> >> 3. The easiest solution - nop .receive() >> >> Just implement a nop .receive() which drops the packet and prints a >> warning message to stderr. This way VIRTIO_NET_F_GUEST_ANNOUNCE >> guests work while legacy guests don't send a gratuitous ARP packet. >> >> Stefan > 3 sounds good to me. > And looks like we can remove the s->nc.receive_disabled = 1; in vhost-user.c in this case.
On Mon, Jun 08, 2015 at 05:32:38PM +0200, Thibaut Collet wrote: > > 3. The easiest solution - nop .receive() > > The solution 3 is similar of my implementation and does not solve issue > pointed by Jason: legacy guest do not send a gratuitous ARP. > > > 2. Fail negotiation when VIRTIO_NET_F_GUEST_ANNOUNCE is not accepted > > file:///home/stefanha/virtio/trunk/virtio-v1.0-cs02.html#x1-450001 > > Could you send me the virtio-v1.0-cs02.html#x1-450001 if you think it can > help me ? Sorry, for the local link. The link is just for background context: http://docs.oasis-open.org/virtio/virtio/v1.0/csprd01/virtio-v1.0-csprd01.html#x1-230001 Stefan
On Mon, Jun 08, 2015 at 05:32:38PM +0200, Thibaut Collet wrote: > > 3. The easiest solution - nop .receive() > > The solution 3 is similar of my implementation and does not solve issue > pointed by Jason: legacy guest do not send a gratuitous ARP. Yes, but it prints a warning so the user is aware they are operating in a degraded state. Sending gratuitous ARP packets minimizes the time that network infrastructure forwards packets to the old host. But the network infrastructure will learn about the new host as long as the guest sends outgoing packets after migration. Putting these two things together, it seems acceptable to do #3. Most users should be running a modern virtio-net that supports the announce feature anyway. Stefan
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index 426b23e..a745f97 100644 --- a/hw/net/vhost_net.c +++ b/hw/net/vhost_net.c @@ -82,6 +82,8 @@ static const int user_feature_bits[] = { VIRTIO_NET_F_CTRL_MAC_ADDR, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, + VIRTIO_NET_F_GUEST_ANNOUNCE, + VIRTIO_NET_F_MQ, VHOST_INVALID_FEATURE_BIT diff --git a/include/net/net.h b/include/net/net.h index e66ca03..a78e9df 100644 --- a/include/net/net.h +++ b/include/net/net.h @@ -85,6 +85,7 @@ struct NetClientState { char *name; char info_str[256]; unsigned receive_disabled : 1; + unsigned self_announce_disabled : 1; NetClientDestructor *destructor; unsigned int queue_index; unsigned rxfilter_notify_enabled:1; diff --git a/net/vhost-user.c b/net/vhost-user.c index 8d26728..b345446 100644 --- a/net/vhost-user.c +++ b/net/vhost-user.c @@ -147,6 +147,8 @@ static int net_vhost_user_init(NetClientState *peer, const char *device, s = DO_UPCAST(VhostUserState, nc, nc); + /* Self announce is managed directly by virtio-net NIC */ + s->nc.self_announce_disabled = 1; /* We don't provide a receive callback */ s->nc.receive_disabled = 1; s->chr = chr; diff --git a/savevm.c b/savevm.c index 3b0e222..7a134b1 100644 --- a/savevm.c +++ b/savevm.c @@ -80,11 +80,16 @@ static void qemu_announce_self_iter(NICState *nic, void *opaque) { uint8_t buf[60]; int len; + NetClientState *nc; - trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr)); - len = announce_self_create(buf, nic->conf->macaddr.a); + nc = qemu_get_queue(nic); - qemu_send_packet_raw(qemu_get_queue(nic), buf, len); + if (!nc->peer->self_announce_disabled) { + trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic->conf->macaddr)); + len = announce_self_create(buf, nic->conf->macaddr.a); + + qemu_send_packet_raw(nc, buf, len); + } }
Add VIRTIO_NET_F_GUEST_ANNOUNCE capability to vhost-net when netdev backend is vhost-user. For netdev backend using virtio-net NIC the self announce is managed directly by the virtio-net NIC and not by the netdev backend itself. To avoid duplication of announces (once from the guest and once from QEMU) a bitfield is added in the NetClientState structure. If this bit is set self announce does not send message to the guest to request gratuitous ARP but let virtio-net NIC set the VIRTIO_NET_S_ANNOUNCE for gratuitous ARP. Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com> --- v2: do not discard anymore packets send to vhost-user: it is GARP request after live migration. As suggested by S. Hajnoczi qemu_announce_self skips virtio-net NIC that already send GARP. hw/net/vhost_net.c | 2 ++ include/net/net.h | 1 + net/vhost-user.c | 2 ++ savevm.c | 11 ++++++++--- 4 files changed, 13 insertions(+), 3 deletions(-)