From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757154AbdLXEpR (ORCPT ); Sat, 23 Dec 2017 23:45:17 -0500 Received: from www262.sakura.ne.jp ([202.181.97.72]:32033 "EHLO www262.sakura.ne.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752720AbdLXEpO (ORCPT ); Sat, 23 Dec 2017 23:45:14 -0500 To: willy@infradead.org, wei.w.wang@intel.com 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: [PATCH v20 4/7] virtio-balloon: VIRTIO_BALLOON_F_SG From: Tetsuo Handa 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> In-Reply-To: <20171224032121.GA5273@bombadil.infradead.org> Message-Id: <201712241345.DIG21823.SLFOOJtQFOMVFH@I-love.SAKURA.ne.jp> X-Mailer: Winbiff [Version 2.51 PL2] X-Accept-Language: ja,en,zh Date: Sun, 24 Dec 2017 13:45:10 +0900 Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > > 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(). > > > @@ -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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f69.google.com (mail-it0-f69.google.com [209.85.214.69]) by kanga.kvack.org (Postfix) with ESMTP id 096DF6B0033 for ; Sat, 23 Dec 2017 23:45:53 -0500 (EST) Received: by mail-it0-f69.google.com with SMTP id r6so14578732itr.1 for ; Sat, 23 Dec 2017 20:45:53 -0800 (PST) Received: from www262.sakura.ne.jp (www262.sakura.ne.jp. [2001:e42:101:1:202:181:97:72]) by mx.google.com with ESMTPS id r96si10857680ioi.90.2017.12.23.20.45.51 for (version=TLS1 cipher=AES128-SHA bits=128/128); Sat, 23 Dec 2017 20:45:51 -0800 (PST) Subject: Re: [PATCH v20 4/7] virtio-balloon: VIRTIO_BALLOON_F_SG From: Tetsuo Handa 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> In-Reply-To: <20171224032121.GA5273@bombadil.infradead.org> Message-Id: <201712241345.DIG21823.SLFOOJtQFOMVFH@I-love.SAKURA.ne.jp> Date: Sun, 24 Dec 2017 13:45:10 +0900 Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Sender: owner-linux-mm@kvack.org List-ID: To: willy@infradead.org, wei.w.wang@intel.com 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 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. > > 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(). > > > @@ -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. -- 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]:45373) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eSyCb-0004J0-4C for qemu-devel@nongnu.org; Sat, 23 Dec 2017 23:47:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eSyCY-0006yd-87 for qemu-devel@nongnu.org; Sat, 23 Dec 2017 23:47:57 -0500 Received: from www262.sakura.ne.jp ([2001:e42:101:1:202:181:97:72]:61170) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eSyCX-00051s-K0 for qemu-devel@nongnu.org; Sat, 23 Dec 2017 23:47:54 -0500 From: Tetsuo Handa 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> In-Reply-To: <20171224032121.GA5273@bombadil.infradead.org> Message-Id: <201712241345.DIG21823.SLFOOJtQFOMVFH@I-love.SAKURA.ne.jp> Date: Sun, 24 Dec 2017 13:45:10 +0900 Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH v20 4/7] virtio-balloon: VIRTIO_BALLOON_F_SG List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: willy@infradead.org, wei.w.wang@intel.com 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 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. > > 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(). > > > @@ -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.