* (unknown),
From: Solen win2 @ 2017-10-02 18:00 UTC (permalink / raw)
To: virtualization
[-- Attachment #1.1: Type: text/plain, Size: 23 bytes --]
Solenwin@freshdesk.com
[-- Attachment #1.2: Type: text/html, Size: 141 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* RE: [PATCH v16 5/5] virtio-balloon: VIRTIO_BALLOON_F_CTRL_VQ
From: Wang, Wei W @ 2017-10-02 16:38 UTC (permalink / raw)
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,
liliang.opensource@gmail.com, 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
In-Reply-To: <20171001060305-mutt-send-email-mst@kernel.org>
On Sunday, October 1, 2017 11:19 AM, Michael S. Tsirkin wrote:
> On Sat, Sep 30, 2017 at 12:05:54PM +0800, Wei Wang wrote:
> > +static void ctrlq_send_cmd(struct virtio_balloon *vb,
> > + struct virtio_balloon_ctrlq_cmd *cmd,
> > + bool inbuf)
> > +{
> > + struct virtqueue *vq = vb->ctrl_vq;
> > +
> > + ctrlq_add_cmd(vq, cmd, inbuf);
> > + if (!inbuf) {
> > + /*
> > + * All the input cmd buffers are replenished here.
> > + * This is necessary because the input cmd buffers are lost
> > + * after live migration. The device needs to rewind all of
> > + * them from the ctrl_vq.
>
> Confused. Live migration somehow loses state? Why is that and why is it a good
> idea? And how do you know this is migration even?
> Looks like all you know is you got free page end. Could be any reason for this.
I think this would be something that the current live migration lacks - what the
device read from the vq is not transferred during live migration, an example is the
stat_vq_elem:
Line 476 at https://github.com/qemu/qemu/blob/master/hw/virtio/virtio-balloon.c
For all the things that are added to the vq and need to be held by the device
to use later need to consider the situation that live migration might happen at any
time and they need to be re-taken from the vq by the device on the destination
machine.
So, even without this live migration optimization feature, I think all the things that are
added to the vq for the device to hold, need a way for the device to rewind back from
the vq - re-adding all the elements to the vq is a trick to keep a record of all of them
on the vq so that the device side rewinding can work.
Please let me know if anything is missed or if you have other suggestions.
> > +static void ctrlq_handle(struct virtqueue *vq) {
> > + struct virtio_balloon *vb = vq->vdev->priv;
> > + struct virtio_balloon_ctrlq_cmd *msg;
> > + unsigned int class, cmd, len;
> > +
> > + msg = (struct virtio_balloon_ctrlq_cmd *)virtqueue_get_buf(vq, &len);
> > + if (unlikely(!msg))
> > + return;
> > +
> > + /* The outbuf is sent by the host for recycling, so just return. */
> > + if (msg == &vb->free_page_cmd_out)
> > + return;
> > +
> > + class = virtio32_to_cpu(vb->vdev, msg->class);
> > + cmd = virtio32_to_cpu(vb->vdev, msg->cmd);
> > +
> > + switch (class) {
> > + case VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE:
> > + if (cmd == VIRTIO_BALLOON_FREE_PAGE_F_STOP) {
> > + vb->report_free_page_stop = true;
> > + } else if (cmd == VIRTIO_BALLOON_FREE_PAGE_F_START) {
> > + vb->report_free_page_stop = false;
> > + queue_work(vb->balloon_wq, &vb-
> >report_free_page_work);
> > + }
> > + vb->free_page_cmd_in.class =
> > +
> VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE;
> > + ctrlq_send_cmd(vb, &vb->free_page_cmd_in, true);
> > + break;
> > + default:
> > + dev_warn(&vb->vdev->dev, "%s: cmd class not supported\n",
> > + __func__);
> > + }
>
> Manipulating report_free_page_stop without any locks looks very suspicious.
> Also, what if we get two start commands? we should restart from beginning,
> should we not?
>
Yes, it will start to report free pages from the beginning.
walk_free_mem_block() doesn't maintain any internal status, so the invoking of
it will always start from the beginning.
> > +/* Ctrlq commands related to VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE
> */
> > +#define VIRTIO_BALLOON_FREE_PAGE_F_STOP 0
> > +#define VIRTIO_BALLOON_FREE_PAGE_F_START 1
> > +
> > #endif /* _LINUX_VIRTIO_BALLOON_H */
>
> The stop command does not appear to be thought through.
>
> Let's assume e.g. you started migration. You ask guest for free pages.
> Then you cancel it. There are a bunch of pages in free vq and you are getting
> more. You now want to start migration again. What to do?
>
> A bunch of vq flushing and waiting will maybe do the trick, but waiting on guest
> is never a great idea.
>
I think the device can flush (pop out what's left in the vq and push them back) the
vq right after the Stop command is sent to the guest, rather than doing the flush
when the 2nd initiation of live migration begins. The entries pushed back to the vq
will be in the used ring, what would the device need to wait for?
> I previously suggested pushing the stop/start commands from guest to host on
> the free page vq, and including an ID in host to guest and guest to host
> commands. This way ctrl vq is just for host to guest commands, and host
> matches commands and knows which command is a free page in response to.
>
> I still think it's a good idea but go ahead and propose something else that works.
>
Thanks for the suggestion. Probably I haven't fully understood it. Please see the example
below:
1) host-to-guest ctrl_vq:
StartCMD, ID=1
2) guest-to-host free_page_vq:
free_page, ID=1
free_page, ID=1
free_page, ID=1
free_page, ID=1
3) host-to-guest ctrl_vq:
StopCMD, ID=1
4) initiate the 2nd try of live migration via host-to-guest ctrl_vq:
StartCMD, ID=2
5) the guest-to-host free_page_vq might look like this:
free_page, ID=1
free_page, ID=1
free_page, ID=2
free_page, ID=2
The device will need to drop (pop out the two entries and push them back)
the first 2 obsolete free pages which are sent by ID=1.
I haven't found the benefits above yet. The device will perform the same operations
to get rid of the old free pages. If we drop the old free pages after the StopCMD (
ID may also not be needed in this case), the overhead won't be added to the live
migration time.
Would you have any thought about this?
Best,
Wei
^ permalink raw reply
* Re: [RFC] [PATCH] mm,oom: Offload OOM notify callback to a kernel thread.
From: Michal Hocko @ 2017-10-02 14:31 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mm, Tetsuo Handa, joonas.lahtinen, jiangshanlai, josh,
jani.nikula, virtualization, airlied, mathieu.desnoyers, rostedt,
rodrigo.vivi, paulmck, intel-gfx
In-Reply-To: <20171002172349-mutt-send-email-mst@kernel.org>
On Mon 02-10-17 17:29:47, Michael S. Tsirkin wrote:
> On Mon, Oct 02, 2017 at 04:19:00PM +0200, Michal Hocko wrote:
> > On Mon 02-10-17 17:11:55, Michael S. Tsirkin wrote:
> > > On Mon, Oct 02, 2017 at 01:50:35PM +0200, Michal Hocko wrote:
> > [...]
> > > > and some
> > > > other call path is allocating while holding the lock. But you seem to be
> > > > right and
> > > > leak_balloon
> > > > tell_host
> > > > virtqueue_add_outbuf
> > > > virtqueue_add
> > > >
> > > > can do GFP_KERNEL allocation and this is clearly wrong. Nobody should
> > > > try to allocate while we are in the OOM path. Michael, is there any way
> > > > to drop this?
> > >
> > > Yes - in practice it won't ever allocate - that path is never taken
> > > with add_outbuf - it is for add_sgs only.
> > >
> > > IMHO the issue is balloon inflation which needs to allocate
> > > memory. It does it under a mutex, and oom handler tries to take the
> > > same mutex.
> >
> > try_lock for the oom notifier path should heal the problem then, righ?
> > At least for as a quick fix.
>
> IMHO it definitely fixes the deadlock. But it does not fix the bug
> that balloon isn't sometimes deflated on oom even though the deflate on
> oom flag is set.
Yes, that would require a more sophisticated fix. And I would argue that
would require to drop oom handler and move deflate logic to a shrinker
to better scale with the memory pressure rather than act on the very
last moment.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC] [PATCH] mm,oom: Offload OOM notify callback to a kernel thread.
From: Michael S. Tsirkin @ 2017-10-02 14:29 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Tetsuo Handa, joonas.lahtinen, jiangshanlai, josh,
jani.nikula, virtualization, airlied, mathieu.desnoyers, rostedt,
rodrigo.vivi, paulmck, intel-gfx
In-Reply-To: <20171002141900.acmcbilwhqethfhq@dhcp22.suse.cz>
On Mon, Oct 02, 2017 at 04:19:00PM +0200, Michal Hocko wrote:
> On Mon 02-10-17 17:11:55, Michael S. Tsirkin wrote:
> > On Mon, Oct 02, 2017 at 01:50:35PM +0200, Michal Hocko wrote:
> [...]
> > > and some
> > > other call path is allocating while holding the lock. But you seem to be
> > > right and
> > > leak_balloon
> > > tell_host
> > > virtqueue_add_outbuf
> > > virtqueue_add
> > >
> > > can do GFP_KERNEL allocation and this is clearly wrong. Nobody should
> > > try to allocate while we are in the OOM path. Michael, is there any way
> > > to drop this?
> >
> > Yes - in practice it won't ever allocate - that path is never taken
> > with add_outbuf - it is for add_sgs only.
> >
> > IMHO the issue is balloon inflation which needs to allocate
> > memory. It does it under a mutex, and oom handler tries to take the
> > same mutex.
>
> try_lock for the oom notifier path should heal the problem then, righ?
> At least for as a quick fix.
IMHO it definitely fixes the deadlock. But it does not fix the bug
that balloon isn't sometimes deflated on oom even though the deflate on
oom flag is set.
> --
> Michal Hocko
> SUSE Labs
^ permalink raw reply
* Re: [PATCH v3 0/2] guard virt_spin_lock() with a static key
From: Peter Zijlstra @ 2017-10-02 14:19 UTC (permalink / raw)
To: Juergen Gross
Cc: jeremy, rusty, x86, linux-kernel, virtualization, chrisw, mingo,
tglx, longman, hpa, xen-devel, akataria, boris.ostrovsky
In-Reply-To: <20170906173625.18158-1-jgross@suse.com>
On Wed, Sep 06, 2017 at 07:36:23PM +0200, Juergen Gross wrote:
> With virt_spin_lock() being guarded by a static key the bare metal case
> can be optimized by patching the call away completely. In case a kernel
> running as a guest it can decide whether to use paravitualized
> spinlocks, the current fallback to the unfair test-and-set scheme, or
> to mimic the bare metal behavior.
>
> V3:
> - remove test for hypervisor environment from virt_spin_lock(9 as
> suggested by Waiman Long
>
> V2:
> - use static key instead of making virt_spin_lock() a pvops function
>
> Juergen Gross (2):
> paravirt/locks: use new static key for controlling call of
> virt_spin_lock()
> paravirt,xen: correct xen_nopvspin case
>
> arch/x86/include/asm/qspinlock.h | 11 ++++++++++-
> arch/x86/kernel/paravirt-spinlocks.c | 6 ++++++
> arch/x86/kernel/smpboot.c | 2 ++
> arch/x86/xen/spinlock.c | 2 ++
> kernel/locking/qspinlock.c | 4 ++++
> 5 files changed, 24 insertions(+), 1 deletion(-)
Sorry for the delay, picked it up now.
^ permalink raw reply
* Re: [RFC] [PATCH] mm,oom: Offload OOM notify callback to a kernel thread.
From: Michal Hocko @ 2017-10-02 14:19 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mm, Tetsuo Handa, joonas.lahtinen, jiangshanlai, josh,
jani.nikula, virtualization, airlied, mathieu.desnoyers, rostedt,
rodrigo.vivi, paulmck, intel-gfx
In-Reply-To: <20171002170642-mutt-send-email-mst@kernel.org>
On Mon 02-10-17 17:11:55, Michael S. Tsirkin wrote:
> On Mon, Oct 02, 2017 at 01:50:35PM +0200, Michal Hocko wrote:
[...]
> > and some
> > other call path is allocating while holding the lock. But you seem to be
> > right and
> > leak_balloon
> > tell_host
> > virtqueue_add_outbuf
> > virtqueue_add
> >
> > can do GFP_KERNEL allocation and this is clearly wrong. Nobody should
> > try to allocate while we are in the OOM path. Michael, is there any way
> > to drop this?
>
> Yes - in practice it won't ever allocate - that path is never taken
> with add_outbuf - it is for add_sgs only.
>
> IMHO the issue is balloon inflation which needs to allocate
> memory. It does it under a mutex, and oom handler tries to take the
> same mutex.
try_lock for the oom notifier path should heal the problem then, righ?
At least for as a quick fix.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC] [PATCH] mm,oom: Offload OOM notify callback to a kernel thread.
From: Michael S. Tsirkin @ 2017-10-02 14:11 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Tetsuo Handa, joonas.lahtinen, jiangshanlai, josh,
jani.nikula, virtualization, airlied, mathieu.desnoyers, rostedt,
rodrigo.vivi, paulmck, intel-gfx
In-Reply-To: <20171002115035.7sph6ul6hsszdwa4@dhcp22.suse.cz>
On Mon, Oct 02, 2017 at 01:50:35PM +0200, Michal Hocko wrote:
> On Mon 02-10-17 20:33:52, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > [Hmm, I do not see the original patch which this has been a reply to]
> >
> > urbl.hostedemail.com and b.barracudacentral.org blocked my IP address,
> > and the rest are "Recipient address rejected: Greylisted" or
> > "Deferred: 451-4.3.0 Multiple destination domains per transaction is unsupported.",
> > and after all dropped at the servers. Sad...
> >
> > >
> > > On Mon 02-10-17 06:59:12, Michael S. Tsirkin wrote:
> > > > On Sun, Oct 01, 2017 at 02:44:34PM +0900, Tetsuo Handa wrote:
> > > > > Tetsuo Handa wrote:
> > > > > > Michael S. Tsirkin wrote:
> > > > > > > On Mon, Sep 11, 2017 at 07:27:19PM +0900, Tetsuo Handa wrote:
> > > > > > > > Hello.
> > > > > > > >
> > > > > > > > I noticed that virtio_balloon is using register_oom_notifier() and
> > > > > > > > leak_balloon() from virtballoon_oom_notify() might depend on
> > > > > > > > __GFP_DIRECT_RECLAIM memory allocation.
> > > > > > > >
> > > > > > > > In leak_balloon(), mutex_lock(&vb->balloon_lock) is called in order to
> > > > > > > > serialize against fill_balloon(). But in fill_balloon(),
> > > > > > > > alloc_page(GFP_HIGHUSER[_MOVABLE] | __GFP_NOMEMALLOC | __GFP_NORETRY) is
> > > > > > > > called with vb->balloon_lock mutex held. Since GFP_HIGHUSER[_MOVABLE] implies
> > > > > > > > __GFP_DIRECT_RECLAIM | __GFP_IO | __GFP_FS, this allocation attempt might
> > > > > > > > depend on somebody else's __GFP_DIRECT_RECLAIM | !__GFP_NORETRY memory
> > > > > > > > allocation. Such __GFP_DIRECT_RECLAIM | !__GFP_NORETRY allocation can reach
> > > > > > > > __alloc_pages_may_oom() and hold oom_lock mutex and call out_of_memory().
> > > > > > > > And leak_balloon() is called by virtballoon_oom_notify() via
> > > > > > > > blocking_notifier_call_chain() callback when vb->balloon_lock mutex is already
> > > > > > > > held by fill_balloon(). As a result, despite __GFP_NORETRY is specified,
> > > > > > > > fill_balloon() can indirectly get stuck waiting for vb->balloon_lock mutex
> > > > > > > > at leak_balloon().
> > >
> > > This is really nasty! And I would argue that this is an abuse of the oom
> > > notifier interface from the virtio code. OOM notifiers are an ugly hack
> > > on its own but all its users have to be really careful to not depend on
> > > any allocation request because that is a straight deadlock situation.
> >
> > Please describe such warning at
> > "int register_oom_notifier(struct notifier_block *nb)" definition.
>
> Yes, we can and should do that. Although I would prefer to simply
> document this API as deprecated. Care to send a patch? I am quite busy
> with other stuff.
>
> > > I do not think that making oom notifier API more complex is the way to
> > > go. Can we simply change the lock to try_lock?
> >
> > Using mutex_trylock(&vb->balloon_lock) alone is not sufficient. Inside the
> > mutex, __GFP_DIRECT_RECLAIM && !__GFP_NORETRY allocation attempt is used
> > which will fail to make progress due to oom_lock already held. Therefore,
> > virtballoon_oom_notify() needs to guarantee that all allocation attempts use
> > GFP_NOWAIT when called from virtballoon_oom_notify().
>
> Ohh, I missed your point and thought the dependency is indirect
I do think this is the case. See below.
> and some
> other call path is allocating while holding the lock. But you seem to be
> right and
> leak_balloon
> tell_host
> virtqueue_add_outbuf
> virtqueue_add
>
> can do GFP_KERNEL allocation and this is clearly wrong. Nobody should
> try to allocate while we are in the OOM path. Michael, is there any way
> to drop this?
Yes - in practice it won't ever allocate - that path is never taken
with add_outbuf - it is for add_sgs only.
IMHO the issue is balloon inflation which needs to allocate
memory. It does it under a mutex, and oom handler tries to take the
same mutex.
> --
> Michal Hocko
> SUSE Labs
^ permalink raw reply
* Re: [PATCH v16 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Michael S. Tsirkin @ 2017-10-02 13:44 UTC (permalink / raw)
To: Wang, Wei W
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,
liliang.opensource@gmail.com, 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
In-Reply-To: <286AC319A985734F985F78AFA26841F73931FDB5@shsmsx102.ccr.corp.intel.com>
On Mon, Oct 02, 2017 at 12:39:30PM +0000, Wang, Wei W wrote:
> On Monday, October 2, 2017 12:30 PM, Michael S. Tsirkin wrote:
> > On Sat, Sep 30, 2017 at 12:05:52PM +0800, Wei Wang wrote:
> > > +static int send_balloon_page_sg(struct virtio_balloon *vb,
> > > + struct virtqueue *vq,
> > > + void *addr,
> > > + uint32_t size,
> > > + bool batch)
> > > +{
> > > + int err;
> > > +
> > > + err = add_one_sg(vq, addr, size);
> > > +
> > > + /* If batchng is requested, we batch till the vq is full */
> >
> > typo
> >
> > > + if (!batch || !vq->num_free)
> > > + kick_and_wait(vq, vb->acked);
> > > +
> > > + return err;
> > > +}
> >
> > If add_one_sg fails, kick_and_wait will hang forever.
> >
> > The reason this might work in because
> > 1. with 1 sg there are no memory allocations 2. if adding fails on vq full, then
> > something
> > is in queue and will wake up kick_and_wait.
> >
> > So in short this is expected to never fail.
> > How about a BUG_ON here then?
> > And make it void, and add a comment with above explanation.
> >
>
>
> Yes, agree that this wouldn't fail - the worker thread performing the ballooning operations has been put into sleep when the vq is full, so I think there shouldn't be anyone else to put more sgs onto the vq then.
> Btw, not sure if we need to mention memory allocation in the comment, I found virtqueue_add() doesn't return any error when allocation (for indirect desc-s) fails - it simply avoids the use of indirect desc.
>
> What do you think of the following?
>
> err = add_one_sg(vq, addr, size);
> /*
> * This is expected to never fail: there is always at least 1 entry available on the vq,
> * because when the vq is full the worker thread that adds the sg will be put into
> * sleep until at least 1 entry is available to use.
> */
> BUG_ON(err);
>
> Best,
> Wei
>
>
>
>
Sounds good.
^ permalink raw reply
* RE: [PATCH v16 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Wang, Wei W @ 2017-10-02 12:39 UTC (permalink / raw)
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,
liliang.opensource@gmail.com, 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
In-Reply-To: <20171002072106-mutt-send-email-mst@kernel.org>
On Monday, October 2, 2017 12:30 PM, Michael S. Tsirkin wrote:
> On Sat, Sep 30, 2017 at 12:05:52PM +0800, Wei Wang wrote:
> > +static int send_balloon_page_sg(struct virtio_balloon *vb,
> > + struct virtqueue *vq,
> > + void *addr,
> > + uint32_t size,
> > + bool batch)
> > +{
> > + int err;
> > +
> > + err = add_one_sg(vq, addr, size);
> > +
> > + /* If batchng is requested, we batch till the vq is full */
>
> typo
>
> > + if (!batch || !vq->num_free)
> > + kick_and_wait(vq, vb->acked);
> > +
> > + return err;
> > +}
>
> If add_one_sg fails, kick_and_wait will hang forever.
>
> The reason this might work in because
> 1. with 1 sg there are no memory allocations 2. if adding fails on vq full, then
> something
> is in queue and will wake up kick_and_wait.
>
> So in short this is expected to never fail.
> How about a BUG_ON here then?
> And make it void, and add a comment with above explanation.
>
Yes, agree that this wouldn't fail - the worker thread performing the ballooning operations has been put into sleep when the vq is full, so I think there shouldn't be anyone else to put more sgs onto the vq then.
Btw, not sure if we need to mention memory allocation in the comment, I found virtqueue_add() doesn't return any error when allocation (for indirect desc-s) fails - it simply avoids the use of indirect desc.
What do you think of the following?
err = add_one_sg(vq, addr, size);
/*
* This is expected to never fail: there is always at least 1 entry available on the vq,
* because when the vq is full the worker thread that adds the sg will be put into
* sleep until at least 1 entry is available to use.
*/
BUG_ON(err);
Best,
Wei
^ permalink raw reply
* Re: [RFC] [PATCH] mm,oom: Offload OOM notify callback to a kernel thread.
From: Michal Hocko @ 2017-10-02 11:50 UTC (permalink / raw)
To: Tetsuo Handa
Cc: mst, airlied, joonas.lahtinen, jiangshanlai, josh, jani.nikula,
virtualization, linux-mm, mathieu.desnoyers, rostedt,
rodrigo.vivi, paulmck, intel-gfx
In-Reply-To: <201710022033.GFE82801.HLOVOFFJtSFQMO@I-love.SAKURA.ne.jp>
On Mon 02-10-17 20:33:52, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > [Hmm, I do not see the original patch which this has been a reply to]
>
> urbl.hostedemail.com and b.barracudacentral.org blocked my IP address,
> and the rest are "Recipient address rejected: Greylisted" or
> "Deferred: 451-4.3.0 Multiple destination domains per transaction is unsupported.",
> and after all dropped at the servers. Sad...
>
> >
> > On Mon 02-10-17 06:59:12, Michael S. Tsirkin wrote:
> > > On Sun, Oct 01, 2017 at 02:44:34PM +0900, Tetsuo Handa wrote:
> > > > Tetsuo Handa wrote:
> > > > > Michael S. Tsirkin wrote:
> > > > > > On Mon, Sep 11, 2017 at 07:27:19PM +0900, Tetsuo Handa wrote:
> > > > > > > Hello.
> > > > > > >
> > > > > > > I noticed that virtio_balloon is using register_oom_notifier() and
> > > > > > > leak_balloon() from virtballoon_oom_notify() might depend on
> > > > > > > __GFP_DIRECT_RECLAIM memory allocation.
> > > > > > >
> > > > > > > In leak_balloon(), mutex_lock(&vb->balloon_lock) is called in order to
> > > > > > > serialize against fill_balloon(). But in fill_balloon(),
> > > > > > > alloc_page(GFP_HIGHUSER[_MOVABLE] | __GFP_NOMEMALLOC | __GFP_NORETRY) is
> > > > > > > called with vb->balloon_lock mutex held. Since GFP_HIGHUSER[_MOVABLE] implies
> > > > > > > __GFP_DIRECT_RECLAIM | __GFP_IO | __GFP_FS, this allocation attempt might
> > > > > > > depend on somebody else's __GFP_DIRECT_RECLAIM | !__GFP_NORETRY memory
> > > > > > > allocation. Such __GFP_DIRECT_RECLAIM | !__GFP_NORETRY allocation can reach
> > > > > > > __alloc_pages_may_oom() and hold oom_lock mutex and call out_of_memory().
> > > > > > > And leak_balloon() is called by virtballoon_oom_notify() via
> > > > > > > blocking_notifier_call_chain() callback when vb->balloon_lock mutex is already
> > > > > > > held by fill_balloon(). As a result, despite __GFP_NORETRY is specified,
> > > > > > > fill_balloon() can indirectly get stuck waiting for vb->balloon_lock mutex
> > > > > > > at leak_balloon().
> >
> > This is really nasty! And I would argue that this is an abuse of the oom
> > notifier interface from the virtio code. OOM notifiers are an ugly hack
> > on its own but all its users have to be really careful to not depend on
> > any allocation request because that is a straight deadlock situation.
>
> Please describe such warning at
> "int register_oom_notifier(struct notifier_block *nb)" definition.
Yes, we can and should do that. Although I would prefer to simply
document this API as deprecated. Care to send a patch? I am quite busy
with other stuff.
> > I do not think that making oom notifier API more complex is the way to
> > go. Can we simply change the lock to try_lock?
>
> Using mutex_trylock(&vb->balloon_lock) alone is not sufficient. Inside the
> mutex, __GFP_DIRECT_RECLAIM && !__GFP_NORETRY allocation attempt is used
> which will fail to make progress due to oom_lock already held. Therefore,
> virtballoon_oom_notify() needs to guarantee that all allocation attempts use
> GFP_NOWAIT when called from virtballoon_oom_notify().
Ohh, I missed your point and thought the dependency is indirect and some
other call path is allocating while holding the lock. But you seem to be
right and
leak_balloon
tell_host
virtqueue_add_outbuf
virtqueue_add
can do GFP_KERNEL allocation and this is clearly wrong. Nobody should
try to allocate while we are in the OOM path. Michael, is there any way
to drop this?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC] [PATCH] mm,oom: Offload OOM notify callback to a kernel thread.
From: Michal Hocko @ 2017-10-02 9:06 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-mm, Tetsuo Handa, joonas.lahtinen, jiangshanlai, josh,
jani.nikula, virtualization, airlied, mathieu.desnoyers, rostedt,
rodrigo.vivi, paulmck, intel-gfx
In-Reply-To: <20171002065801-mutt-send-email-mst@kernel.org>
[Hmm, I do not see the original patch which this has been a reply to]
On Mon 02-10-17 06:59:12, Michael S. Tsirkin wrote:
> On Sun, Oct 01, 2017 at 02:44:34PM +0900, Tetsuo Handa wrote:
> > Tetsuo Handa wrote:
> > > Michael S. Tsirkin wrote:
> > > > On Mon, Sep 11, 2017 at 07:27:19PM +0900, Tetsuo Handa wrote:
> > > > > Hello.
> > > > >
> > > > > I noticed that virtio_balloon is using register_oom_notifier() and
> > > > > leak_balloon() from virtballoon_oom_notify() might depend on
> > > > > __GFP_DIRECT_RECLAIM memory allocation.
> > > > >
> > > > > In leak_balloon(), mutex_lock(&vb->balloon_lock) is called in order to
> > > > > serialize against fill_balloon(). But in fill_balloon(),
> > > > > alloc_page(GFP_HIGHUSER[_MOVABLE] | __GFP_NOMEMALLOC | __GFP_NORETRY) is
> > > > > called with vb->balloon_lock mutex held. Since GFP_HIGHUSER[_MOVABLE] implies
> > > > > __GFP_DIRECT_RECLAIM | __GFP_IO | __GFP_FS, this allocation attempt might
> > > > > depend on somebody else's __GFP_DIRECT_RECLAIM | !__GFP_NORETRY memory
> > > > > allocation. Such __GFP_DIRECT_RECLAIM | !__GFP_NORETRY allocation can reach
> > > > > __alloc_pages_may_oom() and hold oom_lock mutex and call out_of_memory().
> > > > > And leak_balloon() is called by virtballoon_oom_notify() via
> > > > > blocking_notifier_call_chain() callback when vb->balloon_lock mutex is already
> > > > > held by fill_balloon(). As a result, despite __GFP_NORETRY is specified,
> > > > > fill_balloon() can indirectly get stuck waiting for vb->balloon_lock mutex
> > > > > at leak_balloon().
This is really nasty! And I would argue that this is an abuse of the oom
notifier interface from the virtio code. OOM notifiers are an ugly hack
on its own but all its users have to be really careful to not depend on
any allocation request because that is a straight deadlock situation.
I do not think that making oom notifier API more complex is the way to
go. Can we simply change the lock to try_lock? If the lock is held we
would simply fall back to the normal OOM handling. As a follow up it
would be great if virtio could use some other form of aging e.g.
shrinker.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] drm/virtio: Replace instances of reference/unreference with get/put
From: Daniel Vetter @ 2017-10-02 7:49 UTC (permalink / raw)
To: Srishti Sharma
Cc: daniel.vetter, linux-kernel, dri-devel, virtualization,
outreachy-kernel, seanpaul
In-Reply-To: <1506679419-7130-1-git-send-email-srishtishar@gmail.com>
On Fri, Sep 29, 2017 at 03:33:39PM +0530, Srishti Sharma wrote:
> Replace reference/unreference with get/put as it is consistent
> with the kernel coding style. Done using the following semantic
> patch by coccinelle.
>
> @r@
> expression e;
> @@
>
> -drm_gem_object_unreference_unlocked(e);
> +drm_gem_object_put_unlocked(e);
>
> Signed-off-by: Srishti Sharma <srishtishar@gmail.com>
Applied to drm-misc-next, thanks.
-Daniel
> ---
> drivers/gpu/drm/virtio/virtgpu_display.c | 4 ++--
> drivers/gpu/drm/virtio/virtgpu_gem.c | 4 ++--
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 12 ++++++------
> 3 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
> index b6d5205..41b0930 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
> @@ -53,7 +53,7 @@ static void virtio_gpu_user_framebuffer_destroy(struct drm_framebuffer *fb)
> struct virtio_gpu_framebuffer *virtio_gpu_fb
> = to_virtio_gpu_framebuffer(fb);
>
> - drm_gem_object_unreference_unlocked(virtio_gpu_fb->obj);
> + drm_gem_object_put_unlocked(virtio_gpu_fb->obj);
> drm_framebuffer_cleanup(fb);
> kfree(virtio_gpu_fb);
> }
> @@ -327,7 +327,7 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev,
> ret = virtio_gpu_framebuffer_init(dev, virtio_gpu_fb, mode_cmd, obj);
> if (ret) {
> kfree(virtio_gpu_fb);
> - drm_gem_object_unreference_unlocked(obj);
> + drm_gem_object_put_unlocked(obj);
> return NULL;
> }
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
> index 72ad7b1..92fb277 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_gem.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
> @@ -72,7 +72,7 @@ int virtio_gpu_gem_create(struct drm_file *file,
> *obj_p = &obj->gem_base;
>
> /* drop reference from allocate - handle holds it now */
> - drm_gem_object_unreference_unlocked(&obj->gem_base);
> + drm_gem_object_put_unlocked(&obj->gem_base);
>
> *handle_p = handle;
> return 0;
> @@ -130,7 +130,7 @@ int virtio_gpu_mode_dumb_mmap(struct drm_file *file_priv,
> return -ENOENT;
> obj = gem_to_virtio_gpu_obj(gobj);
> *offset_p = virtio_gpu_object_mmap_offset(obj);
> - drm_gem_object_unreference_unlocked(gobj);
> + drm_gem_object_put_unlocked(gobj);
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index b94bd54..0528edb 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -86,7 +86,7 @@ static void virtio_gpu_unref_list(struct list_head *head)
> bo = buf->bo;
> qobj = container_of(bo, struct virtio_gpu_object, tbo);
>
> - drm_gem_object_unreference_unlocked(&qobj->gem_base);
> + drm_gem_object_put_unlocked(&qobj->gem_base);
> }
> }
>
> @@ -304,7 +304,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
> }
> return ret;
> }
> - drm_gem_object_unreference_unlocked(obj);
> + drm_gem_object_put_unlocked(obj);
>
> rc->res_handle = res_id; /* similiar to a VM address */
> rc->bo_handle = handle;
> @@ -341,7 +341,7 @@ static int virtio_gpu_resource_info_ioctl(struct drm_device *dev, void *data,
>
> ri->size = qobj->gem_base.size;
> ri->res_handle = qobj->hw_res_handle;
> - drm_gem_object_unreference_unlocked(gobj);
> + drm_gem_object_put_unlocked(gobj);
> return 0;
> }
>
> @@ -389,7 +389,7 @@ static int virtio_gpu_transfer_from_host_ioctl(struct drm_device *dev,
> out_unres:
> virtio_gpu_object_unreserve(qobj);
> out:
> - drm_gem_object_unreference_unlocked(gobj);
> + drm_gem_object_put_unlocked(gobj);
> return ret;
> }
>
> @@ -439,7 +439,7 @@ static int virtio_gpu_transfer_to_host_ioctl(struct drm_device *dev, void *data,
> out_unres:
> virtio_gpu_object_unreserve(qobj);
> out:
> - drm_gem_object_unreference_unlocked(gobj);
> + drm_gem_object_put_unlocked(gobj);
> return ret;
> }
>
> @@ -462,7 +462,7 @@ static int virtio_gpu_wait_ioctl(struct drm_device *dev, void *data,
> nowait = true;
> ret = virtio_gpu_object_wait(qobj, nowait);
>
> - drm_gem_object_unreference_unlocked(gobj);
> + drm_gem_object_put_unlocked(gobj);
> return ret;
> }
>
> --
> 2.7.4
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH v16 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Michael S. Tsirkin @ 2017-10-02 4:30 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mawilcox, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, willy, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mhocko,
mgorman
In-Reply-To: <1506744354-20979-4-git-send-email-wei.w.wang@intel.com>
Looks good to me. minor comments below.
On Sat, Sep 30, 2017 at 12:05:52PM +0800, Wei Wang wrote:
> @@ -141,13 +146,128 @@ static void set_page_pfns(struct virtio_balloon *vb,
> page_to_balloon_pfn(page) + i);
> }
>
> +
> +static void kick_and_wait(struct virtqueue *vq, wait_queue_head_t wq_head)
> +{
> + unsigned int len;
> +
> + virtqueue_kick(vq);
> + wait_event(wq_head, virtqueue_get_buf(vq, &len));
> +}
> +
> +static int add_one_sg(struct virtqueue *vq, void *addr, uint32_t size)
> +{
> + struct scatterlist sg;
> + unsigned int len;
> +
> + sg_init_one(&sg, addr, size);
> +
> + /* Detach all the used buffers from the vq */
> + while (virtqueue_get_buf(vq, &len))
> + ;
> +
> + return virtqueue_add_inbuf(vq, &sg, 1, vq, GFP_KERNEL);
> +}
> +
> +static int send_balloon_page_sg(struct virtio_balloon *vb,
> + struct virtqueue *vq,
> + void *addr,
> + uint32_t size,
> + bool batch)
> +{
> + int err;
> +
> + err = add_one_sg(vq, addr, size);
> +
> + /* If batchng is requested, we batch till the vq is full */
typo
> + if (!batch || !vq->num_free)
> + kick_and_wait(vq, vb->acked);
> +
> + return err;
> +}
If add_one_sg fails, kick_and_wait will hang forever.
The reason this might work in because
1. with 1 sg there are no memory allocations
2. if adding fails on vq full, then something
is in queue and will wake up kick_and_wait.
So in short this is expected to never fail.
How about a BUG_ON here then?
And make it void, and add a comment with above explanation.
> +
> +/*
> + * Send balloon pages in sgs to host. The balloon pages are recorded in the
> + * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
> + * The page xbitmap is searched for continuous "1" bits, which correspond
> + * to continuous pages, to chunk into sgs.
> + *
> + * @page_xb_start and @page_xb_end form the range of bits in the xbitmap that
> + * need to be searched.
> + */
> +static void tell_host_sgs(struct virtio_balloon *vb,
> + struct virtqueue *vq,
> + unsigned long page_xb_start,
> + unsigned long page_xb_end)
> +{
> + unsigned long sg_pfn_start, sg_pfn_end;
> + void *sg_addr;
> + uint32_t sg_len, sg_max_len = round_down(UINT_MAX, PAGE_SIZE);
> + int err = 0;
> +
> + sg_pfn_start = page_xb_start;
> + while (sg_pfn_start < page_xb_end) {
> + sg_pfn_start = xb_find_next_set_bit(&vb->page_xb, sg_pfn_start,
> + page_xb_end);
> + if (sg_pfn_start == page_xb_end + 1)
> + break;
> + sg_pfn_end = xb_find_next_zero_bit(&vb->page_xb,
> + sg_pfn_start + 1,
> + page_xb_end);
> + sg_addr = (void *)pfn_to_kaddr(sg_pfn_start);
> + sg_len = (sg_pfn_end - sg_pfn_start) << PAGE_SHIFT;
> + while (sg_len > sg_max_len) {
> + err = send_balloon_page_sg(vb, vq, sg_addr, sg_max_len,
> + true);
> + if (unlikely(err < 0))
> + goto err_out;
> + sg_addr += sg_max_len;
> + sg_len -= sg_max_len;
> + }
> + err = send_balloon_page_sg(vb, vq, sg_addr, sg_len, true);
> + if (unlikely(err < 0))
> + goto err_out;
> + sg_pfn_start = sg_pfn_end + 1;
> + }
> +
> + /*
> + * The last few sgs may not reach the batch size, but need a kick to
> + * notify the device to handle them.
> + */
> + if (vq->num_free != virtqueue_get_vring_size(vq))
> + kick_and_wait(vq, vb->acked);
> +
> + xb_clear_bit_range(&vb->page_xb, page_xb_start, page_xb_end);
> + return;
> +
> +err_out:
> + dev_warn(&vb->vdev->dev, "%s failure: %d\n", __func__, err);
so fundamentally just make send_balloon_page_sg void then.
> +}
> +
> +static inline void xb_set_page(struct virtio_balloon *vb,
> + struct page *page,
> + unsigned long *pfn_min,
> + unsigned long *pfn_max)
> +{
> + unsigned long pfn = page_to_pfn(page);
> +
> + *pfn_min = min(pfn, *pfn_min);
> + *pfn_max = max(pfn, *pfn_max);
> + xb_preload(GFP_KERNEL);
> + xb_set_bit(&vb->page_xb, pfn);
> + xb_preload_end();
> +}
> +
> static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> {
> struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> unsigned num_allocated_pages;
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> + unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>
> /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + if (!use_sg)
> + num = min(num, ARRAY_SIZE(vb->pfns));
>
> mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> @@ -162,7 +282,12 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> msleep(200);
> break;
> }
> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +
> + if (use_sg)
> + xb_set_page(vb, page, &pfn_min, &pfn_max);
> + else
> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +
> vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> if (!virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> @@ -171,8 +296,12 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
>
> num_allocated_pages = vb->num_pfns;
> /* Did we get any? */
> - if (vb->num_pfns != 0)
> - tell_host(vb, vb->inflate_vq);
> + if (vb->num_pfns) {
> + if (use_sg)
> + tell_host_sgs(vb, vb->inflate_vq, pfn_min, pfn_max);
> + else
> + tell_host(vb, vb->inflate_vq);
> + }
> mutex_unlock(&vb->balloon_lock);
>
> return num_allocated_pages;
> @@ -198,9 +327,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> struct page *page;
> struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> LIST_HEAD(pages);
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> + unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>
> - /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + /* Traditionally, we can only do one array worth at a time. */
> + if (!use_sg)
> + num = min(num, ARRAY_SIZE(vb->pfns));
>
> mutex_lock(&vb->balloon_lock);
> /* We can't release more pages than taken */
> @@ -210,7 +342,11 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> page = balloon_page_dequeue(vb_dev_info);
> if (!page)
> break;
> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> + if (use_sg)
> + xb_set_page(vb, page, &pfn_min, &pfn_max);
> + else
> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +
> list_add(&page->lru, &pages);
> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> }
> @@ -221,8 +357,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> * is true, we *have* to do it in this order
> */
> - if (vb->num_pfns != 0)
> - tell_host(vb, vb->deflate_vq);
> + if (vb->num_pfns) {
> + if (use_sg)
> + tell_host_sgs(vb, vb->deflate_vq, pfn_min, pfn_max);
> + else
> + tell_host(vb, vb->deflate_vq);
> + }
> release_pages_balloon(vb, &pages);
> mutex_unlock(&vb->balloon_lock);
> return num_freed_pages;
> @@ -441,6 +581,7 @@ static int init_vqs(struct virtio_balloon *vb)
> }
>
> #ifdef CONFIG_BALLOON_COMPACTION
> +
> /*
> * virtballoon_migratepage - perform the balloon page migration on behalf of
> * a compation thread. (called under page lock)
> @@ -464,6 +605,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
> {
> struct virtio_balloon *vb = container_of(vb_dev_info,
> struct virtio_balloon, vb_dev_info);
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> unsigned long flags;
>
> /*
> @@ -485,16 +627,24 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
> vb_dev_info->isolated_pages--;
> __count_vm_event(BALLOON_MIGRATE);
> spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
> - vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> - set_page_pfns(vb, vb->pfns, newpage);
> - tell_host(vb, vb->inflate_vq);
> -
> + if (use_sg) {
> + send_balloon_page_sg(vb, vb->inflate_vq, page_address(newpage),
> + PAGE_SIZE, false);
> + } else {
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb, vb->pfns, newpage);
> + tell_host(vb, vb->inflate_vq);
> + }
> /* balloon's page migration 2nd step -- deflate "page" */
> balloon_page_delete(page);
> - vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> - set_page_pfns(vb, vb->pfns, page);
> - tell_host(vb, vb->deflate_vq);
> -
> + if (use_sg) {
> + send_balloon_page_sg(vb, vb->deflate_vq, page_address(page),
> + PAGE_SIZE, false);
> + } else {
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb, vb->pfns, page);
> + tell_host(vb, vb->deflate_vq);
> + }
> mutex_unlock(&vb->balloon_lock);
>
> put_page(page); /* balloon reference */
> @@ -553,6 +703,9 @@ static int virtballoon_probe(struct virtio_device *vdev)
> if (err)
> goto out_free_vb;
>
> + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SG))
> + xb_init(&vb->page_xb);
> +
> vb->nb.notifier_call = virtballoon_oom_notify;
> vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
> err = register_oom_notifier(&vb->nb);
> @@ -669,6 +822,7 @@ static unsigned int features[] = {
> VIRTIO_BALLOON_F_MUST_TELL_HOST,
> VIRTIO_BALLOON_F_STATS_VQ,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
> + VIRTIO_BALLOON_F_SG,
> };
>
> static struct virtio_driver virtio_balloon_driver = {
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 343d7dd..37780a7 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -34,6 +34,7 @@
> #define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */
> #define VIRTIO_BALLOON_F_STATS_VQ 1 /* Memory Stats virtqueue */
> #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */
> +#define VIRTIO_BALLOON_F_SG 3 /* Use sg instead of PFN lists */
>
> /* Size of a PFN in the balloon interface. */
> #define VIRTIO_BALLOON_PFN_SHIFT 12
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net-next] vhost_net: do not stall on zerocopy depletion
From: Michael S. Tsirkin @ 2017-10-02 4:08 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Willem de Bruijn, Network Development, Koichiro Den,
virtualization, David Miller
In-Reply-To: <CAF=yD-KotdpHs96GomMKR-BqG3Gyrvo+to0sk2=a6E5BKjgpkg@mail.gmail.com>
On Fri, Sep 29, 2017 at 09:25:27PM -0400, Willem de Bruijn wrote:
> On Fri, Sep 29, 2017 at 3:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Wed, Sep 27, 2017 at 08:25:56PM -0400, Willem de Bruijn wrote:
> >> From: Willem de Bruijn <willemb@google.com>
> >>
> >> Vhost-net has a hard limit on the number of zerocopy skbs in flight.
> >> When reached, transmission stalls. Stalls cause latency, as well as
> >> head-of-line blocking of other flows that do not use zerocopy.
> >>
> >> Instead of stalling, revert to copy-based transmission.
> >>
> >> Tested by sending two udp flows from guest to host, one with payload
> >> of VHOST_GOODCOPY_LEN, the other too small for zerocopy (1B). The
> >> large flow is redirected to a netem instance with 1MBps rate limit
> >> and deep 1000 entry queue.
> >>
> >> modprobe ifb
> >> ip link set dev ifb0 up
> >> tc qdisc add dev ifb0 root netem limit 1000 rate 1MBit
> >>
> >> tc qdisc add dev tap0 ingress
> >> tc filter add dev tap0 parent ffff: protocol ip \
> >> u32 match ip dport 8000 0xffff \
> >> action mirred egress redirect dev ifb0
> >>
> >> Before the delay, both flows process around 80K pps. With the delay,
> >> before this patch, both process around 400. After this patch, the
> >> large flow is still rate limited, while the small reverts to its
> >> original rate. See also discussion in the first link, below.
> >>
> >> The limit in vhost_exceeds_maxpend must be carefully chosen. When
> >> vq->num >> 1, the flows remain correlated. This value happens to
> >> correspond to VHOST_MAX_PENDING for vq->num == 256. Allow smaller
> >> fractions and ensure correctness also for much smaller values of
> >> vq->num, by testing the min() of both explicitly. See also the
> >> discussion in the second link below.
> >>
> >> Link:http://lkml.kernel.org/r/CAF=yD-+Wk9sc9dXMUq1+x_hh=3ThTXa6BnZkygP3tgVpjbp93g@mail.gmail.com
> >> Link:http://lkml.kernel.org/r/20170819064129.27272-1-den@klaipeden.com
> >> Signed-off-by: Willem de Bruijn <willemb@google.com>
> >
> > I'd like to see the effect on the non rate limited case though.
> > If guest is quick won't we have lots of copies then?
>
> Yes, but not significantly more than without this patch.
>
> I ran 1, 10 and 100 flow tcp_stream throughput tests from a sender
> in the guest to a receiver in the host.
>
> To answer the other benchmark question first, I did not see anything
> noteworthy when increasing vq->num from 256 to 1024.
>
> With 1 and 10 flows without this patch all packets use zerocopy.
> With the patch, less than 1% eschews zerocopy.
>
> With 100 flows, even without this patch, 90+% of packets are copied.
> Some zerocopy packets from vhost_net fail this test in tun.c
>
> if (iov_iter_npages(&i, INT_MAX) <= MAX_SKB_FRAGS)
>
> Generating packets with up to 21 frags. I'm not sure yet why or
> what the fraction of these packets is. But this in turn can
> disable zcopy_used in vhost_net_tx_select_zcopy for a
> larger share of packets:
>
> return !net->tx_flush &&
> net->tx_packets / 64 >= net->tx_zcopy_err;
>
> Because the number of copied and zerocopy packets are the
> same before and after the patch, so are the overall throughput
> numbers.
OK, thanks!
Are you looking into new warnings that kbuild system reported
with this patch?
Thanks,
--
MST
^ permalink raw reply
* Re: [RFC] [PATCH] mm,oom: Offload OOM notify callback to a kernel thread.
From: Michael S. Tsirkin @ 2017-10-02 3:59 UTC (permalink / raw)
To: Tetsuo Handa
Cc: airlied, joonas.lahtinen, jiangshanlai, josh, jani.nikula,
virtualization, linux-mm, mathieu.desnoyers, rostedt,
rodrigo.vivi, paulmck, intel-gfx
In-Reply-To: <201710011444.IBD05725.VJSFHOOMOFtLQF@I-love.SAKURA.ne.jp>
On Sun, Oct 01, 2017 at 02:44:34PM +0900, Tetsuo Handa wrote:
> Tetsuo Handa wrote:
> > Michael S. Tsirkin wrote:
> > > On Mon, Sep 11, 2017 at 07:27:19PM +0900, Tetsuo Handa wrote:
> > > > Hello.
> > > >
> > > > I noticed that virtio_balloon is using register_oom_notifier() and
> > > > leak_balloon() from virtballoon_oom_notify() might depend on
> > > > __GFP_DIRECT_RECLAIM memory allocation.
> > > >
> > > > In leak_balloon(), mutex_lock(&vb->balloon_lock) is called in order to
> > > > serialize against fill_balloon(). But in fill_balloon(),
> > > > alloc_page(GFP_HIGHUSER[_MOVABLE] | __GFP_NOMEMALLOC | __GFP_NORETRY) is
> > > > called with vb->balloon_lock mutex held. Since GFP_HIGHUSER[_MOVABLE] implies
> > > > __GFP_DIRECT_RECLAIM | __GFP_IO | __GFP_FS, this allocation attempt might
> > > > depend on somebody else's __GFP_DIRECT_RECLAIM | !__GFP_NORETRY memory
> > > > allocation. Such __GFP_DIRECT_RECLAIM | !__GFP_NORETRY allocation can reach
> > > > __alloc_pages_may_oom() and hold oom_lock mutex and call out_of_memory().
> > > > And leak_balloon() is called by virtballoon_oom_notify() via
> > > > blocking_notifier_call_chain() callback when vb->balloon_lock mutex is already
> > > > held by fill_balloon(). As a result, despite __GFP_NORETRY is specified,
> > > > fill_balloon() can indirectly get stuck waiting for vb->balloon_lock mutex
> > > > at leak_balloon().
> > >
> > > That would be tricky to fix. I guess we'll need to drop the lock
> > > while allocating memory - not an easy fix.
> > >
> > > > Also, in leak_balloon(), virtqueue_add_outbuf(GFP_KERNEL) is called via
> > > > tell_host(). Reaching __alloc_pages_may_oom() from this virtqueue_add_outbuf()
> > > > request from leak_balloon() from virtballoon_oom_notify() from
> > > > blocking_notifier_call_chain() from out_of_memory() leads to OOM lockup
> > > > because oom_lock mutex is already held before calling out_of_memory().
> > >
> > > I guess we should just do
> > >
> > > GFP_KERNEL & ~__GFP_DIRECT_RECLAIM there then?
> >
> > Yes, but GFP_KERNEL & ~__GFP_DIRECT_RECLAIM will effectively be GFP_NOWAIT, for
> > __GFP_IO and __GFP_FS won't make sense without __GFP_DIRECT_RECLAIM. It might
> > significantly increases possibility of memory allocation failure.
> >
> > >
> > >
> > > >
> > > > OOM notifier callback should not (directly or indirectly) depend on
> > > > __GFP_DIRECT_RECLAIM memory allocation attempt. Can you fix this dependency?
> > >
> >
> > Another idea would be to use a kernel thread (or workqueue) so that
> > virtballoon_oom_notify() can wait with timeout.
> >
> > We could offload entire blocking_notifier_call_chain(&oom_notify_list, 0, &freed)
> > call to a kernel thread (or workqueue) with timeout if MM folks agree.
> >
>
> Below is a patch which offloads blocking_notifier_call_chain() call. What do you think?
> ----------------------------------------
> [RFC] [PATCH] mm,oom: Offload OOM notify callback to a kernel thread.
>
> Since oom_notify_list is traversed via blocking_notifier_call_chain(),
> it is legal to sleep inside OOM notifier callback function.
>
> However, since oom_notify_list is traversed with oom_lock held,
> __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory allocation attempt cannot
> fail when traversing oom_notify_list entries. Therefore, OOM notifier
> callback function should not (directly or indirectly) depend on
> __GFP_DIRECT_RECLAIM && !__GFP_NORETRY memory allocation attempt.
>
> Currently there are 5 register_oom_notifier() users in the mainline kernel.
>
> arch/powerpc/platforms/pseries/cmm.c
> arch/s390/mm/cmm.c
> drivers/gpu/drm/i915/i915_gem_shrinker.c
> drivers/virtio/virtio_balloon.c
> kernel/rcu/tree_plugin.h
>
> Among these users, at least virtio_balloon.c has possibility of OOM lockup
> because it is using mutex which can depend on GFP_KERNEL memory allocations.
> (Both cmm.c seem to be safe as they use spinlocks. I'm not sure about
> tree_plugin.h and i915_gem_shrinker.c . Please check.)
>
> But converting such allocations to use GFP_NOWAIT is not only prone to
> allocation failures under memory pressure but also difficult to audit
> whether all locations are converted to use GFP_NOWAIT.
>
> Therefore, this patch offloads blocking_notifier_call_chain() call to a
> dedicated kernel thread and wait for completion with timeout of 5 seconds
> so that we can completely forget about possibility of OOM lockup due to
> OOM notifier callback function.
>
> (5 seconds is chosen from my guess that blocking_notifier_call_chain()
> should not take long, for we are using mutex_trylock(&oom_lock) at
> __alloc_pages_may_oom() based on an assumption that out_of_memory() should
> reclaim memory shortly.)
>
> The kernel thread is created upon first register_oom_notifier() call.
> Thus, those environments which do not use register_oom_notifier() will
> not waste resource for the dedicated kernel thread.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
> mm/oom_kill.c | 40 ++++++++++++++++++++++++++++++++++++----
> 1 file changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index dee0f75..d9744f7 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -981,9 +981,37 @@ static void check_panic_on_oom(struct oom_control *oc,
> }
>
> static BLOCKING_NOTIFIER_HEAD(oom_notify_list);
> +static bool oom_notifier_requested;
> +static unsigned long oom_notifier_freed;
> +static struct task_struct *oom_notifier_th;
> +static DECLARE_WAIT_QUEUE_HEAD(oom_notifier_request_wait);
> +static DECLARE_WAIT_QUEUE_HEAD(oom_notifier_response_wait);
> +
> +static int oom_notifier(void *unused)
> +{
> + while (true) {
> + wait_event_freezable(oom_notifier_request_wait,
> + oom_notifier_requested);
> + blocking_notifier_call_chain(&oom_notify_list, 0,
> + &oom_notifier_freed);
> + oom_notifier_requested = false;
> + wake_up(&oom_notifier_response_wait);
> + }
> + return 0;
> +}
>
> int register_oom_notifier(struct notifier_block *nb)
> {
> + if (!oom_notifier_th) {
> + struct task_struct *th = kthread_run(oom_notifier, NULL,
> + "oom_notifier");
> +
> + if (IS_ERR(th)) {
> + pr_err("Unable to start OOM notifier thread.\n");
> + return (int) PTR_ERR(th);
> + }
> + oom_notifier_th = th;
> + }
> return blocking_notifier_chain_register(&oom_notify_list, nb);
> }
> EXPORT_SYMBOL_GPL(register_oom_notifier);
> @@ -1005,17 +1033,21 @@ int unregister_oom_notifier(struct notifier_block *nb)
> */
> bool out_of_memory(struct oom_control *oc)
> {
> - unsigned long freed = 0;
> enum oom_constraint constraint = CONSTRAINT_NONE;
>
> if (oom_killer_disabled)
> return false;
>
> - if (!is_memcg_oom(oc)) {
> - blocking_notifier_call_chain(&oom_notify_list, 0, &freed);
> - if (freed > 0)
> + if (!is_memcg_oom(oc) && oom_notifier_th) {
> + oom_notifier_requested = true;
> + wake_up(&oom_notifier_request_wait);
> + wait_event_timeout(oom_notifier_response_wait,
> + !oom_notifier_requested, 5 * HZ);
I guess this means what was earlier a deadlock will free up after 5
seconds, by a 5 sec downtime is still a lot, isn't it?
> + if (oom_notifier_freed) {
> + oom_notifier_freed = 0;
> /* Got some memory back in the last second. */
> return true;
> + }
> }
>
> /*
> --
> 1.8.3.1
^ permalink raw reply
* Re: [PATCH v16 0/5] Virtio-balloon Enhancement
From: Damian Tometzki @ 2017-10-01 13:25 UTC (permalink / raw)
To: Wei Wang, virtio-dev, linux-kernel, qemu-devel, virtualization,
kvm, linux-mm, mst, mhocko, akpm, mawilcox
Cc: aarcange, yang.zhang.wz, liliang.opensource, willy, amit.shah,
quan.xu, cornelia.huck, pbonzini, mgorman
In-Reply-To: <1506744354-20979-1-git-send-email-wei.w.wang@intel.com>
Hello,
where i can found the patch in git.kernel.org ?
Best regards
Damian
Am Samstag, den 30.09.2017, 12:05 +0800 schrieb Wei Wang:
> This patch series enhances the existing virtio-balloon with the
> following
> new features:
> 1) fast ballooning: transfer ballooned pages between the guest and
> host in
> chunks using sgs, instead of one array each time; and
> 2) free page block reporting: a new virtqueue to report guest free
> pages
> to the host.
>
> The second feature can be used to accelerate live migration of VMs.
> Here
> are some details:
>
> Live migration needs to transfer the VM's memory from the source
> machine
> to the destination round by round. For the 1st round, all the VM's
> memory
> is transferred. From the 2nd round, only the pieces of memory that
> were
> written by the guest (after the 1st round) are transferred. One
> method
> that is popularly used by the hypervisor to track which part of
> memory is
> written is to write-protect all the guest memory.
>
> The second feature enables the optimization of the 1st round memory
> transfer - the hypervisor can skip the transfer of guest free pages
> in the
> 1st round. It is not concerned that the memory pages are used after
> they
> are given to the hypervisor as a hint of the free pages, because they
> will
> be tracked by the hypervisor and transferred in the next round if
> they are
> used and written.
>
> Change Log:
> v15->v16:
> 1) mm: stop reporting the free pfn range if the callback returns
> false;
> 2) mm: move some implementaion of walk_free_mem_block into a function
> to
> make the code layout looks better;
> 3) xbitmap: added some optimizations suggested by Matthew, please
> refer to
> the ChangLog in the xbitmap patch for details.
> 4) xbitmap: added a test suite
> 5) virtio-balloon: bail out with a warning when virtqueue_add_inbuf
> returns
> an error
> 6) virtio-balloon: some small code re-arrangement, e.g. detachinf
> used buf
> from the vq before adding a new buf
>
> v14->v15:
> 1) mm: make the report callback return a bool value - returning 1 to
> stop
> walking through the free page list.
> 2) virtio-balloon: batching sgs of balloon pages till the vq is full
> 3) virtio-balloon: create a new workqueue, rather than using the
> default
> system_wq, to queue the free page reporting work item.
> 4) virtio-balloon: add a ctrl_vq to be a central control plane which
> will
> handle all the future control related commands between the host and
> guest.
> Add free page report as the first feature controlled under ctrl_vq,
> and
> the free_page_vq is a data plane vq dedicated to the transmission of
> free
> page blocks.
>
> v13->v14:
> 1) xbitmap: move the code from lib/radix-tree.c to lib/xbitmap.c.
> 2) xbitmap: consolidate the implementation of xb_bit_set/clear/test
> into
> one xb_bit_ops.
> 3) xbitmap: add documents for the exported APIs.
> 4) mm: rewrite the function to walk through free page blocks.
> 5) virtio-balloon: when reporting a free page blcok to the device, if
> the
> vq is full (less likey to happen in practice), just skip reporting
> this
> block, instead of busywaiting till an entry gets released.
> 6) virtio-balloon: fail the probe function if adding the signal buf
> in
> init_vqs fails.
>
> v12->v13:
> 1) mm: use a callback function to handle the the free page blocks
> from the
> report function. This avoids exposing the zone internal to a kernel
> module.
> 2) virtio-balloon: send balloon pages or a free page block using a
> single
> sg each time. This has the benefits of simpler implementation with no
> new
> APIs.
> 3) virtio-balloon: the free_page_vq is used to report free pages only
> (no
> multiple usages interleaving)
> 4) virtio-balloon: Balloon pages and free page blocks are sent via
> input
> sgs, and the completion signal to the host is sent via an output sg.
>
> v11->v12:
> 1) xbitmap: use the xbitmap from Matthew Wilcox to record ballooned
> pages.
> 2) virtio-ring: enable the driver to build up a desc chain using
> vring
> desc.
> 3) virtio-ring: Add locking to the existing START_USE() and END_USE()
> macro to lock/unlock the vq when a vq operation starts/ends.
> 4) virtio-ring: add virtqueue_kick_sync() and virtqueue_kick_async()
> 5) virtio-balloon: describe chunks of ballooned pages and free pages
> blocks directly using one or more chains of desc from the vq.
>
> v10->v11:
> 1) virtio_balloon: use vring_desc to describe a chunk;
> 2) virtio_ring: support to add an indirect desc table to virtqueue;
> 3) virtio_balloon: use cmdq to report guest memory statistics.
>
> v9->v10:
> 1) mm: put report_unused_page_block() under CONFIG_VIRTIO_BALLOON;
> 2) virtio-balloon: add virtballoon_validate();
> 3) virtio-balloon: msg format change;
> 4) virtio-balloon: move miscq handling to a task on
> system_freezable_wq;
> 5) virtio-balloon: code cleanup.
>
> v8->v9:
> 1) Split the two new features, VIRTIO_BALLOON_F_BALLOON_CHUNKS and
> VIRTIO_BALLOON_F_MISC_VQ, which were mixed together in the previous
> implementation;
> 2) Simpler function to get the free page block.
>
> v7->v8:
> 1) Use only one chunk format, instead of two.
> 2) re-write the virtio-balloon implementation patch.
> 3) commit changes
> 4) patch re-org
>
> Matthew Wilcox (2):
> lib/xbitmap: Introduce xbitmap
> radix tree test suite: add tests for xbitmap
>
> Wei Wang (3):
> virtio-balloon: VIRTIO_BALLOON_F_SG
> mm: support reporting free page blocks
> virtio-balloon: VIRTIO_BALLOON_F_CTRL_VQ
>
> drivers/virtio/virtio_balloon.c | 437
> +++++++++++++++++++++++++++++---
> include/linux/mm.h | 6 +
> include/linux/radix-tree.h | 2 +
> include/linux/xbitmap.h | 66 +++++
> include/uapi/linux/virtio_balloon.h | 16 ++
> lib/Makefile | 2 +-
> lib/radix-tree.c | 42 ++-
> lib/xbitmap.c | 264 +++++++++++++++++++
> mm/page_alloc.c | 91 +++++++
> tools/include/linux/bitmap.h | 34 +++
> tools/include/linux/kernel.h | 2 +
> tools/testing/radix-tree/Makefile | 7 +-
> tools/testing/radix-tree/linux/kernel.h | 2 -
> tools/testing/radix-tree/main.c | 5 +
> tools/testing/radix-tree/test.h | 1 +
> tools/testing/radix-tree/xbitmap.c | 269 ++++++++++++++++++++
> 16 files changed, 1203 insertions(+), 43 deletions(-)
> create mode 100644 include/linux/xbitmap.h
> create mode 100644 lib/xbitmap.c
> create mode 100644 tools/testing/radix-tree/xbitmap.c
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v16 0/5] Virtio-balloon Enhancement
From: Damian Tometzki @ 2017-10-01 13:16 UTC (permalink / raw)
To: Wei Wang, virtio-dev, linux-kernel, qemu-devel, virtualization,
kvm, linux-mm, mst, mhocko, akpm, mawilcox
Cc: aarcange, yang.zhang.wz, liliang.opensource, willy, amit.shah,
quan.xu, cornelia.huck, pbonzini, mgorman
In-Reply-To: <1506744354-20979-1-git-send-email-wei.w.wang@intel.com>
Hello,
where i can found the patch in git.kernel.org ?
Am Samstag, den 30.09.2017, 12:05 +0800 schrieb Wei Wang:
> This patch series enhances the existing virtio-balloon with the
> following
> new features:
> 1) fast ballooning: transfer ballooned pages between the guest and
> host in
> chunks using sgs, instead of one array each time; and
> 2) free page block reporting: a new virtqueue to report guest free
> pages
> to the host.
>
> The second feature can be used to accelerate live migration of VMs.
> Here
> are some details:
>
> Live migration needs to transfer the VM's memory from the source
> machine
> to the destination round by round. For the 1st round, all the VM's
> memory
> is transferred. From the 2nd round, only the pieces of memory that
> were
> written by the guest (after the 1st round) are transferred. One
> method
> that is popularly used by the hypervisor to track which part of
> memory is
> written is to write-protect all the guest memory.
>
> The second feature enables the optimization of the 1st round memory
> transfer - the hypervisor can skip the transfer of guest free pages
> in the
> 1st round. It is not concerned that the memory pages are used after
> they
> are given to the hypervisor as a hint of the free pages, because they
> will
> be tracked by the hypervisor and transferred in the next round if
> they are
> used and written.
>
> Change Log:
> v15->v16:
> 1) mm: stop reporting the free pfn range if the callback returns
> false;
> 2) mm: move some implementaion of walk_free_mem_block into a function
> to
> make the code layout looks better;
> 3) xbitmap: added some optimizations suggested by Matthew, please
> refer to
> the ChangLog in the xbitmap patch for details.
> 4) xbitmap: added a test suite
> 5) virtio-balloon: bail out with a warning when virtqueue_add_inbuf
> returns
> an error
> 6) virtio-balloon: some small code re-arrangement, e.g. detachinf
> used buf
> from the vq before adding a new buf
>
> v14->v15:
> 1) mm: make the report callback return a bool value - returning 1 to
> stop
> walking through the free page list.
> 2) virtio-balloon: batching sgs of balloon pages till the vq is full
> 3) virtio-balloon: create a new workqueue, rather than using the
> default
> system_wq, to queue the free page reporting work item.
> 4) virtio-balloon: add a ctrl_vq to be a central control plane which
> will
> handle all the future control related commands between the host and
> guest.
> Add free page report as the first feature controlled under ctrl_vq,
> and
> the free_page_vq is a data plane vq dedicated to the transmission of
> free
> page blocks.
>
> v13->v14:
> 1) xbitmap: move the code from lib/radix-tree.c to lib/xbitmap.c.
> 2) xbitmap: consolidate the implementation of xb_bit_set/clear/test
> into
> one xb_bit_ops.
> 3) xbitmap: add documents for the exported APIs.
> 4) mm: rewrite the function to walk through free page blocks.
> 5) virtio-balloon: when reporting a free page blcok to the device, if
> the
> vq is full (less likey to happen in practice), just skip reporting
> this
> block, instead of busywaiting till an entry gets released.
> 6) virtio-balloon: fail the probe function if adding the signal buf
> in
> init_vqs fails.
>
> v12->v13:
> 1) mm: use a callback function to handle the the free page blocks
> from the
> report function. This avoids exposing the zone internal to a kernel
> module.
> 2) virtio-balloon: send balloon pages or a free page block using a
> single
> sg each time. This has the benefits of simpler implementation with no
> new
> APIs.
> 3) virtio-balloon: the free_page_vq is used to report free pages only
> (no
> multiple usages interleaving)
> 4) virtio-balloon: Balloon pages and free page blocks are sent via
> input
> sgs, and the completion signal to the host is sent via an output sg.
>
> v11->v12:
> 1) xbitmap: use the xbitmap from Matthew Wilcox to record ballooned
> pages.
> 2) virtio-ring: enable the driver to build up a desc chain using
> vring
> desc.
> 3) virtio-ring: Add locking to the existing START_USE() and END_USE()
> macro to lock/unlock the vq when a vq operation starts/ends.
> 4) virtio-ring: add virtqueue_kick_sync() and virtqueue_kick_async()
> 5) virtio-balloon: describe chunks of ballooned pages and free pages
> blocks directly using one or more chains of desc from the vq.
>
> v10->v11:
> 1) virtio_balloon: use vring_desc to describe a chunk;
> 2) virtio_ring: support to add an indirect desc table to virtqueue;
> 3) virtio_balloon: use cmdq to report guest memory statistics.
>
> v9->v10:
> 1) mm: put report_unused_page_block() under CONFIG_VIRTIO_BALLOON;
> 2) virtio-balloon: add virtballoon_validate();
> 3) virtio-balloon: msg format change;
> 4) virtio-balloon: move miscq handling to a task on
> system_freezable_wq;
> 5) virtio-balloon: code cleanup.
>
> v8->v9:
> 1) Split the two new features, VIRTIO_BALLOON_F_BALLOON_CHUNKS and
> VIRTIO_BALLOON_F_MISC_VQ, which were mixed together in the previous
> implementation;
> 2) Simpler function to get the free page block.
>
> v7->v8:
> 1) Use only one chunk format, instead of two.
> 2) re-write the virtio-balloon implementation patch.
> 3) commit changes
> 4) patch re-org
>
> Matthew Wilcox (2):
> lib/xbitmap: Introduce xbitmap
> radix tree test suite: add tests for xbitmap
>
> Wei Wang (3):
> virtio-balloon: VIRTIO_BALLOON_F_SG
> mm: support reporting free page blocks
> virtio-balloon: VIRTIO_BALLOON_F_CTRL_VQ
>
> drivers/virtio/virtio_balloon.c | 437
> +++++++++++++++++++++++++++++---
> include/linux/mm.h | 6 +
> include/linux/radix-tree.h | 2 +
> include/linux/xbitmap.h | 66 +++++
> include/uapi/linux/virtio_balloon.h | 16 ++
> lib/Makefile | 2 +-
> lib/radix-tree.c | 42 ++-
> lib/xbitmap.c | 264 +++++++++++++++++++
> mm/page_alloc.c | 91 +++++++
> tools/include/linux/bitmap.h | 34 +++
> tools/include/linux/kernel.h | 2 +
> tools/testing/radix-tree/Makefile | 7 +-
> tools/testing/radix-tree/linux/kernel.h | 2 -
> tools/testing/radix-tree/main.c | 5 +
> tools/testing/radix-tree/test.h | 1 +
> tools/testing/radix-tree/xbitmap.c | 269 ++++++++++++++++++++
> 16 files changed, 1203 insertions(+), 43 deletions(-)
> create mode 100644 include/linux/xbitmap.h
> create mode 100644 lib/xbitmap.c
> create mode 100644 tools/testing/radix-tree/xbitmap.c
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] packed ring layout proposal v3
From: Michael S. Tsirkin @ 2017-10-01 4:08 UTC (permalink / raw)
To: Liang, Cunming
Cc: virtio-dev@lists.oasis-open.org, Steven Luong (sluong),
virtualization@lists.linux-foundation.org
In-Reply-To: <D0158A423229094DA7ABF71CF2FA0DA34E1AD49B@SHSMSX152.ccr.corp.intel.com>
On Thu, Sep 28, 2017 at 09:44:35AM +0000, Liang, Cunming wrote:
>
> Get it now. Please correct me if I missing something.
>
>
> Flags status hints,
>
> - DESC_DRIVER only: driver owns the descriptor w/o available info ready for device to use
>
> - DESC_DRIVER | DESC_WRAP: driver has prepared an available descriptor, device hasn't used it yet
>
> - None: device has used the descriptor, and write descriptor out
>
> - DESC_WRAP only: shall not happen, device make sure to clear it
>
>
> Polling behavior is,
>
> - Device monitor DESC_WRAP bit set or not; If set, go to use descriptor and clear DESC_DRIVER bit in the end (note: always need to clear DESC_WRAP)
>
> - Driver monitor DESC_DRIVER bit cleared or not; If cleared, reclaim descriptor(set DESC_DRIVER) and set DESC_WRAP once new available descriptor get ready to go
>
>
> --
> Steve
Hmm no, not what I had in mind.
DESC_DRIVER: used by driver to poll. Driver sets it when writing a
descriptor. Device clears it when overwriting a descriptor.
Thus driver uses DESC_DRIVER to detect that device data in
descriptor is valid.
DESC_WRAP: used by device to poll. Driver sets it to a *different*
value every time it overwrites a descriptor. How to achieve it?
since descriptors are written out in ring order,
simply maintain the current value internally (start value 1) and flip it
every time you overwrite the first descriptor.
Device leaves it intact when overwriting a descriptor.
After writing down this explanation, I think the names aren't
great.
Let me try an alternative explanation.
---------------
A two-bit field, DRIVER_OWNER, signals the buffer ownership.
It has 4 possible values:
values 0x1, 0x11 are written by driver
values 0x0, 0x10 are written by device
each time driver writes out a descriptor, it must make sure
that the high bit in OWNER changes.
each time device writes out a descriptor, it must make sure
that the high bit in OWNER does not change.
this is exactly the same functionally, DRIVER is high bit and
WRAP is the low bit. Does this make things clearer?
---------------
Maybe the difference between device and driver
is confusing. We can fix that by changing the values.
Here is an alternative. Let me know if you like it better -
I need to think a bit more to make sure it works,
but to give you an idea:
---------------
A two-bit field, DRIVER_OWNER, signals the buffer ownership.
It has 4 possible values:
values 0x1, 0x10 are written by driver
values 0x0, 0x11 are written by device
each time driver writes out a descriptor, it must make sure
that the high bit in OWNER changes. Thus first time
it writes 0x10, next time 0x1, then 0x10 again.
each time device writes out a descriptor, it must make sure
that the low bit in OWNER changes. Thus first time
it writes 0x11, next time 0x0, then 0x11 again.
---------------
> > -----Original Message-----
> > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > Sent: Thursday, September 28, 2017 7:49 AM
> > To: Steven Luong (sluong)
> > Cc: Liang, Cunming; virtio-dev@lists.oasis-open.org;
> > virtualization@lists.linux-foundation.org
> > Subject: Re: [virtio-dev] packed ring layout proposal v3
> >
> > On Tue, Sep 26, 2017 at 11:38:18PM +0000, Steven Luong (sluong) wrote:
> > > Michael,
> > >
> > > Would you please give an example or two how these two flags
> > DESC_DRIVER and DESC_WRAP are used together? Like others, I am
> > confused by the description and still don’t quite grok it.
> > >
> > > Steven
> >
> > My bad, I will need to work on it. Here is an example:
> >
> > Let's assume device promised to consume packets in order
> >
> > ring size = 2
> >
> > Ring is 0 initialized.
> >
> > Device initially polls DESC[0].flags for WRAP bit to change.
> >
> > driver adds:
> >
> > DESC[0].addr = 1234
> > DESC[0].id = 0
> > DESC[0].flags = DESC_DRIVER | DESC_NEXT | DESC_WRAP
> >
> > and
> >
> > DESC[0].addr = 5678
> > DESC[1].id = 1
> > DESC[1].flags = DESC_DRIVER | DESC_WRAP
> >
> >
> > it now starts polling DESC[0] flags.
> >
> >
> > Device reads 1234, executes it, does not use it.
> >
> > Device reads 5678, executes it, and uses it:
> >
> > DESC[0].id = 1
> > DESC[0].flags = 0
> >
> > Device now polls DESC[0].flags for WRAP bit to change.
> >
> > Now driver sees that DRIVER bit has been cleared, so it nows that id is valid. I
> > sees id 1, therefore id 0 and 1 has been read and are safe to overwrite.
> >
> > So it writes it out. It wrapped around to beginning of ring, so it flips the
> > WRAP bit to 0 on all descriptors now:
> >
> > DESC[0].addr = 9ABC
> > DESC[0].id = 0
> > DESC[0].flags = DESC_DRIVER | DESC_NEXT
> >
> >
> > DESC[0].addr = DEF0
> > DESC[0].id = 1
> > DESC[0].flags = DESC_DRIVER
> >
> >
> > Next round wrap will be 1 again.
> >
> >
> > To summarise:
> >
> > DRIVER bit is used by driver to detect device has used one or more
> > descriptors. WRAP is is used by device to detect driver has made a new
> > descriptor available.
> >
> >
> > --
> > MST
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [kbuild-all] [PATCH net-next] vhost_net: do not stall on zerocopy depletion
From: Fengguang Wu @ 2017-10-01 3:26 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, Willem de Bruijn, den, virtualization, kbuild-all, davem
In-Reply-To: <20171001062018-mutt-send-email-mst@kernel.org>
On Sun, Oct 01, 2017 at 06:20:49AM +0300, Michael S. Tsirkin wrote:
>On Sun, Oct 01, 2017 at 08:09:30AM +0800, kbuild test robot wrote:
>> Hi Willem,
>>
>> [auto build test WARNING on net-next/master]
>>
>> url: https://github.com/0day-ci/linux/commits/Willem-de-Bruijn/vhost_net-do-not-stall-on-zerocopy-depletion/20171001-054709
>> reproduce:
>> # apt-get install sparse
>> make ARCH=x86_64 allmodconfig
>> make C=1 CF=-D__CHECK_ENDIAN__
>
>BTW __CHECK_ENDIAN__ is the default now, I think you can drop it from
>your scripts.
Good tip, thank you! However since we're testing old kernels, too,
we'll need to keep it for probably 1-3 more years.
Thanks,
Fengguang
>>
>> sparse warnings: (new ones prefixed by >>)
>>
>>
>> vim +440 drivers/vhost/net.c
>>
>> 433
>> 434 static bool vhost_exceeds_maxpend(struct vhost_net *net)
>> 435 {
>> 436 struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
>> 437 struct vhost_virtqueue *vq = &nvq->vq;
>> 438
>> 439 return (nvq->upend_idx + UIO_MAXIOV - nvq->done_idx) % UIO_MAXIOV >
>> > 440 min(VHOST_MAX_PEND, vq->num >> 2);
>> 441 }
>> 442
>>
>> ---
>> 0-DAY kernel test infrastructure Open Source Technology Center
>> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>_______________________________________________
>kbuild-all mailing list
>kbuild-all@lists.01.org
>https://lists.01.org/mailman/listinfo/kbuild-all
^ permalink raw reply
* Re: [PATCH net-next] vhost_net: do not stall on zerocopy depletion
From: Michael S. Tsirkin @ 2017-10-01 3:20 UTC (permalink / raw)
To: kbuild test robot
Cc: netdev, Willem de Bruijn, den, virtualization, kbuild-all, davem
In-Reply-To: <201710010753.KeY9a5xF%fengguang.wu@intel.com>
On Sun, Oct 01, 2017 at 08:09:30AM +0800, kbuild test robot wrote:
> Hi Willem,
>
> [auto build test WARNING on net-next/master]
>
> url: https://github.com/0day-ci/linux/commits/Willem-de-Bruijn/vhost_net-do-not-stall-on-zerocopy-depletion/20171001-054709
> reproduce:
> # apt-get install sparse
> make ARCH=x86_64 allmodconfig
> make C=1 CF=-D__CHECK_ENDIAN__
BTW __CHECK_ENDIAN__ is the default now, I think you can drop it from
your scripts.
>
> sparse warnings: (new ones prefixed by >>)
>
>
> vim +440 drivers/vhost/net.c
>
> 433
> 434 static bool vhost_exceeds_maxpend(struct vhost_net *net)
> 435 {
> 436 struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
> 437 struct vhost_virtqueue *vq = &nvq->vq;
> 438
> 439 return (nvq->upend_idx + UIO_MAXIOV - nvq->done_idx) % UIO_MAXIOV >
> > 440 min(VHOST_MAX_PEND, vq->num >> 2);
> 441 }
> 442
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* Re: [PATCH v16 5/5] virtio-balloon: VIRTIO_BALLOON_F_CTRL_VQ
From: Michael S. Tsirkin @ 2017-10-01 3:18 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mawilcox, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, willy, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mhocko,
mgorman
In-Reply-To: <1506744354-20979-6-git-send-email-wei.w.wang@intel.com>
On Sat, Sep 30, 2017 at 12:05:54PM +0800, Wei Wang wrote:
> Add a new vq, ctrl_vq, to handle commands between the host and guest.
> With this feature, we will be able to have the control plane and data
> plane separated. In other words, the control related commands of each
> feature will be sent via the ctrl_vq, meanwhile each feature may have
> its own vq used as a data plane.
>
> Free page report is the the first new feature controlled via ctrl_vq,
> and a new cmd class, VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE, is added.
> Currently, this feature has two cmds:
> VIRTIO_BALLOON_FREE_PAGE_F_START: This cmd is sent from host to guest
> to start the free page report work.
> VIRTIO_BALLOON_FREE_PAGE_F_STOP: This cmd is bidirectional. The guest
> would send the cmd to the host to indicate the reporting work is done.
> The host would send the cmd to the guest to actively request the stop
> of the reporting work.
>
> The free_page_vq is used to transmit the guest free page blocks to the
> host.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> ---
> drivers/virtio/virtio_balloon.c | 249 +++++++++++++++++++++++++++++++++---
> include/uapi/linux/virtio_balloon.h | 15 +++
> 2 files changed, 244 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 6952e19..70dc4ae 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -55,7 +55,13 @@ static struct vfsmount *balloon_mnt;
>
> struct virtio_balloon {
> struct virtio_device *vdev;
> - struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
> + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *ctrl_vq,
> + *free_page_vq;
> +
> + /* Balloon's own wq for cpu-intensive work items */
> + struct workqueue_struct *balloon_wq;
> + /* The work items submitted to the balloon wq are listed here */
> + struct work_struct report_free_page_work;
>
> /* The balloon servicing is delegated to a freezable workqueue. */
> struct work_struct update_balloon_stats_work;
> @@ -65,6 +71,9 @@ struct virtio_balloon {
> spinlock_t stop_update_lock;
> bool stop_update;
>
> + /* Stop reporting free pages */
> + bool report_free_page_stop;
> +
> /* Waiting for host to ack the pages we released. */
> wait_queue_head_t acked;
>
> @@ -93,6 +102,11 @@ struct virtio_balloon {
>
> /* To register callback in oom notifier call chain */
> struct notifier_block nb;
> +
> + /* Host to guest ctrlq cmd buf for free page report */
> + struct virtio_balloon_ctrlq_cmd free_page_cmd_in;
> + /* Guest to Host ctrlq cmd buf for free page report */
> + struct virtio_balloon_ctrlq_cmd free_page_cmd_out;
> };
>
> static struct virtio_device_id id_table[] = {
> @@ -186,6 +200,24 @@ static int send_balloon_page_sg(struct virtio_balloon *vb,
> return err;
> }
>
> +static int send_free_page_sg(struct virtqueue *vq, void *addr, uint32_t size)
> +{
> + int ret = 0;
> +
> + /*
> + * Since this is an optimization feature, losing a couplle of free
typo
> + * pages to report isn't important. We simply resturn without adding
> + * the page if the vq is full.
> + */
> + if (vq->num_free) {
> + ret = add_one_sg(vq, addr, size);
> + if (!ret)
> + virtqueue_kick(vq);
> + }
> +
> + return ret;
> +}
> +
> /*
> * Send balloon pages in sgs to host. The balloon pages are recorded in the
> * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
> @@ -542,42 +574,210 @@ static void update_balloon_size_func(struct work_struct *work)
> queue_work(system_freezable_wq, work);
> }
>
> -static int init_vqs(struct virtio_balloon *vb)
> +static bool virtio_balloon_send_free_pages(void *opaque, unsigned long pfn,
> + unsigned long nr_pages)
> +{
> + struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
> + void *addr = (void *)pfn_to_kaddr(pfn);
> + uint32_t len = nr_pages << PAGE_SHIFT;
> +
> + if (vb->report_free_page_stop)
> + return false;
> +
> + /* If the vq is broken, stop reporting the free pages. */
> + if (send_free_page_sg(vb->free_page_vq, addr, len) < 0)
> + return false;
> +
> + return true;
> +}
> +
> +static void ctrlq_add_cmd(struct virtqueue *vq,
> + struct virtio_balloon_ctrlq_cmd *cmd,
> + bool inbuf)
> {
> - struct virtqueue *vqs[3];
> - vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
> - static const char * const names[] = { "inflate", "deflate", "stats" };
> - int err, nvqs;
> + struct scatterlist sg;
> + int err;
> +
> + sg_init_one(&sg, cmd, sizeof(struct virtio_balloon_ctrlq_cmd));
> + if (inbuf)
> + err = virtqueue_add_inbuf(vq, &sg, 1, cmd, GFP_KERNEL);
> + else
> + err = virtqueue_add_outbuf(vq, &sg, 1, cmd, GFP_KERNEL);
> +
> + /* Sanity check: this can't really happen */
> + WARN_ON(err);
> +}
> +
> +static void ctrlq_send_cmd(struct virtio_balloon *vb,
> + struct virtio_balloon_ctrlq_cmd *cmd,
> + bool inbuf)
> +{
> + struct virtqueue *vq = vb->ctrl_vq;
> +
> + ctrlq_add_cmd(vq, cmd, inbuf);
> + if (!inbuf) {
> + /*
> + * All the input cmd buffers are replenished here.
> + * This is necessary because the input cmd buffers are lost
> + * after live migration. The device needs to rewind all of
> + * them from the ctrl_vq.
Confused. Live migration somehow loses state? Why is that and why
is it a good idea? And how do you know this is migration even?
Looks like all you know is you got free page end. Could be any
reason for this.
> + */
> + ctrlq_add_cmd(vq, &vb->free_page_cmd_in, true);
> + }
> + virtqueue_kick(vq);
> +}
>
> +static void report_free_page_end(struct virtio_balloon *vb)
> +{
> /*
> - * We expect two virtqueues: inflate and deflate, and
> - * optionally stat.
> + * The host may have already requested to stop the reporting before we
> + * finish, so no need to notify the host in this case.
> */
> - nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2;
> - err = virtio_find_vqs(vb->vdev, nvqs, vqs, callbacks, names, NULL);
> + if (vb->report_free_page_stop)
> + return;
> +
> + vb->free_page_cmd_out.class = VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE;
> + vb->free_page_cmd_out.cmd = VIRTIO_BALLOON_FREE_PAGE_F_STOP;
> + ctrlq_send_cmd(vb, &vb->free_page_cmd_out, false);
> + vb->report_free_page_stop = true;
> +}
> +
> +static void report_free_page(struct work_struct *work)
> +{
> + struct virtio_balloon *vb;
> +
> + vb = container_of(work, struct virtio_balloon, report_free_page_work);
> + walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
> + report_free_page_end(vb);
> +}
> +
> +static void ctrlq_handle(struct virtqueue *vq)
> +{
> + struct virtio_balloon *vb = vq->vdev->priv;
> + struct virtio_balloon_ctrlq_cmd *msg;
> + unsigned int class, cmd, len;
> +
> + msg = (struct virtio_balloon_ctrlq_cmd *)virtqueue_get_buf(vq, &len);
> + if (unlikely(!msg))
> + return;
> +
> + /* The outbuf is sent by the host for recycling, so just return. */
> + if (msg == &vb->free_page_cmd_out)
> + return;
> +
> + class = virtio32_to_cpu(vb->vdev, msg->class);
> + cmd = virtio32_to_cpu(vb->vdev, msg->cmd);
> +
> + switch (class) {
> + case VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE:
> + if (cmd == VIRTIO_BALLOON_FREE_PAGE_F_STOP) {
> + vb->report_free_page_stop = true;
> + } else if (cmd == VIRTIO_BALLOON_FREE_PAGE_F_START) {
> + vb->report_free_page_stop = false;
> + queue_work(vb->balloon_wq, &vb->report_free_page_work);
> + }
> + vb->free_page_cmd_in.class =
> + VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE;
> + ctrlq_send_cmd(vb, &vb->free_page_cmd_in, true);
> + break;
> + default:
> + dev_warn(&vb->vdev->dev, "%s: cmd class not supported\n",
> + __func__);
> + }
Manipulating report_free_page_stop without any locks looks
very suspicious.
Also, what if we get two start commands? we should restart
from beginning, should we not?
> +}
> +
> +static int init_vqs(struct virtio_balloon *vb)
> +{
> + struct virtqueue **vqs;
> + vq_callback_t **callbacks;
> + const char **names;
> + struct scatterlist sg;
> + int i, nvqs, err = -ENOMEM;
> +
> + /* Inflateq and deflateq are used unconditionally */
> + nvqs = 2;
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ))
> + nvqs++;
> + /* If ctrlq is enabled, the free page vq will also be created */
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_CTRL_VQ))
> + nvqs += 2;
Since you made it generic, free page should
have its own flag not rely on ctrl vq.
> +
> + /* Allocate space for find_vqs parameters */
> + vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL);
> + if (!vqs)
> + goto err_vq;
> + callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL);
> + if (!callbacks)
> + goto err_callback;
> + names = kmalloc_array(nvqs, sizeof(*names), GFP_KERNEL);
> + if (!names)
> + goto err_names;
> +
> + callbacks[0] = balloon_ack;
> + names[0] = "inflate";
> + callbacks[1] = balloon_ack;
> + names[1] = "deflate";
> +
> + i = 2;
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> + callbacks[i] = stats_request;
> + names[i] = "stats";
> + i++;
> + }
> +
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_CTRL_VQ)) {
> + callbacks[i] = ctrlq_handle;
> + names[i++] = "ctrlq";
> + callbacks[i] = NULL;
> + names[i] = "free_page_vq";
> + }
> +
> + err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names,
> + NULL, NULL);
> if (err)
> - return err;
> + goto err_find;
>
> vb->inflate_vq = vqs[0];
> vb->deflate_vq = vqs[1];
> + i = 2;
> if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> - struct scatterlist sg;
> - unsigned int num_stats;
> - vb->stats_vq = vqs[2];
> -
> + vb->stats_vq = vqs[i++];
> /*
> * Prime this virtqueue with one buffer so the hypervisor can
> * use it to signal us later (it can't be broken yet!).
> */
> - num_stats = update_balloon_stats(vb);
> -
> - sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
> + sg_init_one(&sg, vb->stats, sizeof(vb->stats));
> if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
> - < 0)
> - BUG();
> + < 0) {
> + dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n",
> + __func__);
> + goto err_find;
> + }
> virtqueue_kick(vb->stats_vq);
> }
> +
> + if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_CTRL_VQ)) {
> + vb->ctrl_vq = vqs[i++];
> + vb->free_page_vq = vqs[i];
> + /* Prime the ctrlq with an inbuf for the host to send a cmd */
> + vb->free_page_cmd_in.class =
> + VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE;
> + ctrlq_send_cmd(vb, &vb->free_page_cmd_in, true);
> + }
> +
> + kfree(names);
> + kfree(callbacks);
> + kfree(vqs);
> return 0;
> +
> +err_find:
> + kfree(names);
> +err_names:
> + kfree(callbacks);
> +err_callback:
> + kfree(vqs);
> +err_vq:
> + return err;
> }
>
> #ifdef CONFIG_BALLOON_COMPACTION
> @@ -706,6 +906,13 @@ static int virtballoon_probe(struct virtio_device *vdev)
> if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SG))
> xb_init(&vb->page_xb);
>
> + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_CTRL_VQ)) {
> + vb->balloon_wq = alloc_workqueue("balloon-wq",
> + WQ_FREEZABLE | WQ_CPU_INTENSIVE, 0);
> + INIT_WORK(&vb->report_free_page_work, report_free_page);
> + vb->report_free_page_stop = true;
> + }
> +
> vb->nb.notifier_call = virtballoon_oom_notify;
> vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
> err = register_oom_notifier(&vb->nb);
> @@ -770,6 +977,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
> spin_unlock_irq(&vb->stop_update_lock);
> cancel_work_sync(&vb->update_balloon_size_work);
> cancel_work_sync(&vb->update_balloon_stats_work);
> + cancel_work_sync(&vb->report_free_page_work);
>
> remove_common(vb);
> #ifdef CONFIG_BALLOON_COMPACTION
> @@ -823,6 +1031,7 @@ static unsigned int features[] = {
> VIRTIO_BALLOON_F_STATS_VQ,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
> VIRTIO_BALLOON_F_SG,
> + VIRTIO_BALLOON_F_CTRL_VQ,
> };
>
> static struct virtio_driver virtio_balloon_driver = {
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 37780a7..dbf0616 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -35,6 +35,7 @@
> #define VIRTIO_BALLOON_F_STATS_VQ 1 /* Memory Stats virtqueue */
> #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */
> #define VIRTIO_BALLOON_F_SG 3 /* Use sg instead of PFN lists */
> +#define VIRTIO_BALLOON_F_CTRL_VQ 4 /* Control Virtqueue */
>
> /* Size of a PFN in the balloon interface. */
> #define VIRTIO_BALLOON_PFN_SHIFT 12
> @@ -83,4 +84,18 @@ struct virtio_balloon_stat {
> __virtio64 val;
> } __attribute__((packed));
>
> +enum {
> + VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE = 0,
> + VIRTIO_BALLOON_CTRLQ_CLASS_MAX,
> +};
> +
> +struct virtio_balloon_ctrlq_cmd {
> + __virtio32 class;
> + __virtio32 cmd;
> +};
> +
> +/* Ctrlq commands related to VIRTIO_BALLOON_CTRLQ_CLASS_FREE_PAGE */
> +#define VIRTIO_BALLOON_FREE_PAGE_F_STOP 0
> +#define VIRTIO_BALLOON_FREE_PAGE_F_START 1
> +
> #endif /* _LINUX_VIRTIO_BALLOON_H */
The stop command does not appear to be thought through.
Let's assume e.g. you started migration. You ask guest for free pages.
Then you cancel it. There are a bunch of pages in free vq and you are
getting more. You now want to start migration again. What to do?
A bunch of vq flushing and waiting will maybe do the trick, but waiting
on guest is never a great idea.
I previously suggested pushing the stop/start commands from guest to
host on the free page vq, and including an ID in host to guest and
guest to host commands. This way ctrl vq is just for host to guest
commands, and host matches commands and knows which command
is a free page in response to.
I still think it's a good idea but go ahead and propose something
else that works.
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net-next] vhost_net: do not stall on zerocopy depletion
From: kbuild test robot @ 2017-10-01 0:09 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Willem de Bruijn, mst, netdev, den, virtualization, kbuild-all,
davem
In-Reply-To: <20170928002556.41240-1-willemdebruijn.kernel@gmail.com>
Hi Willem,
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Willem-de-Bruijn/vhost_net-do-not-stall-on-zerocopy-depletion/20171001-054709
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
vim +440 drivers/vhost/net.c
433
434 static bool vhost_exceeds_maxpend(struct vhost_net *net)
435 {
436 struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
437 struct vhost_virtqueue *vq = &nvq->vq;
438
439 return (nvq->upend_idx + UIO_MAXIOV - nvq->done_idx) % UIO_MAXIOV >
> 440 min(VHOST_MAX_PEND, vq->num >> 2);
441 }
442
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* Re: [PATCH net-next] vhost_net: do not stall on zerocopy depletion
From: kbuild test robot @ 2017-09-30 22:20 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Willem de Bruijn, mst, netdev, den, virtualization, kbuild-all,
davem
In-Reply-To: <20170928002556.41240-1-willemdebruijn.kernel@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]
Hi Willem,
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Willem-de-Bruijn/vhost_net-do-not-stall-on-zerocopy-depletion/20171001-054709
config: tile-allyesconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 4.6.2
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=tile
All warnings (new ones prefixed by >>):
drivers/vhost/net.c: In function 'vhost_exceeds_maxpend':
>> drivers/vhost/net.c:440:9: warning: comparison of distinct pointer types lacks a cast [enabled by default]
vim +440 drivers/vhost/net.c
433
434 static bool vhost_exceeds_maxpend(struct vhost_net *net)
435 {
436 struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
437 struct vhost_virtqueue *vq = &nvq->vq;
438
439 return (nvq->upend_idx + UIO_MAXIOV - nvq->done_idx) % UIO_MAXIOV >
> 440 min(VHOST_MAX_PEND, vq->num >> 2);
441 }
442
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50505 bytes --]
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] vhost_net: do not stall on zerocopy depletion
From: kbuild test robot @ 2017-09-30 22:12 UTC (permalink / raw)
To: Willem de Bruijn
Cc: Willem de Bruijn, mst, netdev, den, virtualization, kbuild-all,
davem
In-Reply-To: <20170928002556.41240-1-willemdebruijn.kernel@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]
Hi Willem,
[auto build test WARNING on net-next/master]
url: https://github.com/0day-ci/linux/commits/Willem-de-Bruijn/vhost_net-do-not-stall-on-zerocopy-depletion/20171001-054709
config: x86_64-randconfig-x002-201740 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
In file included from include/linux/list.h:8:0,
from include/linux/wait.h:6,
from include/linux/eventfd.h:12,
from drivers/vhost/net.c:10:
drivers/vhost/net.c: In function 'vhost_exceeds_maxpend':
include/linux/kernel.h:772:16: warning: comparison of distinct pointer types lacks a cast
(void) (&min1 == &min2); \
^
include/linux/kernel.h:775:2: note: in expansion of macro '__min'
__min(typeof(x), typeof(y), \
^~~~~
>> drivers/vhost/net.c:440:9: note: in expansion of macro 'min'
min(VHOST_MAX_PEND, vq->num >> 2);
^~~
vim +/min +440 drivers/vhost/net.c
433
434 static bool vhost_exceeds_maxpend(struct vhost_net *net)
435 {
436 struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
437 struct vhost_virtqueue *vq = &nvq->vq;
438
439 return (nvq->upend_idx + UIO_MAXIOV - nvq->done_idx) % UIO_MAXIOV >
> 440 min(VHOST_MAX_PEND, vq->num >> 2);
441 }
442
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25413 bytes --]
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* RE: [PATCH v15 2/5] lib/xbitmap: add xb_find_next_bit() and xb_zero()
From: Wang, Wei W @ 2017-09-30 4:24 UTC (permalink / raw)
To: Matthew Wilcox
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, mst@redhat.com, qemu-devel@nongnu.org,
amit.shah@redhat.com, liliang.opensource@gmail.com,
mawilcox@microsoft.com, linux-kernel@vger.kernel.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" <mhocko>
In-Reply-To: <20170911132710.GB32538@bombadil.infradead.org>
On Monday, September 11, 2017 9:27 PM, Matthew Wilcox wrote
> On Mon, Aug 28, 2017 at 06:08:30PM +0800, Wei Wang wrote:
> > +/**
> > + * xb_zero - zero a range of bits in the xbitmap
> > + * @xb: the xbitmap that the bits reside in
> > + * @start: the start of the range, inclusive
> > + * @end: the end of the range, inclusive */ void xb_zero(struct xb
> > +*xb, unsigned long start, unsigned long end) {
> > + unsigned long i;
> > +
> > + for (i = start; i <= end; i++)
> > + xb_clear_bit(xb, i);
> > +}
> > +EXPORT_SYMBOL(xb_zero);
>
> Um. This is not exactly going to be quick if you're clearing a range of bits.
> I think it needs to be more along the lines of this:
>
> void xb_clear(struct xb *xb, unsigned long start, unsigned long end) {
> struct radix_tree_root *root = &xb->xbrt;
> struct radix_tree_node *node;
> void **slot;
> struct ida_bitmap *bitmap;
>
> for (; end < start; start = (start | (IDA_BITMAP_BITS - 1)) + 1) {
> unsigned long index = start / IDA_BITMAP_BITS;
> unsigned long bit = start % IDA_BITMAP_BITS;
>
> bitmap = __radix_tree_lookup(root, index, &node, &slot);
> if (radix_tree_exception(bitmap)) {
> unsigned long ebit = bit + 2;
> unsigned long tmp = (unsigned long)bitmap;
> if (ebit >= BITS_PER_LONG)
> continue;
> tmp &= ... something ...;
> if (tmp == RADIX_TREE_EXCEPTIONAL_ENTRY)
> __radix_tree_delete(root, node, slot);
> else
> rcu_assign_pointer(*slot, (void *)tmp);
> } else if (bitmap) {
> unsigned int nbits = end - start + 1;
> if (nbits + bit > IDA_BITMAP_BITS)
> nbits = IDA_BITMAP_BITS - bit;
> bitmap_clear(bitmap->bitmap, bit, nbits);
> if (bitmap_empty(bitmap->bitmap, IDA_BITMAP_BITS)) {
> kfree(bitmap);
> __radix_tree_delete(root, node, slot);
> }
> }
> }
> }
>
> Also note that this should be called xb_clear(), not xb_zero() to fit in with
> bitmap_clear(). And this needs a thorough test suite testing all values for 'start'
> and 'end' between 0 and at least 1024; probably much higher. And a variable
> number of bits need to be set before calling
> xb_clear() in the test suite.
>
> Also, this implementation above is missing a few tricks. For example, if 'bit' is 0
> and 'nbits' == IDA_BITMAP_BITS, we can simply call kfree without first zeroing
> out the bits and then checking if the whole thing is zero.
Thanks for the optimization suggestions. We've seen significant improvement of
the ballooning time. Some other optimizations (stated in the changelog) haven't
been included in the new version. If possible, we can leave that to a second step
optimization outside this patch series.
Best,
Wei
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox