* 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: [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: [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: [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: [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: [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 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: [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] 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: [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: [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: [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: [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: [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: [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: [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: 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: [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: [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
* (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 net-next] vhost_net: do not stall on zerocopy depletion
From: Willem de Bruijn @ 2017-10-02 21:34 UTC (permalink / raw)
To: Michael S. Tsirkin
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 9:25 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> 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.
This seems to be a mix of page alignment and compound pages.
The iov_len is always well below the maximum, but frags exceed
page size and can start high in the initial page.
tun_get_user: num_pages=21 max=17 iov_len=6 len=65226
0: p_off=3264 len=6888
1: p_off=1960 len=16384
2: p_off=1960 len=6232
3: p_off=0 len=10152
4: p_off=1960 len=16384
5: p_off=1960 len=9120
> 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;
>
Testing iov_iter_npages() in handle_tx to inform zcopy_used allows
skipping these without turning off zerocopy for all other packets.
After implementing that, tx_zcopy_err drops to zero, but only around
40% of packets use zerocopy.
^ permalink raw reply
* Re: [virtio-dev] [RFC] virtio-iommu version 0.4
From: Auger Eric @ 2017-10-03 13:04 UTC (permalink / raw)
To: Jean-Philippe Brucker, iommu, kvm, virtualization, virtio-dev
Cc: lorenzo.pieralisi, mst, marc.zyngier, will.deacon, robin.murphy,
eric.auger.pro
In-Reply-To: <20170804181927.12148-1-jean-philippe.brucker@arm.com>
Hi Jean,
On 04/08/2017 20:19, Jean-Philippe Brucker wrote:
> This is the continuation of my proposal for virtio-iommu, the para-
> virtualized IOMMU. Here is a summary of the changes since last time [1]:
>
> * The virtio-iommu document now resembles an actual specification. It is
> split into a formal description of the virtio device, and implementation
> notes. Please find sources and binaries at [2].
>
> * Added a probe request to describe to the guest different properties that
> do not fit in firmware or in the virtio config space. This is a
> necessary stepping stone for extending the virtio-iommu.
>
> * There is a working Qemu prototype [3], thanks to Eric Auger and Bharat
> Bhushan.
>
> You can find the Linux driver and kvmtool device at [4] and [5]. I
> plan to rework driver and kvmtool device slightly before sending the
> patches.
>
> To understand the virtio-iommu, I advise to first read introduction and
> motivation, then skim through implementation notes and finally look at the
> device specification.
>
> I wasn't sure how to organize the review. For those who prefer to comment
> inline, I attached v0.4 of device-operations.tex and topology.tex+MSI.tex
> to this thread. They are the biggest chunks of the document. But LaTeX
> isn't very pleasant to read, so you can simply send a list of comments in
> relation to section numbers and a few words of context, we'll manage.
>
> ---
> Version numbers 0.1-0.4 are arbitrary. I'm hoping they allow to compare
> more easily differences since the RFC (see [6]), but haven't been made
> public so far. This is the first public posting since initial proposal
> [1], and the following describes all changes.
>
> ## v0.1 ##
>
> Content is the same as the RFC, but formatted to LaTeX. 'make' generates
> one PDF and one HTML document.
>
> ## v0.2 ##
>
> Add introductions, improve topology example and firmware description based
> on feedback and a number of useful discussions.
>
> ## v0.3 ##
>
> Add normative sections (MUST, SHOULD, etc). Clarify some things, tighten
> the device and driver behaviour. Unmap semantics are consolidated; they
> are now closer to VFIO Type1 v2 semantics.
>
> ## v0.4 ##
>
> Introduce PROBE requests. They provide per-endpoint information to the
> driver that couldn't be described otherwise.
>
> For the moment, they allow to handle MSIs on x86 virtual platforms (see
> 3.2). To do that we communicate reserved IOVA regions, that will also be
> useful for describing regions that cannot be mapped for a given endpoint,
> for instance addresses that correspond to a PCI bridge window.
>
> Introducing such a large framework for this tiny feature may seem
> overkill, but it is needed for future extensions of the virtio-iommu and I
> believe it really is worth the effort.
>
> ## Future ##
>
> Other extensions are in preparation. I won't detail them here because v0.4
> already is a lot to digest, but in short, building on top of PROBE:
>
> * First, since the IOMMU is paravirtualized, the device can expose some
> properties of the physical topology to the guest, and let it allocate
> resources more efficiently. For example, when the virtio-iommu manages
> both physical and emulated endpoints, with different underlying IOMMUs,
> we now have a way to describe multiple page and block granularities,
> instead of forcing the guest to use the most restricted one for all
> endpoints. This will most likely be in v0.5.
>
> * Then on top of that, a major improvement will describe hardware
> acceleration features available to the guest. There is what I call "Page
> Table Handover" (or simply, from the host POV, "Nested"), the ability
> for the guest to manipulate its own page tables instead of sending
> MAP/UNMAP requests to the host. This, along with IO Page Fault
> reporting, will also permit SVM virtualization on different platforms.
>
> Thanks,
> Jean
>
> [1] http://www.spinics.net/lists/kvm/msg147990.html
> [2] git://linux-arm.org/virtio-iommu.git branch viommu/v0.4
> http://www.linux-arm.org/git?p=virtio-iommu.git;a=blob;f=dist/v0.4/virtio-iommu-v0.4.pdf
> I reiterate the disclaimers: don't use this document as a reference,
> it's a draft. It's also not an OASIS document yet. It may be riddled
> with mistakes. As this is a working draft, it is unstable and I do not
> guarantee backward compatibility of future versions.
> [3] https://lists.gnu.org/archive/html/qemu-arm/2017-08/msg00004.html
> [4] git://linux-arm.org/linux-jpb.git virtio-iommu/v0.4
> Warning: UAPI headers have changed! They didn't follow the spec,
> please update. (Use branch v0.1, that has the old headers, for the
> Qemu prototype [3])
When rebasing the v0.4 driver on master I observe a regression: commands
are not received properly by QEMU (typically an attach command is
received with a type of 0). After a bisection of the guest kernel the
first commit the problem appears is:
commit e3067861ba6650a566a6273738c23c956ad55c02
arm64: add basic VMAP_STACK support
After reverting this patch, things resume working.
I observe the problem with a 4kB page guest kernel.
Thanks
Eric
> [5] git://linux-arm.org/kvmtool-jpb.git virtio-iommu/v0.4
> Warning: command-line has changed! Use --viommu vfio[,opts] and
> --viommu virtio[,opts] to instantiate a device.
> [6] http://www.linux-arm.org/git?p=virtio-iommu.git;a=tree;f=dist/diffs
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>
^ permalink raw reply
* Re: [PATCH v16 4/5] mm: support reporting free page blocks
From: Michal Hocko @ 2017-10-03 14:50 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mst, qemu-devel, amit.shah,
liliang.opensource, mawilcox, linux-kernel, willy, virtualization,
linux-mm, yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm,
mgorman
In-Reply-To: <1506744354-20979-5-git-send-email-wei.w.wang@intel.com>
On Sat 30-09-17 12:05:53, Wei Wang wrote:
> This patch adds support to walk through the free page blocks in the
> system and report them via a callback function. Some page blocks may
> leave the free list after zone->lock is released, so it is the caller's
> responsibility to either detect or prevent the use of such pages.
>
> One use example of this patch is to accelerate live migration by skipping
> the transfer of free pages reported from the guest. A popular method used
> by the hypervisor to track which part of memory is written during live
> migration is to write-protect all the guest memory. So, those pages that
> are reported as free pages but are written after the report function
> returns will be captured by the hypervisor, and they will be added to the
> next round of memory transfer.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> include/linux/mm.h | 6 ++++
> mm/page_alloc.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 97 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 46b9ac5..d9652c2 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1835,6 +1835,12 @@ extern void free_area_init_node(int nid, unsigned long * zones_size,
> unsigned long zone_start_pfn, unsigned long *zholes_size);
> extern void free_initmem(void);
>
> +extern void walk_free_mem_block(void *opaque,
> + int min_order,
> + bool (*report_pfn_range)(void *opaque,
> + unsigned long pfn,
> + unsigned long num));
> +
> /*
> * Free reserved pages within range [PAGE_ALIGN(start), end & PAGE_MASK)
> * into the buddy system. The freed pages will be poisoned with pattern
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6d00f74..c6bb874 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4762,6 +4762,97 @@ void show_free_areas(unsigned int filter, nodemask_t *nodemask)
> show_swap_cache_info();
> }
>
> +/*
> + * Walk through a free page list and report the found pfn range via the
> + * callback.
> + *
> + * Return false if the callback requests to stop reporting. Otherwise,
> + * return true.
> + */
> +static bool walk_free_page_list(void *opaque,
> + struct zone *zone,
> + int order,
> + enum migratetype mt,
> + bool (*report_pfn_range)(void *,
> + unsigned long,
> + unsigned long))
> +{
> + struct page *page;
> + struct list_head *list;
> + unsigned long pfn, flags;
> + bool ret;
> +
> + spin_lock_irqsave(&zone->lock, flags);
> + list = &zone->free_area[order].free_list[mt];
> + list_for_each_entry(page, list, lru) {
> + pfn = page_to_pfn(page);
> + ret = report_pfn_range(opaque, pfn, 1 << order);
> + if (!ret)
> + break;
> + }
> + spin_unlock_irqrestore(&zone->lock, flags);
> +
> + return ret;
> +}
> +
> +/**
> + * walk_free_mem_block - Walk through the free page blocks in the system
> + * @opaque: the context passed from the caller
> + * @min_order: the minimum order of free lists to check
> + * @report_pfn_range: the callback to report the pfn range of the free pages
> + *
> + * If the callback returns false, stop iterating the list of free page blocks.
> + * Otherwise, continue to report.
> + *
> + * Please note that there are no locking guarantees for the callback and
> + * that the reported pfn range might be freed or disappear after the
> + * callback returns so the caller has to be very careful how it is used.
> + *
> + * The callback itself must not sleep or perform any operations which would
> + * require any memory allocations directly (not even GFP_NOWAIT/GFP_ATOMIC)
> + * or via any lock dependency. It is generally advisable to implement
> + * the callback as simple as possible and defer any heavy lifting to a
> + * different context.
> + *
> + * There is no guarantee that each free range will be reported only once
> + * during one walk_free_mem_block invocation.
> + *
> + * pfn_to_page on the given range is strongly discouraged and if there is
> + * an absolute need for that make sure to contact MM people to discuss
> + * potential problems.
> + *
> + * The function itself might sleep so it cannot be called from atomic
> + * contexts.
> + *
> + * In general low orders tend to be very volatile and so it makes more
> + * sense to query larger ones first for various optimizations which like
> + * ballooning etc... This will reduce the overhead as well.
> + */
> +void walk_free_mem_block(void *opaque,
> + int min_order,
> + bool (*report_pfn_range)(void *opaque,
> + unsigned long pfn,
> + unsigned long num))
> +{
> + struct zone *zone;
> + int order;
> + enum migratetype mt;
> + bool ret;
> +
> + for_each_populated_zone(zone) {
> + for (order = MAX_ORDER - 1; order >= min_order; order--) {
> + for (mt = 0; mt < MIGRATE_TYPES; mt++) {
> + ret = walk_free_page_list(opaque, zone,
> + order, mt,
> + report_pfn_range);
> + if (!ret)
> + return;
> + }
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(walk_free_mem_block);
> +
> static void zoneref_set_zone(struct zone *zone, struct zoneref *zoneref)
> {
> zoneref->zone = zone;
> --
> 2.7.4
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [virtio-dev] packed ring layout proposal v3
From: Jens Freimann @ 2017-10-04 12:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev@lists.oasis-open.org, Steven Luong (sluong),
virtualization@lists.linux-foundation.org
In-Reply-To: <20171001063817-mutt-send-email-mst@kernel.org>
On Sun, Oct 01, 2017 at 04:08:29AM +0000, Michael S. Tsirkin wrote:
>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.
Basically DESC_HW from v2 split in two?
>
>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.
This is confusing me a bit.
My understanding is:
1. the internally kept wrap value only flipped when the first
descriptor is overwritten
2. the moment the first descriptor is written the internal wrap value
is flipped 0->1 or 1->0 and this value is written to every descriptor
DESC_WRAP until we reach the first descriptor again
>
>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
The 0x prefix might add to the confusion here. It is really just two
bits, no?
>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?
So far it makes sense to me.
>---------------
>
>
>
>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.
DESC_WRAP is changed by the device now, so this would work differently
than in the scenario from above.
This would mean we don't need the internally kept wrap value, right?
regards,
Jens
^ permalink raw reply
* Re: [virtio-dev] packed ring layout proposal v3
From: Michael S. Tsirkin @ 2017-10-04 12:58 UTC (permalink / raw)
To: Jens Freimann
Cc: virtio-dev@lists.oasis-open.org, Steven Luong (sluong),
virtualization@lists.linux-foundation.org
In-Reply-To: <20171004123901.oomueufqg52uhas4@localhost.localdomain>
On Wed, Oct 04, 2017 at 02:39:01PM +0200, Jens Freimann wrote:
> On Sun, Oct 01, 2017 at 04:08:29AM +0000, Michael S. Tsirkin wrote:
> > 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.
>
> Basically DESC_HW from v2 split in two?
Yes in order to avoid overwriting all descriptors.
> >
> > 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.
>
> This is confusing me a bit.
>
> My understanding is: 1. the internally kept wrap value only flipped when the
> first
> descriptor is overwritten
>
> 2. the moment the first descriptor is written the internal wrap value
> is flipped 0->1 or 1->0 and this value is written to every descriptor
> DESC_WRAP until we reach the first descriptor again
Yes this is what I tried to say. Can you suggest a better wording then?
> >
> > 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
>
> The 0x prefix might add to the confusion here. It is really just two
> bits, no?
Ouch. Yes I meant 0b. Thanks!
> > 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?
>
> So far it makes sense to me.
> > ---------------
> >
> >
> >
> > 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.
>
> DESC_WRAP is changed by the device now, so this would work differently
> than in the scenario from above. This would mean we don't need the
> internally kept wrap value, right?
I think no but I did not complete simulating this yet so I don't
yet know if it even works.
>
> regards,
> Jens
^ 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