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

On Sat, Nov 18, 2017 at 05:22:28AM +0000, Wang, Wei W wrote:

On Friday, November 17, 2017 8:45 PM, Michael S. Tsirkin wrote:

quoted

On Fri, Nov 17, 2017 at 07:35:03PM +0800, Wei Wang wrote:

quoted

On 11/16/2017 09:27 PM, Wei Wang wrote:

quoted

On 11/16/2017 04:32 AM, Michael S. Tsirkin wrote:

quoted

On Fri, Nov 03, 2017 at 04:13:06PM +0800, Wei Wang wrote:

quoted

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.

quoted

quoted

quoted

quoted

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).

quoted

quoted

quoted

quoted

- free_page_cmd_id: the sequence id of the free page report
command given by the host.

Signed-off-by: Wei Wang <redacted>
Signed-off-by: Liang Li <redacted>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Michal Hocko <mhocko@kernel.org>
---

+
+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);
        send_free_page_cmd_id(vb, &vb->stop_cmd_id);


walk_free_mem_block() should be a more generic API, and this potential
page poisoning issue is specific to live migration which is only one
use case of this function, so I think it is better to handle it in the
special use case itself.

Best,
Wei

It's a quick work-around but it doesn't make me very happy.

AFAIK e.g. RHEL has a debug kernel with poisoning enabled.
If this never uses free page hinting at all, it will be much less useful for
debugging guests.

I understand your concern. I think people who use debugging guests
don't regard performance as the first priority, and most vendors
usually wouldn't use debugging guests for their products.

And when one of these crashes but only after migration what do you do?  A
very common step is for Red Hat support is to ask people to try
reproducing with a debug build.

IOT being able to debug guests is important, if a debugging guest takes
a significantly different path from non-debug one, we have a problem.

How about taking it as the initial solution? We can exploit more
solutions after this series is done.

Best,
Wei

I think it's fine as a separate patch.

-- 
MST