| Message ID | 20171013155140.124530-1-willemdebruijn.kernel@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS_RESET | expand |
On Fri, Oct 13, 2017 at 11:51:40AM -0400, Willem de Bruijn wrote: > From: Willem de Bruijn <willemb@google.com> > > Implement the reset communication request defined in the VIRTIO 1.0 > specification and introduces in Linux in commit c00bbcf862896 ("virtio: > add VIRTIO_CONFIG_S_NEEDS_RESET device status bit"). > > Use the virtnet_reset function introduced in commit 2de2f7f40ef9 > ("virtio_net: XDP support for adjust_head"). That was removed in > commit 4941d472bf95 ("virtio-net: do not reset during XDP set"), > because no longer used. Bring it back, minus the xdp specific code. > > Before tearing down any state, virtnet_freeze_down quiesces the > device with netif_tx_disable. virtnet_reset also ensures that no > other config operations can run concurrently. > > On successful reset, the host can observe that the flag has been > cleared. There is no need for the explicit control flag introduced > in the previous RFC of this patch. > > Changes > RFC -> v1 > - drop VIRTIO_NET_CTRL_RESET_ACK message > - drop VIRTIO_F_CAN_RESET flag to notify guest support > > Signed-off-by: Willem de Bruijn <willemb@google.com> > --- > drivers/net/virtio_net.c | 48 ++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 44 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index fc059f193e7d..8e768b54844f 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -1903,13 +1903,14 @@ static const struct ethtool_ops virtnet_ethtool_ops = { > .set_link_ksettings = virtnet_set_link_ksettings, > }; > > -static void virtnet_freeze_down(struct virtio_device *vdev) > +static void virtnet_freeze_down(struct virtio_device *vdev, bool in_config) > { > struct virtnet_info *vi = vdev->priv; > int i; > > - /* Make sure no work handler is accessing the device */ > - flush_work(&vi->config_work); > + /* Make sure no other work handler is accessing the device */ > + if (!in_config) > + flush_work(&vi->config_work); > > netif_device_detach(vi->dev); > netif_tx_disable(vi->dev); > @@ -1924,6 +1925,7 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > } > > static int init_vqs(struct virtnet_info *vi); > +static void remove_vq_common(struct virtnet_info *vi); > > static int virtnet_restore_up(struct virtio_device *vdev) > { > @@ -1952,6 +1954,40 @@ static int virtnet_restore_up(struct virtio_device *vdev) > return err; > } > > +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? Also, it seems that LINK_ANNOUNCE requests will get ignored even if they got set before the reset, leading to downtime. > @@ -2136,6 +2172,10 @@ static void virtnet_config_changed_work(struct work_struct *work) > virtnet_ack_link_announce(vi); > } > > + if (vi->vdev->config->get_status(vi->vdev) & > + VIRTIO_CONFIG_S_NEEDS_RESET) > + virtnet_reset(vi); > + > /* Ignore unknown (future) status bits */ > v &= VIRTIO_NET_S_LINK_UP; > > @@ -2756,7 +2796,7 @@ static __maybe_unused int virtnet_freeze(struct virtio_device *vdev) > struct virtnet_info *vi = vdev->priv; > > virtnet_cpu_notif_remove(vi); > - virtnet_freeze_down(vdev); > + virtnet_freeze_down(vdev, false); > remove_vq_common(vi); > > return 0; > -- > 2.15.0.rc0.271.g36b669edcc-goog
>> +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. > 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.
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. > > 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.
On Mon, Oct 16, 2017 at 11:31 AM, Michael S. Tsirkin <mst@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? >> > 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.
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@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. > >> > 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.
On Mon, Oct 16, 2017 at 12:38 PM, Michael S. Tsirkin <mst@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@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.
On 2017年10月17日 06:34, Willem de Bruijn wrote: > On Mon, Oct 16, 2017 at 12:38 PM, Michael S. Tsirkin <mst@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@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
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@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@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 It might not keep doing that in the future though. And virtio-net in userspace doesn't ignore the filters.
On Mon, Oct 16, 2017 at 11:44 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > 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@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@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 > > It might not keep doing that in the future though. > And virtio-net in userspace doesn't ignore the filters. How about the guest honor the request only if no state has been offloaded to the host? This is the common case for vhost_net, and not expected to change soon. Even when it does, we have a graceful degradation strategy. Guest revert state prior to reset and reapply. Though for the time being, solving this only in the case without state offload would be solve my use case.
On 2017年12月29日 03:11, Willem de Bruijn wrote: > On Mon, Oct 16, 2017 at 11:44 PM, Michael S. Tsirkin <mst@redhat.com> wrote: >> 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@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@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 >> It might not keep doing that in the future though. >> And virtio-net in userspace doesn't ignore the filters. > How about the guest honor the request only if no state has been > offloaded to the host? > > This is the common case for vhost_net, and not expected to change > soon. FYI, I'm implementing to use tun eBPF filter for virtio-net. So recovering filter should be considered. Thanks > > Even when it does, we have a graceful degradation strategy. Guest > revert state prior to reset and reapply. Though for the time being, > solving this only in the case without state offload would be solve my > use case.
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index fc059f193e7d..8e768b54844f 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1903,13 +1903,14 @@ static const struct ethtool_ops virtnet_ethtool_ops = { .set_link_ksettings = virtnet_set_link_ksettings, }; -static void virtnet_freeze_down(struct virtio_device *vdev) +static void virtnet_freeze_down(struct virtio_device *vdev, bool in_config) { struct virtnet_info *vi = vdev->priv; int i; - /* Make sure no work handler is accessing the device */ - flush_work(&vi->config_work); + /* Make sure no other work handler is accessing the device */ + if (!in_config) + flush_work(&vi->config_work); netif_device_detach(vi->dev); netif_tx_disable(vi->dev); @@ -1924,6 +1925,7 @@ static void virtnet_freeze_down(struct virtio_device *vdev) } static int init_vqs(struct virtnet_info *vi); +static void remove_vq_common(struct virtnet_info *vi); static int virtnet_restore_up(struct virtio_device *vdev) { @@ -1952,6 +1954,40 @@ static int virtnet_restore_up(struct virtio_device *vdev) return err; } +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; @@ -2136,6 +2172,10 @@ static void virtnet_config_changed_work(struct work_struct *work) virtnet_ack_link_announce(vi); } + if (vi->vdev->config->get_status(vi->vdev) & + VIRTIO_CONFIG_S_NEEDS_RESET) + virtnet_reset(vi); + /* Ignore unknown (future) status bits */ v &= VIRTIO_NET_S_LINK_UP; @@ -2756,7 +2796,7 @@ static __maybe_unused int virtnet_freeze(struct virtio_device *vdev) struct virtnet_info *vi = vdev->priv; virtnet_cpu_notif_remove(vi); - virtnet_freeze_down(vdev); + virtnet_freeze_down(vdev, false); remove_vq_common(vi); return 0;