On Mon, Jan 07, 2019 at 03:01:04PM +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.
>
> The cmd_id_received is also renamed to cmd_id_received_cache, and
> the value should be obtained via virtio_balloon_cmd_id_received.
>
> Reported-by: Christian Borntraeger <borntrae...@de.ibm.com>
> Signed-off-by: Wei Wang <wei.w.w...@intel.com>
> Reviewed-by: Cornelia Huck <coh...@redhat.com>
> Reviewed-by: Halil Pasic <pa...@linux.ibm.com>
> ---
> drivers/virtio/virtio_balloon.c | 98
> +++++++++++++++++++++++++++--------------
> 1 file changed, 65 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 728ecd1..fb12fe2 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,14 +81,20 @@ 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;
> spinlock_t free_page_list_lock;
> /* The number of free page blocks on the above list */
> unsigned long num_free_page_blocks;
> - /* The cmd id received from host */
> - u32 cmd_id_received;
> + /*
> + * The cmd id received from host.
> + * Read it via virtio_balloon_cmd_id_received to get the latest value
> + * sent from host.
> + */
> + u32 cmd_id_received_cache;
> /* The cmd id that is actively in use */
> __virtio32 cmd_id_active;
> /* Buffer to store the stop sign */
> @@ -390,37 +400,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)
> @@ -527,6 +531,17 @@ static int init_vqs(struct virtio_balloon *vb)
> return 0;
> }
>
> +static u32 virtio_balloon_cmd_id_received(struct virtio_balloon *vb)
> +{
> + if (test_and_clear_bit(VIRTIO_BALLOON_CONFIG_READ_CMD_ID,
> + &vb->config_read_bitmap))
> + virtio_cread(vb->vdev, struct virtio_balloon_config,
> + free_page_report_cmd_id,
> + &vb->cmd_id_received_cache);
> +
> + return vb->cmd_id_received_cache;
> +}
> +
> static int send_cmd_id_start(struct virtio_balloon *vb)
> {
> struct scatterlist sg;
> @@ -537,7 +552,8 @@ static int send_cmd_id_start(struct virtio_balloon *vb)
> while (virtqueue_get_buf(vq, &unused))
> ;
>
> - vb->cmd_id_active = cpu_to_virtio32(vb->vdev, vb->cmd_id_received);
> + vb->cmd_id_active = virtio32_to_cpu(vb->vdev,
> + virtio_balloon_cmd_id_received(vb));
You switches cpu_to_virtio32 to over to virtio32_to_cpu.
That will produce a sparse warning I think.
Pls always run the static checker when you submit patches.
> sg_init_one(&sg, &vb->cmd_id_active, sizeof(vb->cmd_id_active));
> err = virtqueue_add_outbuf(vq, &sg, 1, &vb->cmd_id_active, GFP_KERNEL);
> if (!err)
> @@ -620,7 +636,8 @@ static int send_free_pages(struct virtio_balloon *vb)
> * stop the reporting.
> */
> cmd_id_active = virtio32_to_cpu(vb->vdev, vb->cmd_id_active);
> - if (cmd_id_active != vb->cmd_id_received)
> + if (unlikely(cmd_id_active !=
> + virtio_balloon_cmd_id_received(vb)))
> break;
>
> /*
> @@ -637,11 +654,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 +674,23 @@ 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);
> + u32 cmd_id_received;
> +
> + cmd_id_received = virtio_balloon_cmd_id_received(vb);
> + if (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 (cmd_id_received != VIRTIO_BALLOON_CMD_ID_STOP &&
> + 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
> @@ -885,7 +917,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
> goto out_del_vqs;
> }
> INIT_WORK(&vb->report_free_page_work, report_free_page_func);
> - vb->cmd_id_received = VIRTIO_BALLOON_CMD_ID_STOP;
> + vb->cmd_id_received_cache = VIRTIO_BALLOON_CMD_ID_STOP;
> vb->cmd_id_active = cpu_to_virtio32(vb->vdev,
> VIRTIO_BALLOON_CMD_ID_STOP);
> vb->cmd_id_stop = cpu_to_virtio32(vb->vdev,
> --
> 2.7.4
