> > +static inline void init_pfn_range(struct virtio_balloon *vb) {
> > + vb->min_pfn = ULONG_MAX;
> > + vb->max_pfn = 0;
> > +}
> > +
> > +static inline void update_pfn_range(struct virtio_balloon *vb,
> > + struct page *page)
> > +{
> > + unsigned long balloon_pfn = page_to_balloon_pfn(page);
> > +
> > + if (balloon_pfn < vb->min_pfn)
> > + vb->min_pfn = balloon_pfn;
> > + if (balloon_pfn > vb->max_pfn)
> > + vb->max_pfn = balloon_pfn;
> > +}
> > +
>
> rename to hint these are all bitmap related.
Will change in v4.
>
>
> > static void tell_host(struct virtio_balloon *vb, struct virtqueue
> > *vq) {
> > - struct scatterlist sg;
> > - unsigned int len;
> > + struct scatterlist sg, sg2[BALLOON_BMAP_COUNT + 1];
> > + unsigned int len, i;
> > +
> > + if (virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_PAGE_BITMAP)) {
> > + struct balloon_bmap_hdr *hdr = vb->bmap_hdr;
> > + unsigned long bmap_len;
> > + int nr_pfn, nr_used_bmap, nr_buf;
> > +
> > + nr_pfn = vb->end_pfn - vb->start_pfn + 1;
> > + nr_pfn = roundup(nr_pfn, BITS_PER_LONG);
> > + nr_used_bmap = nr_pfn / PFNS_PER_BMAP;
> > + bmap_len = nr_pfn / BITS_PER_BYTE;
> > + nr_buf = nr_used_bmap + 1;
> > +
> > + /* cmd, reserved and req_id are init to 0, unused here */
> > + hdr->page_shift = cpu_to_virtio16(vb->vdev, PAGE_SHIFT);
> > + hdr->start_pfn = cpu_to_virtio64(vb->vdev, vb->start_pfn);
> > + hdr->bmap_len = cpu_to_virtio64(vb->vdev, bmap_len);
> > + sg_init_table(sg2, nr_buf);
> > + sg_set_buf(&sg2[0], hdr, sizeof(struct balloon_bmap_hdr));
> > + for (i = 0; i < nr_used_bmap; i++) {
> > + unsigned int buf_len = BALLOON_BMAP_SIZE;
> > +
> > + if (i + 1 == nr_used_bmap)
> > + buf_len = bmap_len - BALLOON_BMAP_SIZE
> * i;
> > + sg_set_buf(&sg2[i + 1], vb->page_bitmap[i],
> buf_len);
> > + }
> >
> > - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> > + while (vq->num_free < nr_buf)
> > + msleep(2);
>
>
> What's going on here? Who is expected to update num_free?
>
I just want to wait until the vq have enough space to write the bitmap, I thought qemu
side will update the vq->num_free, is it wrong?
>
>
> > + if (virtqueue_add_outbuf(vq, sg2, nr_buf, vb, GFP_KERNEL)
> == 0)
> > + virtqueue_kick(vq);
> >
> > - /* We should always be able to add one buffer to an empty queue.
> */
> > - virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> > - virtqueue_kick(vq);
> > + } else {
> > + sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb-
> >num_pfns);
> > +
> > + /* We should always be able to add one buffer to an empty
> > + * queue. */
>
> Pls use a multiple comment style consistent with kernel coding style.
Will change in next version.
>
> > + virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> > + virtqueue_kick(vq);
> > + }
> >
> > /* When host has read buffer, this completes via balloon_ack */
> > wait_event(vb->acked, virtqueue_get_buf(vq, &len)); @@ -138,13
> > +199,93 @@ static void set_page_pfns(struct virtio_balloon *vb,
> > page_to_balloon_pfn(page) + i); }
> >
> > -static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> > +static void extend_page_bitmap(struct virtio_balloon *vb) {
> > + int i;
> > + unsigned long bmap_len, bmap_count;
> > +
> > + bmap_len = ALIGN(get_max_pfn(), BITS_PER_LONG) /
> BITS_PER_BYTE;
> > + bmap_count = bmap_len / BALLOON_BMAP_SIZE;
> > + if (bmap_len % BALLOON_BMAP_SIZE)
> > + bmap_count++;
> > + if (bmap_count > BALLOON_BMAP_COUNT)
> > + bmap_count = BALLOON_BMAP_COUNT;
> > +
>
> This is doing simple things in tricky ways.
> Please use macros such as ALIGN and max instead of if.
>
Will change.
>
> > + for (i = 1; i < bmap_count; i++) {
>
> why 1?
In probe stage, already allocated one bitmap.
>
> > + vb->page_bitmap[i] = kmalloc(BALLOON_BMAP_SIZE,
> GFP_ATOMIC);
>
> why GFP_ATOMIC?
Yes, GFP_ATOMIC is not necessary.
> and what will free the previous buffer?
The previous buffer will not be freed.
>
>
> > + if (vb->page_bitmap[i])
> > + vb->nr_page_bmap++;
> > + else
> > + break;
>
> and what will happen then?
I plan to use the previous allocated buffer to save the bitmap, need more code for kmalloc failure?
> > -static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> > +static unsigned int leak_balloon(struct virtio_balloon *vb, size_t num,
> > + bool use_bmap)
>
> this is just a feature bit - why not get it internally?
Indeed.
> > @@ -218,8 +374,14 @@ static unsigned leak_balloon(struct virtio_balloon
> *vb, size_t num)
> > * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> > * is true, we *have* to do it in this order
> > */
> > - if (vb->num_pfns != 0)
> > - tell_host(vb, vb->deflate_vq);
> > + if (vb->num_pfns != 0) {
> > + if (use_bmap)
> > + set_page_bitmap(vb, &pages, vb->deflate_vq);
> > + else
> > + tell_host(vb, vb->deflate_vq);
> > +
> > + release_pages_balloon(vb, &pages);
> > + }
> > release_pages_balloon(vb, &pages);
> > mutex_unlock(&vb->balloon_lock);
> > return num_freed_pages;
> > @@ -354,13 +516,15 @@ static int virtballoon_oom_notify(struct
> notifier_block *self,
> > struct virtio_balloon *vb;
> > unsigned long *freed;
> > unsigned num_freed_pages;
> > + bool use_bmap;
> >
> > vb = container_of(self, struct virtio_balloon, nb);
> > if (!virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > return NOTIFY_OK;
> >
> > freed = parm;
> > - num_freed_pages = leak_balloon(vb, oom_pages);
> > + use_bmap = virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_PAGE_BITMAP);
> > + num_freed_pages = leak_balloon(vb, oom_pages, use_bmap);
> > update_balloon_size(vb);
> > *freed += num_freed_pages;
> >
> > @@ -380,15 +544,19 @@ static void update_balloon_size_func(struct
> > work_struct *work) {
> > struct virtio_balloon *vb;
> > s64 diff;
> > + bool use_bmap;
> >
> > vb = container_of(work, struct virtio_balloon,
> > update_balloon_size_work);
> > diff = towards_target(vb);
> > + use_bmap = virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_PAGE_BITMAP);
> > + if (use_bmap && diff && vb->nr_page_bmap == 1)
> > + extend_page_bitmap(vb);
>
> So you allocate it on first use, then keep it around until device remove?
> Seems ugly.
Yes, this version behave like this.
> Needs comments explaining the motivation for this.
> Can't we free it immediately when it becomes unused?
>
Yes, it can be freed immediately, will change in v4.
Thanks for your time and your valuable comments! I will send out the v4 soon.
Liang
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
