Petr Mladek <pmladek@xxxxxxx> writes:
> Workqueues have clean and rich API for all basic operations. The code is usually
> easier and better readable. It can be easily tuned for the given purpose.
OK, sure.
> -static void fill_balloon(struct virtio_balloon *vb, size_t num)
> +static void fill_balloon(struct virtio_balloon *vb, size_t diff)
> {
> struct balloon_dev_info *vb_dev_info = vb->vb_dev_info;
> + size_t num;
> + bool done;
>
> /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + num = min(diff, ARRAY_SIZE(vb->pfns));
> + done = (num == diff) ? true : false;
>
> mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> @@ -143,6 +143,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> VIRTIO_BALLOON_PAGES_PER_PAGE);
> /* Sleep for at least 1/5 of a second before retry. */
> msleep(200);
> + done = false;
> break;
> }
> set_page_pfns(vb->pfns + vb->num_pfns, page);
> @@ -154,6 +155,9 @@ static void fill_balloon(struct virtio_balloon *vb, size_t num)
> if (vb->num_pfns != 0)
> tell_host(vb, vb->inflate_vq);
> mutex_unlock(&vb->balloon_lock);
> +
> + if (!done)
> + queue_work(vb->wq, &vb->wq_work);
> }
Hmm, this is a bit convoluted. I would spell it out by keeping a
num_done counter:
if (num_done < diff)
queue_work(vb->wq, &vb->wq_work);
> static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
> @@ -168,20 +172,25 @@ static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
> }
> }
>
> -static void leak_balloon(struct virtio_balloon *vb, size_t num)
> +static void leak_balloon(struct virtio_balloon *vb, size_t diff)
> {
> struct page *page;
> struct balloon_dev_info *vb_dev_info = vb->vb_dev_info;
> + size_t num;
> + bool done;
>
> /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + num = min(diff, ARRAY_SIZE(vb->pfns));
> + done = (num == diff) ? true : false;
>
> mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
> page = balloon_page_dequeue(vb_dev_info);
> - if (!page)
> + if (!page) {
> + done = false;
> break;
> + }
> set_page_pfns(vb->pfns + vb->num_pfns, page);
> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> }
> @@ -195,6 +204,9 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
> tell_host(vb, vb->deflate_vq);
> mutex_unlock(&vb->balloon_lock);
> release_pages_by_pfn(vb->pfns, vb->num_pfns);
> +
> + if (!done)
> + queue_work(vb->wq, &vb->wq_work);
> }
Here too.
> @@ -471,11 +469,13 @@ static int virtballoon_probe(struct virtio_device *vdev)
> if (err)
> goto out_free_vb_mapping;
>
> - vb->thread = kthread_run(balloon, vb, "vballoon");
> - if (IS_ERR(vb->thread)) {
> - err = PTR_ERR(vb->thread);
> + vb->wq = alloc_workqueue("vballoon_wq",
> + WQ_FREEZABLE | WQ_MEM_RECLAIM, 0);
> + if (!vb->wq) {
> + err = -ENOMEM;
> goto out_del_vqs;
> }
> + INIT_WORK(&vb->wq_work, balloon);
That looks racy to me; shouldn't we init vq-work before allocating wq?
Otherwise, looks good!
Thanks,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
