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

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

[Denis V. Lunev]

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.

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:
-- 
2.7.4

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

[Michael S. Tsirkin]

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
    


> 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.


> -- 
> 2.7.4

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@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.
>
>> -- 
>> 2.7.4

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@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
>
>