From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47624) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z1vFQ-0002tg-1m for qemu-devel@nongnu.org; Mon, 08 Jun 2015 07:29:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z1vFM-0004G3-IF for qemu-devel@nongnu.org; Mon, 08 Jun 2015 07:29:43 -0400 Received: from mail-ig0-f180.google.com ([209.85.213.180]:37375) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z1vFM-0004Fj-CH for qemu-devel@nongnu.org; Mon, 08 Jun 2015 07:29:40 -0400 Received: by igbsb11 with SMTP id sb11so54677326igb.0 for ; Mon, 08 Jun 2015 04:29:39 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20150608121022-mutt-send-email-mst@redhat.com> References: <55701BED.2090802@redhat.com> <1433510652-13866-1-git-send-email-thibaut.collet@6wind.com> <20150608121022-mutt-send-email-mst@redhat.com> Date: Mon, 8 Jun 2015 13:29:39 +0200 Message-ID: From: Thibaut Collet Content-Type: multipart/alternative; boundary=089e0111b970175ac60517fff553 Subject: Re: [Qemu-devel] [PATCH v2] net: Add support of VIRTIO_NET_F_GUEST_ANNOUNCE for vhost-net/vhost-user List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Jason Wang , qemu-devel@nongnu.org, Stefan Hajnoczi , quintela@redhat.com --089e0111b970175ac60517fff553 Content-Type: text/plain; charset=UTF-8 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 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 > > --- > > 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 > --089e0111b970175ac60517fff553 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi,
<= span style=3D"font-size:13px">
> I don't think qemu_send_packet_raw can = ever work for vhost user.
> What happens if you merely add VIRTIO_NET_F_GUEST_ANN= OUNCE
>= ; to the feature list, and drop the rest of the patch?

If you merely=C2=A0add VIR= TIO_NET_F_GUEST_ANNOUNCE you have the GARP with recent guest after a live m= igration.
The rest of the p= atch (can be set in a separate commit) avoid a qemu crashes with live migra= tion when vhost-user is present:
=C2=A0- When a live migration migration is complete a RARP is automat= ically send to any net backend (self = announce mechanism)
=C2=A0-= vhost user does not provide any receive function and RARP request is store= d in the vhost-user queue
= =C2=A0- 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 recei= ve callback of vhost-user that is NULL.

Support o= f live migration for vhost user needs the whole patch. (and maore if we wan= t support legacy guest with no support of=C2=A0VIRTIO_NET_F_GUEST_ANNOUNCE)


> I d= on't get it.=C2=A0 Why do you need any extra messages for old drivers?= =C2=A0 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 t= hat manages=C2=A0VIRTIO_NET_F_GUEST_A= NNOUNCE. In this case we must send the RARP on the network interface create= d by vhost-user.
My first i= dea to do that was to add a message between QEMU and=C2=A0vhost client/backend (vapp or other) = to let the vhost cli= ent/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 l= ater 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:1= 2PM +0200, Thibaut Collet wrote:
> Add VIRTIO_NET_F_GUEST_ANNOUNCE capability to vhost-net when netdev ba= ckend is
> vhost-user.
>
> For netdev backend using virtio-net NIC the self announce is managed d= irectly
> by the virtio-net NIC and not by the netdev backend itself.
> To avoid duplication of announces (once from the guest and once from Q= EMU) 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 fo= r
> gratuitous ARP.
>
> Signed-off-by: Thibaut Collet <thibaut.collet@6wind.com>
> ---
> v2: do not discard anymore packets send to vhost-user: it is GARP requ= est after
>=C2=A0 =C2=A0 =C2=A0live migration.
>=C2=A0 =C2=A0 =C2=A0As suggested by S. Hajnoczi qemu_announce_self skip= s virtio-net NIC that
>=C2=A0 =C2=A0 =C2=A0already send GARP.
>
>=C2=A0 hw/net/vhost_net.c |=C2=A0 =C2=A0 2 ++
>=C2=A0 include/net/net.h=C2=A0 |=C2=A0 =C2=A0 1 +
>=C2=A0 net/vhost-user.c=C2=A0 =C2=A0|=C2=A0 =C2=A0 2 ++
>=C2=A0 savevm.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 =C2=A01= 1 ++++++++---
>=C2=A0 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[] =3D {
>=C2=A0 =C2=A0 =C2=A0 VIRTIO_NET_F_CTRL_MAC_ADDR,
>=C2=A0 =C2=A0 =C2=A0 VIRTIO_NET_F_CTRL_GUEST_OFFLOADS,
>
> +=C2=A0 =C2=A0 VIRTIO_NET_F_GUEST_ANNOUNCE,
> +
>=C2=A0 =C2=A0 =C2=A0 VIRTIO_NET_F_MQ,
>
>=C2=A0 =C2=A0 =C2=A0 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 {
>=C2=A0 =C2=A0 =C2=A0 char *name;
>=C2=A0 =C2=A0 =C2=A0 char info_str[256];
>=C2=A0 =C2=A0 =C2=A0 unsigned receive_disabled : 1;
> +=C2=A0 =C2=A0 unsigned self_announce_disabled : 1;
>=C2=A0 =C2=A0 =C2=A0 NetClientDestructor *destructor;
>=C2=A0 =C2=A0 =C2=A0 unsigned int queue_index;
>=C2=A0 =C2=A0 =C2=A0 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 *pee= r, const char *device,
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 s =3D DO_UPCAST(VhostUserState, nc, = nc);
>
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Self announce is managed directly by v= irtio-net NIC */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 s->nc.self_announce_disabled =3D 1; >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* We don't provide a receive ca= llback */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 s->nc.receive_disabled =3D 1;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 s->chr =3D 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)
>=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 uint8_t buf[60];
>=C2=A0 =C2=A0 =C2=A0 int len;
> +=C2=A0 =C2=A0 NetClientState *nc;
>
> -=C2=A0 =C2=A0 trace_qemu_announce_self_iter(qemu_ether_ntoa(&nic-= >conf->macaddr));
> -=C2=A0 =C2=A0 len =3D announce_self_create(buf, nic->conf->maca= ddr.a);
> +=C2=A0 =C2=A0 nc =3D qemu_get_queue(nic);
>
> -=C2=A0 =C2=A0 qemu_send_packet_raw(qemu_get_queue(nic), buf, len); > +=C2=A0 =C2=A0 if (!nc->peer->self_announce_disabled) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 trace_qemu_announce_self_iter(qemu_ether_= ntoa(&nic->conf->macaddr));
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 len =3D announce_self_create(buf, nic->= ;conf->macaddr.a);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_send_packet_raw(nc, buf, len);
> +=C2=A0 =C2=A0 }
>=C2=A0 }
>

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

--089e0111b970175ac60517fff553--