Re: [PATCH 1/1] update balloon size in balloon "probe"

[Konstantin Neumoin]

On 09/23/2016 09:58 PM, Michael S. Tsirkin wrote:
> On Fri, Sep 23, 2016 at 04:47:57PM +0300, Denis V. Lunev wrote:
> > From: Konstantin Neumoin <kneumoin@xxxxxxxxxxxxx>
> >
> > Patch
> >      Commit 3d2a3774c1b046f548ebea0391a602fd5685a307
> >      Author: Michael S. Tsirkin <mst@xxxxxxxxxx>
> >      Date:   Tue Mar 10 11:55:08 2015 +1030
> >      virtio-balloon: do not call blocking ops when !TASK_RUNNING
> > has added a regression. Original code with wait_event_interruptible
> > checked the condition before start waiting and started balloon operations
> > if necessary.
> I don't get it, sorry.
>
> +               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);
>
> Seems to check the condition before waiting.
>
> The issue is more likely with this patch:
>
> 	commit fad7b7b27b6a168ca8ebc84482043886f837b89d
> 	Author: Petr Mladek <pmladek@xxxxxxxx>
> 	Date:   Mon Jan 25 17:38:05 2016 +0100
>
> 	    virtio_balloon: Use a workqueue instead of "vballoon" kthread
yes, you are right, sorry, commit message is incorrect.
>      
>
>
> > Right now balloon is not inflated if ballon target is set before the
> > driver is loaded.
> >
> > Signed-off-by: Konstantin Neumoin <kneumoin@xxxxxxxxxxxxx>
> > Signed-off-by: Denis V. Lunev <den@xxxxxxxxxx>
> > CC: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
> > ---
> >   drivers/virtio/virtio_balloon.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index 4e7003d..0a6c10f 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -577,6 +577,10 @@ static int virtballoon_probe(struct virtio_device *vdev)
> >   
> >   	virtio_device_ready(vdev);
> >   
> > +	if (towards_target(vb))
> > +		virtballoon_changed(vdev);
> > +	update_balloon_size(vb);
> > +
> >   	return 0;
> >   
> >   out_del_vqs:
>
> I know we have same thing on restore, but it seems bogus
> there as well:
>          if (towards_target(vb))
>                  virtballoon_changed(vdev);
>          update_balloon_size(vb);
>
> makes no sense because virtballoon_changed merely queues
> the work.
 in virtballoon_probe we just init work items, but if we have a target 
- num_pages != 0
we will catch it only after virtballoon_changed will be called, isn't it?

 So, I believe, on probe, we have to do the same thing as on restore: 
check towards_target(vb)
and call virtballoon_changed(vdev) if necessary.
> > --
> > 2.7.4