From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751350AbdLQFWY (ORCPT ); Sun, 17 Dec 2017 00:22:24 -0500 Received: from mga18.intel.com ([134.134.136.126]:40367 "EHLO mga18.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750789AbdLQFWW (ORCPT ); Sun, 17 Dec 2017 00:22:22 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,414,1508828400"; d="scan'208";a="12481926" Message-ID: <5A35FF89.8040500@intel.com> Date: Sun, 17 Dec 2017 13:24:25 +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: Tetsuo Handa , willy@infradead.org CC: virtio-dev@lists.oasis-open.org, linux-kernel@vger.kernel.org, qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, linux-mm@kvack.org, mst@redhat.com, mhocko@kernel.org, akpm@linux-foundation.org, mawilcox@microsoft.com, david@redhat.com, cornelia.huck@de.ibm.com, mgorman@techsingularity.net, aarcange@redhat.com, amit.shah@redhat.com, pbonzini@redhat.com, liliang.opensource@gmail.com, yang.zhang.wz@gmail.com, quan.xu@aliyun.com, nilal@redhat.com, riel@redhat.com Subject: Re: [PATCH v19 3/7] xbitmap: add more operations References: <1513079759-14169-1-git-send-email-wei.w.wang@intel.com> <1513079759-14169-4-git-send-email-wei.w.wang@intel.com> <20171215184256.GA27160@bombadil.infradead.org> <5A34F193.5040700@intel.com> <201712162028.FEB87079.FOJFMQHVOSLtFO@I-love.SAKURA.ne.jp> In-Reply-To: <201712162028.FEB87079.FOJFMQHVOSLtFO@I-love.SAKURA.ne.jp> 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 12/16/2017 07:28 PM, Tetsuo Handa wrote: > Wei Wang wrote: >> On 12/16/2017 02:42 AM, Matthew Wilcox wrote: >>> On Tue, Dec 12, 2017 at 07:55:55PM +0800, Wei Wang wrote: >>>> +int xb_preload_and_set_bit(struct xb *xb, unsigned long bit, gfp_t gfp); >>> I'm struggling to understand when one would use this. The xb_ API >>> requires you to handle your own locking. But specifying GFP flags >>> here implies you can sleep. So ... um ... there's no locking? >> In the regular use cases, people would do xb_preload() before taking the >> lock, and the xb_set/clear within the lock. >> >> In the virtio-balloon usage, we have a large number of bits to set with >> the balloon_lock being held (we're not unlocking for each bit), so we >> used the above wrapper to do preload and set within the balloon_lock, >> and passed in GFP_NOWAIT to avoid sleeping. Probably we can change to >> put this wrapper implementation to virtio-balloon, since it would not be >> useful for the regular cases. > GFP_NOWAIT is chosen in order not to try to OOM-kill something, isn't it? Yes, I think that's right the issue we are discussing here (also discussed in the deadlock patch before): Suppose we use a sleep-able flag GFP_KERNEL, which gets the caller (fill_balloon or leak_balloon) into sleep with balloon_lock being held, and the memory reclaiming from GFP_KERNEL would fall into the OOM code path which first invokes the oom_notify-->leak_balloon to release some balloon memory, which needs to take the balloon_lock that is being held by the task who is sleeping. So, using GFP_NOWAIT avoids sleeping to get memory through directly memory reclaiming, which could fall into that OOM code path that needs to take the balloon_lock. > But passing GFP_NOWAIT means that we can handle allocation failure. There is > no need to use preload approach when we can handle allocation failure. I think the reason we need xb_preload is because radix tree insertion needs the memory being preallocated already (it couldn't suffer from memory failure during the process of inserting, probably because handling the failure there isn't easy, Matthew may know the backstory of this) So, I think we can handle the memory failure with xb_preload, which stops going into the radix tree APIs, but shouldn't call radix tree APIs without the related memory preallocated. Best, Wei