lkml.org 
[lkml]   [2019]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC][Patch v9 3/6] KVM: Enables the kernel to report isolated pages
    From
    Date

    On 3/6/19 4:30 PM, Alexander Duyck wrote:
    > On Wed, Mar 6, 2019 at 7:51 AM Nitesh Narayan Lal <nitesh@redhat.com> wrote:
    >> This patch enables the kernel to report the isolated pages
    >> to the host via virtio balloon driver.
    >> In order to do so a new virtuqeue (hinting_vq) is added to the
    >> virtio balloon driver. As the host responds back after freeing
    >> the pages, all the isolated pages are returned back to the buddy
    >> via __free_one_page().
    >>
    >> Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com>
    > I ran into a few build issues due to this patch. Comments below.
    >
    >> ---
    >> drivers/virtio/virtio_balloon.c | 72 ++++++++++++++++++++++++++++-
    >> include/linux/page_hinting.h | 4 ++
    >> include/uapi/linux/virtio_balloon.h | 8 ++++
    >> virt/kvm/page_hinting.c | 18 ++++++--
    >> 4 files changed, 98 insertions(+), 4 deletions(-)
    >>
    >> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
    >> index 728ecd1eea30..cfe7574b5204 100644
    >> --- a/drivers/virtio/virtio_balloon.c
    >> +++ b/drivers/virtio/virtio_balloon.c
    >> @@ -57,13 +57,15 @@ enum virtio_balloon_vq {
    >> VIRTIO_BALLOON_VQ_INFLATE,
    >> VIRTIO_BALLOON_VQ_DEFLATE,
    >> VIRTIO_BALLOON_VQ_STATS,
    >> + VIRTIO_BALLOON_VQ_HINTING,
    >> VIRTIO_BALLOON_VQ_FREE_PAGE,
    >> VIRTIO_BALLOON_VQ_MAX
    >> };
    >>
    >> struct virtio_balloon {
    >> struct virtio_device *vdev;
    >> - struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
    >> + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq,
    >> + *hinting_vq;
    >>
    >> /* Balloon's own wq for cpu-intensive work items */
    >> struct workqueue_struct *balloon_wq;
    >> @@ -122,6 +124,56 @@ static struct virtio_device_id id_table[] = {
    >> { 0 },
    >> };
    >>
    >> +#ifdef CONFIG_KVM_FREE_PAGE_HINTING
    >> +int virtballoon_page_hinting(struct virtio_balloon *vb,
    >> + void *hinting_req,
    >> + int entries)
    >> +{
    >> + struct scatterlist sg;
    >> + struct virtqueue *vq = vb->hinting_vq;
    >> + int err;
    >> + int unused;
    >> + struct virtio_balloon_hint_req *hint_req;
    >> + u64 gpaddr;
    >> +
    >> + hint_req = kmalloc(sizeof(struct virtio_balloon_hint_req), GFP_KERNEL);
    >> + while (virtqueue_get_buf(vq, &unused))
    >> + ;
    >> +
    >> + gpaddr = virt_to_phys(hinting_req);
    >> + hint_req->phys_addr = cpu_to_virtio64(vb->vdev, gpaddr);
    >> + hint_req->count = cpu_to_virtio32(vb->vdev, entries);
    >> + sg_init_one(&sg, hint_req, sizeof(struct virtio_balloon_hint_req));
    >> + err = virtqueue_add_outbuf(vq, &sg, 1, hint_req, GFP_KERNEL);
    >> + if (!err)
    >> + virtqueue_kick(vb->hinting_vq);
    >> + else
    >> + kfree(hint_req);
    >> + return err;
    >> +}
    >> +
    >> +static void hinting_ack(struct virtqueue *vq)
    >> +{
    >> + int len = sizeof(struct virtio_balloon_hint_req);
    >> + struct virtio_balloon_hint_req *hint_req = virtqueue_get_buf(vq, &len);
    >> + void *v_addr = phys_to_virt(hint_req->phys_addr);
    >> +
    >> + release_buddy_pages(v_addr, hint_req->count);
    >> + kfree(hint_req);
    >> +}
    >> +
    > You use release_buddy_pages here, but never exported it in the call
    > down below. Since this can be built as a module and I believe the page
    > hinting can be built either into the kernel or as a seperate module
    > shouldn't you be exporting it?
    Thanks for pointing this out.
    >
    >> +static void enable_hinting(struct virtio_balloon *vb)
    >> +{
    >> + request_hypercall = (void *)&virtballoon_page_hinting;
    >> + balloon_ptr = vb;
    >> +}
    >> +
    >> +static void disable_hinting(void)
    >> +{
    >> + balloon_ptr = NULL;
    >> +}
    >> +#endif
    >> +
    >> static u32 page_to_balloon_pfn(struct page *page)
    >> {
    >> unsigned long pfn = page_to_pfn(page);
    >> @@ -481,6 +533,7 @@ static int init_vqs(struct virtio_balloon *vb)
    >> names[VIRTIO_BALLOON_VQ_DEFLATE] = "deflate";
    >> names[VIRTIO_BALLOON_VQ_STATS] = NULL;
    >> names[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
    >> + names[VIRTIO_BALLOON_VQ_HINTING] = NULL;
    >>
    >> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
    >> names[VIRTIO_BALLOON_VQ_STATS] = "stats";
    >> @@ -492,11 +545,18 @@ static int init_vqs(struct virtio_balloon *vb)
    >> callbacks[VIRTIO_BALLOON_VQ_FREE_PAGE] = NULL;
    >> }
    >>
    >> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_HINTING)) {
    >> + names[VIRTIO_BALLOON_VQ_HINTING] = "hinting_vq";
    >> + callbacks[VIRTIO_BALLOON_VQ_HINTING] = hinting_ack;
    >> + }
    >> err = vb->vdev->config->find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX,
    >> vqs, callbacks, names, NULL, NULL);
    >> if (err)
    >> return err;
    >>
    >> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_HINTING))
    >> + vb->hinting_vq = vqs[VIRTIO_BALLOON_VQ_HINTING];
    >> +
    >> vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE];
    >> vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE];
    >> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
    >> @@ -908,6 +968,11 @@ static int virtballoon_probe(struct virtio_device *vdev)
    >> if (err)
    >> goto out_del_balloon_wq;
    >> }
    >> +
    >> +#ifdef CONFIG_KVM_FREE_PAGE_HINTING
    >> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_HINTING))
    >> + enable_hinting(vb);
    >> +#endif
    >> virtio_device_ready(vdev);
    >>
    >> if (towards_target(vb))
    >> @@ -950,6 +1015,10 @@ static void virtballoon_remove(struct virtio_device *vdev)
    >> cancel_work_sync(&vb->update_balloon_size_work);
    >> cancel_work_sync(&vb->update_balloon_stats_work);
    >>
    >> +#ifdef CONFIG_KVM_FREE_PAGE_HINTING
    >> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_HINTING))
    >> + disable_hinting();
    >> +#endif
    >> if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
    >> cancel_work_sync(&vb->report_free_page_work);
    >> destroy_workqueue(vb->balloon_wq);
    >> @@ -1009,6 +1078,7 @@ static unsigned int features[] = {
    >> VIRTIO_BALLOON_F_MUST_TELL_HOST,
    >> VIRTIO_BALLOON_F_STATS_VQ,
    >> VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
    >> + VIRTIO_BALLOON_F_HINTING,
    >> VIRTIO_BALLOON_F_FREE_PAGE_HINT,
    >> VIRTIO_BALLOON_F_PAGE_POISON,
    >> };
    >> diff --git a/include/linux/page_hinting.h b/include/linux/page_hinting.h
    >> index d554a2581826..a32af8851081 100644
    >> --- a/include/linux/page_hinting.h
    >> +++ b/include/linux/page_hinting.h
    >> @@ -11,6 +11,8 @@
    >> #define HINTING_THRESHOLD 128
    >> #define FREE_PAGE_HINTING_MIN_ORDER (MAX_ORDER - 1)
    >>
    >> +extern void *balloon_ptr;
    >> +
    >> void guest_free_page_enqueue(struct page *page, int order);
    >> void guest_free_page_try_hinting(void);
    >> extern int __isolate_free_page(struct page *page, unsigned int order);
    >> @@ -18,3 +20,5 @@ extern void __free_one_page(struct page *page, unsigned long pfn,
    >> struct zone *zone, unsigned int order,
    >> int migratetype);
    >> void release_buddy_pages(void *obj_to_free, int entries);
    >> +extern int (*request_hypercall)(void *balloon_ptr,
    >> + void *hinting_req, int entries);
    >> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
    >> index a1966cd7b677..a7e909d77447 100644
    >> --- a/include/uapi/linux/virtio_balloon.h
    >> +++ b/include/uapi/linux/virtio_balloon.h
    >> @@ -29,6 +29,7 @@
    >> #include <linux/virtio_types.h>
    >> #include <linux/virtio_ids.h>
    >> #include <linux/virtio_config.h>
    >> +#include <linux/page_hinting.h>
    >>
    >> /* The feature bitmap for virtio balloon */
    >> #define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */
    > So I am pretty sure that this isn't valid. You have a file in
    > include/uapi/linux referencing one in include/linux. As such when the
    > userspace headers are built off of this they cannot access the kernel
    > include file.
    >
    >> @@ -36,6 +37,7 @@
    >> #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */
    >> #define VIRTIO_BALLOON_F_FREE_PAGE_HINT 3 /* VQ to report free pages */
    >> #define VIRTIO_BALLOON_F_PAGE_POISON 4 /* Guest is using page poisoning */
    >> +#define VIRTIO_BALLOON_F_HINTING 5 /* Page hinting virtqueue */
    >>
    >> /* Size of a PFN in the balloon interface. */
    >> #define VIRTIO_BALLOON_PFN_SHIFT 12
    >> @@ -108,4 +110,10 @@ struct virtio_balloon_stat {
    >> __virtio64 val;
    >> } __attribute__((packed));
    >>
    >> +#ifdef CONFIG_KVM_FREE_PAGE_HINTING
    >> +struct virtio_balloon_hint_req {
    >> + __virtio64 phys_addr;
    >> + __virtio64 count;
    >> +};
    >> +#endif
    >> #endif /* _LINUX_VIRTIO_BALLOON_H */
    >> diff --git a/virt/kvm/page_hinting.c b/virt/kvm/page_hinting.c
    >> index 9885b372b5a9..eb0c0ddfe990 100644
    >> --- a/virt/kvm/page_hinting.c
    >> +++ b/virt/kvm/page_hinting.c
    >> @@ -31,11 +31,16 @@ struct guest_isolated_pages {
    >> unsigned int order;
    >> };
    >>
    >> -void release_buddy_pages(void *obj_to_free, int entries)
    >> +int (*request_hypercall)(void *balloon_ptr, void *hinting_req, int entries);
    >> +EXPORT_SYMBOL(request_hypercall);
    >> +void *balloon_ptr;
    >> +EXPORT_SYMBOL(balloon_ptr);
    >> +
    > Why are you using a standard EXPORT_SYMBOL here instead of
    > EXPORT_SYMBOL_GPL? It seems like these are core functions that can
    > impact the memory allocator. It might make more sense to use
    > EXPORT_SYMBOL_GPL.
    >
    >> +void release_buddy_pages(void *hinting_req, int entries)
    >> {
    >> int i = 0;
    >> int mt = 0;
    >> - struct guest_isolated_pages *isolated_pages_obj = obj_to_free;
    >> + struct guest_isolated_pages *isolated_pages_obj = hinting_req;
    >>
    >> while (i < entries) {
    >> struct page *page = pfn_to_page(isolated_pages_obj[i].pfn);
    > See my comment above, I am pretty sure you need to be exporting this.
    > I had to change this in order to be able to build.
    Thanks, I will take note and correct it in the next version.
    --
    Regards
    Nitesh

    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2019-03-07 14:24    [W:3.145 / U:2.772 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site