Re: [Qemu-devel] [PATCH v2] balloon: Fix failure of updating guest memory status

[Ladi Prosek]

On Wed, Aug 3, 2016 at 9:25 AM, Ladi Prosek <address@hidden> wrote:
> On Tue, Aug 2, 2016 at 2:11 AM, Michael S. Tsirkin <address@hidden> wrote:
>> On Mon, Aug 01, 2016 at 11:59:31PM +0000, Li, Liang Z wrote:
>>> > On Wed, Jul 06, 2016 at 12:49:06PM +0000, Li, Liang Z wrote:
>>> > > > > > > After live migration, 'guest-stats' can't get the expected
>>> > > > > > > memory status in the guest. This issue is caused by commit
>>> > 4eae2a657d.
>>> > > > > > > The value of 's->stats_vq_elem' will be NULL after live
>>> > > > > > > migration, and the check in the function
>>> > > > > > > 'balloon_stats_poll_cb()' will prevent the 'virtio_notify()'
>>> > > > > > > from executing. So guest will not update the memory status.
>>> > > > > > >
>>> > > > > > > Commit 4eae2a657d is doing the right thing, but 
>>> > > > > > > 's->stats_vq_elem'
>>> > > > > > > should be treated as part of balloon device state and migrated
>>> > > > > > > to destination if it's not NULL to make everything works well.
>>> > > > > > >
>>> > > > > > > Signed-off-by: Liang Li <address@hidden>
>>> > > > > > > Suggested-by: Paolo Bonzini <address@hidden>
>>> > > > > > > Cc: Michael S. Tsirkin <address@hidden>
>>> > > > > > > Cc: Ladi Prosek <address@hidden>
>>> > > > > > > Cc: Paolo Bonzini <address@hidden>
>>> > > > > >
>>> > > > > > I agree there's an issue but we don't change versions anymore.
>>> > > > > > Breaking migrations for everyone is also not nice.
>>> > > > > >
>>> > > > > > How about queueing virtio_balloon_receive_stats so it will get
>>> > > > > > invoked when vm starts?
>>> > > > > >
>>> > > > >
>>> > > > > Could you give more explanation about how it works?  I can't catch 
>>> > > > > you.
>>> > > > >
>>> > > > > Thanks!
>>> > > > > Liang
>>> > > >
>>> > > > virtqueue_discard before migration
>>> > > >
>>> > > > virtio_balloon_receive_stats after migration
>>> > > >
>>> > >
>>> > > Sorry, I still can't catch you. Maybe it's easier for you to submit a
>>> > > patch than writing a lot a words to make me understand your idea.
>>> >
>>> > I'm rather busy now.  I might look into it towards end of the month.
>>> >
>>> > > I just don't understand why not to use the version to make things
>>> > > easier, is that not the original intent of version id?
>>> >
>>> > This was the original idea but we stopped using version ids since they 
>>> > have
>>> > many shortcomings.
>>> >
>>> > > If we want to extend the device and more states are needed, the idea
>>> > > you suggest can be used as a common solution?
>>> > >
>>> > > Thanks!
>>> > > Liang
>>> >
>>> > The idea is to try to avoid adding more state. that's not always possible 
>>> > but in
>>> > this case element was seen but not consumed yet, so it should be possible
>>> > for destination to simply get it from the VQ again.
>>> >
>>> > > > --
>>> > > > MST
>>>
>>> Hi Michel,
>>>
>>> Do you have time for this issue recently?
>>>
>>> Thanks!
>>> Liang
>
> Hi Liang,
>
> I should be able to look into it this week if you help me with testing.
>
> Thanks,
> Ladi

Please try the attached patch. I have tested it with a simple
'migrate' to save the state and then '-incoming' to load it back.

One question for you: is it expected that stats_poll_interval is not
preserved by save/load? I had to explicitly set
guest-stats-polling-interval on the receiving VM to start getting
stats again. It's also the reason why the new
virtio_balloon_receive_stats call is not under if
(balloon_stats_enabled(s)) because this condition always evaluates to
false for me.

Thanks!
Ladi

>> Sorry, doesn't look like I will.
>> Idea is to make sure balloon_stats_poll_cb runs
>> on source. This will set stats_vq_elem to NULL.
>>
>>
>> --
>> MST

0001-balloon-preserve-stats-virtqueue-state-across-migrat.patch
Description: Text Data