Re: [PATCH] virtio_console: Stop doing DMA on the stack
* Michael S. Tsirkin wrote:
> On Thu, Sep 08, 2016 at 08:49:43AM +0200, Ingo Molnar wrote:
> >
> > * Amit Shah wrote:
> >
> > > On (Tue) 30 Aug 2016 [08:04:15], Andy Lutomirski wrote:
> > > > virtio_console uses a small DMA buffer for control requests. Move
> > > > that buffer into heap memory.
> > > >
> > > > Doing virtio DMA on the stack is normally okay on non-DMA-API virtio
> > > > systems (which is currently most of them), but it breaks completely
> > > > if the stack is virtually mapped.
> > > >
> > > > Tested by typing both directions using picocom aimed at /dev/hvc0.
> > > >
> > > > Signed-off-by: Andy Lutomirski
> > >
> > > Looks fine,
> > >
> > > Reviewed-by: Amit Shah
> > >
> > > > ---
> > > >
> > > > Hi all-
> > > >
> > > > This is currently broken in tip:x86/asm. If you (Amit) like this patch,
> > > > would it make sense for Ingo to add it to -tip?
> > >
> > > Yes, I'm fine with that.
> >
> > Thanks! FYI, this patch now lives as:
> >
> > 9472fe7040bb ("virtio_console: Stop doing DMA on the stack")
> >
> > in tip:x86/asm, and is targeted for a v4.9 merge.
> >
> > Thanks,
> >
> > Ingo
>
> Thinking about it, maybe we should put it in 4.8
> after all, for benefit of systems using DMA API with virtio.
> Thoughts?
So CONFIG_VMAP_STACK=y is only going to be enabled for v4.9 - the enabling
commit
is part of tip:x86/asm as well. So AFAICS the commit is not strictly needed for
v4.8.
Thanks,
Ingo
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PULL] virtio: fixes for 4.8
The following changes since commit 3eab887a55424fc2c27553b7bfe32330df83f7b8: Linux 4.8-rc4 (2016-08-28 15:04:33 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus for you to fetch changes up to 5e59d9a1aed26abcc79abe78af5cfd34e53cbe7f: virtio_console: Stop doing DMA on the stack (2016-09-09 21:12:45 +0300) virtio: fixes for 4.8 This includes a couple of bugfixs for virtio. The virtio console patch is actually also in x86/tip targeting 4.9 because it helps vmap stacks, but it also fixes IOMMU_PLATFORM which was added in 4.8, and it seems important not to ship that in a broken configuration. Signed-off-by: Michael S. Tsirkin Andy Lutomirski (1): virtio_console: Stop doing DMA on the stack Baoyou Xie (1): virtio: mark vring_dma_dev() static drivers/char/virtio_console.c | 23 +++ drivers/virtio/virtio_ring.c | 2 +- 2 files changed, 16 insertions(+), 9 deletions(-) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_console: Stop doing DMA on the stack
On Thu, Sep 08, 2016 at 08:49:43AM +0200, Ingo Molnar wrote:
>
> * Amit Shah wrote:
>
> > On (Tue) 30 Aug 2016 [08:04:15], Andy Lutomirski wrote:
> > > virtio_console uses a small DMA buffer for control requests. Move
> > > that buffer into heap memory.
> > >
> > > Doing virtio DMA on the stack is normally okay on non-DMA-API virtio
> > > systems (which is currently most of them), but it breaks completely
> > > if the stack is virtually mapped.
> > >
> > > Tested by typing both directions using picocom aimed at /dev/hvc0.
> > >
> > > Signed-off-by: Andy Lutomirski
> >
> > Looks fine,
> >
> > Reviewed-by: Amit Shah
> >
> > > ---
> > >
> > > Hi all-
> > >
> > > This is currently broken in tip:x86/asm. If you (Amit) like this patch,
> > > would it make sense for Ingo to add it to -tip?
> >
> > Yes, I'm fine with that.
>
> Thanks! FYI, this patch now lives as:
>
> 9472fe7040bb ("virtio_console: Stop doing DMA on the stack")
>
> in tip:x86/asm, and is targeted for a v4.9 merge.
>
> Thanks,
>
> Ingo
Thinking about it, maybe we should put it in 4.8
after all, for benefit of systems using DMA API with virtio.
Thoughts?
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/1] balloon: stop inflate balloon after oom notification
On Fri, Sep 09, 2016 at 04:54:44PM +0300, Denis V. Lunev wrote:
> From: Konstantin Neumoin
>
> At this moment oom notification in balloon does not work as expected.
> After virtballoon_oom_notify there is an infinitive loop:
> - virtballoon_oom_notify was called and balloon was deflated
> - balloon get notification that config was changed, compare target and
>actual and try to reach target again.
>
> This patch adds global variable fail_counter which indicates that oom has
> been happened. We check that fail_counter was changed between calls
> update_balloon_size_func. In this case we should not try to inflate balloon
> even if actual != target.
Doesn't look right to me. What if it triggered an hour ago?
We really need mm core to expose an oom_in_progress flag that we can test.
More implementation comments below.
>
> Signed-off-by: Konstantin Neumoin
> Signed-off-by: Denis V. Lunev
> CC: Michael S. Tsirkin
> ---
> drivers/virtio/virtio_balloon.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 4e7003d..253bf05 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -50,6 +50,8 @@ MODULE_PARM_DESC(oom_pages, "pages to free on OOM");
> static struct vfsmount *balloon_mnt;
> #endif
>
> +static unsigned long fail_count;
> +
> struct virtio_balloon {
> struct virtio_device *vdev;
> struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
> @@ -361,6 +363,8 @@ static int virtballoon_oom_notify(struct notifier_block
> *self,
> unsigned long *freed;
> unsigned num_freed_pages;
>
> + fail_count++;
> +
> vb = container_of(self, struct virtio_balloon, nb);
> if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> return NOTIFY_OK;
OOM should not interfere with ballooning unless
VIRTIO_BALLOON_F_DEFLATE_ON_OOM has been negotiated.
> @@ -386,11 +390,22 @@ static void update_balloon_size_func(struct work_struct
> *work)
> {
> struct virtio_balloon *vb;
> s64 diff;
> + static unsigned long fc;
> +
> + if (fc == 0)
> + fc = fail_count;
Why?
>
> vb = container_of(work, struct virtio_balloon,
> update_balloon_size_work);
> diff = towards_target(vb);
>
> + if (fc != fail_count) {
> + fc = fail_count;
Unlikely to work correctly if there are multiple balloon devices.
> + /* Don't inflate balloon after oom notification */
> + if (diff > 0)
> + return;
> + }
> +
> if (diff > 0)
> diff -= fill_balloon(vb, diff);
> else if (diff < 0)
I'd rather make it per-device.
Absence of memory ordering primitives of any kind also
looks suspicious.
> --
> 2.7.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 1/1] balloon: stop inflate balloon after oom notification
From: Konstantin Neumoin
At this moment oom notification in balloon does not work as expected.
After virtballoon_oom_notify there is an infinitive loop:
- virtballoon_oom_notify was called and balloon was deflated
- balloon get notification that config was changed, compare target and
actual and try to reach target again.
This patch adds global variable fail_counter which indicates that oom has
been happened. We check that fail_counter was changed between calls
update_balloon_size_func. In this case we should not try to inflate balloon
even if actual != target.
Signed-off-by: Konstantin Neumoin
Signed-off-by: Denis V. Lunev
CC: Michael S. Tsirkin
---
drivers/virtio/virtio_balloon.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 4e7003d..253bf05 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -50,6 +50,8 @@ MODULE_PARM_DESC(oom_pages, "pages to free on OOM");
static struct vfsmount *balloon_mnt;
#endif
+static unsigned long fail_count;
+
struct virtio_balloon {
struct virtio_device *vdev;
struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
@@ -361,6 +363,8 @@ static int virtballoon_oom_notify(struct notifier_block
*self,
unsigned long *freed;
unsigned num_freed_pages;
+ fail_count++;
+
vb = container_of(self, struct virtio_balloon, nb);
if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
return NOTIFY_OK;
@@ -386,11 +390,22 @@ static void update_balloon_size_func(struct work_struct
*work)
{
struct virtio_balloon *vb;
s64 diff;
+ static unsigned long fc;
+
+ if (fc == 0)
+ fc = fail_count;
vb = container_of(work, struct virtio_balloon,
update_balloon_size_work);
diff = towards_target(vb);
+ if (fc != fail_count) {
+ fc = fail_count;
+ /* Don't inflate balloon after oom notification */
+ if (diff > 0)
+ return;
+ }
+
if (diff > 0)
diff -= fill_balloon(vb, diff);
else if (diff < 0)
--
2.7.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
