Re: [net PATCH v5 5/6] virtio_net: refactor freeze/restore logic into virtnet reset logic

[Michael S. Tsirkin]

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(>config_work);
> +
> + netif_device_detach(vi->dev);
> + cancel_delayed_work_sync(>refill);
> +
> + if (netif_running(vi->dev)) {
> + for (i = 0; i < vi->max_queue_pairs; i++)
> + napi_disable(>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, >rq[i], GFP_KERNEL))
> + schedule_delayed_work(>refill, 0);
> +
> + for (i = 0; i < vi->max_queue_pairs; i++)
> + virtnet_napi_enable(>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(>config_work);
> -
> - netif_device_detach(vi->dev);
> - cancel_delayed_work_sync(>refill);
> -
> - if (netif_running(vi->dev)) {
> - for (i = 0; i < vi->max_queue_pairs; i++)
> - napi_disable(>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, >rq[i], GFP_KERNEL))
> - schedule_delayed_work(>refill, 0);
> -
> - for (i = 0; i < vi->max_queue_pairs; i++)
> - virtnet_napi_enable(>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)
>  }
>  

[net PATCH v5 5/6] virtio_net: refactor freeze/restore logic into virtnet reset logic

[John Fastabend]

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(>config_work);
+
+   netif_device_detach(vi->dev);
+   cancel_delayed_work_sync(>refill);
+
+   if (netif_running(vi->dev)) {
+   for (i = 0; i < vi->max_queue_pairs; i++)
+   napi_disable(>rq[i].napi);
+   }
+}
+
+static int init_vqs(struct virtnet_info *vi);
+
+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, >rq[i], GFP_KERNEL))
+   schedule_delayed_work(>refill, 0);
+
+   for (i = 0; i < vi->max_queue_pairs; i++)
+   virtnet_napi_enable(>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(>config_work);
-
-   netif_device_detach(vi->dev);
-   cancel_delayed_work_sync(>refill);
-
-   if (netif_running(vi->dev)) {
-   for (i = 0; i < vi->max_queue_pairs; i++)
-   napi_disable(>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, >rq[i], GFP_KERNEL))
-   schedule_delayed_work(>refill, 0);
-
-   for (i = 0; i < vi->max_queue_pairs; i++)
-   virtnet_napi_enable(>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(>config_lock);
dev->config_enabled = false;
spin_unlock_irq(>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)
 {