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

[Konstantin Neumoin <kneumoin@virtuozzo.com>]

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@virtuozzo.com>
>>>
>>> Patch
>>>      Commit 3d2a3774c1b046f548ebea0391a602fd5685a307
>>>      Author: Michael S. Tsirkin <mst@redhat.com>
>>>      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@suse.com>
>>     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@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: "Michael S. Tsirkin" <mst@redhat.com>
>>> ---
>>>   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
>
>