* [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-03 5:31 ` Wei Wang
0 siblings, 0 replies; 32+ messages in thread
From: Wei Wang @ 2019-01-03 5:31 UTC (permalink / raw)
To: virtio-dev, linux-kernel, virtualization, kvm, mst, cohuck
Cc: borntraeger, pbonzini, dgilbert, wei.w.wang, pasic
virtio-ccw has deadlock issues with reading config registers inside the
interrupt context, so we tweak the virtballoon_changed implementation
by moving the config read operations into the related workqueue contexts.
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
drivers/virtio/virtio_balloon.c | 54 ++++++++++++++++++++---------------------
1 file changed, 26 insertions(+), 28 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 728ecd1..9a82a11 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -394,33 +394,15 @@ static void virtballoon_changed(struct virtio_device *vdev)
{
struct virtio_balloon *vb = vdev->priv;
unsigned long flags;
- s64 diff = towards_target(vb);
-
- if (diff) {
- spin_lock_irqsave(&vb->stop_update_lock, flags);
- if (!vb->stop_update)
- queue_work(system_freezable_wq,
- &vb->update_balloon_size_work);
- spin_unlock_irqrestore(&vb->stop_update_lock, flags);
- }
- if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
- virtio_cread(vdev, struct virtio_balloon_config,
- free_page_report_cmd_id, &vb->cmd_id_received);
- if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
- /* Pass ULONG_MAX to give back all the free pages */
- return_free_pages_to_mm(vb, ULONG_MAX);
- } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
- vb->cmd_id_received !=
- virtio32_to_cpu(vdev, vb->cmd_id_active)) {
- spin_lock_irqsave(&vb->stop_update_lock, flags);
- if (!vb->stop_update) {
- queue_work(vb->balloon_wq,
- &vb->report_free_page_work);
- }
- spin_unlock_irqrestore(&vb->stop_update_lock, flags);
- }
+ spin_lock_irqsave(&vb->stop_update_lock, flags);
+ if (!vb->stop_update) {
+ queue_work(system_freezable_wq,
+ &vb->update_balloon_size_work);
+ if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
+ queue_work(vb->balloon_wq, &vb->report_free_page_work);
}
+ spin_unlock_irqrestore(&vb->stop_update_lock, flags);
}
static void update_balloon_size(struct virtio_balloon *vb)
@@ -637,11 +619,9 @@ static int send_free_pages(struct virtio_balloon *vb)
return 0;
}
-static void report_free_page_func(struct work_struct *work)
+static void virtio_balloon_report_free_page(struct virtio_balloon *vb)
{
int err;
- struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
- report_free_page_work);
struct device *dev = &vb->vdev->dev;
/* Start by sending the received cmd id to host with an outbuf. */
@@ -659,6 +639,24 @@ static void report_free_page_func(struct work_struct *work)
dev_err(dev, "Failed to send a stop id, err = %d\n", err);
}
+static void report_free_page_func(struct work_struct *work)
+{
+ struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
+ report_free_page_work);
+
+ virtio_cread(vb->vdev, struct virtio_balloon_config,
+ free_page_report_cmd_id, &vb->cmd_id_received);
+
+ if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
+ /* Pass ULONG_MAX to give back all the free pages */
+ return_free_pages_to_mm(vb, ULONG_MAX);
+ } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
+ vb->cmd_id_received !=
+ virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) {
+ virtio_balloon_report_free_page(vb);
+ }
+}
+
#ifdef CONFIG_BALLOON_COMPACTION
/*
* virtballoon_migratepage - perform the balloon page migration on behalf of
--
2.7.4
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-03 5:31 ` Wei Wang
0 siblings, 0 replies; 32+ messages in thread
From: Wei Wang @ 2019-01-03 5:31 UTC (permalink / raw)
To: virtio-dev, linux-kernel, virtualization, kvm, mst, cohuck
Cc: pasic, dgilbert, pbonzini
virtio-ccw has deadlock issues with reading config registers inside the
interrupt context, so we tweak the virtballoon_changed implementation
by moving the config read operations into the related workqueue contexts.
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
drivers/virtio/virtio_balloon.c | 54 ++++++++++++++++++++---------------------
1 file changed, 26 insertions(+), 28 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 728ecd1..9a82a11 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -394,33 +394,15 @@ static void virtballoon_changed(struct virtio_device *vdev)
{
struct virtio_balloon *vb = vdev->priv;
unsigned long flags;
- s64 diff = towards_target(vb);
-
- if (diff) {
- spin_lock_irqsave(&vb->stop_update_lock, flags);
- if (!vb->stop_update)
- queue_work(system_freezable_wq,
- &vb->update_balloon_size_work);
- spin_unlock_irqrestore(&vb->stop_update_lock, flags);
- }
- if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
- virtio_cread(vdev, struct virtio_balloon_config,
- free_page_report_cmd_id, &vb->cmd_id_received);
- if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
- /* Pass ULONG_MAX to give back all the free pages */
- return_free_pages_to_mm(vb, ULONG_MAX);
- } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
- vb->cmd_id_received !=
- virtio32_to_cpu(vdev, vb->cmd_id_active)) {
- spin_lock_irqsave(&vb->stop_update_lock, flags);
- if (!vb->stop_update) {
- queue_work(vb->balloon_wq,
- &vb->report_free_page_work);
- }
- spin_unlock_irqrestore(&vb->stop_update_lock, flags);
- }
+ spin_lock_irqsave(&vb->stop_update_lock, flags);
+ if (!vb->stop_update) {
+ queue_work(system_freezable_wq,
+ &vb->update_balloon_size_work);
+ if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
+ queue_work(vb->balloon_wq, &vb->report_free_page_work);
}
+ spin_unlock_irqrestore(&vb->stop_update_lock, flags);
}
static void update_balloon_size(struct virtio_balloon *vb)
@@ -637,11 +619,9 @@ static int send_free_pages(struct virtio_balloon *vb)
return 0;
}
-static void report_free_page_func(struct work_struct *work)
+static void virtio_balloon_report_free_page(struct virtio_balloon *vb)
{
int err;
- struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
- report_free_page_work);
struct device *dev = &vb->vdev->dev;
/* Start by sending the received cmd id to host with an outbuf. */
@@ -659,6 +639,24 @@ static void report_free_page_func(struct work_struct *work)
dev_err(dev, "Failed to send a stop id, err = %d\n", err);
}
+static void report_free_page_func(struct work_struct *work)
+{
+ struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
+ report_free_page_work);
+
+ virtio_cread(vb->vdev, struct virtio_balloon_config,
+ free_page_report_cmd_id, &vb->cmd_id_received);
+
+ if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
+ /* Pass ULONG_MAX to give back all the free pages */
+ return_free_pages_to_mm(vb, ULONG_MAX);
+ } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
+ vb->cmd_id_received !=
+ virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) {
+ virtio_balloon_report_free_page(vb);
+ }
+}
+
#ifdef CONFIG_BALLOON_COMPACTION
/*
* virtballoon_migratepage - perform the balloon page migration on behalf of
--
2.7.4
^ permalink raw reply related [flat|nested] 32+ messages in thread* [virtio-dev] Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
2019-01-03 5:31 ` Wei Wang
@ 2019-01-03 9:40 ` Cornelia Huck
-1 siblings, 0 replies; 32+ messages in thread
From: Cornelia Huck @ 2019-01-03 9:40 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, mst, borntraeger,
pbonzini, dgilbert, pasic
On Thu, 3 Jan 2019 13:31:01 +0800
Wei Wang <wei.w.wang@intel.com> wrote:
> virtio-ccw has deadlock issues with reading config registers inside the
s/config registers/the config space/ ?
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
Also credit Christian with a Reported-by:?
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> drivers/virtio/virtio_balloon.c | 54 ++++++++++++++++++++---------------------
> 1 file changed, 26 insertions(+), 28 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-03 9:40 ` Cornelia Huck
0 siblings, 0 replies; 32+ messages in thread
From: Cornelia Huck @ 2019-01-03 9:40 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, mst, borntraeger,
pbonzini, dgilbert, pasic
On Thu, 3 Jan 2019 13:31:01 +0800
Wei Wang <wei.w.wang@intel.com> wrote:
> virtio-ccw has deadlock issues with reading config registers inside the
s/config registers/the config space/ ?
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
Also credit Christian with a Reported-by:?
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> drivers/virtio/virtio_balloon.c | 54 ++++++++++++++++++++---------------------
> 1 file changed, 26 insertions(+), 28 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [virtio-dev] Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
2019-01-03 9:40 ` Cornelia Huck
@ 2019-01-03 10:31 ` Wei Wang
-1 siblings, 0 replies; 32+ messages in thread
From: Wei Wang @ 2019-01-03 10:31 UTC (permalink / raw)
To: Cornelia Huck
Cc: virtio-dev, linux-kernel, virtualization, kvm, mst, borntraeger,
pbonzini, dgilbert, pasic
On 01/03/2019 05:40 PM, Cornelia Huck wrote:
> On Thu, 3 Jan 2019 13:31:01 +0800
> Wei Wang <wei.w.wang@intel.com> wrote:
>
>> virtio-ccw has deadlock issues with reading config registers inside the
> s/config registers/the config space/ ?
Sounds good.
>
>> interrupt context, so we tweak the virtballoon_changed implementation
>> by moving the config read operations into the related workqueue contexts.
> Also credit Christian with a Reported-by:?
Yes, definitely. Sorry for missing that.
Best,
Wei
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-03 10:31 ` Wei Wang
0 siblings, 0 replies; 32+ messages in thread
From: Wei Wang @ 2019-01-03 10:31 UTC (permalink / raw)
To: Cornelia Huck
Cc: virtio-dev, linux-kernel, virtualization, kvm, mst, borntraeger,
pbonzini, dgilbert, pasic
On 01/03/2019 05:40 PM, Cornelia Huck wrote:
> On Thu, 3 Jan 2019 13:31:01 +0800
> Wei Wang <wei.w.wang@intel.com> wrote:
>
>> virtio-ccw has deadlock issues with reading config registers inside the
> s/config registers/the config space/ ?
Sounds good.
>
>> interrupt context, so we tweak the virtballoon_changed implementation
>> by moving the config read operations into the related workqueue contexts.
> Also credit Christian with a Reported-by:?
Yes, definitely. Sorry for missing that.
Best,
Wei
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
2019-01-03 9:40 ` Cornelia Huck
(?)
(?)
@ 2019-01-03 10:31 ` Wei Wang
-1 siblings, 0 replies; 32+ messages in thread
From: Wei Wang @ 2019-01-03 10:31 UTC (permalink / raw)
To: Cornelia Huck
Cc: virtio-dev, kvm, mst, linux-kernel, virtualization, pasic,
pbonzini, dgilbert
On 01/03/2019 05:40 PM, Cornelia Huck wrote:
> On Thu, 3 Jan 2019 13:31:01 +0800
> Wei Wang <wei.w.wang@intel.com> wrote:
>
>> virtio-ccw has deadlock issues with reading config registers inside the
> s/config registers/the config space/ ?
Sounds good.
>
>> interrupt context, so we tweak the virtballoon_changed implementation
>> by moving the config read operations into the related workqueue contexts.
> Also credit Christian with a Reported-by:?
Yes, definitely. Sorry for missing that.
Best,
Wei
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
2019-01-03 5:31 ` Wei Wang
` (2 preceding siblings ...)
(?)
@ 2019-01-03 9:40 ` Cornelia Huck
-1 siblings, 0 replies; 32+ messages in thread
From: Cornelia Huck @ 2019-01-03 9:40 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, kvm, mst, linux-kernel, virtualization, pasic,
pbonzini, dgilbert
On Thu, 3 Jan 2019 13:31:01 +0800
Wei Wang <wei.w.wang@intel.com> wrote:
> virtio-ccw has deadlock issues with reading config registers inside the
s/config registers/the config space/ ?
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
Also credit Christian with a Reported-by:?
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> drivers/virtio/virtio_balloon.c | 54 ++++++++++++++++++++---------------------
> 1 file changed, 26 insertions(+), 28 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 32+ messages in thread* [virtio-dev] Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
2019-01-03 5:31 ` Wei Wang
@ 2019-01-03 12:16 ` Halil Pasic
-1 siblings, 0 replies; 32+ messages in thread
From: Halil Pasic @ 2019-01-03 12:16 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, mst, cohuck,
borntraeger, pbonzini, dgilbert
On Thu, 3 Jan 2019 13:31:01 +0800
Wei Wang <wei.w.wang@intel.com> wrote:
> virtio-ccw has deadlock issues with reading config registers inside the
> interrupt context
I would say something like 'virtio-ccw does not support using
virtio_config_ops from an atomic context' as the limitation is not
limited to read config (aka. get()). But I'm fine with your formulation
as well.
> , so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
[..]
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-03 12:16 ` Halil Pasic
0 siblings, 0 replies; 32+ messages in thread
From: Halil Pasic @ 2019-01-03 12:16 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, mst, cohuck,
borntraeger, pbonzini, dgilbert
On Thu, 3 Jan 2019 13:31:01 +0800
Wei Wang <wei.w.wang@intel.com> wrote:
> virtio-ccw has deadlock issues with reading config registers inside the
> interrupt context
I would say something like 'virtio-ccw does not support using
virtio_config_ops from an atomic context' as the limitation is not
limited to read config (aka. get()). But I'm fine with your formulation
as well.
> , so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
[..]
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
2019-01-03 5:31 ` Wei Wang
` (4 preceding siblings ...)
(?)
@ 2019-01-03 16:42 ` Michael S. Tsirkin
-1 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2019-01-03 16:42 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, kvm, cohuck, linux-kernel, virtualization, pasic,
pbonzini, dgilbert
On Thu, Jan 03, 2019 at 01:31:01PM +0800, Wei Wang wrote:
> virtio-ccw has deadlock issues with reading config registers inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> drivers/virtio/virtio_balloon.c | 54 ++++++++++++++++++++---------------------
> 1 file changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 728ecd1..9a82a11 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -394,33 +394,15 @@ static void virtballoon_changed(struct virtio_device *vdev)
> {
> struct virtio_balloon *vb = vdev->priv;
> unsigned long flags;
> - s64 diff = towards_target(vb);
> -
> - if (diff) {
> - spin_lock_irqsave(&vb->stop_update_lock, flags);
> - if (!vb->stop_update)
> - queue_work(system_freezable_wq,
> - &vb->update_balloon_size_work);
> - spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> - }
>
> - if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> - virtio_cread(vdev, struct virtio_balloon_config,
> - free_page_report_cmd_id, &vb->cmd_id_received);
There's one problem with this approach:
previously updating the cmd_id_received here would immediately
stop the report in send_free_pages.
With this approach we are waiting for the wq to schedule,
which might be blocked waiting for report to complete.
So host can no longer quickly stop the report in progress.
A simple work-around would be to set some kind of flag whenever there
is a change interrupt, then have send_free_pages test it
and re-read cmd_id_received.
Needs to be an atomic I guess ...
> - if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> - /* Pass ULONG_MAX to give back all the free pages */
> - return_free_pages_to_mm(vb, ULONG_MAX);
> - } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> - vb->cmd_id_received !=
> - virtio32_to_cpu(vdev, vb->cmd_id_active)) {
> - spin_lock_irqsave(&vb->stop_update_lock, flags);
> - if (!vb->stop_update) {
> - queue_work(vb->balloon_wq,
> - &vb->report_free_page_work);
> - }
> - spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> - }
> + spin_lock_irqsave(&vb->stop_update_lock, flags);
> + if (!vb->stop_update) {
> + queue_work(system_freezable_wq,
> + &vb->update_balloon_size_work);
> + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> + queue_work(vb->balloon_wq, &vb->report_free_page_work);
> }
> + spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> }
>
> static void update_balloon_size(struct virtio_balloon *vb)
> @@ -637,11 +619,9 @@ static int send_free_pages(struct virtio_balloon *vb)
> return 0;
> }
>
> -static void report_free_page_func(struct work_struct *work)
> +static void virtio_balloon_report_free_page(struct virtio_balloon *vb)
> {
> int err;
> - struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> - report_free_page_work);
> struct device *dev = &vb->vdev->dev;
>
> /* Start by sending the received cmd id to host with an outbuf. */
> @@ -659,6 +639,24 @@ static void report_free_page_func(struct work_struct *work)
> dev_err(dev, "Failed to send a stop id, err = %d\n", err);
> }
>
> +static void report_free_page_func(struct work_struct *work)
> +{
> + struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> + report_free_page_work);
> +
> + virtio_cread(vb->vdev, struct virtio_balloon_config,
> + free_page_report_cmd_id, &vb->cmd_id_received);
> +
> + if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> + /* Pass ULONG_MAX to give back all the free pages */
> + return_free_pages_to_mm(vb, ULONG_MAX);
> + } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> + vb->cmd_id_received !=
> + virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) {
> + virtio_balloon_report_free_page(vb);
> + }
> +}
> +
> #ifdef CONFIG_BALLOON_COMPACTION
> /*
> * virtballoon_migratepage - perform the balloon page migration on behalf of
> --
> 2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread* [virtio-dev] Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
2019-01-03 5:31 ` Wei Wang
@ 2019-01-03 16:42 ` Michael S. Tsirkin
-1 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2019-01-03 16:42 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, cohuck,
borntraeger, pbonzini, dgilbert, pasic
On Thu, Jan 03, 2019 at 01:31:01PM +0800, Wei Wang wrote:
> virtio-ccw has deadlock issues with reading config registers inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> drivers/virtio/virtio_balloon.c | 54 ++++++++++++++++++++---------------------
> 1 file changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 728ecd1..9a82a11 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -394,33 +394,15 @@ static void virtballoon_changed(struct virtio_device *vdev)
> {
> struct virtio_balloon *vb = vdev->priv;
> unsigned long flags;
> - s64 diff = towards_target(vb);
> -
> - if (diff) {
> - spin_lock_irqsave(&vb->stop_update_lock, flags);
> - if (!vb->stop_update)
> - queue_work(system_freezable_wq,
> - &vb->update_balloon_size_work);
> - spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> - }
>
> - if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> - virtio_cread(vdev, struct virtio_balloon_config,
> - free_page_report_cmd_id, &vb->cmd_id_received);
There's one problem with this approach:
previously updating the cmd_id_received here would immediately
stop the report in send_free_pages.
With this approach we are waiting for the wq to schedule,
which might be blocked waiting for report to complete.
So host can no longer quickly stop the report in progress.
A simple work-around would be to set some kind of flag whenever there
is a change interrupt, then have send_free_pages test it
and re-read cmd_id_received.
Needs to be an atomic I guess ...
> - if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> - /* Pass ULONG_MAX to give back all the free pages */
> - return_free_pages_to_mm(vb, ULONG_MAX);
> - } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> - vb->cmd_id_received !=
> - virtio32_to_cpu(vdev, vb->cmd_id_active)) {
> - spin_lock_irqsave(&vb->stop_update_lock, flags);
> - if (!vb->stop_update) {
> - queue_work(vb->balloon_wq,
> - &vb->report_free_page_work);
> - }
> - spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> - }
> + spin_lock_irqsave(&vb->stop_update_lock, flags);
> + if (!vb->stop_update) {
> + queue_work(system_freezable_wq,
> + &vb->update_balloon_size_work);
> + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> + queue_work(vb->balloon_wq, &vb->report_free_page_work);
> }
> + spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> }
>
> static void update_balloon_size(struct virtio_balloon *vb)
> @@ -637,11 +619,9 @@ static int send_free_pages(struct virtio_balloon *vb)
> return 0;
> }
>
> -static void report_free_page_func(struct work_struct *work)
> +static void virtio_balloon_report_free_page(struct virtio_balloon *vb)
> {
> int err;
> - struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> - report_free_page_work);
> struct device *dev = &vb->vdev->dev;
>
> /* Start by sending the received cmd id to host with an outbuf. */
> @@ -659,6 +639,24 @@ static void report_free_page_func(struct work_struct *work)
> dev_err(dev, "Failed to send a stop id, err = %d\n", err);
> }
>
> +static void report_free_page_func(struct work_struct *work)
> +{
> + struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> + report_free_page_work);
> +
> + virtio_cread(vb->vdev, struct virtio_balloon_config,
> + free_page_report_cmd_id, &vb->cmd_id_received);
> +
> + if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> + /* Pass ULONG_MAX to give back all the free pages */
> + return_free_pages_to_mm(vb, ULONG_MAX);
> + } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> + vb->cmd_id_received !=
> + virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) {
> + virtio_balloon_report_free_page(vb);
> + }
> +}
> +
> #ifdef CONFIG_BALLOON_COMPACTION
> /*
> * virtballoon_migratepage - perform the balloon page migration on behalf of
> --
> 2.7.4
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-03 16:42 ` Michael S. Tsirkin
0 siblings, 0 replies; 32+ messages in thread
From: Michael S. Tsirkin @ 2019-01-03 16:42 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, cohuck,
borntraeger, pbonzini, dgilbert, pasic
On Thu, Jan 03, 2019 at 01:31:01PM +0800, Wei Wang wrote:
> virtio-ccw has deadlock issues with reading config registers inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> drivers/virtio/virtio_balloon.c | 54 ++++++++++++++++++++---------------------
> 1 file changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 728ecd1..9a82a11 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -394,33 +394,15 @@ static void virtballoon_changed(struct virtio_device *vdev)
> {
> struct virtio_balloon *vb = vdev->priv;
> unsigned long flags;
> - s64 diff = towards_target(vb);
> -
> - if (diff) {
> - spin_lock_irqsave(&vb->stop_update_lock, flags);
> - if (!vb->stop_update)
> - queue_work(system_freezable_wq,
> - &vb->update_balloon_size_work);
> - spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> - }
>
> - if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
> - virtio_cread(vdev, struct virtio_balloon_config,
> - free_page_report_cmd_id, &vb->cmd_id_received);
There's one problem with this approach:
previously updating the cmd_id_received here would immediately
stop the report in send_free_pages.
With this approach we are waiting for the wq to schedule,
which might be blocked waiting for report to complete.
So host can no longer quickly stop the report in progress.
A simple work-around would be to set some kind of flag whenever there
is a change interrupt, then have send_free_pages test it
and re-read cmd_id_received.
Needs to be an atomic I guess ...
> - if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> - /* Pass ULONG_MAX to give back all the free pages */
> - return_free_pages_to_mm(vb, ULONG_MAX);
> - } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> - vb->cmd_id_received !=
> - virtio32_to_cpu(vdev, vb->cmd_id_active)) {
> - spin_lock_irqsave(&vb->stop_update_lock, flags);
> - if (!vb->stop_update) {
> - queue_work(vb->balloon_wq,
> - &vb->report_free_page_work);
> - }
> - spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> - }
> + spin_lock_irqsave(&vb->stop_update_lock, flags);
> + if (!vb->stop_update) {
> + queue_work(system_freezable_wq,
> + &vb->update_balloon_size_work);
> + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> + queue_work(vb->balloon_wq, &vb->report_free_page_work);
> }
> + spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> }
>
> static void update_balloon_size(struct virtio_balloon *vb)
> @@ -637,11 +619,9 @@ static int send_free_pages(struct virtio_balloon *vb)
> return 0;
> }
>
> -static void report_free_page_func(struct work_struct *work)
> +static void virtio_balloon_report_free_page(struct virtio_balloon *vb)
> {
> int err;
> - struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> - report_free_page_work);
> struct device *dev = &vb->vdev->dev;
>
> /* Start by sending the received cmd id to host with an outbuf. */
> @@ -659,6 +639,24 @@ static void report_free_page_func(struct work_struct *work)
> dev_err(dev, "Failed to send a stop id, err = %d\n", err);
> }
>
> +static void report_free_page_func(struct work_struct *work)
> +{
> + struct virtio_balloon *vb = container_of(work, struct virtio_balloon,
> + report_free_page_work);
> +
> + virtio_cread(vb->vdev, struct virtio_balloon_config,
> + free_page_report_cmd_id, &vb->cmd_id_received);
> +
> + if (vb->cmd_id_received == VIRTIO_BALLOON_CMD_ID_DONE) {
> + /* Pass ULONG_MAX to give back all the free pages */
> + return_free_pages_to_mm(vb, ULONG_MAX);
> + } else if (vb->cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> + vb->cmd_id_received !=
> + virtio32_to_cpu(vb->vdev, vb->cmd_id_active)) {
> + virtio_balloon_report_free_page(vb);
> + }
> +}
> +
> #ifdef CONFIG_BALLOON_COMPACTION
> /*
> * virtballoon_migratepage - perform the balloon page migration on behalf of
> --
> 2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [virtio-dev] Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
2019-01-03 16:42 ` Michael S. Tsirkin
(?)
@ 2019-01-04 5:14 ` Wei Wang
-1 siblings, 0 replies; 32+ messages in thread
From: Wei Wang @ 2019-01-04 5:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev, linux-kernel, virtualization, kvm, cohuck,
borntraeger, pbonzini, dgilbert, pasic
On 01/04/2019 12:42 AM, Michael S. Tsirkin wrote:
> On Thu, Jan 03, 2019 at 01:31:01PM +0800, Wei Wang wrote:
>> virtio-ccw has deadlock issues with reading config registers inside the
>> interrupt context, so we tweak the virtballoon_changed implementation
>> by moving the config read operations into the related workqueue contexts.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> ---
>> drivers/virtio/virtio_balloon.c | 54 ++++++++++++++++++++---------------------
>> 1 file changed, 26 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index 728ecd1..9a82a11 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -394,33 +394,15 @@ static void virtballoon_changed(struct virtio_device *vdev)
>> {
>> struct virtio_balloon *vb = vdev->priv;
>> unsigned long flags;
>> - s64 diff = towards_target(vb);
>> -
>> - if (diff) {
>> - spin_lock_irqsave(&vb->stop_update_lock, flags);
>> - if (!vb->stop_update)
>> - queue_work(system_freezable_wq,
>> - &vb->update_balloon_size_work);
>> - spin_unlock_irqrestore(&vb->stop_update_lock, flags);
>> - }
>>
>> - if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>> - virtio_cread(vdev, struct virtio_balloon_config,
>> - free_page_report_cmd_id, &vb->cmd_id_received);
> There's one problem with this approach:
>
> previously updating the cmd_id_received here would immediately
> stop the report in send_free_pages.
>
> With this approach we are waiting for the wq to schedule,
> which might be blocked waiting for report to complete.
> So host can no longer quickly stop the report in progress.
>
> A simple work-around would be to set some kind of flag whenever there
> is a change interrupt, then have send_free_pages test it
> and re-read cmd_id_received.
>
> Needs to be an atomic I guess ...
>
Yes, sounds better..will update the patch.
Best,
Wei
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [virtio-dev] Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-04 5:14 ` Wei Wang
0 siblings, 0 replies; 32+ messages in thread
From: Wei Wang @ 2019-01-04 5:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev, linux-kernel, virtualization, kvm, cohuck,
borntraeger, pbonzini, dgilbert, pasic
On 01/04/2019 12:42 AM, Michael S. Tsirkin wrote:
> On Thu, Jan 03, 2019 at 01:31:01PM +0800, Wei Wang wrote:
>> virtio-ccw has deadlock issues with reading config registers inside the
>> interrupt context, so we tweak the virtballoon_changed implementation
>> by moving the config read operations into the related workqueue contexts.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> ---
>> drivers/virtio/virtio_balloon.c | 54 ++++++++++++++++++++---------------------
>> 1 file changed, 26 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index 728ecd1..9a82a11 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -394,33 +394,15 @@ static void virtballoon_changed(struct virtio_device *vdev)
>> {
>> struct virtio_balloon *vb = vdev->priv;
>> unsigned long flags;
>> - s64 diff = towards_target(vb);
>> -
>> - if (diff) {
>> - spin_lock_irqsave(&vb->stop_update_lock, flags);
>> - if (!vb->stop_update)
>> - queue_work(system_freezable_wq,
>> - &vb->update_balloon_size_work);
>> - spin_unlock_irqrestore(&vb->stop_update_lock, flags);
>> - }
>>
>> - if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>> - virtio_cread(vdev, struct virtio_balloon_config,
>> - free_page_report_cmd_id, &vb->cmd_id_received);
> There's one problem with this approach:
>
> previously updating the cmd_id_received here would immediately
> stop the report in send_free_pages.
>
> With this approach we are waiting for the wq to schedule,
> which might be blocked waiting for report to complete.
> So host can no longer quickly stop the report in progress.
>
> A simple work-around would be to set some kind of flag whenever there
> is a change interrupt, then have send_free_pages test it
> and re-read cmd_id_received.
>
> Needs to be an atomic I guess ...
>
Yes, sounds better..will update the patch.
Best,
Wei
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-04 5:14 ` Wei Wang
0 siblings, 0 replies; 32+ messages in thread
From: Wei Wang @ 2019-01-04 5:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev, linux-kernel, virtualization, kvm, cohuck,
borntraeger, pbonzini, dgilbert, pasic
On 01/04/2019 12:42 AM, Michael S. Tsirkin wrote:
> On Thu, Jan 03, 2019 at 01:31:01PM +0800, Wei Wang wrote:
>> virtio-ccw has deadlock issues with reading config registers inside the
>> interrupt context, so we tweak the virtballoon_changed implementation
>> by moving the config read operations into the related workqueue contexts.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> ---
>> drivers/virtio/virtio_balloon.c | 54 ++++++++++++++++++++---------------------
>> 1 file changed, 26 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index 728ecd1..9a82a11 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -394,33 +394,15 @@ static void virtballoon_changed(struct virtio_device *vdev)
>> {
>> struct virtio_balloon *vb = vdev->priv;
>> unsigned long flags;
>> - s64 diff = towards_target(vb);
>> -
>> - if (diff) {
>> - spin_lock_irqsave(&vb->stop_update_lock, flags);
>> - if (!vb->stop_update)
>> - queue_work(system_freezable_wq,
>> - &vb->update_balloon_size_work);
>> - spin_unlock_irqrestore(&vb->stop_update_lock, flags);
>> - }
>>
>> - if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>> - virtio_cread(vdev, struct virtio_balloon_config,
>> - free_page_report_cmd_id, &vb->cmd_id_received);
> There's one problem with this approach:
>
> previously updating the cmd_id_received here would immediately
> stop the report in send_free_pages.
>
> With this approach we are waiting for the wq to schedule,
> which might be blocked waiting for report to complete.
> So host can no longer quickly stop the report in progress.
>
> A simple work-around would be to set some kind of flag whenever there
> is a change interrupt, then have send_free_pages test it
> and re-read cmd_id_received.
>
> Needs to be an atomic I guess ...
>
Yes, sounds better..will update the patch.
Best,
Wei
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [virtio-dev] Re: [PATCH v1 1/2] virtio-balloon: tweak config_changed implementation
2019-01-03 16:42 ` Michael S. Tsirkin
(?)
(?)
@ 2019-01-04 5:14 ` Wei Wang
-1 siblings, 0 replies; 32+ messages in thread
From: Wei Wang @ 2019-01-04 5:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev, kvm, cohuck, linux-kernel, virtualization, pasic,
pbonzini, dgilbert
On 01/04/2019 12:42 AM, Michael S. Tsirkin wrote:
> On Thu, Jan 03, 2019 at 01:31:01PM +0800, Wei Wang wrote:
>> virtio-ccw has deadlock issues with reading config registers inside the
>> interrupt context, so we tweak the virtballoon_changed implementation
>> by moving the config read operations into the related workqueue contexts.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> ---
>> drivers/virtio/virtio_balloon.c | 54 ++++++++++++++++++++---------------------
>> 1 file changed, 26 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index 728ecd1..9a82a11 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -394,33 +394,15 @@ static void virtballoon_changed(struct virtio_device *vdev)
>> {
>> struct virtio_balloon *vb = vdev->priv;
>> unsigned long flags;
>> - s64 diff = towards_target(vb);
>> -
>> - if (diff) {
>> - spin_lock_irqsave(&vb->stop_update_lock, flags);
>> - if (!vb->stop_update)
>> - queue_work(system_freezable_wq,
>> - &vb->update_balloon_size_work);
>> - spin_unlock_irqrestore(&vb->stop_update_lock, flags);
>> - }
>>
>> - if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>> - virtio_cread(vdev, struct virtio_balloon_config,
>> - free_page_report_cmd_id, &vb->cmd_id_received);
> There's one problem with this approach:
>
> previously updating the cmd_id_received here would immediately
> stop the report in send_free_pages.
>
> With this approach we are waiting for the wq to schedule,
> which might be blocked waiting for report to complete.
> So host can no longer quickly stop the report in progress.
>
> A simple work-around would be to set some kind of flag whenever there
> is a change interrupt, then have send_free_pages test it
> and re-read cmd_id_received.
>
> Needs to be an atomic I guess ...
>
Yes, sounds better..will update the patch.
Best,
Wei
^ permalink raw reply [flat|nested] 32+ messages in thread