Re: [virtio-dev] Re: [PATCH v25 2/2] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT

[Wei Wang]

On 01/25/2018 09:49 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 25, 2018 at 05:14:06PM +0800, Wei Wang wrote:
> > +
> > +static void report_free_page_func(struct work_struct *work)
> > +{
> > +	struct virtio_balloon *vb;
> > +	int ret;
> > +
> > +	vb = container_of(work, struct virtio_balloon, report_free_page_work);
> > +
> > +	/* Start by sending the received cmd id to host with an outbuf */
> > +	ret = send_cmd_id(vb, vb->cmd_id_received);
> > +	if (unlikely(ret))
> > +		goto err;
> > +
> > +	ret = walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
> > +	if (unlikely(ret < 0))
> > +		goto err;
> > +
> > +	/* End by sending a stop id to host with an outbuf */
> > +	ret = send_cmd_id(vb, VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID);
> > +	if (likely(!ret))
> > +		return;
> > +err:
> > +	dev_err(&vb->vdev->dev, "%s failure: free page vq is broken\n",
> > +		__func__);
> > +}
> > +
> So that's very simple, but it only works well if the whole
> free list fits in the queue or host processes the queue faster
> than the guest. What if it doesn't?

This is the case that the virtqueue gets full, and I think we've agreed 
that this is an optimization feature and losing some hints to report 
isn't important, right?

Actually, in the tests, there is no chance to see the ring is full. If 
we check the host patches that were shared before, the device side 
operation is quite simple, it just clears the related bits from the 
bitmap, and then continues to take entries from the virtqueue till the 
virtqueue gets empty.


> If we had restartability you could just drop the lock
> and wait for a vq interrupt to make more progress, which
> would be better I think.

Restartability means that caller needs to record the state where it was 
when it stopped last time. The controversy is that the free list is not 
static once the lock is dropped, so everything is dynamically changing, 
including the state that was recorded. The method we are using is more 
prudent, IMHO. How about taking the fundamental solution, and seek to 
improve incrementally in the future?


Best,
Wei