| Message ID | 1465304408-16545-1-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
On Tue, Jun 7, 2016 at 9:00 PM, Peter Lieven <pl@kamp.de> wrote: > commit fefe2a78 accidently dropped the code path for injecting > raw packets. This feature is needed for sending gratuitous ARPs > after an incoming migration has completed. The result is increased > network downtime for vservers where the network card is not virtio-net > with the VIRTIO_NET_F_GUEST_ANNOUNCE feature. > > Fixes: fefe2a78abde932e0f340b21bded2c86def1d242 > Cc: qemu-stable@nongnu.org > Cc: hongyang.yang@easystack.cn > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > v1->v2: assert that only raw packets with a plain buffer come in. [Paolo] > > net/net.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/net/net.c b/net/net.c > index 5f3e5a9..5e1b5fa 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -714,6 +714,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState > *sender, > NetClientState *nc = opaque; > int ret; > > + /* we currently only support sending raw packets via > qemu_send_packet_raw. > + * If we want generic raw iov support we need to implement something > like > + * .receive_raw_iov in NetClientInfo first. */ > + assert(!(flags & QEMU_NET_PACKET_FLAG_RAW) || iovcnt == 1); > + > This assert is hard to understand IMO...I think move assert(iovcnt==1) as well as the comment to if condition below as Paolo said previously is better... > if (nc->link_down) { > return iov_size(iov, iovcnt); > } > @@ -722,7 +727,10 @@ ssize_t qemu_deliver_packet_iov(NetClientState > *sender, > return 0; > } > > - if (nc->info->receive_iov) { > + if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { > + /* this is required for qemu_announce_self() */ > + ret = nc->info->receive_raw(nc, iov[0].iov_base, iov[0].iov_len); > + } else if (nc->info->receive_iov) { > ret = nc->info->receive_iov(nc, iov, iovcnt); > } else { > ret = nc_sendv_compat(nc, iov, iovcnt, flags); > -- > 1.9.1 > > >
Am 07.06.2016 um 17:28 schrieb Yang Hongyang: > > > On Tue, Jun 7, 2016 at 9:00 PM, Peter Lieven <pl@kamp.de <mailto:pl@kamp.de>> wrote: > > commit fefe2a78 accidently dropped the code path for injecting > raw packets. This feature is needed for sending gratuitous ARPs > after an incoming migration has completed. The result is increased > network downtime for vservers where the network card is not virtio-net > with the VIRTIO_NET_F_GUEST_ANNOUNCE feature. > > Fixes: fefe2a78abde932e0f340b21bded2c86def1d242 > Cc: qemu-stable@nongnu.org <mailto:qemu-stable@nongnu.org> > Cc: hongyang.yang@easystack.cn <mailto:hongyang.yang@easystack.cn> > Signed-off-by: Peter Lieven <pl@kamp.de <mailto:pl@kamp.de>> > --- > v1->v2: assert that only raw packets with a plain buffer come in. [Paolo] > > net/net.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/net/net.c b/net/net.c > index 5f3e5a9..5e1b5fa 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -714,6 +714,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, > NetClientState *nc = opaque; > int ret; > > + /* we currently only support sending raw packets via qemu_send_packet_raw. > + * If we want generic raw iov support we need to implement something like > + * .receive_raw_iov in NetClientInfo first. */ > + assert(!(flags & QEMU_NET_PACKET_FLAG_RAW) || iovcnt == 1); > + > > > This assert is hard to understand IMO...I think move assert(iovcnt==1) as well as the > comment to if condition below as Paolo said previously is better... This assert would only trigger if the NetClient implements .receive_raw. This way it could happen that someone injects RAW packets via an other way than qemu_send_packet_raw. But he might not see that its not allowed if he uses a backend that does not implement sending raw packets. If then someone uses a tap backend things break again. Peter
On 2016年06月07日 21:00, Peter Lieven wrote: > commit fefe2a78 accidently dropped the code path for injecting > raw packets. This feature is needed for sending gratuitous ARPs > after an incoming migration has completed. The result is increased > network downtime for vservers where the network card is not virtio-net > with the VIRTIO_NET_F_GUEST_ANNOUNCE feature. > > Fixes: fefe2a78abde932e0f340b21bded2c86def1d242 > Cc: qemu-stable@nongnu.org > Cc: hongyang.yang@easystack.cn > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > v1->v2: assert that only raw packets with a plain buffer come in. [Paolo] > > net/net.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/net/net.c b/net/net.c > index 5f3e5a9..5e1b5fa 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -714,6 +714,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, > NetClientState *nc = opaque; > int ret; > > + /* we currently only support sending raw packets via qemu_send_packet_raw. > + * If we want generic raw iov support we need to implement something like > + * .receive_raw_iov in NetClientInfo first. */ > + assert(!(flags & QEMU_NET_PACKET_FLAG_RAW) || iovcnt == 1); > + > if (nc->link_down) { > return iov_size(iov, iovcnt); > } > @@ -722,7 +727,10 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, > return 0; > } > > - if (nc->info->receive_iov) { > + if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { > + /* this is required for qemu_announce_self() */ > + ret = nc->info->receive_raw(nc, iov[0].iov_base, iov[0].iov_len); > + } else if (nc->info->receive_iov) { > ret = nc->info->receive_iov(nc, iov, iovcnt); > } else { > ret = nc_sendv_compat(nc, iov, iovcnt, flags); We still have raw packet support in nv_sendv_compat, can we use it? (and then there's no need for the assert above). Thanks
Am 08.06.2016 um 08:39 schrieb Jason Wang: > > > On 2016年06月07日 21:00, Peter Lieven wrote: >> commit fefe2a78 accidently dropped the code path for injecting >> raw packets. This feature is needed for sending gratuitous ARPs >> after an incoming migration has completed. The result is increased >> network downtime for vservers where the network card is not virtio-net >> with the VIRTIO_NET_F_GUEST_ANNOUNCE feature. >> >> Fixes: fefe2a78abde932e0f340b21bded2c86def1d242 >> Cc: qemu-stable@nongnu.org >> Cc: hongyang.yang@easystack.cn >> Signed-off-by: Peter Lieven <pl@kamp.de> >> --- >> v1->v2: assert that only raw packets with a plain buffer come in. [Paolo] >> >> net/net.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/net/net.c b/net/net.c >> index 5f3e5a9..5e1b5fa 100644 >> --- a/net/net.c >> +++ b/net/net.c >> @@ -714,6 +714,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, >> NetClientState *nc = opaque; >> int ret; >> + /* we currently only support sending raw packets via qemu_send_packet_raw. >> + * If we want generic raw iov support we need to implement something like >> + * .receive_raw_iov in NetClientInfo first. */ >> + assert(!(flags & QEMU_NET_PACKET_FLAG_RAW) || iovcnt == 1); >> + >> if (nc->link_down) { >> return iov_size(iov, iovcnt); >> } >> @@ -722,7 +727,10 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, >> return 0; >> } >> - if (nc->info->receive_iov) { >> + if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { >> + /* this is required for qemu_announce_self() */ >> + ret = nc->info->receive_raw(nc, iov[0].iov_base, iov[0].iov_len); >> + } else if (nc->info->receive_iov) { >> ret = nc->info->receive_iov(nc, iov, iovcnt); >> } else { >> ret = nc_sendv_compat(nc, iov, iovcnt, flags); > > We still have raw packet support in nv_sendv_compat, can we use it? (and then there's no need for the assert above). Okay, so Version 1 of the patch is more appropiate. I would either use V1 or revert fefe2a78. The issue is quite serious for live migration. Peter
On 2016年06月08日 15:13, Peter Lieven wrote: > Am 08.06.2016 um 08:39 schrieb Jason Wang: >> >> On 2016年06月07日 21:00, Peter Lieven wrote: >>> commit fefe2a78 accidently dropped the code path for injecting >>> raw packets. This feature is needed for sending gratuitous ARPs >>> after an incoming migration has completed. The result is increased >>> network downtime for vservers where the network card is not virtio-net >>> with the VIRTIO_NET_F_GUEST_ANNOUNCE feature. >>> >>> Fixes: fefe2a78abde932e0f340b21bded2c86def1d242 >>> Cc: qemu-stable@nongnu.org >>> Cc: hongyang.yang@easystack.cn >>> Signed-off-by: Peter Lieven <pl@kamp.de> >>> --- >>> v1->v2: assert that only raw packets with a plain buffer come in. [Paolo] >>> >>> net/net.c | 10 +++++++++- >>> 1 file changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/net/net.c b/net/net.c >>> index 5f3e5a9..5e1b5fa 100644 >>> --- a/net/net.c >>> +++ b/net/net.c >>> @@ -714,6 +714,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, >>> NetClientState *nc = opaque; >>> int ret; >>> + /* we currently only support sending raw packets via qemu_send_packet_raw. >>> + * If we want generic raw iov support we need to implement something like >>> + * .receive_raw_iov in NetClientInfo first. */ >>> + assert(!(flags & QEMU_NET_PACKET_FLAG_RAW) || iovcnt == 1); >>> + >>> if (nc->link_down) { >>> return iov_size(iov, iovcnt); >>> } >>> @@ -722,7 +727,10 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, >>> return 0; >>> } >>> - if (nc->info->receive_iov) { >>> + if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { >>> + /* this is required for qemu_announce_self() */ >>> + ret = nc->info->receive_raw(nc, iov[0].iov_base, iov[0].iov_len); >>> + } else if (nc->info->receive_iov) { >>> ret = nc->info->receive_iov(nc, iov, iovcnt); >>> } else { >>> ret = nc_sendv_compat(nc, iov, iovcnt, flags); >> We still have raw packet support in nv_sendv_compat, can we use it? (and then there's no need for the assert above). > Okay, so Version 1 of the patch is more appropiate. I would either use V1 or revert fefe2a78. The issue is quite serious > for live migration. > > Peter > > V1 checks iovcnt and call receive_raw with hard coded first iov. Since we have raw packet support in nc_sendv_compat(), I mean when we detect QEMU_NET_PACKET_FLAG_RAW, call nc_sendv_compat() here.
----- Original Message ----- > From: "Jason Wang" <jasowang@redhat.com> > To: "Peter Lieven" <pl@kamp.de>, qemu-devel@nongnu.org > Cc: pbonzini@redhat.com, qemu-stable@nongnu.org, "hongyang yang" <hongyang.yang@easystack.cn> > Sent: Wednesday, June 8, 2016 9:54:53 AM > Subject: Re: [Qemu-devel] [Qemu-stable] [PATCH V2] net: fix qemu_announce_self not emitting packets > > > > On 2016年06月08日 15:13, Peter Lieven wrote: > > Am 08.06.2016 um 08:39 schrieb Jason Wang: > >> > >> On 2016年06月07日 21:00, Peter Lieven wrote: > >>> commit fefe2a78 accidently dropped the code path for injecting > >>> raw packets. This feature is needed for sending gratuitous ARPs > >>> after an incoming migration has completed. The result is increased > >>> network downtime for vservers where the network card is not virtio-net > >>> with the VIRTIO_NET_F_GUEST_ANNOUNCE feature. > >>> > >>> Fixes: fefe2a78abde932e0f340b21bded2c86def1d242 > >>> Cc: qemu-stable@nongnu.org > >>> Cc: hongyang.yang@easystack.cn > >>> Signed-off-by: Peter Lieven <pl@kamp.de> > >>> --- > >>> v1->v2: assert that only raw packets with a plain buffer come in. [Paolo] > >>> > >>> net/net.c | 10 +++++++++- > >>> 1 file changed, 9 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/net/net.c b/net/net.c > >>> index 5f3e5a9..5e1b5fa 100644 > >>> --- a/net/net.c > >>> +++ b/net/net.c > >>> @@ -714,6 +714,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState > >>> *sender, > >>> NetClientState *nc = opaque; > >>> int ret; > >>> + /* we currently only support sending raw packets via > >>> qemu_send_packet_raw. > >>> + * If we want generic raw iov support we need to implement something > >>> like > >>> + * .receive_raw_iov in NetClientInfo first. */ > >>> + assert(!(flags & QEMU_NET_PACKET_FLAG_RAW) || iovcnt == 1); > >>> + > >>> if (nc->link_down) { > >>> return iov_size(iov, iovcnt); > >>> } > >>> @@ -722,7 +727,10 @@ ssize_t qemu_deliver_packet_iov(NetClientState > >>> *sender, > >>> return 0; > >>> } > >>> - if (nc->info->receive_iov) { > >>> + if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { > >>> + /* this is required for qemu_announce_self() */ > >>> + ret = nc->info->receive_raw(nc, iov[0].iov_base, > >>> iov[0].iov_len); > >>> + } else if (nc->info->receive_iov) { > >>> ret = nc->info->receive_iov(nc, iov, iovcnt); > >>> } else { > >>> ret = nc_sendv_compat(nc, iov, iovcnt, flags); > >> We still have raw packet support in nv_sendv_compat, can we use it? (and > >> then there's no need for the assert above). > > Okay, so Version 1 of the patch is more appropiate. I would either use V1 > > or revert fefe2a78. The issue is quite serious > > for live migration. Or you can do even simpler: if (nc->info->receive_iov && !(flags & QEMU_NET_PACKET_FLAG_RAW))) nc->info->receive_iov else nc_sendv_compat Paolo > > > > Peter > > > > > > V1 checks iovcnt and call receive_raw with hard coded first iov. Since > we have raw packet support in nc_sendv_compat(), I mean when we detect > QEMU_NET_PACKET_FLAG_RAW, call nc_sendv_compat() here. >
Am 08.06.2016 um 12:23 schrieb Paolo Bonzini: > > ----- Original Message ----- >> From: "Jason Wang" <jasowang@redhat.com> >> To: "Peter Lieven" <pl@kamp.de>, qemu-devel@nongnu.org >> Cc: pbonzini@redhat.com, qemu-stable@nongnu.org, "hongyang yang" <hongyang.yang@easystack.cn> >> Sent: Wednesday, June 8, 2016 9:54:53 AM >> Subject: Re: [Qemu-devel] [Qemu-stable] [PATCH V2] net: fix qemu_announce_self not emitting packets >> >> >> >> On 2016年06月08日 15:13, Peter Lieven wrote: >>> Am 08.06.2016 um 08:39 schrieb Jason Wang: >>>> On 2016年06月07日 21:00, Peter Lieven wrote: >>>>> commit fefe2a78 accidently dropped the code path for injecting >>>>> raw packets. This feature is needed for sending gratuitous ARPs >>>>> after an incoming migration has completed. The result is increased >>>>> network downtime for vservers where the network card is not virtio-net >>>>> with the VIRTIO_NET_F_GUEST_ANNOUNCE feature. >>>>> >>>>> Fixes: fefe2a78abde932e0f340b21bded2c86def1d242 >>>>> Cc: qemu-stable@nongnu.org >>>>> Cc: hongyang.yang@easystack.cn >>>>> Signed-off-by: Peter Lieven <pl@kamp.de> >>>>> --- >>>>> v1->v2: assert that only raw packets with a plain buffer come in. [Paolo] >>>>> >>>>> net/net.c | 10 +++++++++- >>>>> 1 file changed, 9 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/net/net.c b/net/net.c >>>>> index 5f3e5a9..5e1b5fa 100644 >>>>> --- a/net/net.c >>>>> +++ b/net/net.c >>>>> @@ -714,6 +714,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState >>>>> *sender, >>>>> NetClientState *nc = opaque; >>>>> int ret; >>>>> + /* we currently only support sending raw packets via >>>>> qemu_send_packet_raw. >>>>> + * If we want generic raw iov support we need to implement something >>>>> like >>>>> + * .receive_raw_iov in NetClientInfo first. */ >>>>> + assert(!(flags & QEMU_NET_PACKET_FLAG_RAW) || iovcnt == 1); >>>>> + >>>>> if (nc->link_down) { >>>>> return iov_size(iov, iovcnt); >>>>> } >>>>> @@ -722,7 +727,10 @@ ssize_t qemu_deliver_packet_iov(NetClientState >>>>> *sender, >>>>> return 0; >>>>> } >>>>> - if (nc->info->receive_iov) { >>>>> + if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { >>>>> + /* this is required for qemu_announce_self() */ >>>>> + ret = nc->info->receive_raw(nc, iov[0].iov_base, >>>>> iov[0].iov_len); >>>>> + } else if (nc->info->receive_iov) { >>>>> ret = nc->info->receive_iov(nc, iov, iovcnt); >>>>> } else { >>>>> ret = nc_sendv_compat(nc, iov, iovcnt, flags); >>>> We still have raw packet support in nv_sendv_compat, can we use it? (and >>>> then there's no need for the assert above). >>> Okay, so Version 1 of the patch is more appropiate. I would either use V1 >>> or revert fefe2a78. The issue is quite serious >>> for live migration. > Or you can do even simpler: > > if (nc->info->receive_iov && !(flags & QEMU_NET_PACKET_FLAG_RAW))) > nc->info->receive_iov > else > nc_sendv_compat I should have looked into this nc_sendv_compat function before ;-) This indeed is the best solution I think. Will send a v3 tomorrow. Peter
diff --git a/net/net.c b/net/net.c index 5f3e5a9..5e1b5fa 100644 --- a/net/net.c +++ b/net/net.c @@ -714,6 +714,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, NetClientState *nc = opaque; int ret; + /* we currently only support sending raw packets via qemu_send_packet_raw. + * If we want generic raw iov support we need to implement something like + * .receive_raw_iov in NetClientInfo first. */ + assert(!(flags & QEMU_NET_PACKET_FLAG_RAW) || iovcnt == 1); + if (nc->link_down) { return iov_size(iov, iovcnt); } @@ -722,7 +727,10 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender, return 0; } - if (nc->info->receive_iov) { + if (flags & QEMU_NET_PACKET_FLAG_RAW && nc->info->receive_raw) { + /* this is required for qemu_announce_self() */ + ret = nc->info->receive_raw(nc, iov[0].iov_base, iov[0].iov_len); + } else if (nc->info->receive_iov) { ret = nc->info->receive_iov(nc, iov, iovcnt); } else { ret = nc_sendv_compat(nc, iov, iovcnt, flags);
commit fefe2a78 accidently dropped the code path for injecting raw packets. This feature is needed for sending gratuitous ARPs after an incoming migration has completed. The result is increased network downtime for vservers where the network card is not virtio-net with the VIRTIO_NET_F_GUEST_ANNOUNCE feature. Fixes: fefe2a78abde932e0f340b21bded2c86def1d242 Cc: qemu-stable@nongnu.org Cc: hongyang.yang@easystack.cn Signed-off-by: Peter Lieven <pl@kamp.de> --- v1->v2: assert that only raw packets with a plain buffer come in. [Paolo] net/net.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)