From: Nitesh Narayan Lal <nitesh@redhat.com> To: Cornelia Huck <cohuck@redhat.com> Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, virtio-dev@lists.oasis-open.org, pbonzini@redhat.com, lcapitulino@redhat.com, pagupta@redhat.com, wei.w.wang@intel.com, yang.zhang.wz@gmail.com, riel@surriel.com, david@redhat.com, mst@redhat.com, dodgen@google.com, konrad.wilk@oracle.com, dhildenb@redhat.com, aarcange@redhat.com, alexander.duyck@gmail.com, john.starks@microsoft.com, dave.hansen@intel.com, mhocko@suse.com Subject: Re: [RFC][Patch v12 2/2] virtio-balloon: interface to support free page reporting Date: Wed, 14 Aug 2019 07:47:40 -0400 [thread overview] Message-ID: <c23f02b1-4bda-7dc6-9e28-4bad0a16cde6@redhat.com> (raw) In-Reply-To: <20190814122949.4946f438.cohuck@redhat.com> On 8/14/19 6:29 AM, Cornelia Huck wrote: > On Mon, 12 Aug 2019 09:12:35 -0400 > Nitesh Narayan Lal <nitesh@redhat.com> wrote: > >> Enables the kernel to negotiate VIRTIO_BALLOON_F_REPORTING feature with >> the host. If it is available and page_reporting_flag is set to true, >> page_reporting is enabled and its callback is configured along with >> the max_pages count which indicates the maximum number of pages that >> can be isolated and reported at a time. Currently, only free pages of >> order >= (MAX_ORDER - 2) are reported. To prevent any false OOM >> max_pages count is set to 16. >> >> By default page_reporting feature is enabled and gets loaded as soon >> as the virtio-balloon driver is loaded. However, it could be disabled >> by writing the page_reporting_flag which is a virtio-balloon parameter. >> >> Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> >> --- >> drivers/virtio/Kconfig | 1 + >> drivers/virtio/virtio_balloon.c | 64 ++++++++++++++++++++++++++++- >> include/uapi/linux/virtio_balloon.h | 1 + >> 3 files changed, 65 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c >> index 226fbb995fb0..defec00d4ee2 100644 >> --- a/drivers/virtio/virtio_balloon.c >> +++ b/drivers/virtio/virtio_balloon.c > (...) > >> +static void virtballoon_page_reporting_setup(struct virtio_balloon *vb) >> +{ >> + struct device *dev = &vb->vdev->dev; >> + int err; >> + >> + vb->page_reporting_conf.report = virtballoon_report_pages; >> + vb->page_reporting_conf.max_pages = PAGE_REPORTING_MAX_PAGES; >> + err = page_reporting_enable(&vb->page_reporting_conf); >> + if (err < 0) { >> + dev_err(dev, "Failed to enable reporting, err = %d\n", err); >> + page_reporting_flag = false; > Should we clear the feature bit in this case as well? I think yes. If I am not wrong then in a case where page reporting setup fails for some reason and at a later point the user wants to re-enable it then for that balloon driver has to be reloaded. Which would mean re-negotiation of the feature bit. > >> + vb->page_reporting_conf.report = NULL; >> + vb->page_reporting_conf.max_pages = 0; >> + return; >> + } >> +} >> + >> static void set_page_pfns(struct virtio_balloon *vb, >> __virtio32 pfns[], struct page *page) >> { >> @@ -476,6 +524,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_REPORTING] = NULL; >> >> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { >> names[VIRTIO_BALLOON_VQ_STATS] = "stats"; >> @@ -487,11 +536,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_REPORTING)) { >> + names[VIRTIO_BALLOON_VQ_REPORTING] = "reporting_vq"; >> + callbacks[VIRTIO_BALLOON_VQ_REPORTING] = balloon_ack; > Do we even want to try to set up the reporting queue if reporting has > been disabled via module parameter? Might make more sense to not even > negotiate the feature bit in that case. True. I think this should be replaced with something like (page_reporting_flag && virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING)). > >> + } >> 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_REPORTING)) >> + vb->reporting_vq = vqs[VIRTIO_BALLOON_VQ_REPORTING]; >> + >> 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)) { >> @@ -924,6 +980,9 @@ static int virtballoon_probe(struct virtio_device *vdev) >> if (err) >> goto out_del_balloon_wq; >> } >> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING) && >> + page_reporting_flag) >> + virtballoon_page_reporting_setup(vb); > In that case, you'd only need to check for the feature bit here. Why is that? I think both the checks should be present here as we need both the conditions to be true to enable page reporting. However, the order should be reversed because of the reason you mentioned earlier. > >> virtio_device_ready(vdev); >> >> if (towards_target(vb)) >> @@ -971,6 +1030,8 @@ static void virtballoon_remove(struct virtio_device *vdev) >> destroy_workqueue(vb->balloon_wq); >> } >> >> + if (page_reporting_flag) >> + page_reporting_disable(&vb->page_reporting_conf); > I think you should not disable it if the feature bit was never offered > in the first place, right? Yes. I should check both feature bit and module parameter here. > >> remove_common(vb); >> #ifdef CONFIG_BALLOON_COMPACTION >> if (vb->vb_dev_info.inode) > (...) -- Thanks Nitesh
WARNING: multiple messages have this Message-ID (diff)
From: Nitesh Narayan Lal <nitesh@redhat.com> To: Cornelia Huck <cohuck@redhat.com> Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, virtio-dev@lists.oasis-open.org, pbonzini@redhat.com, lcapitulino@redhat.com, pagupta@redhat.com, wei.w.wang@intel.com, yang.zhang.wz@gmail.com, riel@surriel.com, david@redhat.com, mst@redhat.com, dodgen@google.com, konrad.wilk@oracle.com, dhildenb@redhat.com, aarcange@redhat.com, alexander.duyck@gmail.com, john.starks@microsoft.com, dave.hansen@intel.com, mhocko@suse.com Subject: [virtio-dev] Re: [RFC][Patch v12 2/2] virtio-balloon: interface to support free page reporting Date: Wed, 14 Aug 2019 07:47:40 -0400 [thread overview] Message-ID: <c23f02b1-4bda-7dc6-9e28-4bad0a16cde6@redhat.com> (raw) In-Reply-To: <20190814122949.4946f438.cohuck@redhat.com> On 8/14/19 6:29 AM, Cornelia Huck wrote: > On Mon, 12 Aug 2019 09:12:35 -0400 > Nitesh Narayan Lal <nitesh@redhat.com> wrote: > >> Enables the kernel to negotiate VIRTIO_BALLOON_F_REPORTING feature with >> the host. If it is available and page_reporting_flag is set to true, >> page_reporting is enabled and its callback is configured along with >> the max_pages count which indicates the maximum number of pages that >> can be isolated and reported at a time. Currently, only free pages of >> order >= (MAX_ORDER - 2) are reported. To prevent any false OOM >> max_pages count is set to 16. >> >> By default page_reporting feature is enabled and gets loaded as soon >> as the virtio-balloon driver is loaded. However, it could be disabled >> by writing the page_reporting_flag which is a virtio-balloon parameter. >> >> Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> >> --- >> drivers/virtio/Kconfig | 1 + >> drivers/virtio/virtio_balloon.c | 64 ++++++++++++++++++++++++++++- >> include/uapi/linux/virtio_balloon.h | 1 + >> 3 files changed, 65 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c >> index 226fbb995fb0..defec00d4ee2 100644 >> --- a/drivers/virtio/virtio_balloon.c >> +++ b/drivers/virtio/virtio_balloon.c > (...) > >> +static void virtballoon_page_reporting_setup(struct virtio_balloon *vb) >> +{ >> + struct device *dev = &vb->vdev->dev; >> + int err; >> + >> + vb->page_reporting_conf.report = virtballoon_report_pages; >> + vb->page_reporting_conf.max_pages = PAGE_REPORTING_MAX_PAGES; >> + err = page_reporting_enable(&vb->page_reporting_conf); >> + if (err < 0) { >> + dev_err(dev, "Failed to enable reporting, err = %d\n", err); >> + page_reporting_flag = false; > Should we clear the feature bit in this case as well? I think yes. If I am not wrong then in a case where page reporting setup fails for some reason and at a later point the user wants to re-enable it then for that balloon driver has to be reloaded. Which would mean re-negotiation of the feature bit. > >> + vb->page_reporting_conf.report = NULL; >> + vb->page_reporting_conf.max_pages = 0; >> + return; >> + } >> +} >> + >> static void set_page_pfns(struct virtio_balloon *vb, >> __virtio32 pfns[], struct page *page) >> { >> @@ -476,6 +524,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_REPORTING] = NULL; >> >> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { >> names[VIRTIO_BALLOON_VQ_STATS] = "stats"; >> @@ -487,11 +536,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_REPORTING)) { >> + names[VIRTIO_BALLOON_VQ_REPORTING] = "reporting_vq"; >> + callbacks[VIRTIO_BALLOON_VQ_REPORTING] = balloon_ack; > Do we even want to try to set up the reporting queue if reporting has > been disabled via module parameter? Might make more sense to not even > negotiate the feature bit in that case. True. I think this should be replaced with something like (page_reporting_flag && virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING)). > >> + } >> 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_REPORTING)) >> + vb->reporting_vq = vqs[VIRTIO_BALLOON_VQ_REPORTING]; >> + >> 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)) { >> @@ -924,6 +980,9 @@ static int virtballoon_probe(struct virtio_device *vdev) >> if (err) >> goto out_del_balloon_wq; >> } >> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_REPORTING) && >> + page_reporting_flag) >> + virtballoon_page_reporting_setup(vb); > In that case, you'd only need to check for the feature bit here. Why is that? I think both the checks should be present here as we need both the conditions to be true to enable page reporting. However, the order should be reversed because of the reason you mentioned earlier. > >> virtio_device_ready(vdev); >> >> if (towards_target(vb)) >> @@ -971,6 +1030,8 @@ static void virtballoon_remove(struct virtio_device *vdev) >> destroy_workqueue(vb->balloon_wq); >> } >> >> + if (page_reporting_flag) >> + page_reporting_disable(&vb->page_reporting_conf); > I think you should not disable it if the feature bit was never offered > in the first place, right? Yes. I should check both feature bit and module parameter here. > >> remove_common(vb); >> #ifdef CONFIG_BALLOON_COMPACTION >> if (vb->vb_dev_info.inode) > (...) -- Thanks Nitesh --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
next prev parent reply other threads:[~2019-08-14 11:47 UTC|newest] Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-12 13:12 [RFC][PATCH v12 0/2] mm: Support for page reporting Nitesh Narayan Lal 2019-08-12 13:12 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-12 13:12 ` [RFC][Patch v12 1/2] mm: page_reporting: core infrastructure Nitesh Narayan Lal 2019-08-12 13:12 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-12 18:47 ` Alexander Duyck 2019-08-12 18:47 ` [virtio-dev] " Alexander Duyck 2019-08-12 18:47 ` Alexander Duyck 2019-08-12 20:04 ` Nitesh Narayan Lal 2019-08-12 20:04 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-20 14:11 ` Nitesh Narayan Lal 2019-08-20 14:11 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-12 20:05 ` David Hildenbrand 2019-08-12 20:05 ` [virtio-dev] " David Hildenbrand 2019-08-13 10:30 ` Nitesh Narayan Lal 2019-08-13 10:30 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-13 10:34 ` David Hildenbrand 2019-08-13 10:34 ` [virtio-dev] " David Hildenbrand 2019-08-13 10:42 ` Nitesh Narayan Lal 2019-08-13 10:42 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-13 10:44 ` David Hildenbrand 2019-08-13 10:44 ` [virtio-dev] " David Hildenbrand 2019-08-13 23:14 ` Alexander Duyck 2019-08-13 23:14 ` [virtio-dev] " Alexander Duyck 2019-08-13 23:14 ` Alexander Duyck 2019-08-14 7:07 ` David Hildenbrand 2019-08-14 7:07 ` [virtio-dev] " David Hildenbrand 2019-08-14 12:49 ` Nitesh Narayan Lal 2019-08-14 12:49 ` Nitesh Narayan Lal 2019-08-14 15:49 ` Nitesh Narayan Lal 2019-08-14 15:49 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-14 16:11 ` Alexander Duyck 2019-08-14 16:11 ` [virtio-dev] " Alexander Duyck 2019-08-14 16:11 ` Alexander Duyck 2019-08-15 13:15 ` Nitesh Narayan Lal 2019-08-15 13:15 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-15 19:22 ` Nitesh Narayan Lal 2019-08-15 19:22 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-15 23:00 ` Alexander Duyck 2019-08-15 23:00 ` [virtio-dev] " Alexander Duyck 2019-08-15 23:00 ` Alexander Duyck 2019-08-16 18:35 ` Nitesh Narayan Lal 2019-08-16 18:35 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-30 15:15 ` Nitesh Narayan Lal 2019-08-30 15:15 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-30 15:31 ` Alexander Duyck 2019-08-30 15:31 ` [virtio-dev] " Alexander Duyck 2019-08-30 15:31 ` Alexander Duyck 2019-08-30 16:05 ` Nitesh Narayan Lal 2019-08-30 16:05 ` [virtio-dev] " Nitesh Narayan Lal 2019-09-04 8:40 ` David Hildenbrand 2019-09-04 8:40 ` David Hildenbrand 2019-10-10 20:36 ` Alexander Duyck 2019-10-10 20:36 ` [virtio-dev] " Alexander Duyck 2019-10-10 20:36 ` Alexander Duyck 2019-10-11 11:02 ` Nitesh Narayan Lal 2019-10-11 11:02 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-12 13:12 ` [RFC][Patch v12 2/2] virtio-balloon: interface to support free page reporting Nitesh Narayan Lal 2019-08-12 13:12 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-14 10:29 ` Cornelia Huck 2019-08-14 10:29 ` [virtio-dev] " Cornelia Huck 2019-08-14 11:47 ` Nitesh Narayan Lal [this message] 2019-08-14 11:47 ` Nitesh Narayan Lal 2019-08-14 13:42 ` Cornelia Huck 2019-08-14 13:42 ` [virtio-dev] " Cornelia Huck 2019-08-14 14:01 ` Nitesh Narayan Lal 2019-08-14 14:01 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-12 13:13 ` [QEMU Patch 1/2] virtio-balloon: adding bit for page reporting support Nitesh Narayan Lal 2019-08-12 13:13 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-12 13:13 ` [QEMU Patch 2/2] virtio-balloon: support for handling page reporting Nitesh Narayan Lal 2019-08-12 13:13 ` [virtio-dev] " Nitesh Narayan Lal 2019-08-12 15:18 ` Alexander Duyck 2019-08-12 15:18 ` [virtio-dev] " Alexander Duyck 2019-08-12 15:18 ` Alexander Duyck 2019-08-12 15:26 ` Nitesh Narayan Lal 2019-08-12 15:26 ` [virtio-dev] " Nitesh Narayan Lal 2019-09-11 12:30 ` [RFC][PATCH v12 0/2] mm: Support for " David Hildenbrand 2019-09-11 12:30 ` [virtio-dev] " David Hildenbrand
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=c23f02b1-4bda-7dc6-9e28-4bad0a16cde6@redhat.com \ --to=nitesh@redhat.com \ --cc=aarcange@redhat.com \ --cc=alexander.duyck@gmail.com \ --cc=cohuck@redhat.com \ --cc=dave.hansen@intel.com \ --cc=david@redhat.com \ --cc=dhildenb@redhat.com \ --cc=dodgen@google.com \ --cc=john.starks@microsoft.com \ --cc=konrad.wilk@oracle.com \ --cc=kvm@vger.kernel.org \ --cc=lcapitulino@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mhocko@suse.com \ --cc=mst@redhat.com \ --cc=pagupta@redhat.com \ --cc=pbonzini@redhat.com \ --cc=riel@surriel.com \ --cc=virtio-dev@lists.oasis-open.org \ --cc=wei.w.wang@intel.com \ --cc=yang.zhang.wz@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.