Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1299871 > unrolled thread

Re: [PATCH v4 2/2] virtio_balloon: Use a workqueue instead of "vballoon" kthread

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 2/2] virtio_balloon: Use a workqueue instead of  "vballoon" kthread "Michael S. Tsirkin" <mst@redhat.com> - 2016-01-01 11:20 +0100
    Re: [PATCH v4 2/2] virtio_balloon: Use a workqueue instead of  "vballoon" kthread Tejun Heo <tj@kernel.org> - 2016-01-02 12:50 +0100
      Re: [PATCH v4 2/2] virtio_balloon: Use a workqueue instead of  "vballoon" kthread "Michael S. Tsirkin" <mst@redhat.com> - 2016-01-02 22:40 +0100
        Re: [PATCH v4 2/2] virtio_balloon: Use a workqueue instead of  "vballoon" kthread Tejun Heo <tj@kernel.org> - 2016-01-03 15:00 +0100
        Re: [PATCH v4 2/2] virtio_balloon: Use a workqueue instead of  "vballoon" kthread Petr Mladek <pmladek@suse.com> - 2016-01-05 15:50 +0100
          Re: [PATCH v4 2/2] virtio_balloon: Use a workqueue instead of  "vballoon" kthread "Michael S. Tsirkin" <mst@redhat.com> - 2016-01-05 16:40 +0100

#1299871 — Re: [PATCH v4 2/2] virtio_balloon: Use a workqueue instead of "vballoon" kthread

On Fri, Dec 04, 2015 at 02:37:51PM +0100, Petr Mladek wrote:
> From: Petr Mladek <pmladek@suse.cz>
> 
> This patch moves the deferred work from the "vballoon" kthread into a
> system freezable workqueue.
> 
> We do not need to maintain and run a dedicated kthread. Also the event
> driven workqueues API makes the logic much easier. Especially, we do
> not longer need an own wait queue, wait function, and freeze point.
> 
> The conversion is pretty straightforward. One cycle of the main loop
> is put into a work. The work is queued instead of waking the kthread.
> 
> fill_balloon() and leak_balloon() have a limit for the amount of modified
> pages. The work re-queues itself when necessary.
> 
> My initial idea was to use a dedicated workqueue. Michael S. Tsirkin
> suggested using a system one. Tejun Heo confirmed that the system
> workqueue has a pretty high concurrency level (256) by default.
> Therefore we need not be afraid of too long blocking.

Right but fill has a 1/5 second sleep on failure - *that*
is problematic for a system queue.

There's also a race introduced on remove, see below.

I'm inclined to tread carefully with this conversion.

> 
> Signed-off-by: Petr Mladek <pmladek@suse.cz>
> ---
>  drivers/virtio/virtio_balloon.c | 82 +++++++++++++----------------------------
>  1 file changed, 26 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index d73a86db2490..960e54b1d0c1 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -22,8 +22,7 @@
>  #include <linux/virtio.h>
>  #include <linux/virtio_balloon.h>
>  #include <linux/swap.h>
> -#include <linux/kthread.h>
> -#include <linux/freezer.h>
> +#include <linux/workqueue.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> @@ -49,11 +48,8 @@ struct virtio_balloon {
>  	struct virtio_device *vdev;
>  	struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
>  
> -	/* Where the ballooning thread waits for config to change. */
> -	wait_queue_head_t config_change;
> -
> -	/* The thread servicing the balloon. */
> -	struct task_struct *thread;
> +	/* The balloon servicing is delegated to a freezable workqueue. */
> +	struct work_struct wq_work;
>  
>  	/* Waiting for host to ack the pages we released. */
>  	wait_queue_head_t acked;
> @@ -255,14 +251,15 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>   * with a single buffer.  From that point forward, all conversations consist of
>   * a hypervisor request (a call to this function) which directs us to refill
>   * the virtqueue with a fresh stats buffer.  Since stats collection can sleep,
> - * we notify our kthread which does the actual work via stats_handle_request().
> + * we delegate the job to a freezable workqueue that will do the actual work via
> + * stats_handle_request().
>   */
>  static void stats_request(struct virtqueue *vq)
>  {
>  	struct virtio_balloon *vb = vq->vdev->priv;
>  
>  	vb->need_stats_update = 1;
> -	wake_up(&vb->config_change);
> +	queue_work(system_freezable_wq, &vb->wq_work);
>  }
>  
>  static void stats_handle_request(struct virtio_balloon *vb)
> @@ -286,7 +283,7 @@ static void virtballoon_changed(struct virtio_device *vdev)
>  {
>  	struct virtio_balloon *vb = vdev->priv;
>  
> -	wake_up(&vb->config_change);
> +	queue_work(system_freezable_wq, &vb->wq_work);
>  }
>  
>  static inline s64 towards_target(struct virtio_balloon *vb)
> @@ -349,43 +346,25 @@ static int virtballoon_oom_notify(struct notifier_block *self,
>  	return NOTIFY_OK;
>  }
>  
> -static int balloon(void *_vballoon)
> +static void balloon(struct work_struct *work)
>  {
> -	struct virtio_balloon *vb = _vballoon;
> -	DEFINE_WAIT_FUNC(wait, woken_wake_function);
> -
> -	set_freezable();
> -	while (!kthread_should_stop()) {
> -		s64 diff;
> -
> -		try_to_freeze();
> -
> -		add_wait_queue(&vb->config_change, &wait);
> -		for (;;) {
> -			if ((diff = towards_target(vb)) != 0 ||
> -			    vb->need_stats_update ||
> -			    kthread_should_stop() ||
> -			    freezing(current))
> -				break;
> -			wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
> -		}
> -		remove_wait_queue(&vb->config_change, &wait);
> +	struct virtio_balloon *vb;
> +	s64 diff;
>  
> -		if (vb->need_stats_update)
> -			stats_handle_request(vb);
> -		if (diff > 0)
> -			fill_balloon(vb, diff);
> -		else if (diff < 0)
> -			leak_balloon(vb, -diff);
> -		update_balloon_size(vb);
> +	vb = container_of(work, struct virtio_balloon, wq_work);
> +	diff = towards_target(vb);
>  
> -		/*
> -		 * For large balloon changes, we could spend a lot of time
> -		 * and always have work to do.  Be nice if preempt disabled.
> -		 */
> -		cond_resched();
> -	}
> -	return 0;
> +	if (vb->need_stats_update)
> +		stats_handle_request(vb);
> +
> +	if (diff > 0)
> +		diff -= fill_balloon(vb, diff);
> +	else if (diff < 0)
> +		diff += leak_balloon(vb, -diff);
> +	update_balloon_size(vb);
> +
> +	if (diff)
> +		queue_work(system_freezable_wq, work);
>  }
>  
>  static int init_vqs(struct virtio_balloon *vb)
> @@ -503,9 +482,9 @@ static int virtballoon_probe(struct virtio_device *vdev)
>  		goto out;
>  	}
>  
> +	INIT_WORK(&vb->wq_work, balloon);
>  	vb->num_pages = 0;
>  	mutex_init(&vb->balloon_lock);
> -	init_waitqueue_head(&vb->config_change);
>  	init_waitqueue_head(&vb->acked);
>  	vb->vdev = vdev;
>  	vb->need_stats_update = 0;
> @@ -527,16 +506,8 @@ static int virtballoon_probe(struct virtio_device *vdev)
>  
>  	virtio_device_ready(vdev);
>  
> -	vb->thread = kthread_run(balloon, vb, "vballoon");
> -	if (IS_ERR(vb->thread)) {
> -		err = PTR_ERR(vb->thread);
> -		goto out_del_vqs;
> -	}
> -
>  	return 0;
>  
> -out_del_vqs:
> -	unregister_oom_notifier(&vb->nb);
>  out_oom_notify:
>  	vdev->config->del_vqs(vdev);
>  out_free_vb:
> @@ -563,7 +534,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
>  	struct virtio_balloon *vb = vdev->priv;
>  
>  	unregister_oom_notifier(&vb->nb);
> -	kthread_stop(vb->thread);
> +	cancel_work_sync(&vb->wq_work);

OK but since job requeues itself, cancelling like this might not be enough.

>  	remove_common(vb);
>  	kfree(vb);
>  }
> @@ -574,10 +545,9 @@ static int virtballoon_freeze(struct virtio_device *vdev)
>  	struct virtio_balloon *vb = vdev->priv;
>  
>  	/*
> -	 * The kthread is already frozen by the PM core before this
> +	 * The workqueue is already frozen by the PM core before this
>  	 * function is called.
>  	 */
> -
>  	remove_common(vb);
>  	return 0;
>  }
> -- 
> 1.8.5.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1300061

Hello,

On Fri, Jan 01, 2016 at 12:18:17PM +0200, Michael S. Tsirkin wrote:
> > My initial idea was to use a dedicated workqueue. Michael S. Tsirkin
> > suggested using a system one. Tejun Heo confirmed that the system
> > workqueue has a pretty high concurrency level (256) by default.
> > Therefore we need not be afraid of too long blocking.
> 
> Right but fill has a 1/5 second sleep on failure - *that*
> is problematic for a system queue.

Why so?  As long as the maximum concurrently used workers are not
high, 1/5 second or even a lot longer sleeps are completely fine.

> > @@ -563,7 +534,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
> >  	struct virtio_balloon *vb = vdev->priv;
> >  
> >  	unregister_oom_notifier(&vb->nb);
> > -	kthread_stop(vb->thread);
> > +	cancel_work_sync(&vb->wq_work);
> 
> OK but since job requeues itself, cancelling like this might not be enough.

As long as there's no further external queueing, cancel_work_sync() is
guaranteed to kill a self-requeueing work item.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300129

On Sat, Jan 02, 2016 at 06:43:16AM -0500, Tejun Heo wrote:
> Hello,
> 
> On Fri, Jan 01, 2016 at 12:18:17PM +0200, Michael S. Tsirkin wrote:
> > > My initial idea was to use a dedicated workqueue. Michael S. Tsirkin
> > > suggested using a system one. Tejun Heo confirmed that the system
> > > workqueue has a pretty high concurrency level (256) by default.
> > > Therefore we need not be afraid of too long blocking.
> > 
> > Right but fill has a 1/5 second sleep on failure - *that*
> > is problematic for a system queue.
> 
> Why so?  As long as the maximum concurrently used workers are not
> high, 1/5 second or even a lot longer sleeps are completely fine.

I always thought the right way to defer executing a work queue item
is to queue delayed work, not sleep + queue work.

Doing a sleep ties up one thread for 1/5 of a second, does it not?
If so, as long as it's the only driver doing this, we'll be fine,
but if many others copy this pattern, things will
start to break, will they not?

> > > @@ -563,7 +534,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
> > >  	struct virtio_balloon *vb = vdev->priv;
> > >  
> > >  	unregister_oom_notifier(&vb->nb);
> > > -	kthread_stop(vb->thread);
> > > +	cancel_work_sync(&vb->wq_work);
> > 
> > OK but since job requeues itself, cancelling like this might not be enough.
> 
> As long as there's no further external queueing, cancel_work_sync() is
> guaranteed to kill a self-requeueing work item.
> 
> Thanks.

I didn't realise this. Thanks!

Unfortunately in this case, there can be further requeueing
if a stats request arrives.

> -- 
> tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1300306

Hello, Michael.

On Sat, Jan 02, 2016 at 11:36:03PM +0200, Michael S. Tsirkin wrote:
> > Why so?  As long as the maximum concurrently used workers are not
> > high, 1/5 second or even a lot longer sleeps are completely fine.
> 
> I always thought the right way to defer executing a work queue item
> is to queue delayed work, not sleep + queue work.

That works too and is preferable if there are gonna be a lot of work
items sleeping but it isn't different from any other blocking.

> Doing a sleep ties up one thread for 1/5 of a second, does it not?

It does.

> If so, as long as it's the only driver doing this, we'll be fine,
> but if many others copy this pattern, things will
> start to break, will they not?

The maximum concurrency on the system_wq is 256 which is pretty high,
so for most use cases, it's fine.  If high concurrency is expected,
it's better to break it out to a separate workqueue.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301578

On Sat 2016-01-02 23:36:03, Michael S. Tsirkin wrote:
> On Sat, Jan 02, 2016 at 06:43:16AM -0500, Tejun Heo wrote:
> > On Fri, Jan 01, 2016 at 12:18:17PM +0200, Michael S. Tsirkin wrote:
> > > > My initial idea was to use a dedicated workqueue. Michael S. Tsirkin
> > > > @@ -563,7 +534,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
> > > >  	struct virtio_balloon *vb = vdev->priv;
> > > >  
> > > >  	unregister_oom_notifier(&vb->nb);
> > > > -	kthread_stop(vb->thread);
> > > > +	cancel_work_sync(&vb->wq_work);
> > > 
> > > OK but since job requeues itself, cancelling like this might not be enough.
> > 
> > As long as there's no further external queueing, cancel_work_sync() is
> > guaranteed to kill a self-requeueing work item.
> > 
> > Thanks.
> 
> I didn't realise this. Thanks!
> 
> Unfortunately in this case, there can be further requeueing
> if a stats request arrives.

Please, is there any point where the stat requests are disabled for
sure? I am not 100% sure but it might be after the reset() call:

    vb->vdev->config->reset(vb->vdev);

Then we could split the kthread into two works: resizing and stats.
The resizing work still must be canceled before leaking the balloon.
But the stats work might be canceled after the reset() call.

In fact, the solution with the two works looks even cleaner.


Thanks for feedback,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301619

On Tue, Jan 05, 2016 at 03:49:18PM +0100, Petr Mladek wrote:
> On Sat 2016-01-02 23:36:03, Michael S. Tsirkin wrote:
> > On Sat, Jan 02, 2016 at 06:43:16AM -0500, Tejun Heo wrote:
> > > On Fri, Jan 01, 2016 at 12:18:17PM +0200, Michael S. Tsirkin wrote:
> > > > > My initial idea was to use a dedicated workqueue. Michael S. Tsirkin
> > > > > @@ -563,7 +534,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
> > > > >  	struct virtio_balloon *vb = vdev->priv;
> > > > >  
> > > > >  	unregister_oom_notifier(&vb->nb);
> > > > > -	kthread_stop(vb->thread);
> > > > > +	cancel_work_sync(&vb->wq_work);
> > > > 
> > > > OK but since job requeues itself, cancelling like this might not be enough.
> > > 
> > > As long as there's no further external queueing, cancel_work_sync() is
> > > guaranteed to kill a self-requeueing work item.
> > > 
> > > Thanks.
> > 
> > I didn't realise this. Thanks!
> > 
> > Unfortunately in this case, there can be further requeueing
> > if a stats request arrives.
> 
> Please, is there any point where the stat requests are disabled for
> sure? I am not 100% sure but it might be after the reset() call:
> 
>     vb->vdev->config->reset(vb->vdev);

Yes.

> Then we could split the kthread into two works: resizing and stats.
> The resizing work still must be canceled before leaking the balloon.
> But the stats work might be canceled after the reset() call.
> 
> In fact, the solution with the two works looks even cleaner.
> 
> 
> Thanks for feedback,
> Petr

I agree - in fact, not blocking stats call while inflate is blocked
would be very nice. As things then happen in parallel, we need to be
careful with locking and stuff.

That would be a good reason to switch to wq.

-- 
MST
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web