Re: [virtio-dev] Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ

From: Wei Wang
Date: Fri Nov 17 2017 - 06:46:29 EST


On 11/17/2017 07:35 PM, Wei Wang wrote:
On 11/16/2017 09:27 PM, Wei Wang wrote:
On 11/16/2017 04:32 AM, Michael S. Tsirkin wrote:
On Fri, Nov 03, 2017 at 04:13:06PM +0800, Wei Wang wrote:
Negotiation of the VIRTIO_BALLOON_F_FREE_PAGE_VQ feature indicates the
support of reporting hints of guest free pages to the host via
virtio-balloon. The host requests the guest to report the free pages by
sending commands via the virtio-balloon configuration registers.

When the guest starts to report, the first element added to the free page
vq is a sequence id of the start reporting command. The id is given by
the host, and it indicates whether the following free pages correspond
to the command. For example, the host may stop the report and start again
with a new command id. The obsolete pages for the previous start command
can be detected by the id dismatching on the host. The id is added to the
vq using an output buffer, and the free pages are added to the vq using
input buffer.

Here are some explainations about the added configuration registers:
- host2guest_cmd: a register used by the host to send commands to the
guest.
- guest2host_cmd: written by the guest to ACK to the host about the
commands that have been received. The host will clear the corresponding
bits on the host2guest_cmd register. The guest also uses this register
to send commands to the host (e.g. when finish free page reporting).
- free_page_cmd_id: the sequence id of the free page report command
given by the host.

Signed-off-by: Wei Wang <wei.w.wang@xxxxxxxxx>
Signed-off-by: Liang Li <liang.z.li@xxxxxxxxx>
Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
Cc: Michal Hocko <mhocko@xxxxxxxxxx>
---

+
+static void report_free_page(struct work_struct *work)
+{
+ struct virtio_balloon *vb;
+
+ vb = container_of(work, struct virtio_balloon, report_free_page_work);
+ report_free_page_cmd_id(vb);
+ walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
+ /*
+ * The last few free page blocks that were added may not reach the
+ * batch size, but need a kick to notify the device to handle them.
+ */
+ virtqueue_kick(vb->free_page_vq);
+ report_free_page_end(vb);
+}
+
I think there's an issue here: if pages are poisoned and hypervisor
subsequently drops them, testing them after allocation will
trigger a false positive.

The specific configuration:

PAGE_POISONING on
PAGE_POISONING_NO_SANITY off
PAGE_POISONING_ZERO off


Solutions:
1. disable the feature in that configuration
suggested as an initial step

Thanks for the finding.
Similar to this option: I'm thinking could we make walk_free_mem_block() simply return if that option is on?
That is, at the beginning of the function:
if (!page_poisoning_enabled())
return;



Thought about it more, I think it would be better to put this logic to virtio_balloon:

send_free_page_cmd_id(vb, &vb->start_cmd_id);
if (page_poisoning_enabled() &&
!IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY))
walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);

logic should be inverse:
if (!(page_poisoning_enabled() &&
!IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY)))

Best,
Wei