* [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-04 7:11 ` Wei Wang
0 siblings, 0 replies; 23+ messages in thread
From: Wei Wang @ 2019-01-04 7:11 UTC (permalink / raw)
To: virtio-dev, linux-kernel, virtualization, kvm, mst, cohuck, pasic,
borntraeger
Cc: pbonzini, dgilbert, wei.w.wang
virtio-ccw has deadlock issues with reading the config space inside the
interrupt context, so we tweak the virtballoon_changed implementation
by moving the config read operations into the related workqueue contexts.
The config_read_bitmap is used as a flag to the workqueue callbacks
about the related config fields that need to be read.
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
drivers/virtio/virtio_balloon.c | 81 +++++++++++++++++++++++++++--------------
1 file changed, 53 insertions(+), 28 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 728ecd1..35ee762 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -61,6 +61,10 @@ enum virtio_balloon_vq {
VIRTIO_BALLOON_VQ_MAX
};
+enum virtio_balloon_config_read {
+ VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
+};
+
struct virtio_balloon {
struct virtio_device *vdev;
struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
@@ -77,6 +81,8 @@ struct virtio_balloon {
/* Prevent updating balloon when it is being canceled. */
spinlock_t stop_update_lock;
bool stop_update;
+ /* Bitmap to indicate if reading the related config fields are needed */
+ unsigned long config_read_bitmap;
/* The list of allocated free pages, waiting to be given back to mm */
struct list_head free_page_list;
@@ -390,37 +396,31 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
return num_returned;
}
+static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
+{
+ if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
+ return;
+
+ /* No need to queue the work if the bit was already set. */
+ if (test_and_set_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
+ &vb->config_read_bitmap))
+ return;
+
+ queue_work(vb->balloon_wq, &vb->report_free_page_work);
+}
+
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);
+ virtio_balloon_queue_free_page_work(vb);
}
+ spin_unlock_irqrestore(&vb->stop_update_lock, flags);
}
static void update_balloon_size(struct virtio_balloon *vb)
@@ -609,6 +609,16 @@ static int get_free_page_and_send(struct virtio_balloon *vb)
return 0;
}
+static void virtio_balloon_read_cmd_id_received(struct virtio_balloon *vb)
+{
+ if (!test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
+ &vb->config_read_bitmap))
+ return;
+
+ virtio_cread(vb->vdev, struct virtio_balloon_config,
+ free_page_report_cmd_id, &vb->cmd_id_received);
+}
+
static int send_free_pages(struct virtio_balloon *vb)
{
int err;
@@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
* stop the reporting.
*/
cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
+ virtio_balloon_read_cmd_id_received(vb);
if (cmd_id_active != vb->cmd_id_received)
break;
@@ -637,11 +648,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 +668,22 @@ 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_balloon_read_cmd_id_received(vb);
+ 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] 23+ messages in thread* Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
2019-01-04 7:11 ` Wei Wang
(?)
@ 2019-01-04 11:02 ` Cornelia Huck
-1 siblings, 0 replies; 23+ messages in thread
From: Cornelia Huck @ 2019-01-04 11:02 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, kvm, mst, linux-kernel, virtualization, pasic,
pbonzini, dgilbert
On Fri, 4 Jan 2019 15:11:52 +0800
Wei Wang <wei.w.wang@intel.com> wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> drivers/virtio/virtio_balloon.c | 81 +++++++++++++++++++++++++++--------------
> 1 file changed, 53 insertions(+), 28 deletions(-)
>
(...)
> @@ -77,6 +81,8 @@ struct virtio_balloon {
> /* Prevent updating balloon when it is being canceled. */
> spinlock_t stop_update_lock;
> bool stop_update;
> + /* Bitmap to indicate if reading the related config fields are needed */
s/are/is/
> + unsigned long config_read_bitmap;
>
> /* The list of allocated free pages, waiting to be given back to mm */
> struct list_head free_page_list;
(...)
Bitmap handling looks sane to me.
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 23+ messages in thread* [virtio-dev] Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
2019-01-04 7:11 ` Wei Wang
@ 2019-01-04 11:02 ` Cornelia Huck
-1 siblings, 0 replies; 23+ messages in thread
From: Cornelia Huck @ 2019-01-04 11:02 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, mst, pasic,
borntraeger, pbonzini, dgilbert
On Fri, 4 Jan 2019 15:11:52 +0800
Wei Wang <wei.w.wang@intel.com> wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> drivers/virtio/virtio_balloon.c | 81 +++++++++++++++++++++++++++--------------
> 1 file changed, 53 insertions(+), 28 deletions(-)
>
(...)
> @@ -77,6 +81,8 @@ struct virtio_balloon {
> /* Prevent updating balloon when it is being canceled. */
> spinlock_t stop_update_lock;
> bool stop_update;
> + /* Bitmap to indicate if reading the related config fields are needed */
s/are/is/
> + unsigned long config_read_bitmap;
>
> /* The list of allocated free pages, waiting to be given back to mm */
> struct list_head free_page_list;
(...)
Bitmap handling looks sane to me.
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] 23+ messages in thread* Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-04 11:02 ` Cornelia Huck
0 siblings, 0 replies; 23+ messages in thread
From: Cornelia Huck @ 2019-01-04 11:02 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, mst, pasic,
borntraeger, pbonzini, dgilbert
On Fri, 4 Jan 2019 15:11:52 +0800
Wei Wang <wei.w.wang@intel.com> wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> drivers/virtio/virtio_balloon.c | 81 +++++++++++++++++++++++++++--------------
> 1 file changed, 53 insertions(+), 28 deletions(-)
>
(...)
> @@ -77,6 +81,8 @@ struct virtio_balloon {
> /* Prevent updating balloon when it is being canceled. */
> spinlock_t stop_update_lock;
> bool stop_update;
> + /* Bitmap to indicate if reading the related config fields are needed */
s/are/is/
> + unsigned long config_read_bitmap;
>
> /* The list of allocated free pages, waiting to be given back to mm */
> struct list_head free_page_list;
(...)
Bitmap handling looks sane to me.
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [virtio-dev] [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
2019-01-04 7:11 ` Wei Wang
(?)
@ 2019-01-04 11:26 ` Halil Pasic
-1 siblings, 0 replies; 23+ messages in thread
From: Halil Pasic @ 2019-01-04 11:26 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, mst, cohuck,
borntraeger, pbonzini, dgilbert
On Fri, 4 Jan 2019 15:11:52 +0800
Wei Wang <wei.w.wang@intel.com> wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 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] 23+ messages in thread
* Re: [virtio-dev] [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-04 11:26 ` Halil Pasic
0 siblings, 0 replies; 23+ messages in thread
From: Halil Pasic @ 2019-01-04 11:26 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, mst, cohuck,
borntraeger, pbonzini, dgilbert
On Fri, 4 Jan 2019 15:11:52 +0800
Wei Wang <wei.w.wang@intel.com> wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-04 11:26 ` Halil Pasic
0 siblings, 0 replies; 23+ messages in thread
From: Halil Pasic @ 2019-01-04 11:26 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, mst, cohuck,
borntraeger, pbonzini, dgilbert
On Fri, 4 Jan 2019 15:11:52 +0800
Wei Wang <wei.w.wang@intel.com> wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [virtio-dev] Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
2019-01-04 7:11 ` Wei Wang
@ 2019-01-04 15:44 ` Michael S. Tsirkin
-1 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2019-01-04 15:44 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, cohuck, pasic,
borntraeger, pbonzini, dgilbert
On Fri, Jan 04, 2019 at 03:11:52PM +0800, Wei Wang wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> drivers/virtio/virtio_balloon.c | 81 +++++++++++++++++++++++++++--------------
> 1 file changed, 53 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 728ecd1..35ee762 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -61,6 +61,10 @@ enum virtio_balloon_vq {
> VIRTIO_BALLOON_VQ_MAX
> };
>
> +enum virtio_balloon_config_read {
> + VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
> +};
> +
> struct virtio_balloon {
> struct virtio_device *vdev;
> struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> @@ -77,6 +81,8 @@ struct virtio_balloon {
> /* Prevent updating balloon when it is being canceled. */
> spinlock_t stop_update_lock;
> bool stop_update;
> + /* Bitmap to indicate if reading the related config fields are needed */
> + unsigned long config_read_bitmap;
>
> /* The list of allocated free pages, waiting to be given back to mm */
> struct list_head free_page_list;
It seems that you never initialize this bitmap. Probably harmless here
but generally using uninitialized memory isn't good.
> @@ -390,37 +396,31 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
> return num_returned;
> }
>
> +static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
> +{
> + if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> + return;
> +
> + /* No need to queue the work if the bit was already set. */
> + if (test_and_set_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> + &vb->config_read_bitmap))
> + return;
> +
> + queue_work(vb->balloon_wq, &vb->report_free_page_work);
> +}
> +
> 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);
> + virtio_balloon_queue_free_page_work(vb);
> }
> + spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> }
>
> static void update_balloon_size(struct virtio_balloon *vb)
> @@ -609,6 +609,16 @@ static int get_free_page_and_send(struct virtio_balloon *vb)
> return 0;
> }
>
> +static void virtio_balloon_read_cmd_id_received(struct virtio_balloon *vb)
> +{
> + if (!test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> + &vb->config_read_bitmap))
> + return;
> +
> + virtio_cread(vb->vdev, struct virtio_balloon_config,
> + free_page_report_cmd_id, &vb->cmd_id_received);
> +}
> +
> static int send_free_pages(struct virtio_balloon *vb)
> {
> int err;
> @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
> * stop the reporting.
> */
> cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
> + virtio_balloon_read_cmd_id_received(vb);
> if (cmd_id_active != vb->cmd_id_received)
> break;
>
> @@ -637,11 +648,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 +668,22 @@ 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_balloon_read_cmd_id_received(vb);
This will not achieve what you are trying to do,
which is cancel reporting if it's in progress.
You need to re-read each time you compare to cmd_id_active.
An API similar to
u32 virtio_balloon_cmd_id_received(vb)
seems to be called for, and I would rename cmd_id_received to
cmd_id_received_cache to make sure we caught all users.
> + 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] 23+ messages in thread* Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-04 15:44 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2019-01-04 15:44 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, cohuck, pasic,
borntraeger, pbonzini, dgilbert
On Fri, Jan 04, 2019 at 03:11:52PM +0800, Wei Wang wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> drivers/virtio/virtio_balloon.c | 81 +++++++++++++++++++++++++++--------------
> 1 file changed, 53 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 728ecd1..35ee762 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -61,6 +61,10 @@ enum virtio_balloon_vq {
> VIRTIO_BALLOON_VQ_MAX
> };
>
> +enum virtio_balloon_config_read {
> + VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
> +};
> +
> struct virtio_balloon {
> struct virtio_device *vdev;
> struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> @@ -77,6 +81,8 @@ struct virtio_balloon {
> /* Prevent updating balloon when it is being canceled. */
> spinlock_t stop_update_lock;
> bool stop_update;
> + /* Bitmap to indicate if reading the related config fields are needed */
> + unsigned long config_read_bitmap;
>
> /* The list of allocated free pages, waiting to be given back to mm */
> struct list_head free_page_list;
It seems that you never initialize this bitmap. Probably harmless here
but generally using uninitialized memory isn't good.
> @@ -390,37 +396,31 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
> return num_returned;
> }
>
> +static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
> +{
> + if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> + return;
> +
> + /* No need to queue the work if the bit was already set. */
> + if (test_and_set_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> + &vb->config_read_bitmap))
> + return;
> +
> + queue_work(vb->balloon_wq, &vb->report_free_page_work);
> +}
> +
> 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);
> + virtio_balloon_queue_free_page_work(vb);
> }
> + spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> }
>
> static void update_balloon_size(struct virtio_balloon *vb)
> @@ -609,6 +609,16 @@ static int get_free_page_and_send(struct virtio_balloon *vb)
> return 0;
> }
>
> +static void virtio_balloon_read_cmd_id_received(struct virtio_balloon *vb)
> +{
> + if (!test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> + &vb->config_read_bitmap))
> + return;
> +
> + virtio_cread(vb->vdev, struct virtio_balloon_config,
> + free_page_report_cmd_id, &vb->cmd_id_received);
> +}
> +
> static int send_free_pages(struct virtio_balloon *vb)
> {
> int err;
> @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
> * stop the reporting.
> */
> cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
> + virtio_balloon_read_cmd_id_received(vb);
> if (cmd_id_active != vb->cmd_id_received)
> break;
>
> @@ -637,11 +648,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 +668,22 @@ 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_balloon_read_cmd_id_received(vb);
This will not achieve what you are trying to do,
which is cancel reporting if it's in progress.
You need to re-read each time you compare to cmd_id_active.
An API similar to
u32 virtio_balloon_cmd_id_received(vb)
seems to be called for, and I would rename cmd_id_received to
cmd_id_received_cache to make sure we caught all users.
> + 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] 23+ messages in thread* RE: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
2019-01-04 15:44 ` Michael S. Tsirkin
(?)
@ 2019-01-05 3:32 ` Wang, Wei W
-1 siblings, 0 replies; 23+ messages in thread
From: Wang, Wei W @ 2019-01-05 3:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev@lists.oasis-open.org, kvm@vger.kernel.org,
cohuck@redhat.com, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, pasic@linux.ibm.com,
pbonzini@redhat.com, dgilbert@redhat.com
On Friday, January 4, 2019 11:45 PM, Michael S. Tsirkin wrote:
> > struct virtio_balloon {
> > struct virtio_device *vdev;
> > struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> > @@ -77,6 +81,8 @@ struct virtio_balloon {
> > /* Prevent updating balloon when it is being canceled. */
> > spinlock_t stop_update_lock;
> > bool stop_update;
> > + /* Bitmap to indicate if reading the related config fields are needed
> */
> > + unsigned long config_read_bitmap;
> >
> > /* The list of allocated free pages, waiting to be given back to mm */
> > struct list_head free_page_list;
>
> It seems that you never initialize this bitmap. Probably harmless here but
> generally using uninitialized memory isn't good.
We've used kzalloc to allocate the vb struct, so it's already 0-initialized :)
> > +
> > static int send_free_pages(struct virtio_balloon *vb) {
> > int err;
> > @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
> > * stop the reporting.
> > */
> > cmd_id_active = virtio32_to_cpu(vb->vdev, vb-
> >cmd_id_active);
> > + virtio_balloon_read_cmd_id_received(vb);
[1]
> > if (cmd_id_active != vb->cmd_id_received)
> > break;
> >
> > @@ -637,11 +648,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 +668,22 @@ 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_balloon_read_cmd_id_received(vb);
>
> This will not achieve what you are trying to do, which is cancel reporting if it's
> in progress.
>
> You need to re-read each time you compare to cmd_id_active.
Yes, already did, please see [1] above
> An API similar to
> u32 virtio_balloon_cmd_id_received(vb)
> seems to be called for, and I would rename cmd_id_received to
> cmd_id_received_cache to make sure we caught all users.
>
I'm not sure about adding "cache" here, cmd_id_received refers to the cmd id
that the driver just received from the device. There is one called "cmd_id_active" which
is the one that the driver is actively using.
So cmd_id_received is already a "cache" to " cmd_id_active " in some sense.
Best,
Wei
^ permalink raw reply [flat|nested] 23+ messages in thread* [virtio-dev] RE: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
2019-01-04 15:44 ` Michael S. Tsirkin
@ 2019-01-05 3:32 ` Wang, Wei W
-1 siblings, 0 replies; 23+ messages in thread
From: Wang, Wei W @ 2019-01-05 3:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev@lists.oasis-open.org, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, kvm@vger.kernel.org,
cohuck@redhat.com, pasic@linux.ibm.com, borntraeger@de.ibm.com,
pbonzini@redhat.com, dgilbert@redhat.com
On Friday, January 4, 2019 11:45 PM, Michael S. Tsirkin wrote:
> > struct virtio_balloon {
> > struct virtio_device *vdev;
> > struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> > @@ -77,6 +81,8 @@ struct virtio_balloon {
> > /* Prevent updating balloon when it is being canceled. */
> > spinlock_t stop_update_lock;
> > bool stop_update;
> > + /* Bitmap to indicate if reading the related config fields are needed
> */
> > + unsigned long config_read_bitmap;
> >
> > /* The list of allocated free pages, waiting to be given back to mm */
> > struct list_head free_page_list;
>
> It seems that you never initialize this bitmap. Probably harmless here but
> generally using uninitialized memory isn't good.
We've used kzalloc to allocate the vb struct, so it's already 0-initialized :)
> > +
> > static int send_free_pages(struct virtio_balloon *vb) {
> > int err;
> > @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
> > * stop the reporting.
> > */
> > cmd_id_active = virtio32_to_cpu(vb->vdev, vb-
> >cmd_id_active);
> > + virtio_balloon_read_cmd_id_received(vb);
[1]
> > if (cmd_id_active != vb->cmd_id_received)
> > break;
> >
> > @@ -637,11 +648,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 +668,22 @@ 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_balloon_read_cmd_id_received(vb);
>
> This will not achieve what you are trying to do, which is cancel reporting if it's
> in progress.
>
> You need to re-read each time you compare to cmd_id_active.
Yes, already did, please see [1] above
> An API similar to
> u32 virtio_balloon_cmd_id_received(vb)
> seems to be called for, and I would rename cmd_id_received to
> cmd_id_received_cache to make sure we caught all users.
>
I'm not sure about adding "cache" here, cmd_id_received refers to the cmd id
that the driver just received from the device. There is one called "cmd_id_active" which
is the one that the driver is actively using.
So cmd_id_received is already a "cache" to " cmd_id_active " in some sense.
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] 23+ messages in thread* RE: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-05 3:32 ` Wang, Wei W
0 siblings, 0 replies; 23+ messages in thread
From: Wang, Wei W @ 2019-01-05 3:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev@lists.oasis-open.org, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, kvm@vger.kernel.org,
cohuck@redhat.com, pasic@linux.ibm.com, borntraeger@de.ibm.com,
pbonzini@redhat.com, dgilbert@redhat.com
On Friday, January 4, 2019 11:45 PM, Michael S. Tsirkin wrote:
> > struct virtio_balloon {
> > struct virtio_device *vdev;
> > struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> > @@ -77,6 +81,8 @@ struct virtio_balloon {
> > /* Prevent updating balloon when it is being canceled. */
> > spinlock_t stop_update_lock;
> > bool stop_update;
> > + /* Bitmap to indicate if reading the related config fields are needed
> */
> > + unsigned long config_read_bitmap;
> >
> > /* The list of allocated free pages, waiting to be given back to mm */
> > struct list_head free_page_list;
>
> It seems that you never initialize this bitmap. Probably harmless here but
> generally using uninitialized memory isn't good.
We've used kzalloc to allocate the vb struct, so it's already 0-initialized :)
> > +
> > static int send_free_pages(struct virtio_balloon *vb) {
> > int err;
> > @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
> > * stop the reporting.
> > */
> > cmd_id_active = virtio32_to_cpu(vb->vdev, vb-
> >cmd_id_active);
> > + virtio_balloon_read_cmd_id_received(vb);
[1]
> > if (cmd_id_active != vb->cmd_id_received)
> > break;
> >
> > @@ -637,11 +648,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 +668,22 @@ 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_balloon_read_cmd_id_received(vb);
>
> This will not achieve what you are trying to do, which is cancel reporting if it's
> in progress.
>
> You need to re-read each time you compare to cmd_id_active.
Yes, already did, please see [1] above
> An API similar to
> u32 virtio_balloon_cmd_id_received(vb)
> seems to be called for, and I would rename cmd_id_received to
> cmd_id_received_cache to make sure we caught all users.
>
I'm not sure about adding "cache" here, cmd_id_received refers to the cmd id
that the driver just received from the device. There is one called "cmd_id_active" which
is the one that the driver is actively using.
So cmd_id_received is already a "cache" to " cmd_id_active " in some sense.
Best,
Wei
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
2019-01-05 3:32 ` Wang, Wei W
(?)
@ 2019-01-06 0:31 ` Michael S. Tsirkin
-1 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2019-01-06 0:31 UTC (permalink / raw)
To: Wang, Wei W
Cc: virtio-dev@lists.oasis-open.org, kvm@vger.kernel.org,
cohuck@redhat.com, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, pasic@linux.ibm.com,
pbonzini@redhat.com, dgilbert@redhat.com
On Sat, Jan 05, 2019 at 03:32:44AM +0000, Wang, Wei W wrote:
> On Friday, January 4, 2019 11:45 PM, Michael S. Tsirkin wrote:
> > > struct virtio_balloon {
> > > struct virtio_device *vdev;
> > > struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> > > @@ -77,6 +81,8 @@ struct virtio_balloon {
> > > /* Prevent updating balloon when it is being canceled. */
> > > spinlock_t stop_update_lock;
> > > bool stop_update;
> > > + /* Bitmap to indicate if reading the related config fields are needed
> > */
> > > + unsigned long config_read_bitmap;
> > >
> > > /* The list of allocated free pages, waiting to be given back to mm */
> > > struct list_head free_page_list;
> >
> > It seems that you never initialize this bitmap. Probably harmless here but
> > generally using uninitialized memory isn't good.
>
> We've used kzalloc to allocate the vb struct, so it's already 0-initialized :)
Ah ok. We do explicitly init other fields like stop_update so
it seems cleaner to init this one too though.
> > > +
> > > static int send_free_pages(struct virtio_balloon *vb) {
> > > int err;
> > > @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
> > > * stop the reporting.
> > > */
> > > cmd_id_active = virtio32_to_cpu(vb->vdev, vb-
> > >cmd_id_active);
> > > + virtio_balloon_read_cmd_id_received(vb);
>
>
> [1]
>
>
> > > if (cmd_id_active != vb->cmd_id_received)
> > > break;
> > >
> > > @@ -637,11 +648,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 +668,22 @@ 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_balloon_read_cmd_id_received(vb);
> >
> > This will not achieve what you are trying to do, which is cancel reporting if it's
> > in progress.
> >
> > You need to re-read each time you compare to cmd_id_active.
>
> Yes, already did, please see [1] above
Oh right. Sorry.
>
> > An API similar to
> > u32 virtio_balloon_cmd_id_received(vb)
> > seems to be called for, and I would rename cmd_id_received to
> > cmd_id_received_cache to make sure we caught all users.
> >
>
> I'm not sure about adding "cache" here, cmd_id_received refers to the cmd id
> that the driver just received from the device. There is one called "cmd_id_active" which
> is the one that the driver is actively using.
> So cmd_id_received is already a "cache" to " cmd_id_active " in some sense.
>
> Best,
> Wei
The point is that any access to cmd_id_received should first call
virtio_balloon_read_cmd_id_received. So a better API is
to have virtio_balloon_read_cmd_id_received return the value
instead of poking at cmd_id_received afterwards.
Renaming cmd_id_received will help make sure all no
call sites were missed and help stress it's never used directly.
^ permalink raw reply [flat|nested] 23+ messages in thread* [virtio-dev] Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
2019-01-05 3:32 ` Wang, Wei W
@ 2019-01-06 0:31 ` Michael S. Tsirkin
-1 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2019-01-06 0:31 UTC (permalink / raw)
To: Wang, Wei W
Cc: virtio-dev@lists.oasis-open.org, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, kvm@vger.kernel.org,
cohuck@redhat.com, pasic@linux.ibm.com, borntraeger@de.ibm.com,
pbonzini@redhat.com, dgilbert@redhat.com
On Sat, Jan 05, 2019 at 03:32:44AM +0000, Wang, Wei W wrote:
> On Friday, January 4, 2019 11:45 PM, Michael S. Tsirkin wrote:
> > > struct virtio_balloon {
> > > struct virtio_device *vdev;
> > > struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> > > @@ -77,6 +81,8 @@ struct virtio_balloon {
> > > /* Prevent updating balloon when it is being canceled. */
> > > spinlock_t stop_update_lock;
> > > bool stop_update;
> > > + /* Bitmap to indicate if reading the related config fields are needed
> > */
> > > + unsigned long config_read_bitmap;
> > >
> > > /* The list of allocated free pages, waiting to be given back to mm */
> > > struct list_head free_page_list;
> >
> > It seems that you never initialize this bitmap. Probably harmless here but
> > generally using uninitialized memory isn't good.
>
> We've used kzalloc to allocate the vb struct, so it's already 0-initialized :)
Ah ok. We do explicitly init other fields like stop_update so
it seems cleaner to init this one too though.
> > > +
> > > static int send_free_pages(struct virtio_balloon *vb) {
> > > int err;
> > > @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
> > > * stop the reporting.
> > > */
> > > cmd_id_active = virtio32_to_cpu(vb->vdev, vb-
> > >cmd_id_active);
> > > + virtio_balloon_read_cmd_id_received(vb);
>
>
> [1]
>
>
> > > if (cmd_id_active != vb->cmd_id_received)
> > > break;
> > >
> > > @@ -637,11 +648,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 +668,22 @@ 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_balloon_read_cmd_id_received(vb);
> >
> > This will not achieve what you are trying to do, which is cancel reporting if it's
> > in progress.
> >
> > You need to re-read each time you compare to cmd_id_active.
>
> Yes, already did, please see [1] above
Oh right. Sorry.
>
> > An API similar to
> > u32 virtio_balloon_cmd_id_received(vb)
> > seems to be called for, and I would rename cmd_id_received to
> > cmd_id_received_cache to make sure we caught all users.
> >
>
> I'm not sure about adding "cache" here, cmd_id_received refers to the cmd id
> that the driver just received from the device. There is one called "cmd_id_active" which
> is the one that the driver is actively using.
> So cmd_id_received is already a "cache" to " cmd_id_active " in some sense.
>
> Best,
> Wei
The point is that any access to cmd_id_received should first call
virtio_balloon_read_cmd_id_received. So a better API is
to have virtio_balloon_read_cmd_id_received return the value
instead of poking at cmd_id_received afterwards.
Renaming cmd_id_received will help make sure all no
call sites were missed and help stress it's never used directly.
---------------------------------------------------------------------
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] 23+ messages in thread* Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
@ 2019-01-06 0:31 ` Michael S. Tsirkin
0 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2019-01-06 0:31 UTC (permalink / raw)
To: Wang, Wei W
Cc: virtio-dev@lists.oasis-open.org, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, kvm@vger.kernel.org,
cohuck@redhat.com, pasic@linux.ibm.com, borntraeger@de.ibm.com,
pbonzini@redhat.com, dgilbert@redhat.com
On Sat, Jan 05, 2019 at 03:32:44AM +0000, Wang, Wei W wrote:
> On Friday, January 4, 2019 11:45 PM, Michael S. Tsirkin wrote:
> > > struct virtio_balloon {
> > > struct virtio_device *vdev;
> > > struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> > > @@ -77,6 +81,8 @@ struct virtio_balloon {
> > > /* Prevent updating balloon when it is being canceled. */
> > > spinlock_t stop_update_lock;
> > > bool stop_update;
> > > + /* Bitmap to indicate if reading the related config fields are needed
> > */
> > > + unsigned long config_read_bitmap;
> > >
> > > /* The list of allocated free pages, waiting to be given back to mm */
> > > struct list_head free_page_list;
> >
> > It seems that you never initialize this bitmap. Probably harmless here but
> > generally using uninitialized memory isn't good.
>
> We've used kzalloc to allocate the vb struct, so it's already 0-initialized :)
Ah ok. We do explicitly init other fields like stop_update so
it seems cleaner to init this one too though.
> > > +
> > > static int send_free_pages(struct virtio_balloon *vb) {
> > > int err;
> > > @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
> > > * stop the reporting.
> > > */
> > > cmd_id_active = virtio32_to_cpu(vb->vdev, vb-
> > >cmd_id_active);
> > > + virtio_balloon_read_cmd_id_received(vb);
>
>
> [1]
>
>
> > > if (cmd_id_active != vb->cmd_id_received)
> > > break;
> > >
> > > @@ -637,11 +648,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 +668,22 @@ 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_balloon_read_cmd_id_received(vb);
> >
> > This will not achieve what you are trying to do, which is cancel reporting if it's
> > in progress.
> >
> > You need to re-read each time you compare to cmd_id_active.
>
> Yes, already did, please see [1] above
Oh right. Sorry.
>
> > An API similar to
> > u32 virtio_balloon_cmd_id_received(vb)
> > seems to be called for, and I would rename cmd_id_received to
> > cmd_id_received_cache to make sure we caught all users.
> >
>
> I'm not sure about adding "cache" here, cmd_id_received refers to the cmd id
> that the driver just received from the device. There is one called "cmd_id_active" which
> is the one that the driver is actively using.
> So cmd_id_received is already a "cache" to " cmd_id_active " in some sense.
>
> Best,
> Wei
The point is that any access to cmd_id_received should first call
virtio_balloon_read_cmd_id_received. So a better API is
to have virtio_balloon_read_cmd_id_received return the value
instead of poking at cmd_id_received afterwards.
Renaming cmd_id_received will help make sure all no
call sites were missed and help stress it's never used directly.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] virtio-balloon: tweak config_changed implementation
2019-01-04 7:11 ` Wei Wang
` (4 preceding siblings ...)
(?)
@ 2019-01-04 15:44 ` Michael S. Tsirkin
-1 siblings, 0 replies; 23+ messages in thread
From: Michael S. Tsirkin @ 2019-01-04 15:44 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, kvm, cohuck, linux-kernel, virtualization, pasic,
pbonzini, dgilbert
On Fri, Jan 04, 2019 at 03:11:52PM +0800, Wei Wang wrote:
> virtio-ccw has deadlock issues with reading the config space inside the
> interrupt context, so we tweak the virtballoon_changed implementation
> by moving the config read operations into the related workqueue contexts.
> The config_read_bitmap is used as a flag to the workqueue callbacks
> about the related config fields that need to be read.
>
> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> drivers/virtio/virtio_balloon.c | 81 +++++++++++++++++++++++++++--------------
> 1 file changed, 53 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 728ecd1..35ee762 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -61,6 +61,10 @@ enum virtio_balloon_vq {
> VIRTIO_BALLOON_VQ_MAX
> };
>
> +enum virtio_balloon_config_read {
> + VIRTIO_BALLOON_CONFIG_READ_CMD_ID = 0,
> +};
> +
> struct virtio_balloon {
> struct virtio_device *vdev;
> struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
> @@ -77,6 +81,8 @@ struct virtio_balloon {
> /* Prevent updating balloon when it is being canceled. */
> spinlock_t stop_update_lock;
> bool stop_update;
> + /* Bitmap to indicate if reading the related config fields are needed */
> + unsigned long config_read_bitmap;
>
> /* The list of allocated free pages, waiting to be given back to mm */
> struct list_head free_page_list;
It seems that you never initialize this bitmap. Probably harmless here
but generally using uninitialized memory isn't good.
> @@ -390,37 +396,31 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
> return num_returned;
> }
>
> +static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
> +{
> + if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> + return;
> +
> + /* No need to queue the work if the bit was already set. */
> + if (test_and_set_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> + &vb->config_read_bitmap))
> + return;
> +
> + queue_work(vb->balloon_wq, &vb->report_free_page_work);
> +}
> +
> 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);
> + virtio_balloon_queue_free_page_work(vb);
> }
> + spin_unlock_irqrestore(&vb->stop_update_lock, flags);
> }
>
> static void update_balloon_size(struct virtio_balloon *vb)
> @@ -609,6 +609,16 @@ static int get_free_page_and_send(struct virtio_balloon *vb)
> return 0;
> }
>
> +static void virtio_balloon_read_cmd_id_received(struct virtio_balloon *vb)
> +{
> + if (!test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> + &vb->config_read_bitmap))
> + return;
> +
> + virtio_cread(vb->vdev, struct virtio_balloon_config,
> + free_page_report_cmd_id, &vb->cmd_id_received);
> +}
> +
> static int send_free_pages(struct virtio_balloon *vb)
> {
> int err;
> @@ -620,6 +630,7 @@ static int send_free_pages(struct virtio_balloon *vb)
> * stop the reporting.
> */
> cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
> + virtio_balloon_read_cmd_id_received(vb);
> if (cmd_id_active != vb->cmd_id_received)
> break;
>
> @@ -637,11 +648,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 +668,22 @@ 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_balloon_read_cmd_id_received(vb);
This will not achieve what you are trying to do,
which is cancel reporting if it's in progress.
You need to re-read each time you compare to cmd_id_active.
An API similar to
u32 virtio_balloon_cmd_id_received(vb)
seems to be called for, and I would rename cmd_id_received to
cmd_id_received_cache to make sure we caught all users.
> + 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] 23+ messages in thread