From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44375) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z1sJU-0007qB-Fs for qemu-devel@nongnu.org; Mon, 08 Jun 2015 04:21:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z1sJQ-0006M5-5G for qemu-devel@nongnu.org; Mon, 08 Jun 2015 04:21:44 -0400 Received: from mail-ig0-f171.google.com ([209.85.213.171]:34200) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z1sJP-0006Lw-Uw for qemu-devel@nongnu.org; Mon, 08 Jun 2015 04:21:40 -0400 Received: by igbhj9 with SMTP id hj9so53939818igb.1 for ; Mon, 08 Jun 2015 01:21:39 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <55752E3C.8070700@redhat.com> References: <55701BED.2090802@redhat.com> <1433510652-13866-1-git-send-email-thibaut.collet@6wind.com> <55752E3C.8070700@redhat.com> Date: Mon, 8 Jun 2015 10:21:38 +0200 Message-ID: From: Thibaut Collet Content-Type: multipart/alternative; boundary=001a113ed184b875bb0517fd5403 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: Jason Wang Cc: mst@redhat.com, qemu-devel@nongnu.org, Stefan Hajnoczi , quintela@redhat.com --001a113ed184b875bb0517fd5403 Content-Type: text/plain; charset=UTF-8 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 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 > > --- > > 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); > > + } > > } > > > > > > --001a113ed184b875bb0517fd5403 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi,

My understanding of gratuitous pack= et with virtio for any backend (vhost user or other):
- When the = VM is loaded (first start or migration) the virtio net interfaces are loade= d (=C2=A0virtio_net_load_device function in hw/net/virtio-net.c)
= - If the guest has the VIRTIO_NET_F_GUEST_ANNOUNCE capability, request to s= end gratuitous packet is done.

1. To enable=C2=A0<= span style=3D"font-size:13px">gratuitous packet through this mechanism I ha= ve added=C2=A0VIRTIO_NET_F_GUEST_ANNOUNCE capability to=C2=A0hw/net/vhost_net.c. So host and guest can negotiate= this feature when vhost-user is used.

2.=C2=A0self announce occurs in case of live migration. During a live migratio= n a GARP is sent to all net backend through a queue dedicated to the net ba= ckend.
=C2=A0 =C2=A0But for vhost-user:
=C2=A0 =C2=A0 = =C2=A0 =C2=A0- this operation is not possible (vhost-user has no queue)
=C2=A0 =C2=A0 =C2=A0 =C2=A0- it is already done with the previous me= chanism.
=C2=A0 =C2=A0Rather to define a queue to vhost user and = notify twice the guest to send gratuitous packet I have disable GARP from s= elf 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 th= e 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 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;
Shouldn't we do this in set_features consider it needs gues= t support or
you want to disable gratuitous packet for vhost-user at all?

>=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 }
>
>


--001a113ed184b875bb0517fd5403--