On 06/11/2018 04:26 PM, Luiz Capitulino wrote:
> On Mon, 11 Jun 2018 11:19:00 -0400
> nilal@xxxxxxxxxx wrote:
>
>> From: Nitesh Narayan Lal <nilal@xxxxxxxxxx>
>>
>> This patch creates a new vq (hinting_vq) to be used for page hinting
>> and adds support in the existing virtio balloon infrastructure so
>> that the hyper list carrying pages which are supposed to be freed
>> could be sent to the host (QEMU) for processing by using hinting_vq.
>>
>> Signed-off-by: Nitesh Narayan Lal <nilal@xxxxxxxxxx>
>> ---
>> drivers/virtio/virtio_balloon.c | 99 ++++++++++++++++++++++++++++++++++++-
>> include/linux/page_hinting.h | 16 ++++++
>> include/uapi/linux/virtio_balloon.h | 1 +
>> virt/kvm/page_hinting.c | 36 ++++++--------
>> 4 files changed, 129 insertions(+), 23 deletions(-)
>> create mode 100644 include/linux/page_hinting.h
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index 6b237e3..217523f 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -32,6 +32,7 @@
>> #include <linux/mm.h>
>> #include <linux/mount.h>
>> #include <linux/magic.h>
>> +#include <linux/page_hinting.h>
>>
>> /*
>> * Balloon device works in 4K page units. So each page is pointed to by
>> @@ -53,8 +54,11 @@ static struct vfsmount *balloon_mnt;
>>
>> struct virtio_balloon {
>> struct virtio_device *vdev;
>> +#ifdef CONFIG_KVM_FREE_PAGE_HINTING
>> + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *hinting_vq;
>> +#else
>> struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
>> -
>> +#endif
> This is duplicating the exiting virtqueue pointers. It's only hinting_vq
> that has to be protected by CONFIG_KVM_FREE_PAGE_HINTING.
I will move hinting_vq into a separate statement, so that other
virtqueue pointers are not duplicated.
>
>> /* The balloon servicing is delegated to a freezable workqueue. */
>> struct work_struct update_balloon_stats_work;
>> struct work_struct update_balloon_size_work;
>> @@ -95,6 +99,33 @@ static struct virtio_device_id id_table[] = {
>> { 0 },
>> };
>>
>> +#ifdef CONFIG_KVM_FREE_PAGE_HINTING
>> +static void tell_host_one_page(struct virtio_balloon *vb, struct virtqueue *vq,
>> + u64 gvaddr, int len)
>> +{
> I think this function actually tells the host the whole array at once,
> right (vs. a single page). In this case, maybe it should be renamed to
> tell_host_free_pfns?
It communicates the address where the first entry of hypervisor_pagelist
is stored. Will it make sense if I rename it to tell_host_free_list?
>
>> + unsigned int id = VIRTQUEUE_DESC_ID_INIT;
>> + u64 gpaddr = virt_to_phys((void *)gvaddr);
>> +
>> + virtqueue_add_chain_desc(vq, gpaddr, len, &id, &id, 0);
>> + virtqueue_add_chain(vq, id, 0, NULL, (void *)gpaddr, NULL);
>> +}
>> +
>> +void virtballoon_page_hinting(struct virtio_balloon *vb, int hyper_entries)
>> +{
>> + u64 gvaddr = (u64)hypervisor_pagelist;
>> +
>> + vb->num_pfns = hyper_entries;
>> + tell_host_one_page(vb, vb->hinting_vq, gvaddr, hyper_entries);
>> +}
>> +
>> +static void hinting_ack(struct virtqueue *vq)
>> +{
>> + struct virtio_balloon *vb = vq->vdev->priv;
>> +
>> + wake_up(&vb->acked);
> Honest question: is there a process to be woken up? If I understood
> correctly, the guest busy-waits for the host ACK, so the calling process is
> never put to sleep. Or did I misunderstand?
Yes, It is not required. I will remove this.
>
>> +}
>> +#endif
>> +
>> static u32 page_to_balloon_pfn(struct page *page)
>> {
>> unsigned long pfn = page_to_pfn(page);
>> @@ -425,6 +456,62 @@ static void update_balloon_size_func(struct work_struct *work)
>> queue_work(system_freezable_wq, work);
>> }
>>
>> +#ifdef CONFIG_KVM_FREE_PAGE_HINTING
>> +static int init_vqs(struct virtio_balloon *vb)
>> +{
> I don't think you can duplicate init_vqs().
I will use a single init_vqs with ifdefs to add the code corresponding
to page hinting in my next patch-series. Thank you for pointing this out.
>
>> + struct virtqueue *vqs[4];
>> + vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request,
>> + hinting_ack };
>> + static const char * const names[] = { "inflate", "deflate", "stats",
>> + "hinting" };
>> + int err, nvqs;
>> + bool stats_vq_support, page_hinting_support;
>> +
>> + /*
>> + * We expect two virtqueues: inflate and deflate, and
>> + * optionally stat and hinting.
>> + */
>> + stats_vq_support = virtio_has_feature(vb->vdev,
>> + VIRTIO_BALLOON_F_STATS_VQ);
>> + page_hinting_support = virtio_has_feature(vb->vdev,
>> + VIRTIO_GUEST_PAGE_HINTING_VQ
>> + );
>> + if (stats_vq_support && page_hinting_support)
>> + nvqs = 4;
>> + else if (stats_vq_support || page_hinting_support)
>> + nvqs = 3;
>> + else
>> + nvqs = 2;
>> +
>> + err = virtio_find_vqs(vb->vdev, nvqs, vqs, callbacks, names, NULL);
>> + if (err)
>> + return err;
>> +
>> + vb->inflate_vq = vqs[0];
>> + vb->deflate_vq = vqs[1];
>> + if (page_hinting_support)
>> + vb->hinting_vq = vqs[3];
>> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
>> + struct scatterlist sg;
>> + unsigned int num_stats;
>> +
>> + vb->stats_vq = vqs[2];
>> +
>> + /*
>> + * Prime this virtqueue with one buffer so the hypervisor can
>> + * use it to signal us later (it can't be broken yet!).
>> + */
>> + num_stats = update_balloon_stats(vb);
>> +
>> + sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
>> + if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
>> + < 0)
>> + BUG();
>> + virtqueue_kick(vb->stats_vq);
>> + }
>> + return 0;
>> +}
>> +#else
>> static int init_vqs(struct virtio_balloon *vb)
>> {
>> struct virtqueue *vqs[3];
>> @@ -463,6 +550,8 @@ static int init_vqs(struct virtio_balloon *vb)
>> return 0;
>> }
>>
>> +#endif
>> +
>> #ifdef CONFIG_BALLOON_COMPACTION
>> /*
>> * virtballoon_migratepage - perform the balloon page migration on behalf of
>> @@ -604,6 +693,13 @@ static int virtballoon_probe(struct virtio_device *vdev)
>>
>> virtio_device_ready(vdev);
>>
>> +#ifdef CONFIG_KVM_FREE_PAGE_HINTING
>> + if (virtio_has_feature(vb->vdev, VIRTIO_GUEST_PAGE_HINTING_VQ)) {
>> + request_hypercall = (void *)&virtballoon_page_hinting;
>> + balloon_ptr = vb;
>> + }
>> +#endif
> Very minor, but I think it would be nicer if you used a function from
> virt/kvm/page_hinting.c instead of assigning global variables. Say:
>
> page_hinting_enable(vb, virtballoon_page_hinting);
I will make this change.
>
>> +
>> if (towards_target(vb))
>> virtballoon_changed(vdev);
>> return 0;
>> @@ -692,6 +788,7 @@ static unsigned int features[] = {
>> VIRTIO_BALLOON_F_MUST_TELL_HOST,
>> VIRTIO_BALLOON_F_STATS_VQ,
>> VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
>> + VIRTIO_GUEST_PAGE_HINTING_VQ,
>> };
>>
>> static struct virtio_driver virtio_balloon_driver = {
>> diff --git a/include/linux/page_hinting.h b/include/linux/page_hinting.h
>> new file mode 100644
>> index 0000000..0bfb646
>> --- /dev/null
>> +++ b/include/linux/page_hinting.h
>> @@ -0,0 +1,16 @@
>> +#define MAX_FGPT_ENTRIES 1000
>> +/*
>> + * hypervisor_pages - It is a dummy structure passed with the hypercall.
>> + * @pfn - page frame number for the page which is to be freed.
>> + * @pages - number of pages which are supposed to be freed.
>> + * A global array object is used to to hold the list of pfn and pages and is
>> + * passed as part of the hypercall.
>> + */
>> +struct hypervisor_pages {
>> + unsigned long pfn;
>> + unsigned int pages;
>> +};
>> +
>> +extern struct hypervisor_pages hypervisor_pagelist[MAX_FGPT_ENTRIES];
>> +extern void (*request_hypercall)(void *, int);
>> +extern void *balloon_ptr;
>> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
>> index 13b8cb5..16d299f 100644
>> --- a/include/uapi/linux/virtio_balloon.h
>> +++ b/include/uapi/linux/virtio_balloon.h
>> @@ -34,6 +34,7 @@
>> #define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */
>> #define VIRTIO_BALLOON_F_STATS_VQ 1 /* Memory Stats virtqueue */
>> #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */
>> +#define VIRTIO_GUEST_PAGE_HINTING_VQ 3 /* Page hinting virtqueue */
>>
>> /* Size of a PFN in the balloon interface. */
>> #define VIRTIO_BALLOON_PFN_SHIFT 12
>> diff --git a/virt/kvm/page_hinting.c b/virt/kvm/page_hinting.c
>> index 97500bb..417582a 100644
>> --- a/virt/kvm/page_hinting.c
>> +++ b/virt/kvm/page_hinting.c
>> @@ -5,8 +5,8 @@
>> #include <linux/sort.h>
>> #include <linux/kernel.h>
>> #include <trace/events/kmem.h>
>> +#include <linux/page_hinting.h>
>>
>> -#define MAX_FGPT_ENTRIES 1000
>> #define HYPERLIST_THRESHOLD 1 /* FIXME: find a good threshold */
>> /*
>> * struct kvm_free_pages - Tracks the pages which are freed by the guest.
>> @@ -21,22 +21,15 @@ struct kvm_free_pages {
>> unsigned int pages;
>> };
>>
>> -/*
>> - * hypervisor_pages - It is a dummy structure passed with the hypercall.
>> - * @pfn - page frame number for the page which is to be freed.
>> - * @pages - number of pages which are supposed to be freed.
>> - * A global array object is used to to hold the list of pfn and pages and is
>> - * passed as part of the hypercall.
>> - */
>> -struct hypervisor_pages {
>> - unsigned long pfn;
>> - unsigned int pages;
>> -};
>> -
>> static __cacheline_aligned_in_smp DEFINE_SEQLOCK(guest_page_lock);
>> DEFINE_PER_CPU(struct kvm_free_pages [MAX_FGPT_ENTRIES], kvm_pt);
>> DEFINE_PER_CPU(int, kvm_pt_idx);
>> struct hypervisor_pages hypervisor_pagelist[MAX_FGPT_ENTRIES];
>> +EXPORT_SYMBOL(hypervisor_pagelist);
>> +void (*request_hypercall)(void *, int);
>> +EXPORT_SYMBOL(request_hypercall);
>> +void *balloon_ptr;
>> +EXPORT_SYMBOL(balloon_ptr);
>>
>> static void empty_hyperlist(void)
>> {
>> @@ -49,13 +42,11 @@ static void empty_hyperlist(void)
>> }
>> }
>>
>> -void make_hypercall(void)
>> +void hyperlist_ready(int entries)
>> {
>> - /*
>> - * Dummy function: Tobe filled later.
>> - */
>> - empty_hyperlist();
>> trace_guest_str_dump("Hypercall to host...:");
>> + request_hypercall(balloon_ptr, entries);
>> + empty_hyperlist();
>> }
>>
>> static int sort_pfn(const void *a1, const void *b1)
>> @@ -156,7 +147,7 @@ int compress_hyperlist(void)
>> if (merge_counter != 0)
>> ret = pack_hyperlist() - 1;
>> else
>> - ret = MAX_FGPT_ENTRIES - 1;
>> + ret = MAX_FGPT_ENTRIES;
>> return ret;
>> }
>>
>> @@ -227,16 +218,16 @@ void arch_free_page_slowpath(void)
>> */
>> if (!prev_free) {
>> hyper_idx++;
>> - hypervisor_pagelist[hyper_idx].pfn = pfn;
>> - hypervisor_pagelist[hyper_idx].pages = 1;
>> trace_guest_free_page_slowpath(
>> hypervisor_pagelist[hyper_idx].pfn,
>> hypervisor_pagelist[hyper_idx].pages);
>> + hypervisor_pagelist[hyper_idx].pfn = pfn;
>> + hypervisor_pagelist[hyper_idx].pages = 1;
>> if (hyper_idx == MAX_FGPT_ENTRIES - 1) {
>> hyper_idx = compress_hyperlist();
>> if (hyper_idx >=
>> HYPERLIST_THRESHOLD) {
>> - make_hypercall();
>> + hyperlist_ready(hyper_idx);
>> hyper_idx = 0;
>> }
>> }
>> @@ -272,6 +263,7 @@ void arch_alloc_page(struct page *page, int order)
>> * free pages is full and a hypercall will be made. Until complete free
>> * page list is traversed no further allocaiton will be allowed.
>> */
>> +
>> do {
>> seq = read_seqbegin(&guest_page_lock);
>> } while (read_seqretry(&guest_page_lock, seq));
--
Regards
Nitesh
Attachment:
signature.asc
Description: OpenPGP digital signature
