[PATCH v2] virtio_balloon: stay awake while adjusting balloon

[PATCH v2] virtio_balloon: stay awake while adjusting balloon

[David Stevens]

From: David Stevens <stevensd@chromium.org>

A virtio_balloon's parent device may be configured so that a
configuration change interrupt is a wakeup event. Extend the processing
of such a wakeup event until the balloon finishes inflating or deflating
by calling pm_stay_awake/pm_relax in the virtio_balloon driver. Note
that these calls are no-ops if the parent device doesn't support wakeup
events or if the wakeup events are not enabled.

This change allows the guest to use system power states such as s2idle
without running the risk the virtio_balloon's cooperative memory
management becoming unresponsive to the host's requests.

Signed-off-by: David Stevens <stevensd@chromium.org>
---
v1 -> v2:
 - Use adjustment_signal_pending flag instead of a sequence number
 - Call pm_stay_awake/pm_relax on parent device instead of adding a wake
   event to the virtio balloon device

 drivers/virtio/virtio_balloon.c | 57 +++++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 1fe93e93f5bc..a3c11159cbe0 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -119,6 +119,11 @@ struct virtio_balloon {
 	/* Free page reporting device */
 	struct virtqueue *reporting_vq;
 	struct page_reporting_dev_info pr_dev_info;
+
+	/* State for keeping the wakeup_source active while adjusting the balloon */
+	spinlock_t adjustment_lock;
+	bool adjustment_signal_pending;
+	bool adjustment_in_progress;
 };
 
 static const struct virtio_device_id id_table[] = {
@@ -437,6 +442,31 @@ static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
 	queue_work(vb->balloon_wq, &vb->report_free_page_work);
 }
 
+static void start_update_balloon_size(struct virtio_balloon *vb)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&vb->adjustment_lock, flags);
+	vb->adjustment_signal_pending = true;
+	if (!vb->adjustment_in_progress) {
+		vb->adjustment_in_progress = true;
+		pm_stay_awake(vb->vdev->dev.parent);
+	}
+	spin_unlock_irqrestore(&vb->adjustment_lock, flags);
+
+	queue_work(system_freezable_wq, &vb->update_balloon_size_work);
+}
+
+static void end_update_balloon_size(struct virtio_balloon *vb)
+{
+	spin_lock(&vb->adjustment_lock);
+	if (!vb->adjustment_signal_pending && vb->adjustment_in_progress) {
+		vb->adjustment_in_progress = false;
+		pm_relax(vb->vdev->dev.parent);
+	}
+	spin_unlock(&vb->adjustment_lock);
+}
+
 static void virtballoon_changed(struct virtio_device *vdev)
 {
 	struct virtio_balloon *vb = vdev->priv;
@@ -444,8 +474,7 @@ static void virtballoon_changed(struct virtio_device *vdev)
 
 	spin_lock_irqsave(&vb->stop_update_lock, flags);
 	if (!vb->stop_update) {
-		queue_work(system_freezable_wq,
-			   &vb->update_balloon_size_work);
+		start_update_balloon_size(vb);
 		virtio_balloon_queue_free_page_work(vb);
 	}
 	spin_unlock_irqrestore(&vb->stop_update_lock, flags);
@@ -476,19 +505,25 @@ static void update_balloon_size_func(struct work_struct *work)
 
 	vb = container_of(work, struct virtio_balloon,
 			  update_balloon_size_work);
-	diff = towards_target(vb);
 
-	if (!diff)
-		return;
+	spin_lock(&vb->adjustment_lock);
+	vb->adjustment_signal_pending = false;
+	spin_unlock(&vb->adjustment_lock);
 
-	if (diff > 0)
-		diff -= fill_balloon(vb, diff);
-	else
-		diff += leak_balloon(vb, -diff);
-	update_balloon_size(vb);
+	diff = towards_target(vb);
+
+	if (diff) {
+		if (diff > 0)
+			diff -= fill_balloon(vb, diff);
+		else
+			diff += leak_balloon(vb, -diff);
+		update_balloon_size(vb);
+	}
 
 	if (diff)
 		queue_work(system_freezable_wq, work);
+	else
+		end_update_balloon_size(vb);
 }
 
 static int init_vqs(struct virtio_balloon *vb)
@@ -992,6 +1027,8 @@ static int virtballoon_probe(struct virtio_device *vdev)
 			goto out_unregister_oom;
 	}
 
+	spin_lock_init(&vb->adjustment_lock);
+
 	virtio_device_ready(vdev);
 
 	if (towards_target(vb))
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH v2] virtio_balloon: stay awake while adjusting balloon

[David Hildenbrand]

On 18.12.23 16:18, David Stevens wrote:
> From: David Stevens <stevensd@chromium.org>
> 
> A virtio_balloon's parent device may be configured so that a
> configuration change interrupt is a wakeup event. Extend the processing
> of such a wakeup event until the balloon finishes inflating or deflating
> by calling pm_stay_awake/pm_relax in the virtio_balloon driver. Note
> that these calls are no-ops if the parent device doesn't support wakeup
> events or if the wakeup events are not enabled.
> 
> This change allows the guest to use system power states such as s2idle
> without running the risk the virtio_balloon's cooperative memory
> management becoming unresponsive to the host's requests.
> 
> Signed-off-by: David Stevens <stevensd@chromium.org>
> ---
> v1 -> v2:
>   - Use adjustment_signal_pending flag instead of a sequence number
>   - Call pm_stay_awake/pm_relax on parent device instead of adding a wake
>     event to the virtio balloon device
> 
>   drivers/virtio/virtio_balloon.c | 57 +++++++++++++++++++++++++++------
>   1 file changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 1fe93e93f5bc..a3c11159cbe0 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -119,6 +119,11 @@ struct virtio_balloon {
>   	/* Free page reporting device */
>   	struct virtqueue *reporting_vq;
>   	struct page_reporting_dev_info pr_dev_info;
> +
> +	/* State for keeping the wakeup_source active while adjusting the balloon */
> +	spinlock_t adjustment_lock;
> +	bool adjustment_signal_pending;
> +	bool adjustment_in_progress;
>   };
>   
>   static const struct virtio_device_id id_table[] = {
> @@ -437,6 +442,31 @@ static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
>   	queue_work(vb->balloon_wq, &vb->report_free_page_work);
>   }
>   
> +static void start_update_balloon_size(struct virtio_balloon *vb)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vb->adjustment_lock, flags);
> +	vb->adjustment_signal_pending = true;
> +	if (!vb->adjustment_in_progress) {
> +		vb->adjustment_in_progress = true;
> +		pm_stay_awake(vb->vdev->dev.parent);
> +	}
> +	spin_unlock_irqrestore(&vb->adjustment_lock, flags);
> +
> +	queue_work(system_freezable_wq, &vb->update_balloon_size_work);
> +}
> +
> +static void end_update_balloon_size(struct virtio_balloon *vb)
> +{
> +	spin_lock(&vb->adjustment_lock);
> +	if (!vb->adjustment_signal_pending && vb->adjustment_in_progress) {

How could vb->adjustment_in_progress ever not be set at this point?

> +		vb->adjustment_in_progress = false;
> +		pm_relax(vb->vdev->dev.parent);
> +	}
> +	spin_unlock(&vb->adjustment_lock);
> +}
> +

LGTM, although I wonder what happens when calling pm_stay_awake() etc. 
on a parent device that is not wakeup-even-capable?

-- 
Cheers,

David / dhildenb

Re: [PATCH v2] virtio_balloon: stay awake while adjusting balloon

[David Stevens]

On Mon, Dec 18, 2023 at 12:33 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 18.12.23 16:18, David Stevens wrote:
> > From: David Stevens <stevensd@chromium.org>
> >
> > A virtio_balloon's parent device may be configured so that a
> > configuration change interrupt is a wakeup event. Extend the processing
> > of such a wakeup event until the balloon finishes inflating or deflating
> > by calling pm_stay_awake/pm_relax in the virtio_balloon driver. Note
> > that these calls are no-ops if the parent device doesn't support wakeup
> > events or if the wakeup events are not enabled.
> >
> > This change allows the guest to use system power states such as s2idle
> > without running the risk the virtio_balloon's cooperative memory
> > management becoming unresponsive to the host's requests.
> >
> > Signed-off-by: David Stevens <stevensd@chromium.org>
> > ---
> > v1 -> v2:
> >   - Use adjustment_signal_pending flag instead of a sequence number
> >   - Call pm_stay_awake/pm_relax on parent device instead of adding a wake
> >     event to the virtio balloon device
> >
> >   drivers/virtio/virtio_balloon.c | 57 +++++++++++++++++++++++++++------
> >   1 file changed, 47 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index 1fe93e93f5bc..a3c11159cbe0 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -119,6 +119,11 @@ struct virtio_balloon {
> >       /* Free page reporting device */
> >       struct virtqueue *reporting_vq;
> >       struct page_reporting_dev_info pr_dev_info;
> > +
> > +     /* State for keeping the wakeup_source active while adjusting the balloon */
> > +     spinlock_t adjustment_lock;
> > +     bool adjustment_signal_pending;
> > +     bool adjustment_in_progress;
> >   };
> >
> >   static const struct virtio_device_id id_table[] = {
> > @@ -437,6 +442,31 @@ static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
> >       queue_work(vb->balloon_wq, &vb->report_free_page_work);
> >   }
> >
> > +static void start_update_balloon_size(struct virtio_balloon *vb)
> > +{
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&vb->adjustment_lock, flags);
> > +     vb->adjustment_signal_pending = true;
> > +     if (!vb->adjustment_in_progress) {
> > +             vb->adjustment_in_progress = true;
> > +             pm_stay_awake(vb->vdev->dev.parent);
> > +     }
> > +     spin_unlock_irqrestore(&vb->adjustment_lock, flags);
> > +
> > +     queue_work(system_freezable_wq, &vb->update_balloon_size_work);
> > +}
> > +
> > +static void end_update_balloon_size(struct virtio_balloon *vb)
> > +{
> > +     spin_lock(&vb->adjustment_lock);
> > +     if (!vb->adjustment_signal_pending && vb->adjustment_in_progress) {
>
> How could vb->adjustment_in_progress ever not be set at this point?
>
> > +             vb->adjustment_in_progress = false;
> > +             pm_relax(vb->vdev->dev.parent);
> > +     }
> > +     spin_unlock(&vb->adjustment_lock);
> > +}
> > +
>
> LGTM, although I wonder what happens when calling pm_stay_awake() etc.
> on a parent device that is not wakeup-even-capable?

If the parent device is not wakeup capable or if wakeup isn't enabled,
then the vb->vdev->dev.parent->power.wakeup pointer will be NULL, so
the NULL checks in __pm_relax/__pm_stay_awake will immediately return.

-David

Re: [PATCH v2] virtio_balloon: stay awake while adjusting balloon

[David Hildenbrand]

On 19.12.23 15:37, David Stevens wrote:
> On Mon, Dec 18, 2023 at 12:33 PM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 18.12.23 16:18, David Stevens wrote:
>>> From: David Stevens <stevensd@chromium.org>
>>>
>>> A virtio_balloon's parent device may be configured so that a
>>> configuration change interrupt is a wakeup event. Extend the processing
>>> of such a wakeup event until the balloon finishes inflating or deflating
>>> by calling pm_stay_awake/pm_relax in the virtio_balloon driver. Note
>>> that these calls are no-ops if the parent device doesn't support wakeup
>>> events or if the wakeup events are not enabled.
>>>
>>> This change allows the guest to use system power states such as s2idle
>>> without running the risk the virtio_balloon's cooperative memory
>>> management becoming unresponsive to the host's requests.
>>>
>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>> ---
>>> v1 -> v2:
>>>    - Use adjustment_signal_pending flag instead of a sequence number
>>>    - Call pm_stay_awake/pm_relax on parent device instead of adding a wake
>>>      event to the virtio balloon device
>>>
>>>    drivers/virtio/virtio_balloon.c | 57 +++++++++++++++++++++++++++------
>>>    1 file changed, 47 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>>> index 1fe93e93f5bc..a3c11159cbe0 100644
>>> --- a/drivers/virtio/virtio_balloon.c
>>> +++ b/drivers/virtio/virtio_balloon.c
>>> @@ -119,6 +119,11 @@ struct virtio_balloon {
>>>        /* Free page reporting device */
>>>        struct virtqueue *reporting_vq;
>>>        struct page_reporting_dev_info pr_dev_info;
>>> +
>>> +     /* State for keeping the wakeup_source active while adjusting the balloon */
>>> +     spinlock_t adjustment_lock;
>>> +     bool adjustment_signal_pending;
>>> +     bool adjustment_in_progress;
>>>    };
>>>
>>>    static const struct virtio_device_id id_table[] = {
>>> @@ -437,6 +442,31 @@ static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
>>>        queue_work(vb->balloon_wq, &vb->report_free_page_work);
>>>    }
>>>
>>> +static void start_update_balloon_size(struct virtio_balloon *vb)
>>> +{
>>> +     unsigned long flags;
>>> +
>>> +     spin_lock_irqsave(&vb->adjustment_lock, flags);
>>> +     vb->adjustment_signal_pending = true;
>>> +     if (!vb->adjustment_in_progress) {
>>> +             vb->adjustment_in_progress = true;
>>> +             pm_stay_awake(vb->vdev->dev.parent);
>>> +     }
>>> +     spin_unlock_irqrestore(&vb->adjustment_lock, flags);
>>> +
>>> +     queue_work(system_freezable_wq, &vb->update_balloon_size_work);
>>> +}
>>> +
>>> +static void end_update_balloon_size(struct virtio_balloon *vb)
>>> +{
>>> +     spin_lock(&vb->adjustment_lock);
>>> +     if (!vb->adjustment_signal_pending && vb->adjustment_in_progress) {
>>
>> How could vb->adjustment_in_progress ever not be set at this point?
>>
>>> +             vb->adjustment_in_progress = false;
>>> +             pm_relax(vb->vdev->dev.parent);
>>> +     }
>>> +     spin_unlock(&vb->adjustment_lock);
>>> +}
>>> +
>>
>> LGTM, although I wonder what happens when calling pm_stay_awake() etc.
>> on a parent device that is not wakeup-even-capable?
> 
> If the parent device is not wakeup capable or if wakeup isn't enabled,
> then the vb->vdev->dev.parent->power.wakeup pointer will be NULL, so
> the NULL checks in __pm_relax/__pm_stay_awake will immediately return.

Ah, I missed that NULL check, makes sense. Thanks!

-- 
Cheers,

David / dhildenb