[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH v5 4/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT
On Sun, Mar 18, 2018 at 06:36:20PM +0800, Wei Wang wrote:
> On 03/16/2018 11:16 PM, Michael S. Tsirkin wrote:
> > On Fri, Mar 16, 2018 at 06:48:28PM +0800, Wei Wang wrote:
> > > The new feature enables the virtio-balloon device to receive hints of
> > > guest free pages from the free page vq.
> > >
> > > balloon_free_page_start - start guest free page hint reporting.
> > > balloon_free_page_stop - stop guest free page hint reporting.
> > >
> > > Note: balloon will report pages which were free at the time
> > > of this call. As the reporting happens asynchronously, dirty bit logging
> > > must be enabled before this call is made. Guest reporting must be
> > > disabled before the migration dirty bitmap is synchronized.
> > >
> > > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > > Signed-off-by: Liang Li <liang.z.li@intel.com>
> > > CC: Michael S. Tsirkin <mst@redhat.com>
> > > CC: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > CC: Juan Quintela <quintela@redhat.com>
> > > ---
> > > balloon.c | 58 +++++--
> > > hw/virtio/virtio-balloon.c | 217 ++++++++++++++++++++++--
> > > include/hw/virtio/virtio-balloon.h | 20 ++-
> > > include/standard-headers/linux/virtio_balloon.h | 7 +
> > > include/sysemu/balloon.h | 15 +-
> > > 5 files changed, 288 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/balloon.c b/balloon.c
> > > index 6bf0a96..87a0410 100644
> > > --- a/balloon.c
> > > +++ b/balloon.c
> > > @@ -36,6 +36,9 @@
> > >
> > > +static void *virtio_balloon_poll_free_page_hints(void *opaque)
> > > +{
> > > + VirtQueueElement *elem;
> > > + VirtIOBalloon *dev = opaque;
> > > + VirtQueue *vq = dev->free_page_vq;
> > > + uint32_t id;
> > > + size_t size;
> > > +
> > > + /* The optimization thread runs only when the guest is running. */
> > > + while (runstate_is_running()) {
> > Note that this check is not guaranteed to be correct
> > when checked like this outside BQL.
> >
> > I think you are better off relying on status
> > callback to synchronise with the backend thread.
> >
>
> It's actually OK here, I think we don't need the guarantee. The device is
> just the consumer of the vq, essentially it doesn't have a dependency (i.e.
> won't block or cause errors) on the guest state.
> For example:
> 1) when the device executes "while (runstate_is_running())" and finds that
> the guest is running, it proceeds;
> 2) the guest is stopped immediately right after the "while
> (runstate_is_running())" check;
> 3) the device side execution reaches virtqueue_pop(), and finds
> "elem==NULL", since the driver (provider) stops adding hints. Then it
> continues by going back to "while (runstate_is_running())", and now it finds
> the guest is stopped, and then exits.
OTOH it seems that if thread stops nothing will wake it up
whem vm is restarted. Such bahaviour change across vmstop/vmstart
is unexpected.
> Essentially, this runstate check is just an optimization to the case that
> the driver is stopped to provide hints while the device side optimization
> thread is still polling the empty vq (i.e. effort in vain). Probably, it
> would be better to check the runstate under "elem==NULL":
>
> while (1) {
> ...
> elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> if (!elem) {
> qemu_spin_unlock(&dev->free_page_lock);
> if (runstate_is_running())
> continue;
> else
> break;
> }
> ...
> }
>
>
> > > + dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
> > > + } else if (dev->free_page_report_status ==
> > > + FREE_PAGE_REPORT_S_START) {
> > > + /*
> > > + * Stop the optimization only when it has started. This avoids
> > > + * a stale stop sign for the previous command.
> > > + */
> > > + dev->free_page_report_status = FREE_PAGE_REPORT_S_STOP;
> > > + qemu_spin_unlock(&dev->free_page_lock);
> > > + break;
> > > + }
> > And else? What if it does not match and status is not start?
> > Don't you need to skip in elem decoding?
>
> No, actually we don't need "else". Please see the code inside if
> (elem->in_num) below. If the status isn't set to START,
> qemu_guest_free_page_hint will not be called to decode the elem.
>
>
> >
> > > + }
> > > +
> > > + if (elem->in_num) {
> > > + if (dev->free_page_report_status == FREE_PAGE_REPORT_S_START &&
> > > + !dev->poison_val) {
> > poison generally disables everything? Add a TODO to handle
> > it in he future pls.
>
>
> OK, will add TODO in the commit.
>
>
>
> @@ -368,6 +499,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
> ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT),
> &error_abort);
> }
> + dev->poison_val = le32_to_cpu(config.poison_val);
>
> > We probably should not assume this value is correct if guest didn't
> > ack the appropriate feature.
>
>
> OK, I'll add a comment to explain that.
>
>
> @@ -475,11 +632,37 @@ static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
> {
> VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> - if (!s->stats_vq_elem && vdev->vm_running &&
> - (status & VIRTIO_CONFIG_S_DRIVER_OK) && virtqueue_rewind(s->svq, 1)) {
> - /* poll stats queue for the element we have discarded when the VM
> - * was stopped */
> - virtio_balloon_receive_stats(vdev, s->svq);
> + if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
> + if (!s->stats_vq_elem && vdev->vm_running &&
> + virtqueue_rewind(s->svq, 1)) {
> + /*
> + * Poll stats queue for the element we have discarded when the VM
> + * was stopped.
> + */
> + virtio_balloon_receive_stats(vdev, s->svq);
> + }
> +
> + if (virtio_balloon_free_page_support(s)) {
> + qemu_add_balloon_handler(virtio_balloon_to_target,
> + virtio_balloon_stat,
> + virtio_balloon_free_page_support,
> + virtio_balloon_free_page_start,
> + virtio_balloon_free_page_stop,
> + s);
> + /*
> + * This handles the case that the guest is being stopped (e.g. by
> + * qmp commands) while the driver is still reporting hints. When
> + * the guest is woken up, it will continue to report hints, which
> + * are not needed. So when the wakeup notifier invokes the
> + * set_status callback here, we get the chance to make sure that
> + * the free page optimization thread is exited via
> + * virtio_balloon_free_page_stop.
> + */
> + virtio_balloon_free_page_stop(s);
>
> > I don't understand the logic here at all.
> > So if status is set to DRIVER_OK, then we stop reporting?
> >
>
> I thought about this more. It's not necessary to call the stop() function
> here, because we have already made the optimization thread exit when the
> guest is stopped. When the guest is woken up, it will continue to report,
> and there is no consumer of the vq (the optimization thread has exited).
> There is no functional issue here, since the driver will just drop the hint
> when the vq is full. From the optimization point of view, I think we can
> consider to replace the above sop() function with
> virtio_notify_config(vdev), which will stop the guest's unnecessary
> reporting.
I do not understand why we want to increment the counter
on vm stop though. It does make sense to stop the thread
but why not resume where we left off when vm is resumed?
>
> Best,
> Wei
In short the code will be easier to reason about if there's some
symmetry in how we start/stop things. If we need the thread to run
together with the VCPU then starting/stopping it from status seems
like a reasonable choice.
--
MST
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]