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

[Konstantin Neumoin]

On 09/26/2016 10:16 AM, Konstantin Neumoin wrote:
> 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.
> >

 Also we could try inflate the balloon once(by fill_balloon()) on probe 
and it leads to config change,
but we will not receive this notification because of config change 
pending will be enabled after
drv->prob() in virtio_dev_probe.
> > > --
> > > 2.7.4