From: "Michael S. Tsirkin" <mst@redhat.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: jasowang@redhat.com, john.r.fastabend@intel.com,
netdev@vger.kernel.org, alexei.starovoitov@gmail.com,
daniel@iogearbox.net
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 [thread overview]
Message-ID: <20170118174930-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20170117222223.20280.76302.stgit@john-Precision-Tower-5810>
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 <john.r.fastabend@intel.com>
> ---
> 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);
next prev parent reply other threads:[~2017-01-18 15:51 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-17 22:19 [net PATCH v5 0/6] virtio_net XDP fixes and adjust_header support John Fastabend
2017-01-17 22:19 ` [net PATCH v5 1/6] virtio_net: use dev_kfree_skb for small buffer XDP receive John Fastabend
2017-01-18 15:48 ` Michael S. Tsirkin
2017-01-23 21:08 ` Michael S. Tsirkin
2017-01-23 21:57 ` John Fastabend
2017-01-24 19:43 ` David Miller
2017-01-24 20:08 ` Michael S. Tsirkin
2017-01-24 20:11 ` David Miller
2017-01-24 20:54 ` Michael S. Tsirkin
2017-01-25 2:57 ` Jason Wang
2017-01-25 3:23 ` Michael S. Tsirkin
2017-01-25 4:02 ` John Fastabend
2017-01-25 5:46 ` Jason Wang
2017-01-25 14:47 ` Michael S. Tsirkin
2017-01-25 14:45 ` Michael S. Tsirkin
2017-01-17 22:20 ` [net PATCH v5 2/6] virtio_net: wrap rtnl_lock in test for calling with lock already held John Fastabend
2017-01-17 22:21 ` [net PATCH v5 3/6] virtio_net: factor out xdp handler for readability John Fastabend
2017-01-18 15:48 ` Michael S. Tsirkin
2017-01-17 22:21 ` [net PATCH v5 4/6] virtio_net: remove duplicate queue pair binding in XDP John Fastabend
2017-01-18 15:49 ` Michael S. Tsirkin
2017-01-17 22:22 ` [net PATCH v5 5/6] virtio_net: refactor freeze/restore logic into virtnet reset logic John Fastabend
2017-01-18 15:50 ` Michael S. Tsirkin [this message]
2017-01-17 22:22 ` [net PATCH v5 6/6] virtio_net: XDP support for adjust_head John Fastabend
2017-01-18 3:35 ` Jason Wang
2017-01-18 15:15 ` Michael S. Tsirkin
2017-01-19 3:05 ` Jason Wang
2017-01-19 21:11 ` Michael S. Tsirkin
2017-01-20 3:26 ` Jason Wang
2017-01-20 3:39 ` John Fastabend
2017-01-20 3:38 ` John Fastabend
2017-01-20 16:59 ` David Laight
2017-01-20 17:48 ` Michael S. Tsirkin
2017-01-22 2:51 ` Jason Wang
2017-01-22 4:14 ` John Fastabend
2017-01-23 17:02 ` Michael S. Tsirkin
2017-01-23 19:22 ` Michael S. Tsirkin
2017-01-23 20:09 ` Michael S. Tsirkin
2017-01-23 22:12 ` John Fastabend
2017-01-23 22:28 ` Michael S. Tsirkin
2017-01-18 15:48 ` [net PATCH v5 0/6] virtio_net XDP fixes and adjust_header support Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170118174930-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=alexei.starovoitov@gmail.com \
--cc=daniel@iogearbox.net \
--cc=jasowang@redhat.com \
--cc=john.fastabend@gmail.com \
--cc=john.r.fastabend@intel.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).