From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754901AbdKQLq1 (ORCPT ); Fri, 17 Nov 2017 06:46:27 -0500 Received: from mga07.intel.com ([134.134.136.100]:44700 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751060AbdKQLqU (ORCPT ); Fri, 17 Nov 2017 06:46:20 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.44,408,1505804400"; d="scan'208";a="8741809" Message-ID: <5A0ECC95.7070300@intel.com> Date: Fri, 17 Nov 2017 19:48:37 +0800 From: Wei Wang User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: "Michael S. Tsirkin" 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 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> In-Reply-To: <5A0EC967.5090407@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>> 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); logic should be inverse: if (!(page_poisoning_enabled() && !IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY))) Best, Wei