Willem de Bruijn
2017-Oct-16 22:34 UTC
[PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
On Mon, Oct 16, 2017 at 12:38 PM, Michael S. Tsirkin <mst at redhat.com> wrote:> On Mon, Oct 16, 2017 at 12:04:57PM -0400, Willem de Bruijn wrote: >> On Mon, Oct 16, 2017 at 11:31 AM, Michael S. Tsirkin <mst at redhat.com> wrote: >> > On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote: >> >> >> +static int virtnet_reset(struct virtnet_info *vi) >> >> >> +{ >> >> >> + struct virtio_device *dev = vi->vdev; >> >> >> + int ret; >> >> >> + >> >> >> + virtio_config_disable(dev); >> >> >> + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED; >> >> >> + virtnet_freeze_down(dev, true); >> >> >> + remove_vq_common(vi); >> >> >> + >> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); >> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); >> >> >> + >> >> >> + ret = virtio_finalize_features(dev); >> >> >> + if (ret) >> >> >> + goto err; >> >> >> + >> >> >> + ret = virtnet_restore_up(dev); >> >> >> + if (ret) >> >> >> + goto err; >> >> >> + >> >> >> + ret = virtnet_set_queues(vi, vi->curr_queue_pairs); >> >> >> + if (ret) >> >> >> + goto err; >> >> >> + >> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); >> >> >> + virtio_config_enable(dev); >> >> >> + return 0; >> >> >> + >> >> >> +err: >> >> >> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); >> >> >> + return ret; >> >> >> +} >> >> >> + >> >> >> static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads) >> >> >> { >> >> >> struct scatterlist sg; >> >> > >> >> > I have a question here though. How do things like MAC address >> >> > get restored? >> >> > >> >> > What about the rx mode? >> >> > >> >> > vlans? >> >> >> >> The function as is releases and reinitializes only ring state. >> >> Device configuration such as mac and vlan persist across >> >> the reset. >> > >> > What gave you this impression? Take a look at e.g. this >> > code in qemu: >> > >> > static void virtio_net_reset(VirtIODevice *vdev) >> > { >> > VirtIONet *n = VIRTIO_NET(vdev); >> > >> > /* Reset back to compatibility mode */ >> > n->promisc = 1; >> > n->allmulti = 0; >> > n->alluni = 0; >> > n->nomulti = 0; >> > n->nouni = 0; >> > n->nobcast = 0; >> > /* multiqueue is disabled by default */ >> > n->curr_queues = 1; >> > timer_del(n->announce_timer); >> > n->announce_counter = 0; >> > n->status &= ~VIRTIO_NET_S_ANNOUNCE; >> > >> > /* Flush any MAC and VLAN filter table state */ >> > n->mac_table.in_use = 0; >> > n->mac_table.first_multi = 0; >> > n->mac_table.multi_overflow = 0; >> > n->mac_table.uni_overflow = 0; >> > memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN); >> > memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac)); >> > qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac); >> > memset(n->vlans, 0, MAX_VLAN >> 3); >> > } >> > >> > So device seems to lose all state, you have to re-program it. >> >> Oh, indeed! The guest does not reset its state, so it might >> be out of sync with the host after the operation. Was this not >> an issue when previously resetting in the context of xdp? > > I suspect it was broken back then, too.Okay. I guess that in principle this is all programmable through virtnet_set_rx_mode, virtnet_vlan_rx_add_vid, etc. But it's a lot more complex than just restoring virtnet_reset. Will need to be careful about concurrency issues at the least. Similar to the ones you point out below.> >> >> > Also, it seems that LINK_ANNOUNCE requests will get ignored >> >> > even if they got set before the reset, leading to downtime. >> >> >> >> Do you mean act on VIRTIO_NET_F_GUEST_ANNOUNCE >> >> requests? That flag is tested and netdev_notify_peers >> >> called before resetting virtio ring state. >> > >> > Yes but I wonder if there's a race where announce >> > is set after it is read but before NEED_RESET is read. >> > >> > Re-reading status from the config before reset >> > might be necessary. >> >> Thanks, I'll have a look. Perhaps a host should simply not >> request a reset while it is waiting for an announce ack. > > It's one option though we can't make this change for existing hosts. > We also have the reverse condition where announce is requested after > NEED_RESET is set.
Jason Wang
2017-Oct-17 03:05 UTC
[PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
On 2017?10?17? 06:34, Willem de Bruijn wrote:> On Mon, Oct 16, 2017 at 12:38 PM, Michael S. Tsirkin <mst at redhat.com> wrote: >> On Mon, Oct 16, 2017 at 12:04:57PM -0400, Willem de Bruijn wrote: >>> On Mon, Oct 16, 2017 at 11:31 AM, Michael S. Tsirkin <mst at redhat.com> wrote: >>>> On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote: >>>>>>> +static int virtnet_reset(struct virtnet_info *vi) >>>>>>> +{ >>>>>>> + struct virtio_device *dev = vi->vdev; >>>>>>> + int ret; >>>>>>> + >>>>>>> + virtio_config_disable(dev); >>>>>>> + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED; >>>>>>> + virtnet_freeze_down(dev, true); >>>>>>> + remove_vq_common(vi); >>>>>>> + >>>>>>> + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); >>>>>>> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); >>>>>>> + >>>>>>> + ret = virtio_finalize_features(dev); >>>>>>> + if (ret) >>>>>>> + goto err; >>>>>>> + >>>>>>> + ret = virtnet_restore_up(dev); >>>>>>> + if (ret) >>>>>>> + goto err; >>>>>>> + >>>>>>> + ret = virtnet_set_queues(vi, vi->curr_queue_pairs); >>>>>>> + if (ret) >>>>>>> + goto err; >>>>>>> + >>>>>>> + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); >>>>>>> + virtio_config_enable(dev); >>>>>>> + return 0; >>>>>>> + >>>>>>> +err: >>>>>>> + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); >>>>>>> + return ret; >>>>>>> +} >>>>>>> + >>>>>>> static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads) >>>>>>> { >>>>>>> struct scatterlist sg; >>>>>> I have a question here though. How do things like MAC address >>>>>> get restored? >>>>>> >>>>>> What about the rx mode? >>>>>> >>>>>> vlans? >>>>> The function as is releases and reinitializes only ring state. >>>>> Device configuration such as mac and vlan persist across >>>>> the reset. >>>> What gave you this impression? Take a look at e.g. this >>>> code in qemu: >>>> >>>> static void virtio_net_reset(VirtIODevice *vdev) >>>> { >>>> VirtIONet *n = VIRTIO_NET(vdev); >>>> >>>> /* Reset back to compatibility mode */ >>>> n->promisc = 1; >>>> n->allmulti = 0; >>>> n->alluni = 0; >>>> n->nomulti = 0; >>>> n->nouni = 0; >>>> n->nobcast = 0; >>>> /* multiqueue is disabled by default */ >>>> n->curr_queues = 1; >>>> timer_del(n->announce_timer); >>>> n->announce_counter = 0; >>>> n->status &= ~VIRTIO_NET_S_ANNOUNCE; >>>> >>>> /* Flush any MAC and VLAN filter table state */ >>>> n->mac_table.in_use = 0; >>>> n->mac_table.first_multi = 0; >>>> n->mac_table.multi_overflow = 0; >>>> n->mac_table.uni_overflow = 0; >>>> memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN); >>>> memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac)); >>>> qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac); >>>> memset(n->vlans, 0, MAX_VLAN >> 3); >>>> } >>>> >>>> So device seems to lose all state, you have to re-program it. >>> Oh, indeed! The guest does not reset its state, so it might >>> be out of sync with the host after the operation. Was this not >>> an issue when previously resetting in the context of xdp? >> I suspect it was broken back then, too. > Okay. I guess that in principle this is all programmable through > virtnet_set_rx_mode, virtnet_vlan_rx_add_vid, etc. But it's a > lot more complex than just restoring virtnet_reset. Will need to > be careful about concurrency issues at the least. Similar to the > ones you point out below. >The problem has been pointed out during developing virtio-net XDP. But it may not be a big issue since vhost_net ignores all kinds of the filters now. Thanks
Michael S. Tsirkin
2017-Oct-17 03:44 UTC
[PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET
On Tue, Oct 17, 2017 at 11:05:07AM +0800, Jason Wang wrote:> > > On 2017?10?17? 06:34, Willem de Bruijn wrote: > > On Mon, Oct 16, 2017 at 12:38 PM, Michael S. Tsirkin <mst at redhat.com> wrote: > > > On Mon, Oct 16, 2017 at 12:04:57PM -0400, Willem de Bruijn wrote: > > > > On Mon, Oct 16, 2017 at 11:31 AM, Michael S. Tsirkin <mst at redhat.com> wrote: > > > > > On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote: > > > > > > > > +static int virtnet_reset(struct virtnet_info *vi) > > > > > > > > +{ > > > > > > > > + struct virtio_device *dev = vi->vdev; > > > > > > > > + int ret; > > > > > > > > + > > > > > > > > + virtio_config_disable(dev); > > > > > > > > + dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED; > > > > > > > > + virtnet_freeze_down(dev, true); > > > > > > > > + remove_vq_common(vi); > > > > > > > > + > > > > > > > > + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); > > > > > > > > + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); > > > > > > > > + > > > > > > > > + ret = virtio_finalize_features(dev); > > > > > > > > + if (ret) > > > > > > > > + goto err; > > > > > > > > + > > > > > > > > + ret = virtnet_restore_up(dev); > > > > > > > > + if (ret) > > > > > > > > + goto err; > > > > > > > > + > > > > > > > > + ret = virtnet_set_queues(vi, vi->curr_queue_pairs); > > > > > > > > + if (ret) > > > > > > > > + goto err; > > > > > > > > + > > > > > > > > + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); > > > > > > > > + virtio_config_enable(dev); > > > > > > > > + return 0; > > > > > > > > + > > > > > > > > +err: > > > > > > > > + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); > > > > > > > > + return ret; > > > > > > > > +} > > > > > > > > + > > > > > > > > static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads) > > > > > > > > { > > > > > > > > struct scatterlist sg; > > > > > > > I have a question here though. How do things like MAC address > > > > > > > get restored? > > > > > > > > > > > > > > What about the rx mode? > > > > > > > > > > > > > > vlans? > > > > > > The function as is releases and reinitializes only ring state. > > > > > > Device configuration such as mac and vlan persist across > > > > > > the reset. > > > > > What gave you this impression? Take a look at e.g. this > > > > > code in qemu: > > > > > > > > > > static void virtio_net_reset(VirtIODevice *vdev) > > > > > { > > > > > VirtIONet *n = VIRTIO_NET(vdev); > > > > > > > > > > /* Reset back to compatibility mode */ > > > > > n->promisc = 1; > > > > > n->allmulti = 0; > > > > > n->alluni = 0; > > > > > n->nomulti = 0; > > > > > n->nouni = 0; > > > > > n->nobcast = 0; > > > > > /* multiqueue is disabled by default */ > > > > > n->curr_queues = 1; > > > > > timer_del(n->announce_timer); > > > > > n->announce_counter = 0; > > > > > n->status &= ~VIRTIO_NET_S_ANNOUNCE; > > > > > > > > > > /* Flush any MAC and VLAN filter table state */ > > > > > n->mac_table.in_use = 0; > > > > > n->mac_table.first_multi = 0; > > > > > n->mac_table.multi_overflow = 0; > > > > > n->mac_table.uni_overflow = 0; > > > > > memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN); > > > > > memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac)); > > > > > qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac); > > > > > memset(n->vlans, 0, MAX_VLAN >> 3); > > > > > } > > > > > > > > > > So device seems to lose all state, you have to re-program it. > > > > Oh, indeed! The guest does not reset its state, so it might > > > > be out of sync with the host after the operation. Was this not > > > > an issue when previously resetting in the context of xdp? > > > I suspect it was broken back then, too. > > Okay. I guess that in principle this is all programmable through > > virtnet_set_rx_mode, virtnet_vlan_rx_add_vid, etc. But it's a > > lot more complex than just restoring virtnet_reset. Will need to > > be careful about concurrency issues at the least. Similar to the > > ones you point out below. > > > > The problem has been pointed out during developing virtio-net XDP. But it > may not be a big issue since vhost_net ignores all kinds of the filters now. > > ThanksIt might not keep doing that in the future though. And virtio-net in userspace doesn't ignore the filters. -- MST