Re: [QEMU Patch 2/2] virtio-balloon: support for handling page reporting

[Nitesh Narayan Lal <nitesh@redhat.com>]

On 8/12/19 11:18 AM, Alexander Duyck wrote:
> On Mon, Aug 12, 2019 at 6:14 AM Nitesh Narayan Lal <nitesh@redhat.com> wrote:
>> Page reporting is a feature which enables the virtual machine to report
>> chunk of free pages to the hypervisor.
>> This patch enables QEMU to process these reports from the VM and discard the
>> unused memory range.
>>
>> Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com>
>> ---
>>  hw/virtio/virtio-balloon.c         | 41 ++++++++++++++++++++++++++++++
>>  include/hw/virtio/virtio-balloon.h |  2 +-
>>  2 files changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> index 25de154307..1132e47ee0 100644
>> --- a/hw/virtio/virtio-balloon.c
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -320,6 +320,39 @@ static void balloon_stats_set_poll_interval(Object *obj, Visitor *v,
>>      balloon_stats_change_timer(s, 0);
>>  }
>>
>> +static void virtio_balloon_handle_reporting(VirtIODevice *vdev, VirtQueue *vq)
>> +{
>> +    VirtQueueElement *elem;
>> +
>> +    while ((elem = virtqueue_pop(vq, sizeof(VirtQueueElement)))) {
>> +        unsigned int i;
>> +
>> +        for (i = 0; i < elem->in_num; i++) {
>> +            void *gaddr = elem->in_sg[i].iov_base;
>> +            size_t size = elem->in_sg[i].iov_len;
>> +            ram_addr_t ram_offset;
>> +            size_t rb_page_size;
>> +           RAMBlock *rb;
>> +
>> +            if (qemu_balloon_is_inhibited())
>> +                continue;
>> +
>> +            rb = qemu_ram_block_from_host(gaddr, false, &ram_offset);
>> +            rb_page_size = qemu_ram_pagesize(rb);
>> +
>> +            /* For now we will simply ignore unaligned memory regions */
>> +            if ((ram_offset | size) & (rb_page_size - 1))
>> +                continue;
>> +
>> +            ram_block_discard_range(rb, ram_offset, size);
>> +        }
>> +
>> +        virtqueue_push(vq, elem, 0);
>> +        virtio_notify(vdev, vq);
>> +        g_free(elem);
>> +    }
>> +}
>> +
> No offense, but I am a bit annoyed.

None taken at all.

>  If you are going to copy my code
> you should at least keep up with the fixes.


Yeah I did refer to your code and just because the quality of your code is
better than what I posted earlier and there is quite a lot for me to learn from it.


> stuff to handle the poison value. If you are going to just duplicate
> my setup you might as well have just pulled the QEMU patches from the
> last submission I did. Then this would have at least has the fix for
> the page poisoning.
>

The only reason I didn't include the poison change as I still need to understand
them.
I have this mentioned in my cover-email.


>  Also it wouldn't hurt to mention that you are
> basing it off of the patch set I submitted since it hasn't been
> accepted yet.


My bad!! This I will surely do from next time.

>
>>  static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>>  {
>>      VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
>> @@ -792,6 +825,12 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>>      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>>      s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
>>
>> +    if (virtio_has_feature(s->host_features,
>> +                           VIRTIO_BALLOON_F_REPORTING)) {
>> +        s->reporting_vq = virtio_add_queue(vdev, 16,
>> +                                          virtio_balloon_handle_reporting);
>> +    }
>> +
>>      if (virtio_has_feature(s->host_features,
>>                             VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
>>          s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
>> @@ -912,6 +951,8 @@ static Property virtio_balloon_properties[] = {
>>       * is disabled, resulting in QEMU 3.1 migration incompatibility.  This
>>       * property retains this quirk for QEMU 4.1 machine types.
>>       */
>> +    DEFINE_PROP_BIT("free-page-reporting", VirtIOBalloon, host_features,
>> +                    VIRTIO_BALLOON_F_REPORTING, true),
>>      DEFINE_PROP_BOOL("qemu-4-0-config-size", VirtIOBalloon,
>>                       qemu_4_0_config_size, false),
>>      DEFINE_PROP_LINK("iothread", VirtIOBalloon, iothread, TYPE_IOTHREAD,
>> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
>> index d1c968d237..15a05e6435 100644
>> --- a/include/hw/virtio/virtio-balloon.h
>> +++ b/include/hw/virtio/virtio-balloon.h
>> @@ -42,7 +42,7 @@ enum virtio_balloon_free_page_report_status {
>>
>>  typedef struct VirtIOBalloon {
>>      VirtIODevice parent_obj;
>> -    VirtQueue *ivq, *dvq, *svq, *free_page_vq;
>> +    VirtQueue *ivq, *dvq, *svq, *free_page_vq, *reporting_vq;
>>      uint32_t free_page_report_status;
>>      uint32_t num_pages;
>>      uint32_t actual;
>> --
>> 2.21.0
>> q
-- 
Thanks
Nitesh