| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1309690349.2250.21.camel@sasha>
Date: Sun, 03 Jul 2011 13:52:29 +0300
From: Sasha Levin <levinsasha928@...il.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: linux-kernel@...r.kernel.org,
Rusty Russell <rusty@...tcorp.com.au>,
virtualization@...ts.linux-foundation.org, kvm@...r.kernel.org,
dave@...ux.vnet.ibm.com, Amit Shah <amit.shah@...hat.com>,
Pekka Enberg <penberg@...nel.org>
Subject: Re: [PATCH] virtio_balloon: Notify guest only after deflating the
balloon
On Sun, 2011-07-03 at 13:30 +0300, Michael S. Tsirkin wrote:
> On Sun, Jul 03, 2011 at 12:46:59PM +0300, Sasha Levin wrote:
> > On Sun, 2011-07-03 at 11:27 +0300, Michael S. Tsirkin wrote:
> > > Doesn't this basically just revert
> > > bf50e69f63d21091e525185c3ae761412be0ba72?
> > >
> >
> > Yes. I haven't noticed that commit.
> >
> > Ignoring the VIRTIO_BALLOON_F_MUST_TELL_HOST feature flag causes an
> > unnecessary delay due to having to kick and wait for the host to process
> > the deflate vq before actually using the memory pages.
>
> OTOH, as the commit points out:
> Without this feature, we might free a page (and have another
> user touch it) while the hypervisor is unprepared for it
>
> Are you doing so many deflates that the speed is important?
>
In the case of kvm tools and qemu for example, the hypervisor doesn't
need to be 'prepared' for it.
When the balloon if inflated, the pages are madvise(MADV_DONTNEED),
which means that we can access those pages immediately when deflating
without having to do anything special to prepare for it.
I'm not saying that speed is critical here, I'm just saying that the
feature was added to the spec for a reason - to prevent unnecessary
overhead like in the case of kvm tools and qemu, and we just choose to
ignore it completely.
> > I've stumbled on it when writing the virtio_balloon driver for kvm
> > tools. Initially my plan was to ignore the deflate vq completely, since
> > the virtio spec says that unless we set the MUST_TELL_HOST flag,
> > "deflation advice is merely a courtesy".
> >
> > I've noticed that if I don't process the deflate vq the guest doesn't
> > use the deflated pages at all - something which contradicts the feature
> > that lets him use the deflated pages without waiting for the deflate vq.
>
> OK, so you have a host that does not process the vq.
> As a result tell_host blocks forever.
> With your patch, guest will block after releasing the
> page, so the next deflate request will get stuck.
> This doesn't seem to be a huge improvement so
> we can conclude such a host is broken?
>
No, the host is not doing anything wrong by not processing deflate vq.
This just leads me to believe that we should either not notify the host,
or not wait_for_completion() when telling the host.
>
>
>
> > >
> > > On Sat, Jul 02, 2011 at 06:06:56AM +0300, Sasha Levin wrote:
> > > > Unless the host requires that requested pages won't be used until
> > > > he us notified (VIRTIO_BALLOON_F_MUST_TELL_HOST), only notify after
> > > > deflating the balloon.
> > > >
> > > > This will avoid having to take an exit before actually using the pages.
> > > >
> > > > Cc: Rusty Russell <rusty@...tcorp.com.au>
> > > > Cc: "Michael S. Tsirkin" <mst@...hat.com>
> > > > Cc: virtualization@...ts.linux-foundation.org
> > > > Cc: kvm@...r.kernel.org
> > > > Signed-off-by: Sasha Levin <levinsasha928@...il.com>
> > > > ---
> > > > drivers/virtio/virtio_balloon.c | 16 ++++++++++------
> > > > 1 files changed, 10 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > > index e058ace..055f95d 100644
> > > > --- a/drivers/virtio/virtio_balloon.c
> > > > +++ b/drivers/virtio/virtio_balloon.c
> > > > @@ -148,14 +148,18 @@ static void leak_balloon(struct virtio_balloon *vb, size_t num)
> > > > vb->num_pages--;
> > > > }
> > > >
> > > > -
> > > > /*
> > > > - * Note that if
> > > > - * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> > > > - * is true, we *have* to do it in this order
> > > > + * If the host doesn't require us to notify him before using
> > > > + * pages which belong to the balloon, update him only after
> > > > + * freeing those pages for guest use.
> > > > */
> > > > - tell_host(vb, vb->deflate_vq);
> > > > - release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST)) {
> > > > + tell_host(vb, vb->deflate_vq);
> > > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > + } else {
> > > > + release_pages_by_pfn(vb->pfns, vb->num_pfns);
> > > > + tell_host(vb, vb->deflate_vq);
> > > > + }
> > > > }
> > > >
> > > > static inline void update_stat(struct virtio_balloon *vb, int idx,
> > > > --
> > > > 1.7.6
> >
> >
> > --
> >
> > Sasha.
--
Sasha.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists