From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [net PATCH v5 5/6] virtio_net: refactor freeze/restore logic into virtnet reset logic Date: Wed, 18 Jan 2017 17:50:58 +0200 Message-ID: <20170118174930-mutt-send-email-mst@kernel.org> References: <20170117221443.20280.62546.stgit@john-Precision-Tower-5810> <20170117222223.20280.76302.stgit@john-Precision-Tower-5810> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: jasowang@redhat.com, john.r.fastabend@intel.com, netdev@vger.kernel.org, alexei.starovoitov@gmail.com, daniel@iogearbox.net To: John Fastabend Return-path: Received: from mx1.redhat.com ([209.132.183.28]:35104 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751561AbdARPvF (ORCPT ); Wed, 18 Jan 2017 10:51:05 -0500 Content-Disposition: inline In-Reply-To: <20170117222223.20280.76302.stgit@john-Precision-Tower-5810> Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Jan 17, 2017 at 02:22:23PM -0800, John Fastabend wrote: > For XDP we will need to reset the queues to allow for buffer headroom > to be configured. In order to do this we need to essentially run the > freeze()/restore() code path. Unfortunately the locking requirements > between the freeze/restore and reset paths are different however so > we can not simply reuse the code. > > This patch refactors the code path and adds a reset helper routine. > > Signed-off-by: John Fastabend > --- > drivers/net/virtio_net.c | 75 ++++++++++++++++++++++++++++------------------ > drivers/virtio/virtio.c | 42 ++++++++++++++------------ > include/linux/virtio.h | 4 ++ > 3 files changed, 73 insertions(+), 48 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 922ca66..62dbf4b 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -1684,6 +1684,49 @@ static void virtnet_init_settings(struct net_device *dev) > .set_settings = virtnet_set_settings, > }; > > +static void virtnet_freeze_down(struct virtio_device *vdev) > +{ > + struct virtnet_info *vi = vdev->priv; > + int i; > + > + /* Make sure no work handler is accessing the device */ > + flush_work(&vi->config_work); > + > + netif_device_detach(vi->dev); > + cancel_delayed_work_sync(&vi->refill); > + > + if (netif_running(vi->dev)) { > + for (i = 0; i < vi->max_queue_pairs; i++) > + napi_disable(&vi->rq[i].napi); > + } > +} > + > +static int init_vqs(struct virtnet_info *vi); I dislike forward declarations for static functions - if you are trying to make the diff more readable (understandable) then pls move this function to before use in a follow-up patch. Same applies to the next patch. > + > +static int virtnet_restore_up(struct virtio_device *vdev) > +{ > + struct virtnet_info *vi = vdev->priv; > + int err, i; > + > + err = init_vqs(vi); > + if (err) > + return err; > + > + virtio_device_ready(vdev); > + > + if (netif_running(vi->dev)) { > + for (i = 0; i < vi->curr_queue_pairs; i++) > + if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL)) > + schedule_delayed_work(&vi->refill, 0); > + > + for (i = 0; i < vi->max_queue_pairs; i++) > + virtnet_napi_enable(&vi->rq[i]); > + } > + > + netif_device_attach(vi->dev); > + return err; > +} > + > static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog) > { > unsigned long int max_sz = PAGE_SIZE - sizeof(struct padded_vnet_hdr); > @@ -2374,21 +2417,9 @@ static void virtnet_remove(struct virtio_device *vdev) > static int virtnet_freeze(struct virtio_device *vdev) > { > struct virtnet_info *vi = vdev->priv; > - int i; > > virtnet_cpu_notif_remove(vi); > - > - /* Make sure no work handler is accessing the device */ > - flush_work(&vi->config_work); > - > - netif_device_detach(vi->dev); > - cancel_delayed_work_sync(&vi->refill); > - > - if (netif_running(vi->dev)) { > - for (i = 0; i < vi->max_queue_pairs; i++) > - napi_disable(&vi->rq[i].napi); > - } > - > + virtnet_freeze_down(vdev); > remove_vq_common(vi); > > return 0; > @@ -2397,25 +2428,11 @@ static int virtnet_freeze(struct virtio_device *vdev) > static int virtnet_restore(struct virtio_device *vdev) > { > struct virtnet_info *vi = vdev->priv; > - int err, i; > + int err; > > - err = init_vqs(vi); > + err = virtnet_restore_up(vdev); > if (err) > return err; > - > - virtio_device_ready(vdev); > - > - if (netif_running(vi->dev)) { > - for (i = 0; i < vi->curr_queue_pairs; i++) > - if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL)) > - schedule_delayed_work(&vi->refill, 0); > - > - for (i = 0; i < vi->max_queue_pairs; i++) > - virtnet_napi_enable(&vi->rq[i]); > - } > - > - netif_device_attach(vi->dev); > - > virtnet_set_queues(vi, vi->curr_queue_pairs); > > err = virtnet_cpu_notif_add(vi); > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index 7062bb0..400d70b 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -100,11 +100,6 @@ static int virtio_uevent(struct device *_dv, struct kobj_uevent_env *env) > dev->id.device, dev->id.vendor); > } > > -static void add_status(struct virtio_device *dev, unsigned status) > -{ > - dev->config->set_status(dev, dev->config->get_status(dev) | status); > -} > - > void virtio_check_driver_offered_feature(const struct virtio_device *vdev, > unsigned int fbit) > { > @@ -145,14 +140,15 @@ void virtio_config_changed(struct virtio_device *dev) > } > EXPORT_SYMBOL_GPL(virtio_config_changed); > > -static void virtio_config_disable(struct virtio_device *dev) > +void virtio_config_disable(struct virtio_device *dev) > { > spin_lock_irq(&dev->config_lock); > dev->config_enabled = false; > spin_unlock_irq(&dev->config_lock); > } > +EXPORT_SYMBOL_GPL(virtio_config_disable); > > -static void virtio_config_enable(struct virtio_device *dev) > +void virtio_config_enable(struct virtio_device *dev) > { > spin_lock_irq(&dev->config_lock); > dev->config_enabled = true; > @@ -161,8 +157,15 @@ static void virtio_config_enable(struct virtio_device *dev) > dev->config_change_pending = false; > spin_unlock_irq(&dev->config_lock); > } > +EXPORT_SYMBOL_GPL(virtio_config_enable); > + > +void virtio_add_status(struct virtio_device *dev, unsigned int status) > +{ > + dev->config->set_status(dev, dev->config->get_status(dev) | status); > +} > +EXPORT_SYMBOL_GPL(virtio_add_status); > > -static int virtio_finalize_features(struct virtio_device *dev) > +int virtio_finalize_features(struct virtio_device *dev) > { > int ret = dev->config->finalize_features(dev); > unsigned status; > @@ -173,7 +176,7 @@ static int virtio_finalize_features(struct virtio_device *dev) > if (!virtio_has_feature(dev, VIRTIO_F_VERSION_1)) > return 0; > > - add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK); > + virtio_add_status(dev, VIRTIO_CONFIG_S_FEATURES_OK); > status = dev->config->get_status(dev); > if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) { > dev_err(&dev->dev, "virtio: device refuses features: %x\n", > @@ -182,6 +185,7 @@ static int virtio_finalize_features(struct virtio_device *dev) > } > return 0; > } > +EXPORT_SYMBOL_GPL(virtio_finalize_features); > > static int virtio_dev_probe(struct device *_d) > { > @@ -193,7 +197,7 @@ static int virtio_dev_probe(struct device *_d) > u64 driver_features_legacy; > > /* We have a driver! */ > - add_status(dev, VIRTIO_CONFIG_S_DRIVER); > + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); > > /* Figure out what features the device supports. */ > device_features = dev->config->get_features(dev); > @@ -247,7 +251,7 @@ static int virtio_dev_probe(struct device *_d) > > return 0; > err: > - add_status(dev, VIRTIO_CONFIG_S_FAILED); > + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); > return err; > > } > @@ -265,7 +269,7 @@ static int virtio_dev_remove(struct device *_d) > WARN_ON_ONCE(dev->config->get_status(dev)); > > /* Acknowledge the device's existence again. */ > - add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); > + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); > return 0; > } > > @@ -316,7 +320,7 @@ int register_virtio_device(struct virtio_device *dev) > dev->config->reset(dev); > > /* Acknowledge that we've seen the device. */ > - add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); > + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); > > INIT_LIST_HEAD(&dev->vqs); > > @@ -325,7 +329,7 @@ int register_virtio_device(struct virtio_device *dev) > err = device_register(&dev->dev); > out: > if (err) > - add_status(dev, VIRTIO_CONFIG_S_FAILED); > + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); > return err; > } > EXPORT_SYMBOL_GPL(register_virtio_device); > @@ -365,18 +369,18 @@ int virtio_device_restore(struct virtio_device *dev) > dev->config->reset(dev); > > /* Acknowledge that we've seen the device. */ > - add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); > + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); > > /* Maybe driver failed before freeze. > * Restore the failed status, for debugging. */ > if (dev->failed) > - add_status(dev, VIRTIO_CONFIG_S_FAILED); > + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); > > if (!drv) > return 0; > > /* We have a driver! */ > - add_status(dev, VIRTIO_CONFIG_S_DRIVER); > + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); > > ret = virtio_finalize_features(dev); > if (ret) > @@ -389,14 +393,14 @@ int virtio_device_restore(struct virtio_device *dev) > } > > /* Finally, tell the device we're all set */ > - add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); > + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK); > > virtio_config_enable(dev); > > return 0; > > err: > - add_status(dev, VIRTIO_CONFIG_S_FAILED); > + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); > return ret; > } > EXPORT_SYMBOL_GPL(virtio_device_restore); > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > index d5eb547..04b0d3f 100644 > --- a/include/linux/virtio.h > +++ b/include/linux/virtio.h > @@ -132,12 +132,16 @@ static inline struct virtio_device *dev_to_virtio(struct device *_dev) > return container_of(_dev, struct virtio_device, dev); > } > > +void virtio_add_status(struct virtio_device *dev, unsigned int status); > int register_virtio_device(struct virtio_device *dev); > void unregister_virtio_device(struct virtio_device *dev); > > void virtio_break_device(struct virtio_device *dev); > > void virtio_config_changed(struct virtio_device *dev); > +void virtio_config_disable(struct virtio_device *dev); > +void virtio_config_enable(struct virtio_device *dev); > +int virtio_finalize_features(struct virtio_device *dev); > #ifdef CONFIG_PM_SLEEP > int virtio_device_freeze(struct virtio_device *dev); > int virtio_device_restore(struct virtio_device *dev);