>>> +++ b/hw/virtio/virtio-balloon.c
>>> @@ -527,7 +527,8 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>>> ret = false;
>>> goto out;
>>> }
>>> - if (id == dev->free_page_report_cmd_id) {
>>> + if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED &&
>>> + id == dev->free_page_report_cmd_id) {
>>> dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
>>> } else {
>>> /*
>>
>> But doesn't that mean that, after the first hint, all further ones will
>> be discarded and we'll enter the STOP state in the else case? Or am I
>> missing something?
>>
>> Shouldn't this be something like
>>
>> if (id == dev->free_page_report_cmd_id) {
>> if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
>> dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
>> }
>> /* Stay in FREE_PAGE_REPORT_S_START as long as the cmd_id match .*/
>> } else { ...
>
> There should only be one element containing an outbuf at the start of
> the report. Once that is processed we should not see the driver
> sending additional outbufs unless it is sending the STOP command ID.
Ok, I assume what Linux guests do is considered the correct protocol.
[...]
>
>>> @@ -592,14 +593,16 @@ static void
>>> virtio_balloon_free_page_start(VirtIOBalloon *s)
>>> return;
>>> }
>>>
>>> - if (s->free_page_report_cmd_id == UINT_MAX) {
>>> + qemu_mutex_lock(&s->free_page_lock);
>>> +
>>> + if (s->free_page_report_cmd_id++ == UINT_MAX) {
>>> s->free_page_report_cmd_id =
>>> VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
>>> - } else {
>>> - s->free_page_report_cmd_id++;
>>> }
>>
>> Somewhat unrelated cleanup.
>
> Agreed. I can drop it if preferred. I just took care of it because I
> was adding the lock above and below to prevent us from getting into
> any wierd states where the command ID might be updated but the report
> status was not.
No hard feelings, it just makes reviewing harder, because one has to
investigate how the changes relate to the locking changes - to find out
they don't. :)
Acked-by: David Hildenbrand <da...@redhat.com>
--
Thanks,
David / dhildenb
