From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752047AbdLXIOO (ORCPT ); Sun, 24 Dec 2017 03:14:14 -0500 Received: from mga04.intel.com ([192.55.52.120]:60449 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750805AbdLXIOM (ORCPT ); Sun, 24 Dec 2017 03:14:12 -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,450,1508828400"; d="scan'208";a="20899726" Message-ID: <5A3F6254.7070306@intel.com> Date: Sun, 24 Dec 2017 16:16:20 +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.xu0@gmail.com, nilal@redhat.com, riel@redhat.com Subject: Re: [virtio-dev] Re: [PATCH v20 4/7] virtio-balloon: VIRTIO_BALLOON_F_SG References: <1513685879-21823-1-git-send-email-wei.w.wang@intel.com> <1513685879-21823-5-git-send-email-wei.w.wang@intel.com> <20171224032121.GA5273@bombadil.infradead.org> <201712241345.DIG21823.SLFOOJtQFOMVFH@I-love.SAKURA.ne.jp> <5A3F5A4A.1070009@intel.com> In-Reply-To: <5A3F5A4A.1070009@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 12/24/2017 03:42 PM, Wei Wang wrote: > On 12/24/2017 12:45 PM, Tetsuo Handa wrote: >> Matthew Wilcox wrote: >>>> + unsigned long pfn = page_to_pfn(page); >>>> + int ret; >>>> + >>>> + *pfn_min = min(pfn, *pfn_min); >>>> + *pfn_max = max(pfn, *pfn_max); >>>> + >>>> + do { >>>> + if (xb_preload(GFP_NOWAIT | __GFP_NOWARN) < 0) >>>> + return -ENOMEM; >>>> + >>>> + ret = xb_set_bit(&vb->page_xb, pfn); >>>> + xb_preload_end(); >>>> + } while (unlikely(ret == -EAGAIN)); >>> OK, so you don't need a spinlock because you're under a mutex? But you >>> can't allocate memory because you're in the balloon driver, and so a >>> GFP_KERNEL allocation might recurse into your driver? >> Right. We can't (directly or indirectly) depend on >> __GFP_DIRECT_RECLAIM && !__GFP_NORETRY >> allocations because the balloon driver needs to handle OOM notifier >> callback. >> >>> Would GFP_NOIO >>> do the job? I'm a little hazy on exactly how the balloon driver works. >> GFP_NOIO implies __GFP_DIRECT_RECLAIM. In the worst case, it can >> lockup due to >> the too small to fail memory allocation rule. GFP_NOIO | >> __GFP_NORETRY would work >> if there is really a guarantee that GFP_NOIO | __GFP_NORETRY never >> depend on >> __GFP_DIRECT_RECLAIM && !__GFP_NORETRY allocations, which is too >> subtle for me to >> validate. The direct reclaim dependency is too complicated to validate. >> I consider that !__GFP_DIRECT_RECLAIM is the future-safe choice. > > What's the problem with (or how is it better than) the "GFP_NOWAIT | > __GFP_NOWARN" we are using here? > > >>> If you can't preload with anything better than that, I think that >>> xb_set_bit() should attempt an allocation with GFP_NOWAIT | >>> __GFP_NOWARN, >>> and then you can skip the preload; it has no value for you. >> Yes, that's why I suggest directly using kzalloc() at xb_set_bit(). > > It has some possibilities to remove that preload if we also do the > bitmap allocation in the xb_set_bit(): > > bitmap = rcu_dereference_raw(*slot); > if (!bitmap) { > bitmap = this_cpu_xchg(ida_bitmap, NULL); > if (!bitmap) { > bitmap = kmalloc(sizeof(*bitmap), gfp); > if (!bitmap) > return -ENOMEM; > } > } > > But why not just follow the radix tree implementation style that puts > the allocation in preload, which would be invoked with a more relaxed > gfp in other use cases? > Its usage in virtio_balloon is just a little special that we need to > put the allocation within the balloon_lock, which doesn't give us the > benefit of using a relaxed gfp in preload, but it doesn't prevent us > from living with the current APIs (i.e. the preload + xb_set pattern). > On the other side, if we do it as above, we have more things that need > to consider. For example, what if the a use case just want the radix > tree implementation style, which means it doesn't want allocation > within xb_set(), then would we be troubled with how to avoid the > allocation path in that case? > > So, I think it is better to stick with the convention by putting the > allocation in preload. Breaking the convention should show obvious > advantages, IMHO. > > >> >>>> @@ -173,8 +292,15 @@ static unsigned fill_balloon(struct >>>> virtio_balloon *vb, size_t num) >>>> while ((page = balloon_page_pop(&pages))) { >>>> balloon_page_enqueue(&vb->vb_dev_info, page); >>>> + if (use_sg) { >>>> + if (xb_set_page(vb, page, &pfn_min, &pfn_max) < 0) { >>>> + __free_page(page); >>>> + continue; >>>> + } >>>> + } else { >>>> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page); >>>> + } >>> Is this the right behaviour? >> I don't think so. In the worst case, we can set no bit using >> xb_set_page(). > >> >>> If we can't record the page in the xb, >>> wouldn't we rather send it across as a single page? >>> >> I think that we need to be able to fallback to !use_sg path when OOM. > > I also have different thoughts: > > 1) For OOM, we have leak_balloon_sg_oom (oom has nothing to do with > fill_balloon), which does not use xbitmap to record pages, thus no > memory allocation. > > 2) If the memory is already under pressure, it is pointless to > continue inflating memory to the host. We need to give thanks to the > memory allocation failure reported by xbitmap, which gets us a chance > to release the inflated pages that have been demonstrated to cause the > memory pressure of the guest. > Forgot to add my conclusion: I think the above behavior is correct. Best, Wei From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wei Wang Subject: Re: [virtio-dev] Re: [PATCH v20 4/7] virtio-balloon: VIRTIO_BALLOON_F_SG Date: Sun, 24 Dec 2017 16:16:20 +0800 Message-ID: <5A3F6254.7070306@intel.com> References: <1513685879-21823-1-git-send-email-wei.w.wang@intel.com> <1513685879-21823-5-git-send-email-wei.w.wang@intel.com> <20171224032121.GA5273@bombadil.infradead.org> <201712241345.DIG21823.SLFOOJtQFOMVFH@I-love.SAKURA.ne.jp> <5A3F5A4A.1070009@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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.xu0@gmail.com, nilal@redhat.com, riel@redhat.com To: Tetsuo Handa , willy@infradead.org Return-path: In-Reply-To: <5A3F5A4A.1070009@intel.com> Sender: owner-linux-mm@kvack.org List-Id: kvm.vger.kernel.org On 12/24/2017 03:42 PM, Wei Wang wrote: > On 12/24/2017 12:45 PM, Tetsuo Handa wrote: >> Matthew Wilcox wrote: >>>> + unsigned long pfn = page_to_pfn(page); >>>> + int ret; >>>> + >>>> + *pfn_min = min(pfn, *pfn_min); >>>> + *pfn_max = max(pfn, *pfn_max); >>>> + >>>> + do { >>>> + if (xb_preload(GFP_NOWAIT | __GFP_NOWARN) < 0) >>>> + return -ENOMEM; >>>> + >>>> + ret = xb_set_bit(&vb->page_xb, pfn); >>>> + xb_preload_end(); >>>> + } while (unlikely(ret == -EAGAIN)); >>> OK, so you don't need a spinlock because you're under a mutex? But you >>> can't allocate memory because you're in the balloon driver, and so a >>> GFP_KERNEL allocation might recurse into your driver? >> Right. We can't (directly or indirectly) depend on >> __GFP_DIRECT_RECLAIM && !__GFP_NORETRY >> allocations because the balloon driver needs to handle OOM notifier >> callback. >> >>> Would GFP_NOIO >>> do the job? I'm a little hazy on exactly how the balloon driver works. >> GFP_NOIO implies __GFP_DIRECT_RECLAIM. In the worst case, it can >> lockup due to >> the too small to fail memory allocation rule. GFP_NOIO | >> __GFP_NORETRY would work >> if there is really a guarantee that GFP_NOIO | __GFP_NORETRY never >> depend on >> __GFP_DIRECT_RECLAIM && !__GFP_NORETRY allocations, which is too >> subtle for me to >> validate. The direct reclaim dependency is too complicated to validate. >> I consider that !__GFP_DIRECT_RECLAIM is the future-safe choice. > > What's the problem with (or how is it better than) the "GFP_NOWAIT | > __GFP_NOWARN" we are using here? > > >>> If you can't preload with anything better than that, I think that >>> xb_set_bit() should attempt an allocation with GFP_NOWAIT | >>> __GFP_NOWARN, >>> and then you can skip the preload; it has no value for you. >> Yes, that's why I suggest directly using kzalloc() at xb_set_bit(). > > It has some possibilities to remove that preload if we also do the > bitmap allocation in the xb_set_bit(): > > bitmap = rcu_dereference_raw(*slot); > if (!bitmap) { > bitmap = this_cpu_xchg(ida_bitmap, NULL); > if (!bitmap) { > bitmap = kmalloc(sizeof(*bitmap), gfp); > if (!bitmap) > return -ENOMEM; > } > } > > But why not just follow the radix tree implementation style that puts > the allocation in preload, which would be invoked with a more relaxed > gfp in other use cases? > Its usage in virtio_balloon is just a little special that we need to > put the allocation within the balloon_lock, which doesn't give us the > benefit of using a relaxed gfp in preload, but it doesn't prevent us > from living with the current APIs (i.e. the preload + xb_set pattern). > On the other side, if we do it as above, we have more things that need > to consider. For example, what if the a use case just want the radix > tree implementation style, which means it doesn't want allocation > within xb_set(), then would we be troubled with how to avoid the > allocation path in that case? > > So, I think it is better to stick with the convention by putting the > allocation in preload. Breaking the convention should show obvious > advantages, IMHO. > > >> >>>> @@ -173,8 +292,15 @@ static unsigned fill_balloon(struct >>>> virtio_balloon *vb, size_t num) >>>> while ((page = balloon_page_pop(&pages))) { >>>> balloon_page_enqueue(&vb->vb_dev_info, page); >>>> + if (use_sg) { >>>> + if (xb_set_page(vb, page, &pfn_min, &pfn_max) < 0) { >>>> + __free_page(page); >>>> + continue; >>>> + } >>>> + } else { >>>> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page); >>>> + } >>> Is this the right behaviour? >> I don't think so. In the worst case, we can set no bit using >> xb_set_page(). > >> >>> If we can't record the page in the xb, >>> wouldn't we rather send it across as a single page? >>> >> I think that we need to be able to fallback to !use_sg path when OOM. > > I also have different thoughts: > > 1) For OOM, we have leak_balloon_sg_oom (oom has nothing to do with > fill_balloon), which does not use xbitmap to record pages, thus no > memory allocation. > > 2) If the memory is already under pressure, it is pointless to > continue inflating memory to the host. We need to give thanks to the > memory allocation failure reported by xbitmap, which gets us a chance > to release the inflated pages that have been demonstrated to cause the > memory pressure of the guest. > Forgot to add my conclusion: I think the above behavior is correct. Best, Wei -- 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 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35047) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eT1QI-0002PM-EC for qemu-devel@nongnu.org; Sun, 24 Dec 2017 03:14:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eT1QF-0007mY-7d for qemu-devel@nongnu.org; Sun, 24 Dec 2017 03:14:18 -0500 Received: from mga14.intel.com ([192.55.52.115]:57172) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eT1QE-0007kn-U8 for qemu-devel@nongnu.org; Sun, 24 Dec 2017 03:14:15 -0500 Message-ID: <5A3F6254.7070306@intel.com> Date: Sun, 24 Dec 2017 16:16:20 +0800 From: Wei Wang MIME-Version: 1.0 References: <1513685879-21823-1-git-send-email-wei.w.wang@intel.com> <1513685879-21823-5-git-send-email-wei.w.wang@intel.com> <20171224032121.GA5273@bombadil.infradead.org> <201712241345.DIG21823.SLFOOJtQFOMVFH@I-love.SAKURA.ne.jp> <5A3F5A4A.1070009@intel.com> In-Reply-To: <5A3F5A4A.1070009@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [virtio-dev] Re: [PATCH v20 4/7] virtio-balloon: VIRTIO_BALLOON_F_SG List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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.xu0@gmail.com, nilal@redhat.com, riel@redhat.com On 12/24/2017 03:42 PM, Wei Wang wrote: > On 12/24/2017 12:45 PM, Tetsuo Handa wrote: >> Matthew Wilcox wrote: >>>> + unsigned long pfn = page_to_pfn(page); >>>> + int ret; >>>> + >>>> + *pfn_min = min(pfn, *pfn_min); >>>> + *pfn_max = max(pfn, *pfn_max); >>>> + >>>> + do { >>>> + if (xb_preload(GFP_NOWAIT | __GFP_NOWARN) < 0) >>>> + return -ENOMEM; >>>> + >>>> + ret = xb_set_bit(&vb->page_xb, pfn); >>>> + xb_preload_end(); >>>> + } while (unlikely(ret == -EAGAIN)); >>> OK, so you don't need a spinlock because you're under a mutex? But you >>> can't allocate memory because you're in the balloon driver, and so a >>> GFP_KERNEL allocation might recurse into your driver? >> Right. We can't (directly or indirectly) depend on >> __GFP_DIRECT_RECLAIM && !__GFP_NORETRY >> allocations because the balloon driver needs to handle OOM notifier >> callback. >> >>> Would GFP_NOIO >>> do the job? I'm a little hazy on exactly how the balloon driver works. >> GFP_NOIO implies __GFP_DIRECT_RECLAIM. In the worst case, it can >> lockup due to >> the too small to fail memory allocation rule. GFP_NOIO | >> __GFP_NORETRY would work >> if there is really a guarantee that GFP_NOIO | __GFP_NORETRY never >> depend on >> __GFP_DIRECT_RECLAIM && !__GFP_NORETRY allocations, which is too >> subtle for me to >> validate. The direct reclaim dependency is too complicated to validate. >> I consider that !__GFP_DIRECT_RECLAIM is the future-safe choice. > > What's the problem with (or how is it better than) the "GFP_NOWAIT | > __GFP_NOWARN" we are using here? > > >>> If you can't preload with anything better than that, I think that >>> xb_set_bit() should attempt an allocation with GFP_NOWAIT | >>> __GFP_NOWARN, >>> and then you can skip the preload; it has no value for you. >> Yes, that's why I suggest directly using kzalloc() at xb_set_bit(). > > It has some possibilities to remove that preload if we also do the > bitmap allocation in the xb_set_bit(): > > bitmap = rcu_dereference_raw(*slot); > if (!bitmap) { > bitmap = this_cpu_xchg(ida_bitmap, NULL); > if (!bitmap) { > bitmap = kmalloc(sizeof(*bitmap), gfp); > if (!bitmap) > return -ENOMEM; > } > } > > But why not just follow the radix tree implementation style that puts > the allocation in preload, which would be invoked with a more relaxed > gfp in other use cases? > Its usage in virtio_balloon is just a little special that we need to > put the allocation within the balloon_lock, which doesn't give us the > benefit of using a relaxed gfp in preload, but it doesn't prevent us > from living with the current APIs (i.e. the preload + xb_set pattern). > On the other side, if we do it as above, we have more things that need > to consider. For example, what if the a use case just want the radix > tree implementation style, which means it doesn't want allocation > within xb_set(), then would we be troubled with how to avoid the > allocation path in that case? > > So, I think it is better to stick with the convention by putting the > allocation in preload. Breaking the convention should show obvious > advantages, IMHO. > > >> >>>> @@ -173,8 +292,15 @@ static unsigned fill_balloon(struct >>>> virtio_balloon *vb, size_t num) >>>> while ((page = balloon_page_pop(&pages))) { >>>> balloon_page_enqueue(&vb->vb_dev_info, page); >>>> + if (use_sg) { >>>> + if (xb_set_page(vb, page, &pfn_min, &pfn_max) < 0) { >>>> + __free_page(page); >>>> + continue; >>>> + } >>>> + } else { >>>> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page); >>>> + } >>> Is this the right behaviour? >> I don't think so. In the worst case, we can set no bit using >> xb_set_page(). > >> >>> If we can't record the page in the xb, >>> wouldn't we rather send it across as a single page? >>> >> I think that we need to be able to fallback to !use_sg path when OOM. > > I also have different thoughts: > > 1) For OOM, we have leak_balloon_sg_oom (oom has nothing to do with > fill_balloon), which does not use xbitmap to record pages, thus no > memory allocation. > > 2) If the memory is already under pressure, it is pointless to > continue inflating memory to the host. We need to give thanks to the > memory allocation failure reported by xbitmap, which gets us a chance > to release the inflated pages that have been demonstrated to cause the > memory pressure of the guest. > Forgot to add my conclusion: I think the above behavior is correct. Best, Wei From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: virtio-dev-return-2886-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 E3EFD5818F92 for ; Sun, 24 Dec 2017 00:14:23 -0800 (PST) Message-ID: <5A3F6254.7070306@intel.com> Date: Sun, 24 Dec 2017 16:16:20 +0800 From: Wei Wang MIME-Version: 1.0 References: <1513685879-21823-1-git-send-email-wei.w.wang@intel.com> <1513685879-21823-5-git-send-email-wei.w.wang@intel.com> <20171224032121.GA5273@bombadil.infradead.org> <201712241345.DIG21823.SLFOOJtQFOMVFH@I-love.SAKURA.ne.jp> <5A3F5A4A.1070009@intel.com> In-Reply-To: <5A3F5A4A.1070009@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [virtio-dev] Re: [PATCH v20 4/7] virtio-balloon: VIRTIO_BALLOON_F_SG 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.xu0@gmail.com, nilal@redhat.com, riel@redhat.com List-ID: On 12/24/2017 03:42 PM, Wei Wang wrote: > On 12/24/2017 12:45 PM, Tetsuo Handa wrote: >> Matthew Wilcox wrote: >>>> + unsigned long pfn = page_to_pfn(page); >>>> + int ret; >>>> + >>>> + *pfn_min = min(pfn, *pfn_min); >>>> + *pfn_max = max(pfn, *pfn_max); >>>> + >>>> + do { >>>> + if (xb_preload(GFP_NOWAIT | __GFP_NOWARN) < 0) >>>> + return -ENOMEM; >>>> + >>>> + ret = xb_set_bit(&vb->page_xb, pfn); >>>> + xb_preload_end(); >>>> + } while (unlikely(ret == -EAGAIN)); >>> OK, so you don't need a spinlock because you're under a mutex? But you >>> can't allocate memory because you're in the balloon driver, and so a >>> GFP_KERNEL allocation might recurse into your driver? >> Right. We can't (directly or indirectly) depend on >> __GFP_DIRECT_RECLAIM && !__GFP_NORETRY >> allocations because the balloon driver needs to handle OOM notifier >> callback. >> >>> Would GFP_NOIO >>> do the job? I'm a little hazy on exactly how the balloon driver works. >> GFP_NOIO implies __GFP_DIRECT_RECLAIM. In the worst case, it can >> lockup due to >> the too small to fail memory allocation rule. GFP_NOIO | >> __GFP_NORETRY would work >> if there is really a guarantee that GFP_NOIO | __GFP_NORETRY never >> depend on >> __GFP_DIRECT_RECLAIM && !__GFP_NORETRY allocations, which is too >> subtle for me to >> validate. The direct reclaim dependency is too complicated to validate. >> I consider that !__GFP_DIRECT_RECLAIM is the future-safe choice. > > What's the problem with (or how is it better than) the "GFP_NOWAIT | > __GFP_NOWARN" we are using here? > > >>> If you can't preload with anything better than that, I think that >>> xb_set_bit() should attempt an allocation with GFP_NOWAIT | >>> __GFP_NOWARN, >>> and then you can skip the preload; it has no value for you. >> Yes, that's why I suggest directly using kzalloc() at xb_set_bit(). > > It has some possibilities to remove that preload if we also do the > bitmap allocation in the xb_set_bit(): > > bitmap = rcu_dereference_raw(*slot); > if (!bitmap) { > bitmap = this_cpu_xchg(ida_bitmap, NULL); > if (!bitmap) { > bitmap = kmalloc(sizeof(*bitmap), gfp); > if (!bitmap) > return -ENOMEM; > } > } > > But why not just follow the radix tree implementation style that puts > the allocation in preload, which would be invoked with a more relaxed > gfp in other use cases? > Its usage in virtio_balloon is just a little special that we need to > put the allocation within the balloon_lock, which doesn't give us the > benefit of using a relaxed gfp in preload, but it doesn't prevent us > from living with the current APIs (i.e. the preload + xb_set pattern). > On the other side, if we do it as above, we have more things that need > to consider. For example, what if the a use case just want the radix > tree implementation style, which means it doesn't want allocation > within xb_set(), then would we be troubled with how to avoid the > allocation path in that case? > > So, I think it is better to stick with the convention by putting the > allocation in preload. Breaking the convention should show obvious > advantages, IMHO. > > >> >>>> @@ -173,8 +292,15 @@ static unsigned fill_balloon(struct >>>> virtio_balloon *vb, size_t num) >>>> while ((page = balloon_page_pop(&pages))) { >>>> balloon_page_enqueue(&vb->vb_dev_info, page); >>>> + if (use_sg) { >>>> + if (xb_set_page(vb, page, &pfn_min, &pfn_max) < 0) { >>>> + __free_page(page); >>>> + continue; >>>> + } >>>> + } else { >>>> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page); >>>> + } >>> Is this the right behaviour? >> I don't think so. In the worst case, we can set no bit using >> xb_set_page(). > >> >>> If we can't record the page in the xb, >>> wouldn't we rather send it across as a single page? >>> >> I think that we need to be able to fallback to !use_sg path when OOM. > > I also have different thoughts: > > 1) For OOM, we have leak_balloon_sg_oom (oom has nothing to do with > fill_balloon), which does not use xbitmap to record pages, thus no > memory allocation. > > 2) If the memory is already under pressure, it is pointless to > continue inflating memory to the host. We need to give thanks to the > memory allocation failure reported by xbitmap, which gets us a chance > to release the inflated pages that have been demonstrated to cause the > memory pressure of the guest. > Forgot to add my conclusion: I think the above behavior is correct. Best, Wei --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org