RE: [PATCH 2/6] virtio-balloon: speed up inflate/deflate process

[Li, Liang Z]

Hi Michael,

Thanks for your comments!

> 
> 2<< 30  is 2G but that is not a useful comment.
> pls explain what is the reason for this selection.
> 

Will change in the next version.

> > +struct balloon_bmap_hdr {
> > +	__virtio32 id;
> > +	__virtio32 page_shift;
> > +	__virtio64 start_pfn;
> > +	__virtio64 bmap_len;
> > +};
> > +
> 
> Put this in an uapi header please.

Will change in the next version.

> > +static inline void init_pfn_range(struct virtio_balloon *vb) {
> > +	vb->min_pfn = (1UL << 48);
> 
> Where does this value come from? Do you want ULONG_MAX?
> This does not fit in long on 32 bit systems.

I just want to make it big enough, ULONG_MAX is better. Will change it.

> >  static void tell_host(struct virtio_balloon *vb, struct virtqueue
> > *vq)  {
> > -	struct scatterlist sg;
> >  	unsigned int len;
> >
> > -	sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> > +	if (virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_PAGE_BITMAP)) {
> > +		struct balloon_bmap_hdr hdr;
> 
> why not init fields here?
> 
> > +		unsigned long bmap_len;
> 
> and here

All the fields and the bmap_len will be updated later, so init is unnecessary?
 
> > +		struct scatterlist sg[2];
> > +
> > +		hdr.id = cpu_to_virtio32(vb->vdev, 0);
> > +		hdr.page_shift = cpu_to_virtio32(vb->vdev, PAGE_SHIFT);
> > +		hdr.start_pfn = cpu_to_virtio64(vb->vdev, vb->start_pfn);
> > +		bmap_len = min(vb->bmap_len,
> > +				(vb->end_pfn - vb->start_pfn) /
> BITS_PER_BYTE);
> > +		hdr.bmap_len = cpu_to_virtio64(vb->vdev, bmap_len);
> > +		sg_init_table(sg, 2);
> > +		sg_set_buf(&sg[0], &hdr, sizeof(hdr));
> > +		sg_set_buf(&sg[1], vb->page_bitmap, bmap_len);
> > +		virtqueue_add_outbuf(vq, sg, 2, vb, GFP_KERNEL);
> 
> might fail if queue size < 2. validate queue size and clear
> VIRTIO_BALLOON_F_PAGE_BITMAP?
> 
not considered yet.

> Alternatively, and I think preferably,
> use first struct balloon_bmap_hdr bytes in the buffer to pass the header to
> host.

How about the bitmap, in another sending?

> > +	struct page *page, *next;
> > +	bool find;
> 
> find -> found

Will change.

> > +	vb->max_pfn = roundup(vb->max_pfn, BITS_PER_LONG);
> > +	for (pfn = vb->min_pfn; pfn < vb->max_pfn;
> > +			pfn += VIRTIO_BALLOON_PFNS_LIMIT) {
> > +		vb->start_pfn = pfn;
> > +		vb->end_pfn = pfn;
> > +		memset(vb->page_bitmap, 0, vb->bmap_len);
> > +		find = false;
> > +		list_for_each_entry_safe(page, next, pages, lru) {
> 
> Why _safe?

No safe is OK. Will change.

> > +			unsigned long balloon_pfn =
> page_to_balloon_pfn(page);
> > +
> > +			if (balloon_pfn < pfn ||
> > +				 balloon_pfn >= pfn +
> VIRTIO_BALLOON_PFNS_LIMIT)
> > +				continue;
> > +			set_bit(balloon_pfn - pfn, vb->page_bitmap);
> > +			if (balloon_pfn > vb->end_pfn)
> > +				vb->end_pfn = balloon_pfn;
> > +			find = true;
> 
> maybe remove page from list? this way we won't go over same entry
> multiple times ...

No, we can't remove the page from list. The list saves all the pages filled in the balloon,
When delating, we fetch the pages from the list to return them to guest.  
If removed, we can't find them.

> > unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> >
> >  	num_allocated_pages = vb->num_pfns;
> >  	/* Did we get any? */
> > -	if (vb->num_pfns != 0)
> > -		tell_host(vb, vb->inflate_vq);
> > +	if (vb->num_pfns != 0) {
> > +		if (use_bmap)
> > +			set_page_bitmap(vb, &vb_dev_info->pages,
> > +					 vb->inflate_vq);
> 
> don't we need pages_lock if we access vb_dev_info->pages?

It is protected by the vb->balloon_lock. not enough?

> > +	vb->page_bitmap = kzalloc(vb->bmap_len, GFP_KERNEL);
> > +	if (!vb->page_bitmap) {
> > +		err = -ENOMEM;
> > +		goto out;
> > +	}
> 
> How about we clear the bitmap feature on this failure?

That' better.  Will change.

Thanks again!
Liang