Message ID | 1366972060-21606-1-git-send-email-jasowang@redhat.com |
---|---|
State | New |
Headers | show |
On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote: > Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost > also when the peer's link is down. But the check was not done properly, the > vhost were only started when: > > 1) peer's link is not down > 2) virtio-net has already been started. > > Since == have a higher precedence than &&, place a brace to make sure both the > conditions were met then does the check. This fixes the crash when doing a savem > after set the link off which let qemu crash and complains: > > virtio_net_save: Assertion `!n->vhost_started' failed. > > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> Hmm okay, but now that I think about this, e.g. if link is up later, vhost will not be started. So the correct thing is maybe to start vhost but use some backend that will drop all packets. And add a callback so we know peer state changed. Hmm do we need a kernel change for this? > --- > hw/net/virtio-net.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 4d2cdd2..6222039 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > return; > } > > - if (!!n->vhost_started == virtio_net_started(n, status) && > - !nc->peer->link_down) { > + if (!!n->vhost_started == > + (virtio_net_started(n, status) && !nc->peer->link_down)) { > return; > } > if (!n->vhost_started) { > -- > 1.7.1
On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote: > On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote: >> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost >> also when the peer's link is down. But the check was not done properly, the >> vhost were only started when: >> >> 1) peer's link is not down >> 2) virtio-net has already been started. >> >> Since == have a higher precedence than &&, place a brace to make sure both the >> conditions were met then does the check. This fixes the crash when doing a savem >> after set the link off which let qemu crash and complains: >> >> virtio_net_save: Assertion `!n->vhost_started' failed. >> >> Cc: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Jason Wang <jasowang@redhat.com> > Hmm okay, but now that I think about this, > e.g. if link is up later, vhost will not be started. If vm has been stopeed, and the link is up later, vhost won't be started. this is expected. If vm has been started, and the link is up later, since n->vhost_started is false but both virtio_net_started() and !nc->peer->link_down is true, so the vhost will be started. Looks ok? > So the correct thing is maybe to start vhost but use > some backend that will drop all packets. > And add a callback so we know peer state changed. > Hmm do we need a kernel change for this? > >> --- >> hw/net/virtio-net.c | 4 ++-- >> 1 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >> index 4d2cdd2..6222039 100644 >> --- a/hw/net/virtio-net.c >> +++ b/hw/net/virtio-net.c >> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) >> return; >> } >> >> - if (!!n->vhost_started == virtio_net_started(n, status) && >> - !nc->peer->link_down) { >> + if (!!n->vhost_started == >> + (virtio_net_started(n, status) && !nc->peer->link_down)) { >> return; >> } >> if (!n->vhost_started) { >> -- >> 1.7.1
On Sat, Apr 27, 2013 at 01:11:16PM +0800, Jason Wang wrote: > On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote: > > On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote: > >> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost > >> also when the peer's link is down. But the check was not done properly, the > >> vhost were only started when: > >> > >> 1) peer's link is not down > >> 2) virtio-net has already been started. > >> > >> Since == have a higher precedence than &&, place a brace to make sure both the > >> conditions were met then does the check. This fixes the crash when doing a savem > >> after set the link off which let qemu crash and complains: > >> > >> virtio_net_save: Assertion `!n->vhost_started' failed. > >> > >> Cc: Michael S. Tsirkin <mst@redhat.com> > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > > Hmm okay, but now that I think about this, > > e.g. if link is up later, vhost will not be started. > > If vm has been stopeed, and the link is up later, vhost won't be > started. this is expected. > If vm has been started, and the link is up later, since n->vhost_started > is false but both virtio_net_started() and !nc->peer->link_down is true, > so the vhost will be started. > > Looks ok? Let me clarify: virtio link is up but peer link is down. So guest will send packets. Will they never be completed? > > So the correct thing is maybe to start vhost but use > > some backend that will drop all packets. > > And add a callback so we know peer state changed. > > Hmm do we need a kernel change for this? > > > >> --- > >> hw/net/virtio-net.c | 4 ++-- > >> 1 files changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >> index 4d2cdd2..6222039 100644 > >> --- a/hw/net/virtio-net.c > >> +++ b/hw/net/virtio-net.c > >> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > >> return; > >> } > >> > >> - if (!!n->vhost_started == virtio_net_started(n, status) && > >> - !nc->peer->link_down) { > >> + if (!!n->vhost_started == > >> + (virtio_net_started(n, status) && !nc->peer->link_down)) { > >> return; > >> } > >> if (!n->vhost_started) { > >> -- > >> 1.7.1
On 04/28/2013 03:32 AM, Michael S. Tsirkin wrote: > On Sat, Apr 27, 2013 at 01:11:16PM +0800, Jason Wang wrote: >> On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote: >>> On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote: >>>> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost >>>> also when the peer's link is down. But the check was not done properly, the >>>> vhost were only started when: >>>> >>>> 1) peer's link is not down >>>> 2) virtio-net has already been started. >>>> >>>> Since == have a higher precedence than &&, place a brace to make sure both the >>>> conditions were met then does the check. This fixes the crash when doing a savem >>>> after set the link off which let qemu crash and complains: >>>> >>>> virtio_net_save: Assertion `!n->vhost_started' failed. >>>> >>>> Cc: Michael S. Tsirkin <mst@redhat.com> >>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>> Hmm okay, but now that I think about this, >>> e.g. if link is up later, vhost will not be started. >> If vm has been stopeed, and the link is up later, vhost won't be >> started. this is expected. >> If vm has been started, and the link is up later, since n->vhost_started >> is false but both virtio_net_started() and !nc->peer->link_down is true, >> so the vhost will be started. >> >> Looks ok? > Let me clarify: virtio link is up but peer link is down. > So guest will send packets. Will they never be > completed? qemu_deliver_packet_iov() will assume the packet were sent when peer link is down. So we are still ok? > > >>> So the correct thing is maybe to start vhost but use >>> some backend that will drop all packets. >>> And add a callback so we know peer state changed. >>> Hmm do we need a kernel change for this? >>> >>>> --- >>>> hw/net/virtio-net.c | 4 ++-- >>>> 1 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>>> index 4d2cdd2..6222039 100644 >>>> --- a/hw/net/virtio-net.c >>>> +++ b/hw/net/virtio-net.c >>>> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) >>>> return; >>>> } >>>> >>>> - if (!!n->vhost_started == virtio_net_started(n, status) && >>>> - !nc->peer->link_down) { >>>> + if (!!n->vhost_started == >>>> + (virtio_net_started(n, status) && !nc->peer->link_down)) { >>>> return; >>>> } >>>> if (!n->vhost_started) { >>>> -- >>>> 1.7.1
On Sun, Apr 28, 2013 at 03:51:32PM +0800, Jason Wang wrote: > On 04/28/2013 03:32 AM, Michael S. Tsirkin wrote: > > On Sat, Apr 27, 2013 at 01:11:16PM +0800, Jason Wang wrote: > >> On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote: > >>> On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote: > >>>> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost > >>>> also when the peer's link is down. But the check was not done properly, the > >>>> vhost were only started when: > >>>> > >>>> 1) peer's link is not down > >>>> 2) virtio-net has already been started. > >>>> > >>>> Since == have a higher precedence than &&, place a brace to make sure both the > >>>> conditions were met then does the check. This fixes the crash when doing a savem > >>>> after set the link off which let qemu crash and complains: > >>>> > >>>> virtio_net_save: Assertion `!n->vhost_started' failed. > >>>> > >>>> Cc: Michael S. Tsirkin <mst@redhat.com> > >>>> Signed-off-by: Jason Wang <jasowang@redhat.com> > >>> Hmm okay, but now that I think about this, > >>> e.g. if link is up later, vhost will not be started. > >> If vm has been stopeed, and the link is up later, vhost won't be > >> started. this is expected. > >> If vm has been started, and the link is up later, since n->vhost_started > >> is false but both virtio_net_started() and !nc->peer->link_down is true, > >> so the vhost will be started. > >> > >> Looks ok? > > Let me clarify: virtio link is up but peer link is down. > > So guest will send packets. Will they never be > > completed? > > qemu_deliver_packet_iov() will assume the packet were sent when peer > link is down. So we are still ok? Right so I think userspace will start dropping packets. I think this is unnecessarily fragile, I think it's best to make sure vhost=on means userspace does not process tx/rx rings. > > > > > >>> So the correct thing is maybe to start vhost but use > >>> some backend that will drop all packets. > >>> And add a callback so we know peer state changed. > >>> Hmm do we need a kernel change for this? > >>> > >>>> --- > >>>> hw/net/virtio-net.c | 4 ++-- > >>>> 1 files changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >>>> index 4d2cdd2..6222039 100644 > >>>> --- a/hw/net/virtio-net.c > >>>> +++ b/hw/net/virtio-net.c > >>>> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > >>>> return; > >>>> } > >>>> > >>>> - if (!!n->vhost_started == virtio_net_started(n, status) && > >>>> - !nc->peer->link_down) { > >>>> + if (!!n->vhost_started == > >>>> + (virtio_net_started(n, status) && !nc->peer->link_down)) { > >>>> return; > >>>> } > >>>> if (!n->vhost_started) { > >>>> -- > >>>> 1.7.1
On 04/28/2013 04:25 PM, Michael S. Tsirkin wrote: > On Sun, Apr 28, 2013 at 03:51:32PM +0800, Jason Wang wrote: >> On 04/28/2013 03:32 AM, Michael S. Tsirkin wrote: >>> On Sat, Apr 27, 2013 at 01:11:16PM +0800, Jason Wang wrote: >>>> On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote: >>>>> On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote: >>>>>> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost >>>>>> also when the peer's link is down. But the check was not done properly, the >>>>>> vhost were only started when: >>>>>> >>>>>> 1) peer's link is not down >>>>>> 2) virtio-net has already been started. >>>>>> >>>>>> Since == have a higher precedence than &&, place a brace to make sure both the >>>>>> conditions were met then does the check. This fixes the crash when doing a savem >>>>>> after set the link off which let qemu crash and complains: >>>>>> >>>>>> virtio_net_save: Assertion `!n->vhost_started' failed. >>>>>> >>>>>> Cc: Michael S. Tsirkin <mst@redhat.com> >>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>>> Hmm okay, but now that I think about this, >>>>> e.g. if link is up later, vhost will not be started. >>>> If vm has been stopeed, and the link is up later, vhost won't be >>>> started. this is expected. >>>> If vm has been started, and the link is up later, since n->vhost_started >>>> is false but both virtio_net_started() and !nc->peer->link_down is true, >>>> so the vhost will be started. >>>> >>>> Looks ok? >>> Let me clarify: virtio link is up but peer link is down. >>> So guest will send packets. Will they never be >>> completed? >> qemu_deliver_packet_iov() will assume the packet were sent when peer >> link is down. So we are still ok? > Right so I think userspace will start dropping packets. > I think this is unnecessarily fragile, I think it's best > to make sure vhost=on means userspace does not > process tx/rx rings. But this is also what other nic does such as e1000. Anyway it's harmless so we can just keep it. > >>> >>>>> So the correct thing is maybe to start vhost but use >>>>> some backend that will drop all packets. >>>>> And add a callback so we know peer state changed. >>>>> Hmm do we need a kernel change for this? >>>>> >>>>>> --- >>>>>> hw/net/virtio-net.c | 4 ++-- >>>>>> 1 files changed, 2 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>>>>> index 4d2cdd2..6222039 100644 >>>>>> --- a/hw/net/virtio-net.c >>>>>> +++ b/hw/net/virtio-net.c >>>>>> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) >>>>>> return; >>>>>> } >>>>>> >>>>>> - if (!!n->vhost_started == virtio_net_started(n, status) && >>>>>> - !nc->peer->link_down) { >>>>>> + if (!!n->vhost_started == >>>>>> + (virtio_net_started(n, status) && !nc->peer->link_down)) { >>>>>> return; >>>>>> } >>>>>> if (!n->vhost_started) { >>>>>> -- >>>>>> 1.7.1
On 04/28/2013 04:25 PM, Michael S. Tsirkin wrote: > On Sun, Apr 28, 2013 at 03:51:32PM +0800, Jason Wang wrote: >> On 04/28/2013 03:32 AM, Michael S. Tsirkin wrote: >>> On Sat, Apr 27, 2013 at 01:11:16PM +0800, Jason Wang wrote: >>>> On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote: >>>>> On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote: >>>>>> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost >>>>>> also when the peer's link is down. But the check was not done properly, the >>>>>> vhost were only started when: >>>>>> >>>>>> 1) peer's link is not down >>>>>> 2) virtio-net has already been started. >>>>>> >>>>>> Since == have a higher precedence than &&, place a brace to make sure both the >>>>>> conditions were met then does the check. This fixes the crash when doing a savem >>>>>> after set the link off which let qemu crash and complains: >>>>>> >>>>>> virtio_net_save: Assertion `!n->vhost_started' failed. >>>>>> >>>>>> Cc: Michael S. Tsirkin <mst@redhat.com> >>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>>> Hmm okay, but now that I think about this, >>>>> e.g. if link is up later, vhost will not be started. >>>> If vm has been stopeed, and the link is up later, vhost won't be >>>> started. this is expected. >>>> If vm has been started, and the link is up later, since n->vhost_started >>>> is false but both virtio_net_started() and !nc->peer->link_down is true, >>>> so the vhost will be started. >>>> >>>> Looks ok? >>> Let me clarify: virtio link is up but peer link is down. >>> So guest will send packets. Will they never be >>> completed? >> qemu_deliver_packet_iov() will assume the packet were sent when peer >> link is down. So we are still ok? > Right so I think userspace will start dropping packets. > I think this is unnecessarily fragile, I think it's best > to make sure vhost=on means userspace does not > process tx/rx rings. It may make sense, but let's do it in the future. So ack or apply this patch to fix the bug first? Thanks >>> >>>>> So the correct thing is maybe to start vhost but use >>>>> some backend that will drop all packets. >>>>> And add a callback so we know peer state changed. >>>>> Hmm do we need a kernel change for this? >>>>> >>>>>> --- >>>>>> hw/net/virtio-net.c | 4 ++-- >>>>>> 1 files changed, 2 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>>>>> index 4d2cdd2..6222039 100644 >>>>>> --- a/hw/net/virtio-net.c >>>>>> +++ b/hw/net/virtio-net.c >>>>>> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) >>>>>> return; >>>>>> } >>>>>> >>>>>> - if (!!n->vhost_started == virtio_net_started(n, status) && >>>>>> - !nc->peer->link_down) { >>>>>> + if (!!n->vhost_started == >>>>>> + (virtio_net_started(n, status) && !nc->peer->link_down)) { >>>>>> return; >>>>>> } >>>>>> if (!n->vhost_started) { >>>>>> -- >>>>>> 1.7.1
On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote: > Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost > also when the peer's link is down. But the check was not done properly, the > vhost were only started when: > > 1) peer's link is not down > 2) virtio-net has already been started. > > Since == have a higher precedence than &&, place a brace to make sure both the > conditions were met then does the check. This fixes the crash when doing a savem > after set the link off which let qemu crash and complains: > > virtio_net_save: Assertion `!n->vhost_started' failed. > > Cc: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Too late for 1.5? > --- > hw/net/virtio-net.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 4d2cdd2..6222039 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) > return; > } > > - if (!!n->vhost_started == virtio_net_started(n, status) && > - !nc->peer->link_down) { > + if (!!n->vhost_started == > + (virtio_net_started(n, status) && !nc->peer->link_down)) { > return; > } > if (!n->vhost_started) { > -- > 1.7.1
Applied. Thanks. Regards, Anthony Liguori
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 4d2cdd2..6222039 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status) return; } - if (!!n->vhost_started == virtio_net_started(n, status) && - !nc->peer->link_down) { + if (!!n->vhost_started == + (virtio_net_started(n, status) && !nc->peer->link_down)) { return; } if (!n->vhost_started) {
Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost also when the peer's link is down. But the check was not done properly, the vhost were only started when: 1) peer's link is not down 2) virtio-net has already been started. Since == have a higher precedence than &&, place a brace to make sure both the conditions were met then does the check. This fixes the crash when doing a savem after set the link off which let qemu crash and complains: virtio_net_save: Assertion `!n->vhost_started' failed. Cc: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/net/virtio-net.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)