From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: virtio-dev-return-2693-cohuck=redhat.com@lists.oasis-open.org Sender: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Received: from lists.oasis-open.org (oasis-open.org [66.179.20.138]) by lists.oasis-open.org (Postfix) with ESMTP id D6A4C5818CAB for ; Fri, 17 Nov 2017 04:44:58 -0800 (PST) Date: Fri, 17 Nov 2017 14:44:48 +0200 From: "Michael S. Tsirkin" Message-ID: <20171117144253-mutt-send-email-mst@kernel.org> References: <1509696786-1597-1-git-send-email-wei.w.wang@intel.com> <1509696786-1597-7-git-send-email-wei.w.wang@intel.com> <20171115220743-mutt-send-email-mst@kernel.org> <5A0D923C.4020807@intel.com> <5A0EC967.5090407@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5A0EC967.5090407@intel.com> Subject: Re: [virtio-dev] Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ To: Wei Wang Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org, kvm@vger.kernel.org, mawilcox@microsoft.com, qemu-devel@nongnu.org, amit.shah@redhat.com, penguin-kernel@I-love.SAKURA.ne.jp, linux-kernel@vger.kernel.org, willy@infradead.org, virtualization@lists.linux-foundation.org, linux-mm@kvack.org, yang.zhang.wz@gmail.com, quan.xu@aliyun.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, akpm@linux-foundation.org, mhocko@kernel.org, mgorman@techsingularity.net, liliang.opensource@gmail.com List-ID: On Fri, Nov 17, 2017 at 07:35:03PM +0800, 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 > > > > Signed-off-by: Liang Li > > > > Cc: Michael S. Tsirkin > > > > Cc: Michal Hocko > > > > --- > > > > > > > > + > > > > +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. -- MST --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ Date: Fri, 17 Nov 2017 14:44:48 +0200 Message-ID: <20171117144253-mutt-send-email-mst@kernel.org> References: <1509696786-1597-1-git-send-email-wei.w.wang@intel.com> <1509696786-1597-7-git-send-email-wei.w.wang@intel.com> <20171115220743-mutt-send-email-mst@kernel.org> <5A0D923C.4020807@intel.com> <5A0EC967.5090407@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org, kvm@vger.kernel.org, mawilcox@microsoft.com, qemu-devel@nongnu.org, amit.shah@redhat.com, penguin-kernel@I-love.SAKURA.ne.jp, linux-kernel@vger.kernel.org, willy@infradead.org, virtualization@lists.linux-foundation.org, linux-mm@kvack.org, yang.zhang.wz@gmail.com, quan.xu@aliyun.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, akpm@linux-foundation.org, mhocko@kernel.org, mgorman@techsingularity.net, liliang.opensource@gmail.com To: Wei Wang Return-path: Sender: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Content-Disposition: inline In-Reply-To: <5A0EC967.5090407@intel.com> List-Id: kvm.vger.kernel.org On Fri, Nov 17, 2017 at 07:35:03PM +0800, 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 > > > > Signed-off-by: Liang Li > > > > Cc: Michael S. Tsirkin > > > > Cc: Michal Hocko > > > > --- > > > > > > > > + > > > > +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. -- MST From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-f70.google.com (mail-oi0-f70.google.com [209.85.218.70]) by kanga.kvack.org (Postfix) with ESMTP id E6C1D6B0038 for ; Fri, 17 Nov 2017 07:44:58 -0500 (EST) Received: by mail-oi0-f70.google.com with SMTP id h6so1035191oia.17 for ; Fri, 17 Nov 2017 04:44:58 -0800 (PST) Received: from mx1.redhat.com (mx1.redhat.com. [209.132.183.28]) by mx.google.com with ESMTPS id s188si1091018oie.136.2017.11.17.04.44.57 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 17 Nov 2017 04:44:57 -0800 (PST) Date: Fri, 17 Nov 2017 14:44:48 +0200 From: "Michael S. Tsirkin" Subject: Re: [virtio-dev] Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ Message-ID: <20171117144253-mutt-send-email-mst@kernel.org> References: <1509696786-1597-1-git-send-email-wei.w.wang@intel.com> <1509696786-1597-7-git-send-email-wei.w.wang@intel.com> <20171115220743-mutt-send-email-mst@kernel.org> <5A0D923C.4020807@intel.com> <5A0EC967.5090407@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5A0EC967.5090407@intel.com> Sender: owner-linux-mm@kvack.org List-ID: To: Wei Wang Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org, kvm@vger.kernel.org, mawilcox@microsoft.com, qemu-devel@nongnu.org, amit.shah@redhat.com, penguin-kernel@I-love.SAKURA.ne.jp, linux-kernel@vger.kernel.org, willy@infradead.org, virtualization@lists.linux-foundation.org, linux-mm@kvack.org, yang.zhang.wz@gmail.com, quan.xu@aliyun.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, akpm@linux-foundation.org, mhocko@kernel.org, mgorman@techsingularity.net, liliang.opensource@gmail.com On Fri, Nov 17, 2017 at 07:35:03PM +0800, 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 > > > > Signed-off-by: Liang Li > > > > Cc: Michael S. Tsirkin > > > > Cc: Michal Hocko > > > > --- > > > > > > > > + > > > > +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. -- MST -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933042AbdKQMpH (ORCPT ); Fri, 17 Nov 2017 07:45:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60362 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932463AbdKQMo5 (ORCPT ); Fri, 17 Nov 2017 07:44:57 -0500 Date: Fri, 17 Nov 2017 14:44:48 +0200 From: "Michael S. Tsirkin" To: Wei Wang Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org, kvm@vger.kernel.org, mawilcox@microsoft.com, qemu-devel@nongnu.org, amit.shah@redhat.com, penguin-kernel@I-love.SAKURA.ne.jp, linux-kernel@vger.kernel.org, willy@infradead.org, virtualization@lists.linux-foundation.org, linux-mm@kvack.org, yang.zhang.wz@gmail.com, quan.xu@aliyun.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, akpm@linux-foundation.org, mhocko@kernel.org, mgorman@techsingularity.net, liliang.opensource@gmail.com Subject: Re: [virtio-dev] Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ Message-ID: <20171117144253-mutt-send-email-mst@kernel.org> References: <1509696786-1597-1-git-send-email-wei.w.wang@intel.com> <1509696786-1597-7-git-send-email-wei.w.wang@intel.com> <20171115220743-mutt-send-email-mst@kernel.org> <5A0D923C.4020807@intel.com> <5A0EC967.5090407@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5A0EC967.5090407@intel.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Fri, 17 Nov 2017 12:44:56 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Nov 17, 2017 at 07:35:03PM +0800, 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 > > > > Signed-off-by: Liang Li > > > > Cc: Michael S. Tsirkin > > > > Cc: Michal Hocko > > > > --- > > > > > > > > + > > > > +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. -- MST From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35878) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eFg10-0007Ni-Ux for qemu-devel@nongnu.org; Fri, 17 Nov 2017 07:45:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eFg0w-0004vg-72 for qemu-devel@nongnu.org; Fri, 17 Nov 2017 07:45:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56088) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eFg0v-0004vF-Uy for qemu-devel@nongnu.org; Fri, 17 Nov 2017 07:44:58 -0500 Date: Fri, 17 Nov 2017 14:44:48 +0200 From: "Michael S. Tsirkin" Message-ID: <20171117144253-mutt-send-email-mst@kernel.org> References: <1509696786-1597-1-git-send-email-wei.w.wang@intel.com> <1509696786-1597-7-git-send-email-wei.w.wang@intel.com> <20171115220743-mutt-send-email-mst@kernel.org> <5A0D923C.4020807@intel.com> <5A0EC967.5090407@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5A0EC967.5090407@intel.com> Subject: Re: [Qemu-devel] [virtio-dev] Re: [PATCH v17 6/6] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wei Wang Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org, kvm@vger.kernel.org, mawilcox@microsoft.com, qemu-devel@nongnu.org, amit.shah@redhat.com, penguin-kernel@I-love.SAKURA.ne.jp, linux-kernel@vger.kernel.org, willy@infradead.org, virtualization@lists.linux-foundation.org, linux-mm@kvack.org, yang.zhang.wz@gmail.com, quan.xu@aliyun.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, akpm@linux-foundation.org, mhocko@kernel.org, mgorman@techsingularity.net, liliang.opensource@gmail.com On Fri, Nov 17, 2017 at 07:35:03PM +0800, 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 > > > > Signed-off-by: Liang Li > > > > Cc: Michael S. Tsirkin > > > > Cc: Michal Hocko > > > > --- > > > > > > > > + > > > > +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. -- MST