Linux Virtualization - Jan 2016 - [PATCH v4 2/2] virtio_balloon: Use a workqueue instead of "vballoon" kthread

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

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

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

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