virtio balloon: do not call blocking ops when !TASK_RUNNING

virtio balloon: do not call blocking ops when !TASK_RUNNING

[Thomas Huth]

 Hi all,

with the recent kernel 3.19, I get a kernel warning when I start my
KVM guest on s390 with virtio balloon enabled:

[    0.839687] do not call blocking ops when !TASK_RUNNING; state=1 set at
               [<0000000000174a1e>] prepare_to_wait_event+0x7e/0x108
[    0.839694] ------------[ cut here ]------------
[    0.839697] WARNING: at kernel/sched/core.c:7326
[    0.839698] Modules linked in:
[    0.839702] CPU: 0 PID: 46 Comm: vballoon Not tainted 3.19.0 #233
[    0.839705] task: 00000000021d0000 ti: 00000000021d8000 task.ti: 00000000021d8000
[    0.839707] Krnl PSW : 0704c00180000000 000000000015bf8e (__might_sleep+0x8e/0x98)
[    0.839713]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:0 PM:0 EA:3
Krnl GPRS: 000000000000000d 00000000021d0000 0000000000000071 0000000000000001
[    0.839718]            0000000000675ace 0000000001998c50 cccccccccccccccc cccccccccccccccc
[    0.839720]            0000000000982134 000000000058f824 0000000000a008a8 0000000000000000
[    0.839722]            00000000000004d9 00000000007ea992 000000000015bf8a 00000000021dbc28
[    0.839731] Krnl Code: 000000000015bf7e: c0200033e838	larl	%r2,7d8fee
           000000000015bf84: c0e50028cd62	brasl	%r14,675a48
          #000000000015bf8a: a7f40001		brc	15,15bf8c
          >000000000015bf8e: 9201a000		mvi	0(%r10),1
           000000000015bf92: a7f4ffe2		brc	15,15bf56
           000000000015bf96: 0707		bcr	0,%r7
           000000000015bf98: ebdff0800024	stmg	%r13,%r15,128(%r15)
           000000000015bf9e: a7f13fe0		tmll	%r15,16352
[    0.839749] Call Trace:
[    0.839751] ([<000000000015bf8a>] __might_sleep+0x8a/0x98)
[    0.839756]  [<000000000028a562>] __kmalloc+0x272/0x350
[    0.839759]  [<000000000058f824>] virtio_ccw_get_config+0x3c/0x100
[    0.839762]  [<000000000049fcb0>] balloon+0x1b8/0x330
[    0.839765]  [<00000000001529c8>] kthread+0x120/0x138
[    0.839767]  [<0000000000683c22>] kernel_thread_starter+0x6/0xc
[    0.839770]  [<0000000000683c1c>] kernel_thread_starter+0x0/0xc
[    0.839772] no locks held by vballoon/46.
[    0.839773] Last Breaking-Event-Address:
[    0.839776]  [<000000000015bf8a>] __might_sleep+0x8a/0x98
[    0.839778] ---[ end trace d27fcdfa27273d7c ]---

The problem seems to be this code in balloon() in
drivers/virtio/virtio_balloon.c:

	wait_event_interruptible(vb->config_change,
				 (diff = towards_target(vb)) != 0
				 || vb->need_stats_update
				 || kthread_should_stop()
				 || freezing(current));

wait_event_interruptible() sets the state of the current task to
TASK_INTERRUPTIBLE, then checks the condition. The condition contains
towards_target() which reads the virtio config space via virtio_cread().
On s390, this then triggers virtio_ccw_get_config() - and this function
calls some other functions again that might sleep (e.g. kzalloc or
wait_event in ccw_io_helper) ... and this causes the new kernel warning
message with kernel 3.19.

I think it would be quite difficult or at least ugly to rewrite
virtio_ccw_get_config() so that it does not call sleepable functions
anymore. So would it be feasible to rewrite the balloon() function that
it does not call the towards_target() in its wait_event condition
anymore? I am unfortunately not that familiar with the balloon code
semantics, so any help is very appreciated here!

 Thanks,
  Thomas

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Cornelia Huck]

On Wed, 25 Feb 2015 11:13:18 +0100
Thomas Huth <thuth@linux.vnet.ibm.com> wrote:

> 
>  Hi all,
> 
> with the recent kernel 3.19, I get a kernel warning when I start my
> KVM guest on s390 with virtio balloon enabled:
> 
> [    0.839687] do not call blocking ops when !TASK_RUNNING; state=1 set at
>                [<0000000000174a1e>] prepare_to_wait_event+0x7e/0x108
> [    0.839694] ------------[ cut here ]------------
> [    0.839697] WARNING: at kernel/sched/core.c:7326
> [    0.839698] Modules linked in:
> [    0.839702] CPU: 0 PID: 46 Comm: vballoon Not tainted 3.19.0 #233
> [    0.839705] task: 00000000021d0000 ti: 00000000021d8000 task.ti: 00000000021d8000
> [    0.839707] Krnl PSW : 0704c00180000000 000000000015bf8e (__might_sleep+0x8e/0x98)
> [    0.839713]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:0 PM:0 EA:3
> Krnl GPRS: 000000000000000d 00000000021d0000 0000000000000071 0000000000000001
> [    0.839718]            0000000000675ace 0000000001998c50 cccccccccccccccc cccccccccccccccc
> [    0.839720]            0000000000982134 000000000058f824 0000000000a008a8 0000000000000000
> [    0.839722]            00000000000004d9 00000000007ea992 000000000015bf8a 00000000021dbc28
> [    0.839731] Krnl Code: 000000000015bf7e: c0200033e838	larl	%r2,7d8fee
>            000000000015bf84: c0e50028cd62	brasl	%r14,675a48
>           #000000000015bf8a: a7f40001		brc	15,15bf8c
>           >000000000015bf8e: 9201a000		mvi	0(%r10),1
>            000000000015bf92: a7f4ffe2		brc	15,15bf56
>            000000000015bf96: 0707		bcr	0,%r7
>            000000000015bf98: ebdff0800024	stmg	%r13,%r15,128(%r15)
>            000000000015bf9e: a7f13fe0		tmll	%r15,16352
> [    0.839749] Call Trace:
> [    0.839751] ([<000000000015bf8a>] __might_sleep+0x8a/0x98)
> [    0.839756]  [<000000000028a562>] __kmalloc+0x272/0x350
> [    0.839759]  [<000000000058f824>] virtio_ccw_get_config+0x3c/0x100
> [    0.839762]  [<000000000049fcb0>] balloon+0x1b8/0x330
> [    0.839765]  [<00000000001529c8>] kthread+0x120/0x138
> [    0.839767]  [<0000000000683c22>] kernel_thread_starter+0x6/0xc
> [    0.839770]  [<0000000000683c1c>] kernel_thread_starter+0x0/0xc
> [    0.839772] no locks held by vballoon/46.
> [    0.839773] Last Breaking-Event-Address:
> [    0.839776]  [<000000000015bf8a>] __might_sleep+0x8a/0x98
> [    0.839778] ---[ end trace d27fcdfa27273d7c ]---
> 
> The problem seems to be this code in balloon() in
> drivers/virtio/virtio_balloon.c:
> 
> 	wait_event_interruptible(vb->config_change,
> 				 (diff = towards_target(vb)) != 0
> 				 || vb->need_stats_update
> 				 || kthread_should_stop()
> 				 || freezing(current));
> 
> wait_event_interruptible() sets the state of the current task to
> TASK_INTERRUPTIBLE, then checks the condition. The condition contains
> towards_target() which reads the virtio config space via virtio_cread().
> On s390, this then triggers virtio_ccw_get_config() - and this function
> calls some other functions again that might sleep (e.g. kzalloc or
> wait_event in ccw_io_helper) ... and this causes the new kernel warning
> message with kernel 3.19.
> 
> I think it would be quite difficult or at least ugly to rewrite
> virtio_ccw_get_config() so that it does not call sleepable functions
> anymore.

Yes: The config-space interacting functions for virtio-ccw trigger
channel I/O, which is by nature asynchronous. No way to get this
non-sleeping without really ugly hacks.

> So would it be feasible to rewrite the balloon() function that
> it does not call the towards_target() in its wait_event condition
> anymore? I am unfortunately not that familiar with the balloon code
> semantics, so any help is very appreciated here!

It might be possible to use nested wait event functions like
wake_woken(), but I haven't looked into that deeply.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Wed, Feb 25, 2015 at 11:13:18AM +0100, Thomas Huth wrote:
> 
>  Hi all,
> 
> with the recent kernel 3.19, I get a kernel warning when I start my
> KVM guest on s390 with virtio balloon enabled:
> 
> [    0.839687] do not call blocking ops when !TASK_RUNNING; state=1 set at
>                [<0000000000174a1e>] prepare_to_wait_event+0x7e/0x108
> [    0.839694] ------------[ cut here ]------------
> [    0.839697] WARNING: at kernel/sched/core.c:7326
> [    0.839698] Modules linked in:
> [    0.839702] CPU: 0 PID: 46 Comm: vballoon Not tainted 3.19.0 #233
> [    0.839705] task: 00000000021d0000 ti: 00000000021d8000 task.ti: 00000000021d8000
> [    0.839707] Krnl PSW : 0704c00180000000 000000000015bf8e (__might_sleep+0x8e/0x98)
> [    0.839713]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:0 PM:0 EA:3
> Krnl GPRS: 000000000000000d 00000000021d0000 0000000000000071 0000000000000001
> [    0.839718]            0000000000675ace 0000000001998c50 cccccccccccccccc cccccccccccccccc
> [    0.839720]            0000000000982134 000000000058f824 0000000000a008a8 0000000000000000
> [    0.839722]            00000000000004d9 00000000007ea992 000000000015bf8a 00000000021dbc28
> [    0.839731] Krnl Code: 000000000015bf7e: c0200033e838	larl	%r2,7d8fee
>            000000000015bf84: c0e50028cd62	brasl	%r14,675a48
>           #000000000015bf8a: a7f40001		brc	15,15bf8c
>           >000000000015bf8e: 9201a000		mvi	0(%r10),1
>            000000000015bf92: a7f4ffe2		brc	15,15bf56
>            000000000015bf96: 0707		bcr	0,%r7
>            000000000015bf98: ebdff0800024	stmg	%r13,%r15,128(%r15)
>            000000000015bf9e: a7f13fe0		tmll	%r15,16352
> [    0.839749] Call Trace:
> [    0.839751] ([<000000000015bf8a>] __might_sleep+0x8a/0x98)
> [    0.839756]  [<000000000028a562>] __kmalloc+0x272/0x350
> [    0.839759]  [<000000000058f824>] virtio_ccw_get_config+0x3c/0x100
> [    0.839762]  [<000000000049fcb0>] balloon+0x1b8/0x330
> [    0.839765]  [<00000000001529c8>] kthread+0x120/0x138
> [    0.839767]  [<0000000000683c22>] kernel_thread_starter+0x6/0xc
> [    0.839770]  [<0000000000683c1c>] kernel_thread_starter+0x0/0xc
> [    0.839772] no locks held by vballoon/46.
> [    0.839773] Last Breaking-Event-Address:
> [    0.839776]  [<000000000015bf8a>] __might_sleep+0x8a/0x98
> [    0.839778] ---[ end trace d27fcdfa27273d7c ]---
> 
> The problem seems to be this code in balloon() in
> drivers/virtio/virtio_balloon.c:
> 
> 	wait_event_interruptible(vb->config_change,
> 				 (diff = towards_target(vb)) != 0
> 				 || vb->need_stats_update
> 				 || kthread_should_stop()
> 				 || freezing(current));
> 
> wait_event_interruptible() sets the state of the current task to
> TASK_INTERRUPTIBLE, then checks the condition. The condition contains
> towards_target() which reads the virtio config space via virtio_cread().
> On s390, this then triggers virtio_ccw_get_config() - and this function
> calls some other functions again that might sleep (e.g. kzalloc or
> wait_event in ccw_io_helper) ... and this causes the new kernel warning
> message with kernel 3.19.
> 
> I think it would be quite difficult or at least ugly to rewrite
> virtio_ccw_get_config() so that it does not call sleepable functions
> anymore. So would it be feasible to rewrite the balloon() function that
> it does not call the towards_target() in its wait_event condition
> anymore? I am unfortunately not that familiar with the balloon code
> semantics, so any help is very appreciated here!
> 
>  Thanks,
>   Thomas

Thanks for finding this!
I just sent a patch that should fix this problem:
	http://article.gmane.org/gmane.linux.kernel.virtualization/24851
Testing would be appreciated.

Thanks again!

-- 
MST

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Rusty Russell]

Thomas Huth <thuth@linux.vnet.ibm.com> writes:
>  Hi all,
>
> with the recent kernel 3.19, I get a kernel warning when I start my
> KVM guest on s390 with virtio balloon enabled:

The deeper problem is that virtio_ccw_get_config just silently fails on
OOM.

Neither get_config nor set_config are expected to fail.

Cornelia, I think ccw and config_area should be allocated inside vcdev.
You could either use pointers, or simply allocate vcdev with GDP_DMA.

This would avoid the kmalloc inside these calls.

Thanks,
Rusty.

>
> [    0.839687] do not call blocking ops when !TASK_RUNNING; state=1 set at
>                [<0000000000174a1e>] prepare_to_wait_event+0x7e/0x108
> [    0.839694] ------------[ cut here ]------------
> [    0.839697] WARNING: at kernel/sched/core.c:7326
> [    0.839698] Modules linked in:
> [    0.839702] CPU: 0 PID: 46 Comm: vballoon Not tainted 3.19.0 #233
> [    0.839705] task: 00000000021d0000 ti: 00000000021d8000 task.ti: 00000000021d8000
> [    0.839707] Krnl PSW : 0704c00180000000 000000000015bf8e (__might_sleep+0x8e/0x98)
> [    0.839713]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:0 PM:0 EA:3
> Krnl GPRS: 000000000000000d 00000000021d0000 0000000000000071 0000000000000001
> [    0.839718]            0000000000675ace 0000000001998c50 cccccccccccccccc cccccccccccccccc
> [    0.839720]            0000000000982134 000000000058f824 0000000000a008a8 0000000000000000
> [    0.839722]            00000000000004d9 00000000007ea992 000000000015bf8a 00000000021dbc28
> [    0.839731] Krnl Code: 000000000015bf7e: c0200033e838	larl	%r2,7d8fee
>            000000000015bf84: c0e50028cd62	brasl	%r14,675a48
>           #000000000015bf8a: a7f40001		brc	15,15bf8c
>           >000000000015bf8e: 9201a000		mvi	0(%r10),1
>            000000000015bf92: a7f4ffe2		brc	15,15bf56
>            000000000015bf96: 0707		bcr	0,%r7
>            000000000015bf98: ebdff0800024	stmg	%r13,%r15,128(%r15)
>            000000000015bf9e: a7f13fe0		tmll	%r15,16352
> [    0.839749] Call Trace:
> [    0.839751] ([<000000000015bf8a>] __might_sleep+0x8a/0x98)
> [    0.839756]  [<000000000028a562>] __kmalloc+0x272/0x350
> [    0.839759]  [<000000000058f824>] virtio_ccw_get_config+0x3c/0x100
> [    0.839762]  [<000000000049fcb0>] balloon+0x1b8/0x330
> [    0.839765]  [<00000000001529c8>] kthread+0x120/0x138
> [    0.839767]  [<0000000000683c22>] kernel_thread_starter+0x6/0xc
> [    0.839770]  [<0000000000683c1c>] kernel_thread_starter+0x0/0xc
> [    0.839772] no locks held by vballoon/46.
> [    0.839773] Last Breaking-Event-Address:
> [    0.839776]  [<000000000015bf8a>] __might_sleep+0x8a/0x98
> [    0.839778] ---[ end trace d27fcdfa27273d7c ]---
>
> The problem seems to be this code in balloon() in
> drivers/virtio/virtio_balloon.c:
>
> 	wait_event_interruptible(vb->config_change,
> 				 (diff = towards_target(vb)) != 0
> 				 || vb->need_stats_update
> 				 || kthread_should_stop()
> 				 || freezing(current));
>
> wait_event_interruptible() sets the state of the current task to
> TASK_INTERRUPTIBLE, then checks the condition. The condition contains
> towards_target() which reads the virtio config space via virtio_cread().
> On s390, this then triggers virtio_ccw_get_config() - and this function
> calls some other functions again that might sleep (e.g. kzalloc or
> wait_event in ccw_io_helper) ... and this causes the new kernel warning
> message with kernel 3.19.
>
> I think it would be quite difficult or at least ugly to rewrite
> virtio_ccw_get_config() so that it does not call sleepable functions
> anymore. So would it be feasible to rewrite the balloon() function that
> it does not call the towards_target() in its wait_event condition
> anymore? I am unfortunately not that familiar with the balloon code
> semantics, so any help is very appreciated here!
>
>  Thanks,
>   Thomas

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Thomas Huth]

On Thu, 26 Feb 2015 11:50:42 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:

> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> >  Hi all,
> >
> > with the recent kernel 3.19, I get a kernel warning when I start my
> > KVM guest on s390 with virtio balloon enabled:
> 
> The deeper problem is that virtio_ccw_get_config just silently fails on
> OOM.
> 
> Neither get_config nor set_config are expected to fail.

AFAIK this is currently not a problem. According to
http://lwn.net/Articles/627419/ these kmalloc calls never
fail because they allocate less than a page.

 Thomas

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Thomas Huth]

On Thu, 26 Feb 2015 11:50:42 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:

> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> >  Hi all,
> >
> > with the recent kernel 3.19, I get a kernel warning when I start my
> > KVM guest on s390 with virtio balloon enabled:
> 
> The deeper problem is that virtio_ccw_get_config just silently fails on
> OOM.
> 
> Neither get_config nor set_config are expected to fail.

AFAIK this is currently not a problem. According to
http://lwn.net/Articles/627419/ these kmalloc calls never
fail because they allocate less than a page.

 Thomas

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 11:50:42AM +1030, Rusty Russell wrote:
> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> >  Hi all,
> >
> > with the recent kernel 3.19, I get a kernel warning when I start my
> > KVM guest on s390 with virtio balloon enabled:
> 
> The deeper problem is that virtio_ccw_get_config just silently fails on
> OOM.
> 
> Neither get_config nor set_config are expected to fail.
> 
> Cornelia, I think ccw and config_area should be allocated inside vcdev.
> You could either use pointers, or simply allocate vcdev with GDP_DMA.
> 
> This would avoid the kmalloc inside these calls.
> 
> Thanks,
> Rusty.

But it won't solve the problem of nested sleepers
with ccw: ATM is invokes ccw_io_helper to execute
commands, and that one calls wait_event
to wait for an interrupt.

Might be fixable but I think my patch looks like a safer
solution for 4.0/3.19, no?

-- 
MST

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 11:50:42AM +1030, Rusty Russell wrote:
> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> >  Hi all,
> >
> > with the recent kernel 3.19, I get a kernel warning when I start my
> > KVM guest on s390 with virtio balloon enabled:
> 
> The deeper problem is that virtio_ccw_get_config just silently fails on
> OOM.

Same problem with virtio_ccw_reset.
But avoiding kmalloc calls in virtio_ccw_get_config isn't enough I think,
it might still sleep.


> 
> Neither get_config nor set_config are expected to fail.
> 
> Cornelia, I think ccw and config_area should be allocated inside vcdev.
> You could either use pointers, or simply allocate vcdev with GDP_DMA.
> 
> This would avoid the kmalloc inside these calls.
> 
> Thanks,
> Rusty.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 11:50:42AM +1030, Rusty Russell wrote:
> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> >  Hi all,
> >
> > with the recent kernel 3.19, I get a kernel warning when I start my
> > KVM guest on s390 with virtio balloon enabled:
> 
> The deeper problem is that virtio_ccw_get_config just silently fails on
> OOM.

Same problem with virtio_ccw_reset.
But avoiding kmalloc calls in virtio_ccw_get_config isn't enough I think,
it might still sleep.


> 
> Neither get_config nor set_config are expected to fail.
> 
> Cornelia, I think ccw and config_area should be allocated inside vcdev.
> You could either use pointers, or simply allocate vcdev with GDP_DMA.
> 
> This would avoid the kmalloc inside these calls.
> 
> Thanks,
> Rusty.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Cornelia Huck]

On Thu, 26 Feb 2015 11:50:42 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:

> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> >  Hi all,
> >
> > with the recent kernel 3.19, I get a kernel warning when I start my
> > KVM guest on s390 with virtio balloon enabled:
> 
> The deeper problem is that virtio_ccw_get_config just silently fails on
> OOM.
> 
> Neither get_config nor set_config are expected to fail.

It is a problem that we cannot relay failures back to the caller: not
only for the memory allocations. We need to do channel I/O, and any
channel I/O can fail. For our virtio case, we don't have to deal with
the failures that may happen on real hardware (like path failures), but
what can happen is a hotunplug, which means we cannot talk to the
device anymore from one moment to the other.

> 
> Cornelia, I think ccw and config_area should be allocated inside vcdev.
> You could either use pointers, or simply allocate vcdev with GDP_DMA.
> 
> This would avoid the kmalloc inside these calls.

I can certainly look into that, but I'm not sure it's worth it. We still
have to deal with possible failures from doing channel I/O.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Cornelia Huck]

On Thu, 26 Feb 2015 11:50:42 +1030
Rusty Russell <rusty@rustcorp.com.au> wrote:

> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> >  Hi all,
> >
> > with the recent kernel 3.19, I get a kernel warning when I start my
> > KVM guest on s390 with virtio balloon enabled:
> 
> The deeper problem is that virtio_ccw_get_config just silently fails on
> OOM.
> 
> Neither get_config nor set_config are expected to fail.

It is a problem that we cannot relay failures back to the caller: not
only for the memory allocations. We need to do channel I/O, and any
channel I/O can fail. For our virtio case, we don't have to deal with
the failures that may happen on real hardware (like path failures), but
what can happen is a hotunplug, which means we cannot talk to the
device anymore from one moment to the other.

> 
> Cornelia, I think ccw and config_area should be allocated inside vcdev.
> You could either use pointers, or simply allocate vcdev with GDP_DMA.
> 
> This would avoid the kmalloc inside these calls.

I can certainly look into that, but I'm not sure it's worth it. We still
have to deal with possible failures from doing channel I/O.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Cornelia Huck]

On Thu, 26 Feb 2015 09:45:29 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Feb 26, 2015 at 11:50:42AM +1030, Rusty Russell wrote:
> > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > >  Hi all,
> > >
> > > with the recent kernel 3.19, I get a kernel warning when I start my
> > > KVM guest on s390 with virtio balloon enabled:
> > 
> > The deeper problem is that virtio_ccw_get_config just silently fails on
> > OOM.
> 
> Same problem with virtio_ccw_reset.
> But avoiding kmalloc calls in virtio_ccw_get_config isn't enough I think,
> it might still sleep.

It is probably a problem with all calls into the transport that assume
an implementation that cannot fail: If we have a channel I/O backing,
we need to be able to handle things not working.

The only case we need to care about for virtio is probably a -ENODEV
triggered by a hotunplug, though.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Peter Zijlstra]

On Thu, Feb 26, 2015 at 09:30:31AM +0100, Michael S. Tsirkin wrote:
> On Thu, Feb 26, 2015 at 11:50:42AM +1030, Rusty Russell wrote:
> > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > >  Hi all,
> > >
> > > with the recent kernel 3.19, I get a kernel warning when I start my
> > > KVM guest on s390 with virtio balloon enabled:
> > 
> > The deeper problem is that virtio_ccw_get_config just silently fails on
> > OOM.
> > 
> > Neither get_config nor set_config are expected to fail.
> > 
> > Cornelia, I think ccw and config_area should be allocated inside vcdev.
> > You could either use pointers, or simply allocate vcdev with GDP_DMA.
> > 
> > This would avoid the kmalloc inside these calls.
> > 
> > Thanks,
> > Rusty.
> 
> But it won't solve the problem of nested sleepers
> with ccw: ATM is invokes ccw_io_helper to execute
> commands, and that one calls wait_event
> to wait for an interrupt.
> 
> Might be fixable but I think my patch looks like a safer
> solution for 4.0/3.19, no?

I've no idea what your patch was since I'm not subscribed to any of the
lists this discussion is had on.

But you can annotate the warning away; _however_ with the annotation
needs to be a big comment explaining why its safe to do so. Typically to
involved talking about how its actually rare for the call to sleep.

So occasional sleeps inside a wait_event() are ok-ish, we'll just get to
go around once more. But once you consistently sleep inside a
wait_event() things go a bit funny.

So for instance; if in ccw_io_helper() we expect that wait_event(,
!doing_io()) to be (mostly) true on first go, then we'll never get into
__wait_event() and ->state won't actually be mucked about with.

The thing to avoid is not actually sleeping (much) but setting
TASK_RUNNING and turning the entire thing into a giant poll loop.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 06:08:49PM +0100, Peter Zijlstra wrote:
> On Thu, Feb 26, 2015 at 09:30:31AM +0100, Michael S. Tsirkin wrote:
> > On Thu, Feb 26, 2015 at 11:50:42AM +1030, Rusty Russell wrote:
> > > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > >  Hi all,
> > > >
> > > > with the recent kernel 3.19, I get a kernel warning when I start my
> > > > KVM guest on s390 with virtio balloon enabled:
> > > 
> > > The deeper problem is that virtio_ccw_get_config just silently fails on
> > > OOM.
> > > 
> > > Neither get_config nor set_config are expected to fail.
> > > 
> > > Cornelia, I think ccw and config_area should be allocated inside vcdev.
> > > You could either use pointers, or simply allocate vcdev with GDP_DMA.
> > > 
> > > This would avoid the kmalloc inside these calls.
> > > 
> > > Thanks,
> > > Rusty.
> > 
> > But it won't solve the problem of nested sleepers
> > with ccw: ATM is invokes ccw_io_helper to execute
> > commands, and that one calls wait_event
> > to wait for an interrupt.
> > 
> > Might be fixable but I think my patch looks like a safer
> > solution for 4.0/3.19, no?
> 
> I've no idea what your patch was since I'm not subscribed to any of the
> lists this discussion is had on.

Oh, sorry about that.
Here it is, below:

----- Forwarded message from "Michael S. Tsirkin" <mst@redhat.com> -----

Date: Wed, 25 Feb 2015 15:36:02 +0100
From: "Michael S. Tsirkin" <mst@redhat.com>
To: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org, Thomas Huth <thuth@linux.vnet.ibm.com>, Rusty Russell <rusty@rustcorp.com.au>,
	virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, Cornelia Huck <cornelia.huck@de.ibm.com>
Subject: [PATCH v2] virtio-balloon: do not call blocking ops when !TASK_RUNNING
Message-ID: <1424874878-17155-1-git-send-email-mst@redhat.com>

virtio balloon has this code:
        wait_event_interruptible(vb->config_change,
                                 (diff = towards_target(vb)) != 0
                                 || vb->need_stats_update
                                 || kthread_should_stop()
                                 || freezing(current));

Which is a problem because towards_target() call might block after
wait_event_interruptible sets task state to TAST_INTERRUPTIBLE, causing
the task_struct::state collision typical of nesting of sleeping
primitives

See also http://lwn.net/Articles/628628/ or Thomas's
bug report
http://article.gmane.org/gmane.linux.kernel.virtualization/24846
for a fuller explanation.

To fix, rewrite using wait_woken.

Cc: stable@vger.kernel.org
Reported-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

changes from v1:
	remove wait_event_interruptible
	noticed by Cornelia Huck <cornelia.huck@de.ibm.com>

 drivers/virtio/virtio_balloon.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0413157..5a6ad6d 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -29,6 +29,7 @@
 #include <linux/module.h>
 #include <linux/balloon_compaction.h>
 #include <linux/oom.h>
+#include <linux/wait.h>
 
 /*
  * Balloon device works in 4K page units.  So each page is pointed to by
@@ -334,17 +335,25 @@ static int virtballoon_oom_notify(struct notifier_block *self,
 static int balloon(void *_vballoon)
 {
 	struct virtio_balloon *vb = _vballoon;
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
 	set_freezable();
 	while (!kthread_should_stop()) {
 		s64 diff;
 
 		try_to_freeze();
-		wait_event_interruptible(vb->config_change,
-					 (diff = towards_target(vb)) != 0
-					 || vb->need_stats_update
-					 || kthread_should_stop()
-					 || freezing(current));
+
+		add_wait_queue(&vb->config_change, &wait);
+		for (;;) {
+			if ((diff = towards_target(vb)) != 0 ||
+			    vb->need_stats_update ||
+			    kthread_should_stop() ||
+			    freezing(current))
+				break;
+			wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
+		}
+		remove_wait_queue(&vb->config_change, &wait);
+
 		if (vb->need_stats_update)
 			stats_handle_request(vb);
 		if (diff > 0)
-- 
MST

----- End forwarded message -----

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 06:08:49PM +0100, Peter Zijlstra wrote:
> On Thu, Feb 26, 2015 at 09:30:31AM +0100, Michael S. Tsirkin wrote:
> > On Thu, Feb 26, 2015 at 11:50:42AM +1030, Rusty Russell wrote:
> > > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > >  Hi all,
> > > >
> > > > with the recent kernel 3.19, I get a kernel warning when I start my
> > > > KVM guest on s390 with virtio balloon enabled:
> > > 
> > > The deeper problem is that virtio_ccw_get_config just silently fails on
> > > OOM.
> > > 
> > > Neither get_config nor set_config are expected to fail.
> > > 
> > > Cornelia, I think ccw and config_area should be allocated inside vcdev.
> > > You could either use pointers, or simply allocate vcdev with GDP_DMA.
> > > 
> > > This would avoid the kmalloc inside these calls.
> > > 
> > > Thanks,
> > > Rusty.
> > 
> > But it won't solve the problem of nested sleepers
> > with ccw: ATM is invokes ccw_io_helper to execute
> > commands, and that one calls wait_event
> > to wait for an interrupt.
> > 
> > Might be fixable but I think my patch looks like a safer
> > solution for 4.0/3.19, no?
> 
> I've no idea what your patch was since I'm not subscribed to any of the
> lists this discussion is had on.

Oh, sorry about that.
Here it is, below:

----- Forwarded message from "Michael S. Tsirkin" <mst@redhat.com> -----

Date: Wed, 25 Feb 2015 15:36:02 +0100
From: "Michael S. Tsirkin" <mst@redhat.com>
To: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org, Thomas Huth <thuth@linux.vnet.ibm.com>, Rusty Russell <rusty@rustcorp.com.au>,
	virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, Cornelia Huck <cornelia.huck@de.ibm.com>
Subject: [PATCH v2] virtio-balloon: do not call blocking ops when !TASK_RUNNING
Message-ID: <1424874878-17155-1-git-send-email-mst@redhat.com>

virtio balloon has this code:
        wait_event_interruptible(vb->config_change,
                                 (diff = towards_target(vb)) != 0
                                 || vb->need_stats_update
                                 || kthread_should_stop()
                                 || freezing(current));

Which is a problem because towards_target() call might block after
wait_event_interruptible sets task state to TAST_INTERRUPTIBLE, causing
the task_struct::state collision typical of nesting of sleeping
primitives

See also http://lwn.net/Articles/628628/ or Thomas's
bug report
http://article.gmane.org/gmane.linux.kernel.virtualization/24846
for a fuller explanation.

To fix, rewrite using wait_woken.

Cc: stable@vger.kernel.org
Reported-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

changes from v1:
	remove wait_event_interruptible
	noticed by Cornelia Huck <cornelia.huck@de.ibm.com>

 drivers/virtio/virtio_balloon.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0413157..5a6ad6d 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -29,6 +29,7 @@
 #include <linux/module.h>
 #include <linux/balloon_compaction.h>
 #include <linux/oom.h>
+#include <linux/wait.h>
 
 /*
  * Balloon device works in 4K page units.  So each page is pointed to by
@@ -334,17 +335,25 @@ static int virtballoon_oom_notify(struct notifier_block *self,
 static int balloon(void *_vballoon)
 {
 	struct virtio_balloon *vb = _vballoon;
+	DEFINE_WAIT_FUNC(wait, woken_wake_function);
 
 	set_freezable();
 	while (!kthread_should_stop()) {
 		s64 diff;
 
 		try_to_freeze();
-		wait_event_interruptible(vb->config_change,
-					 (diff = towards_target(vb)) != 0
-					 || vb->need_stats_update
-					 || kthread_should_stop()
-					 || freezing(current));
+
+		add_wait_queue(&vb->config_change, &wait);
+		for (;;) {
+			if ((diff = towards_target(vb)) != 0 ||
+			    vb->need_stats_update ||
+			    kthread_should_stop() ||
+			    freezing(current))
+				break;
+			wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
+		}
+		remove_wait_queue(&vb->config_change, &wait);
+
 		if (vb->need_stats_update)
 			stats_handle_request(vb);
 		if (diff > 0)
-- 
MST

----- End forwarded message -----

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 06:27:43PM +0100, Michael S. Tsirkin wrote:
> On Thu, Feb 26, 2015 at 06:08:49PM +0100, Peter Zijlstra wrote:
> > On Thu, Feb 26, 2015 at 09:30:31AM +0100, Michael S. Tsirkin wrote:
> > > On Thu, Feb 26, 2015 at 11:50:42AM +1030, Rusty Russell wrote:
> > > > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > >  Hi all,
> > > > >
> > > > > with the recent kernel 3.19, I get a kernel warning when I start my
> > > > > KVM guest on s390 with virtio balloon enabled:
> > > > 
> > > > The deeper problem is that virtio_ccw_get_config just silently fails on
> > > > OOM.
> > > > 
> > > > Neither get_config nor set_config are expected to fail.
> > > > 
> > > > Cornelia, I think ccw and config_area should be allocated inside vcdev.
> > > > You could either use pointers, or simply allocate vcdev with GDP_DMA.
> > > > 
> > > > This would avoid the kmalloc inside these calls.
> > > > 
> > > > Thanks,
> > > > Rusty.
> > > 
> > > But it won't solve the problem of nested sleepers
> > > with ccw: ATM is invokes ccw_io_helper to execute
> > > commands, and that one calls wait_event
> > > to wait for an interrupt.
> > > 
> > > Might be fixable but I think my patch looks like a safer
> > > solution for 4.0/3.19, no?
> > 
> > I've no idea what your patch was since I'm not subscribed to any of the
> > lists this discussion is had on.
> 
> Oh, sorry about that.
> Here it is, below:
> 
> ----- Forwarded message from "Michael S. Tsirkin" <mst@redhat.com> -----
> 
> Date: Wed, 25 Feb 2015 15:36:02 +0100
> From: "Michael S. Tsirkin" <mst@redhat.com>
> To: linux-kernel@vger.kernel.org
> Cc: stable@vger.kernel.org, Thomas Huth <thuth@linux.vnet.ibm.com>, Rusty Russell <rusty@rustcorp.com.au>,
> 	virtualization@lists.linux-foundation.org, kvm@vger.kernel.org, Cornelia Huck <cornelia.huck@de.ibm.com>
> Subject: [PATCH v2] virtio-balloon: do not call blocking ops when !TASK_RUNNING
> Message-ID: <1424874878-17155-1-git-send-email-mst@redhat.com>
> 
> virtio balloon has this code:
>         wait_event_interruptible(vb->config_change,
>                                  (diff = towards_target(vb)) != 0
>                                  || vb->need_stats_update
>                                  || kthread_should_stop()
>                                  || freezing(current));
> 
> Which is a problem because towards_target() call might block after
> wait_event_interruptible sets task state to TAST_INTERRUPTIBLE, causing
> the task_struct::state collision typical of nesting of sleeping
> primitives
> 
> See also http://lwn.net/Articles/628628/ or Thomas's
> bug report
> http://article.gmane.org/gmane.linux.kernel.virtualization/24846
> for a fuller explanation.
> 
> To fix, rewrite using wait_woken.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> changes from v1:
> 	remove wait_event_interruptible
> 	noticed by Cornelia Huck <cornelia.huck@de.ibm.com>
> 
>  drivers/virtio/virtio_balloon.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 0413157..5a6ad6d 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -29,6 +29,7 @@
>  #include <linux/module.h>
>  #include <linux/balloon_compaction.h>
>  #include <linux/oom.h>
> +#include <linux/wait.h>
>  
>  /*
>   * Balloon device works in 4K page units.  So each page is pointed to by
> @@ -334,17 +335,25 @@ static int virtballoon_oom_notify(struct notifier_block *self,
>  static int balloon(void *_vballoon)
>  {
>  	struct virtio_balloon *vb = _vballoon;
> +	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>  
>  	set_freezable();
>  	while (!kthread_should_stop()) {
>  		s64 diff;
>  
>  		try_to_freeze();
> -		wait_event_interruptible(vb->config_change,
> -					 (diff = towards_target(vb)) != 0
> -					 || vb->need_stats_update
> -					 || kthread_should_stop()
> -					 || freezing(current));
> +
> +		add_wait_queue(&vb->config_change, &wait);
> +		for (;;) {
> +			if ((diff = towards_target(vb)) != 0 ||
> +			    vb->need_stats_update ||
> +			    kthread_should_stop() ||
> +			    freezing(current))
> +				break;
> +			wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
> +		}
> +		remove_wait_queue(&vb->config_change, &wait);
> +
>  		if (vb->need_stats_update)
>  			stats_handle_request(vb);
>  		if (diff > 0)
> -- 
> MST

WRT which, I have a question. IIUC it's OK for towards_target
in this code to call wait_event in its turn, assuming that
*that* wait_event is not not calling blocking ops.
Right?


> ----- End forwarded message -----

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Rusty Russell]

Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> On Thu, 26 Feb 2015 11:50:42 +1030
> Rusty Russell <rusty@rustcorp.com.au> wrote:
>
>> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
>> >  Hi all,
>> >
>> > with the recent kernel 3.19, I get a kernel warning when I start my
>> > KVM guest on s390 with virtio balloon enabled:
>> 
>> The deeper problem is that virtio_ccw_get_config just silently fails on
>> OOM.
>> 
>> Neither get_config nor set_config are expected to fail.
>
> AFAIK this is currently not a problem. According to
> http://lwn.net/Articles/627419/ these kmalloc calls never
> fail because they allocate less than a page.

I strongly suggest you unlearn that fact.

The fix for this is in two parts:

1) Annotate using sched_annotate_sleep() and add a comment: we may spin
   a few times in low memory situations, but this isn't a high
   performance path.

2) Handle get_config (and other) failure in some more elegant way.

Cheers,
Rusty.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > On Thu, 26 Feb 2015 11:50:42 +1030
> > Rusty Russell <rusty@rustcorp.com.au> wrote:
> >
> >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> >> >  Hi all,
> >> >
> >> > with the recent kernel 3.19, I get a kernel warning when I start my
> >> > KVM guest on s390 with virtio balloon enabled:
> >> 
> >> The deeper problem is that virtio_ccw_get_config just silently fails on
> >> OOM.
> >> 
> >> Neither get_config nor set_config are expected to fail.
> >
> > AFAIK this is currently not a problem. According to
> > http://lwn.net/Articles/627419/ these kmalloc calls never
> > fail because they allocate less than a page.
> 
> I strongly suggest you unlearn that fact.
> The fix for this is in two parts:
> 
> 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
>    a few times in low memory situations, but this isn't a high
>    performance path.
> 
> 2) Handle get_config (and other) failure in some more elegant way.
> 
> Cheers,
> Rusty.

I agree, but I'd like to point out that even without kmalloc,
on s390 get_config is blocking - it's waiting
for a hardware interrupt.

And it makes sense: config is not data path, I don't think
we should spin there.

So I think besides these two parts, we still need my two patches:
    virtio-balloon: do not call blocking ops when !TASK_RUNNING
    virtio_console: avoid config access from irq
in 4.0.

agree?

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > On Thu, 26 Feb 2015 11:50:42 +1030
> > Rusty Russell <rusty@rustcorp.com.au> wrote:
> >
> >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> >> >  Hi all,
> >> >
> >> > with the recent kernel 3.19, I get a kernel warning when I start my
> >> > KVM guest on s390 with virtio balloon enabled:
> >> 
> >> The deeper problem is that virtio_ccw_get_config just silently fails on
> >> OOM.
> >> 
> >> Neither get_config nor set_config are expected to fail.
> >
> > AFAIK this is currently not a problem. According to
> > http://lwn.net/Articles/627419/ these kmalloc calls never
> > fail because they allocate less than a page.
> 
> I strongly suggest you unlearn that fact.
> The fix for this is in two parts:
> 
> 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
>    a few times in low memory situations, but this isn't a high
>    performance path.
> 
> 2) Handle get_config (and other) failure in some more elegant way.
> 
> Cheers,
> Rusty.

I agree, but I'd like to point out that even without kmalloc,
on s390 get_config is blocking - it's waiting
for a hardware interrupt.

And it makes sense: config is not data path, I don't think
we should spin there.

So I think besides these two parts, we still need my two patches:
    virtio-balloon: do not call blocking ops when !TASK_RUNNING
    virtio_console: avoid config access from irq
in 4.0.

agree?

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Cornelia Huck]

On Mon, 2 Mar 2015 12:13:58 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > On Thu, 26 Feb 2015 11:50:42 +1030
> > > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > >
> > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > >> >  Hi all,
> > >> >
> > >> > with the recent kernel 3.19, I get a kernel warning when I start my
> > >> > KVM guest on s390 with virtio balloon enabled:
> > >> 
> > >> The deeper problem is that virtio_ccw_get_config just silently fails on
> > >> OOM.
> > >> 
> > >> Neither get_config nor set_config are expected to fail.
> > >
> > > AFAIK this is currently not a problem. According to
> > > http://lwn.net/Articles/627419/ these kmalloc calls never
> > > fail because they allocate less than a page.
> > 
> > I strongly suggest you unlearn that fact.
> > The fix for this is in two parts:
> > 
> > 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> >    a few times in low memory situations, but this isn't a high
> >    performance path.
> > 
> > 2) Handle get_config (and other) failure in some more elegant way.

Do you mean we need to enable the caller to deal with get_config
failures (and the transport to relay those failures)? I agree with that.

> > 
> > Cheers,
> > Rusty.
> 
> I agree, but I'd like to point out that even without kmalloc,
> on s390 get_config is blocking - it's waiting
> for a hardware interrupt.
> 
> And it makes sense: config is not data path, I don't think
> we should spin there.
> 
> So I think besides these two parts, we still need my two patches:
>     virtio-balloon: do not call blocking ops when !TASK_RUNNING
>     virtio_console: avoid config access from irq
> in 4.0.
> 
> agree?

I agree that we need those fixes as well.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Cornelia Huck]

On Mon, 2 Mar 2015 12:13:58 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > On Thu, 26 Feb 2015 11:50:42 +1030
> > > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > >
> > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > >> >  Hi all,
> > >> >
> > >> > with the recent kernel 3.19, I get a kernel warning when I start my
> > >> > KVM guest on s390 with virtio balloon enabled:
> > >> 
> > >> The deeper problem is that virtio_ccw_get_config just silently fails on
> > >> OOM.
> > >> 
> > >> Neither get_config nor set_config are expected to fail.
> > >
> > > AFAIK this is currently not a problem. According to
> > > http://lwn.net/Articles/627419/ these kmalloc calls never
> > > fail because they allocate less than a page.
> > 
> > I strongly suggest you unlearn that fact.
> > The fix for this is in two parts:
> > 
> > 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> >    a few times in low memory situations, but this isn't a high
> >    performance path.
> > 
> > 2) Handle get_config (and other) failure in some more elegant way.

Do you mean we need to enable the caller to deal with get_config
failures (and the transport to relay those failures)? I agree with that.

> > 
> > Cheers,
> > Rusty.
> 
> I agree, but I'd like to point out that even without kmalloc,
> on s390 get_config is blocking - it's waiting
> for a hardware interrupt.
> 
> And it makes sense: config is not data path, I don't think
> we should spin there.
> 
> So I think besides these two parts, we still need my two patches:
>     virtio-balloon: do not call blocking ops when !TASK_RUNNING
>     virtio_console: avoid config access from irq
> in 4.0.
> 
> agree?

I agree that we need those fixes as well.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Mon, Mar 02, 2015 at 12:31:06PM +0100, Cornelia Huck wrote:
> On Mon, 2 Mar 2015 12:13:58 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> > > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > On Thu, 26 Feb 2015 11:50:42 +1030
> > > > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > >
> > > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > >> >  Hi all,
> > > >> >
> > > >> > with the recent kernel 3.19, I get a kernel warning when I start my
> > > >> > KVM guest on s390 with virtio balloon enabled:
> > > >> 
> > > >> The deeper problem is that virtio_ccw_get_config just silently fails on
> > > >> OOM.
> > > >> 
> > > >> Neither get_config nor set_config are expected to fail.
> > > >
> > > > AFAIK this is currently not a problem. According to
> > > > http://lwn.net/Articles/627419/ these kmalloc calls never
> > > > fail because they allocate less than a page.
> > > 
> > > I strongly suggest you unlearn that fact.
> > > The fix for this is in two parts:
> > > 
> > > 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> > >    a few times in low memory situations, but this isn't a high
> > >    performance path.
> > > 
> > > 2) Handle get_config (and other) failure in some more elegant way.
> 
> Do you mean we need to enable the caller to deal with get_config
> failures (and the transport to relay those failures)? I agree with that.

We can certainly tweak code to bypass need to kmalloc
on get_config.

Why is it doing these allocs? What's wrong with using
vcdev->config directly?


> > > 
> > > Cheers,
> > > Rusty.
> > 
> > I agree, but I'd like to point out that even without kmalloc,
> > on s390 get_config is blocking - it's waiting
> > for a hardware interrupt.
> > 
> > And it makes sense: config is not data path, I don't think
> > we should spin there.
> > 
> > So I think besides these two parts, we still need my two patches:
> >     virtio-balloon: do not call blocking ops when !TASK_RUNNING
> >     virtio_console: avoid config access from irq
> > in 4.0.
> > 
> > agree?
> 
> I agree that we need those fixes as well.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Cornelia Huck]

On Mon, 2 Mar 2015 12:46:57 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Mar 02, 2015 at 12:31:06PM +0100, Cornelia Huck wrote:
> > On Mon, 2 Mar 2015 12:13:58 +0100
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> > > > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > On Thu, 26 Feb 2015 11:50:42 +1030
> > > > > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > > >
> > > > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > >> >  Hi all,
> > > > >> >
> > > > >> > with the recent kernel 3.19, I get a kernel warning when I start my
> > > > >> > KVM guest on s390 with virtio balloon enabled:
> > > > >> 
> > > > >> The deeper problem is that virtio_ccw_get_config just silently fails on
> > > > >> OOM.
> > > > >> 
> > > > >> Neither get_config nor set_config are expected to fail.
> > > > >
> > > > > AFAIK this is currently not a problem. According to
> > > > > http://lwn.net/Articles/627419/ these kmalloc calls never
> > > > > fail because they allocate less than a page.
> > > > 
> > > > I strongly suggest you unlearn that fact.
> > > > The fix for this is in two parts:
> > > > 
> > > > 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> > > >    a few times in low memory situations, but this isn't a high
> > > >    performance path.
> > > > 
> > > > 2) Handle get_config (and other) failure in some more elegant way.
> > 
> > Do you mean we need to enable the caller to deal with get_config
> > failures (and the transport to relay those failures)? I agree with that.
> 
> We can certainly tweak code to bypass need to kmalloc
> on get_config.
> 
> Why is it doing these allocs? What's wrong with using
> vcdev->config directly?

We'd need to make sure that vcdev->config is allocated with GFP_DMA, as
we need it to be under 2G. And we need to be more careful wrt
serialization, especially if we want to reuse the ccw structure as
well, for example. Nothing complicated, I'd just need some free time to
do it :)

The more likely reason for get_config to fail is a device hotunplug,
however. We'll get a seperate notification about that (via machine
check + channel report), but it would be nice if we could stop poking
the device immediately, as there's no use trying to do something with
it anymore.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Cornelia Huck]

On Mon, 2 Mar 2015 12:46:57 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Mar 02, 2015 at 12:31:06PM +0100, Cornelia Huck wrote:
> > On Mon, 2 Mar 2015 12:13:58 +0100
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> > > > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > On Thu, 26 Feb 2015 11:50:42 +1030
> > > > > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > > >
> > > > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > >> >  Hi all,
> > > > >> >
> > > > >> > with the recent kernel 3.19, I get a kernel warning when I start my
> > > > >> > KVM guest on s390 with virtio balloon enabled:
> > > > >> 
> > > > >> The deeper problem is that virtio_ccw_get_config just silently fails on
> > > > >> OOM.
> > > > >> 
> > > > >> Neither get_config nor set_config are expected to fail.
> > > > >
> > > > > AFAIK this is currently not a problem. According to
> > > > > http://lwn.net/Articles/627419/ these kmalloc calls never
> > > > > fail because they allocate less than a page.
> > > > 
> > > > I strongly suggest you unlearn that fact.
> > > > The fix for this is in two parts:
> > > > 
> > > > 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> > > >    a few times in low memory situations, but this isn't a high
> > > >    performance path.
> > > > 
> > > > 2) Handle get_config (and other) failure in some more elegant way.
> > 
> > Do you mean we need to enable the caller to deal with get_config
> > failures (and the transport to relay those failures)? I agree with that.
> 
> We can certainly tweak code to bypass need to kmalloc
> on get_config.
> 
> Why is it doing these allocs? What's wrong with using
> vcdev->config directly?

We'd need to make sure that vcdev->config is allocated with GFP_DMA, as
we need it to be under 2G. And we need to be more careful wrt
serialization, especially if we want to reuse the ccw structure as
well, for example. Nothing complicated, I'd just need some free time to
do it :)

The more likely reason for get_config to fail is a device hotunplug,
however. We'll get a seperate notification about that (via machine
check + channel report), but it would be nice if we could stop poking
the device immediately, as there's no use trying to do something with
it anymore.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Mon, Mar 02, 2015 at 01:11:02PM +0100, Cornelia Huck wrote:
> On Mon, 2 Mar 2015 12:46:57 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Mar 02, 2015 at 12:31:06PM +0100, Cornelia Huck wrote:
> > > On Mon, 2 Mar 2015 12:13:58 +0100
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> > > > > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > > On Thu, 26 Feb 2015 11:50:42 +1030
> > > > > > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > > > >
> > > > > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > >> >  Hi all,
> > > > > >> >
> > > > > >> > with the recent kernel 3.19, I get a kernel warning when I start my
> > > > > >> > KVM guest on s390 with virtio balloon enabled:
> > > > > >> 
> > > > > >> The deeper problem is that virtio_ccw_get_config just silently fails on
> > > > > >> OOM.
> > > > > >> 
> > > > > >> Neither get_config nor set_config are expected to fail.
> > > > > >
> > > > > > AFAIK this is currently not a problem. According to
> > > > > > http://lwn.net/Articles/627419/ these kmalloc calls never
> > > > > > fail because they allocate less than a page.
> > > > > 
> > > > > I strongly suggest you unlearn that fact.
> > > > > The fix for this is in two parts:
> > > > > 
> > > > > 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> > > > >    a few times in low memory situations, but this isn't a high
> > > > >    performance path.
> > > > > 
> > > > > 2) Handle get_config (and other) failure in some more elegant way.
> > > 
> > > Do you mean we need to enable the caller to deal with get_config
> > > failures (and the transport to relay those failures)? I agree with that.
> > 
> > We can certainly tweak code to bypass need to kmalloc
> > on get_config.
> > 
> > Why is it doing these allocs? What's wrong with using
> > vcdev->config directly?
> 
> We'd need to make sure that vcdev->config is allocated with GFP_DMA, as
> we need it to be under 2G. And we need to be more careful wrt
> serialization, especially if we want to reuse the ccw structure as
> well, for example. Nothing complicated, I'd just need some free time to
> do it :)
> 
> The more likely reason for get_config to fail is a device hotunplug,
> however. We'll get a seperate notification about that (via machine
> check + channel report), but it would be nice if we could stop poking
> the device immediately, as there's no use trying to do something with
> it anymore.

Normally, hotunplug requires guest cooperation.
IOW unplug request should send guest interrupt,
then block until guest confirms it's not using the
device anymore.
virtio pci already handles that fine, can't ccw
do something similar?

-- 
MST

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Mon, Mar 02, 2015 at 01:11:02PM +0100, Cornelia Huck wrote:
> On Mon, 2 Mar 2015 12:46:57 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Mar 02, 2015 at 12:31:06PM +0100, Cornelia Huck wrote:
> > > On Mon, 2 Mar 2015 12:13:58 +0100
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> > > > > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > > On Thu, 26 Feb 2015 11:50:42 +1030
> > > > > > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > > > >
> > > > > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > >> >  Hi all,
> > > > > >> >
> > > > > >> > with the recent kernel 3.19, I get a kernel warning when I start my
> > > > > >> > KVM guest on s390 with virtio balloon enabled:
> > > > > >> 
> > > > > >> The deeper problem is that virtio_ccw_get_config just silently fails on
> > > > > >> OOM.
> > > > > >> 
> > > > > >> Neither get_config nor set_config are expected to fail.
> > > > > >
> > > > > > AFAIK this is currently not a problem. According to
> > > > > > http://lwn.net/Articles/627419/ these kmalloc calls never
> > > > > > fail because they allocate less than a page.
> > > > > 
> > > > > I strongly suggest you unlearn that fact.
> > > > > The fix for this is in two parts:
> > > > > 
> > > > > 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> > > > >    a few times in low memory situations, but this isn't a high
> > > > >    performance path.
> > > > > 
> > > > > 2) Handle get_config (and other) failure in some more elegant way.
> > > 
> > > Do you mean we need to enable the caller to deal with get_config
> > > failures (and the transport to relay those failures)? I agree with that.
> > 
> > We can certainly tweak code to bypass need to kmalloc
> > on get_config.
> > 
> > Why is it doing these allocs? What's wrong with using
> > vcdev->config directly?
> 
> We'd need to make sure that vcdev->config is allocated with GFP_DMA, as
> we need it to be under 2G. And we need to be more careful wrt
> serialization, especially if we want to reuse the ccw structure as
> well, for example. Nothing complicated, I'd just need some free time to
> do it :)
> 
> The more likely reason for get_config to fail is a device hotunplug,
> however. We'll get a seperate notification about that (via machine
> check + channel report), but it would be nice if we could stop poking
> the device immediately, as there's no use trying to do something with
> it anymore.

Normally, hotunplug requires guest cooperation.
IOW unplug request should send guest interrupt,
then block until guest confirms it's not using the
device anymore.
virtio pci already handles that fine, can't ccw
do something similar?

-- 
MST

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Cornelia Huck]

On Mon, 2 Mar 2015 13:19:43 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Mar 02, 2015 at 01:11:02PM +0100, Cornelia Huck wrote:
> > On Mon, 2 Mar 2015 12:46:57 +0100
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Mon, Mar 02, 2015 at 12:31:06PM +0100, Cornelia Huck wrote:
> > > > On Mon, 2 Mar 2015 12:13:58 +0100
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> > > > > > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > > > On Thu, 26 Feb 2015 11:50:42 +1030
> > > > > > > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > > > > >
> > > > > > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > > >> >  Hi all,
> > > > > > >> >
> > > > > > >> > with the recent kernel 3.19, I get a kernel warning when I start my
> > > > > > >> > KVM guest on s390 with virtio balloon enabled:
> > > > > > >> 
> > > > > > >> The deeper problem is that virtio_ccw_get_config just silently fails on
> > > > > > >> OOM.
> > > > > > >> 
> > > > > > >> Neither get_config nor set_config are expected to fail.
> > > > > > >
> > > > > > > AFAIK this is currently not a problem. According to
> > > > > > > http://lwn.net/Articles/627419/ these kmalloc calls never
> > > > > > > fail because they allocate less than a page.
> > > > > > 
> > > > > > I strongly suggest you unlearn that fact.
> > > > > > The fix for this is in two parts:
> > > > > > 
> > > > > > 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> > > > > >    a few times in low memory situations, but this isn't a high
> > > > > >    performance path.
> > > > > > 
> > > > > > 2) Handle get_config (and other) failure in some more elegant way.
> > > > 
> > > > Do you mean we need to enable the caller to deal with get_config
> > > > failures (and the transport to relay those failures)? I agree with that.
> > > 
> > > We can certainly tweak code to bypass need to kmalloc
> > > on get_config.
> > > 
> > > Why is it doing these allocs? What's wrong with using
> > > vcdev->config directly?
> > 
> > We'd need to make sure that vcdev->config is allocated with GFP_DMA, as
> > we need it to be under 2G. And we need to be more careful wrt
> > serialization, especially if we want to reuse the ccw structure as
> > well, for example. Nothing complicated, I'd just need some free time to
> > do it :)
> > 
> > The more likely reason for get_config to fail is a device hotunplug,
> > however. We'll get a seperate notification about that (via machine
> > check + channel report), but it would be nice if we could stop poking
> > the device immediately, as there's no use trying to do something with
> > it anymore.
> 
> Normally, hotunplug requires guest cooperation.
> IOW unplug request should send guest interrupt,
> then block until guest confirms it's not using the
> device anymore.
> virtio pci already handles that fine, can't ccw
> do something similar?

Hotunplug for channel devices does not require guest feedback. (In
fact, I was surprised to hear that there is somthing like guest
cooperation on other platforms.) Basically, the guest is simply
presented with the fact that the device is gone and has to deal with
it. It does not matter whether the device was removed by operator
request or due to a hardware failure.

(We do have support in the s390 channel device core to be able to deal
with devices going away and coming back gracefully. ccw devices can be
put into a special state where they retain their configuration so that
they can be reactivated if they become available again. For example,
dasd (disk) devices survive being detached and reattached just fine,
even under I/O load. See the ->notify() callback of the ccw driver for
details.)

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Mon, Mar 02, 2015 at 01:11:02PM +0100, Cornelia Huck wrote:
> On Mon, 2 Mar 2015 12:46:57 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Mar 02, 2015 at 12:31:06PM +0100, Cornelia Huck wrote:
> > > On Mon, 2 Mar 2015 12:13:58 +0100
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> > > > > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > > On Thu, 26 Feb 2015 11:50:42 +1030
> > > > > > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > > > >
> > > > > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > >> >  Hi all,
> > > > > >> >
> > > > > >> > with the recent kernel 3.19, I get a kernel warning when I start my
> > > > > >> > KVM guest on s390 with virtio balloon enabled:
> > > > > >> 
> > > > > >> The deeper problem is that virtio_ccw_get_config just silently fails on
> > > > > >> OOM.
> > > > > >> 
> > > > > >> Neither get_config nor set_config are expected to fail.
> > > > > >
> > > > > > AFAIK this is currently not a problem. According to
> > > > > > http://lwn.net/Articles/627419/ these kmalloc calls never
> > > > > > fail because they allocate less than a page.
> > > > > 
> > > > > I strongly suggest you unlearn that fact.
> > > > > The fix for this is in two parts:
> > > > > 
> > > > > 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> > > > >    a few times in low memory situations, but this isn't a high
> > > > >    performance path.
> > > > > 
> > > > > 2) Handle get_config (and other) failure in some more elegant way.
> > > 
> > > Do you mean we need to enable the caller to deal with get_config
> > > failures (and the transport to relay those failures)? I agree with that.
> > 
> > We can certainly tweak code to bypass need to kmalloc
> > on get_config.
> > 
> > Why is it doing these allocs? What's wrong with using
> > vcdev->config directly?
> 
> We'd need to make sure that vcdev->config is allocated with GFP_DMA, as
> we need it to be under 2G.

I see - and that's expensive when there are many devices?
One simple solution is to have a global buffer that everyone
reuses. It'll need a lock, naturally.

> And we need to be more careful wrt
> serialization,

Why does passing in vcdev->config mean we need to be more careful
than when we allocate a buffer and then memcpy into vcdev->config?

> especially if we want to reuse the ccw structure as
> well, for example. Nothing complicated, I'd just need some free time to
> do it :)
> 
> The more likely reason for get_config to fail is a device hotunplug,
> however. We'll get a seperate notification about that (via machine
> check + channel report), but it would be nice if we could stop poking
> the device immediately, as there's no use trying to do something with
> it anymore.

-- 
MST

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Mon, Mar 02, 2015 at 01:11:02PM +0100, Cornelia Huck wrote:
> On Mon, 2 Mar 2015 12:46:57 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Mar 02, 2015 at 12:31:06PM +0100, Cornelia Huck wrote:
> > > On Mon, 2 Mar 2015 12:13:58 +0100
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> > > > > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > > On Thu, 26 Feb 2015 11:50:42 +1030
> > > > > > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > > > >
> > > > > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > >> >  Hi all,
> > > > > >> >
> > > > > >> > with the recent kernel 3.19, I get a kernel warning when I start my
> > > > > >> > KVM guest on s390 with virtio balloon enabled:
> > > > > >> 
> > > > > >> The deeper problem is that virtio_ccw_get_config just silently fails on
> > > > > >> OOM.
> > > > > >> 
> > > > > >> Neither get_config nor set_config are expected to fail.
> > > > > >
> > > > > > AFAIK this is currently not a problem. According to
> > > > > > http://lwn.net/Articles/627419/ these kmalloc calls never
> > > > > > fail because they allocate less than a page.
> > > > > 
> > > > > I strongly suggest you unlearn that fact.
> > > > > The fix for this is in two parts:
> > > > > 
> > > > > 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> > > > >    a few times in low memory situations, but this isn't a high
> > > > >    performance path.
> > > > > 
> > > > > 2) Handle get_config (and other) failure in some more elegant way.
> > > 
> > > Do you mean we need to enable the caller to deal with get_config
> > > failures (and the transport to relay those failures)? I agree with that.
> > 
> > We can certainly tweak code to bypass need to kmalloc
> > on get_config.
> > 
> > Why is it doing these allocs? What's wrong with using
> > vcdev->config directly?
> 
> We'd need to make sure that vcdev->config is allocated with GFP_DMA, as
> we need it to be under 2G.

I see - and that's expensive when there are many devices?
One simple solution is to have a global buffer that everyone
reuses. It'll need a lock, naturally.

> And we need to be more careful wrt
> serialization,

Why does passing in vcdev->config mean we need to be more careful
than when we allocate a buffer and then memcpy into vcdev->config?

> especially if we want to reuse the ccw structure as
> well, for example. Nothing complicated, I'd just need some free time to
> do it :)
> 
> The more likely reason for get_config to fail is a device hotunplug,
> however. We'll get a seperate notification about that (via machine
> check + channel report), but it would be nice if we could stop poking
> the device immediately, as there's no use trying to do something with
> it anymore.

-- 
MST

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Mon, Mar 02, 2015 at 01:35:05PM +0100, Cornelia Huck wrote:
> On Mon, 2 Mar 2015 13:19:43 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Mar 02, 2015 at 01:11:02PM +0100, Cornelia Huck wrote:
> > > On Mon, 2 Mar 2015 12:46:57 +0100
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Mon, Mar 02, 2015 at 12:31:06PM +0100, Cornelia Huck wrote:
> > > > > On Mon, 2 Mar 2015 12:13:58 +0100
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> > > > > > > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > > > > On Thu, 26 Feb 2015 11:50:42 +1030
> > > > > > > > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > > > > > >
> > > > > > > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > > > >> >  Hi all,
> > > > > > > >> >
> > > > > > > >> > with the recent kernel 3.19, I get a kernel warning when I start my
> > > > > > > >> > KVM guest on s390 with virtio balloon enabled:
> > > > > > > >> 
> > > > > > > >> The deeper problem is that virtio_ccw_get_config just silently fails on
> > > > > > > >> OOM.
> > > > > > > >> 
> > > > > > > >> Neither get_config nor set_config are expected to fail.
> > > > > > > >
> > > > > > > > AFAIK this is currently not a problem. According to
> > > > > > > > http://lwn.net/Articles/627419/ these kmalloc calls never
> > > > > > > > fail because they allocate less than a page.
> > > > > > > 
> > > > > > > I strongly suggest you unlearn that fact.
> > > > > > > The fix for this is in two parts:
> > > > > > > 
> > > > > > > 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> > > > > > >    a few times in low memory situations, but this isn't a high
> > > > > > >    performance path.
> > > > > > > 
> > > > > > > 2) Handle get_config (and other) failure in some more elegant way.
> > > > > 
> > > > > Do you mean we need to enable the caller to deal with get_config
> > > > > failures (and the transport to relay those failures)? I agree with that.
> > > > 
> > > > We can certainly tweak code to bypass need to kmalloc
> > > > on get_config.
> > > > 
> > > > Why is it doing these allocs? What's wrong with using
> > > > vcdev->config directly?
> > > 
> > > We'd need to make sure that vcdev->config is allocated with GFP_DMA, as
> > > we need it to be under 2G. And we need to be more careful wrt
> > > serialization, especially if we want to reuse the ccw structure as
> > > well, for example. Nothing complicated, I'd just need some free time to
> > > do it :)
> > > 
> > > The more likely reason for get_config to fail is a device hotunplug,
> > > however. We'll get a seperate notification about that (via machine
> > > check + channel report), but it would be nice if we could stop poking
> > > the device immediately, as there's no use trying to do something with
> > > it anymore.
> > 
> > Normally, hotunplug requires guest cooperation.
> > IOW unplug request should send guest interrupt,
> > then block until guest confirms it's not using the
> > device anymore.
> > virtio pci already handles that fine, can't ccw
> > do something similar?
> 
> Hotunplug for channel devices does not require guest feedback. (In
> fact, I was surprised to hear that there is somthing like guest
> cooperation on other platforms.)

Consider a storage device. If you don't flush out caches
before removing the disk, you might lose a bunch of data.

> Basically, the guest is simply
> presented with the fact that the device is gone and has to deal with
> it. It does not matter whether the device was removed by operator
> request or due to a hardware failure.
> 
> (We do have support in the s390 channel device core to be able to deal
> with devices going away and coming back gracefully. ccw devices can be
> put into a special state where they retain their configuration so that
> they can be reactivated if they become available again. For example,
> dasd (disk) devices survive being detached and reattached just fine,
> even under I/O load.
> See the ->notify() callback of the ccw driver for
> details.)

How does guest distinguish between this and intentional permanent
removal?

-- 
MST

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Mon, Mar 02, 2015 at 01:35:05PM +0100, Cornelia Huck wrote:
> On Mon, 2 Mar 2015 13:19:43 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Mar 02, 2015 at 01:11:02PM +0100, Cornelia Huck wrote:
> > > On Mon, 2 Mar 2015 12:46:57 +0100
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Mon, Mar 02, 2015 at 12:31:06PM +0100, Cornelia Huck wrote:
> > > > > On Mon, 2 Mar 2015 12:13:58 +0100
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> > > > > > > Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > > > > On Thu, 26 Feb 2015 11:50:42 +1030
> > > > > > > > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > > > > > >
> > > > > > > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > > > > > > >> >  Hi all,
> > > > > > > >> >
> > > > > > > >> > with the recent kernel 3.19, I get a kernel warning when I start my
> > > > > > > >> > KVM guest on s390 with virtio balloon enabled:
> > > > > > > >> 
> > > > > > > >> The deeper problem is that virtio_ccw_get_config just silently fails on
> > > > > > > >> OOM.
> > > > > > > >> 
> > > > > > > >> Neither get_config nor set_config are expected to fail.
> > > > > > > >
> > > > > > > > AFAIK this is currently not a problem. According to
> > > > > > > > http://lwn.net/Articles/627419/ these kmalloc calls never
> > > > > > > > fail because they allocate less than a page.
> > > > > > > 
> > > > > > > I strongly suggest you unlearn that fact.
> > > > > > > The fix for this is in two parts:
> > > > > > > 
> > > > > > > 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> > > > > > >    a few times in low memory situations, but this isn't a high
> > > > > > >    performance path.
> > > > > > > 
> > > > > > > 2) Handle get_config (and other) failure in some more elegant way.
> > > > > 
> > > > > Do you mean we need to enable the caller to deal with get_config
> > > > > failures (and the transport to relay those failures)? I agree with that.
> > > > 
> > > > We can certainly tweak code to bypass need to kmalloc
> > > > on get_config.
> > > > 
> > > > Why is it doing these allocs? What's wrong with using
> > > > vcdev->config directly?
> > > 
> > > We'd need to make sure that vcdev->config is allocated with GFP_DMA, as
> > > we need it to be under 2G. And we need to be more careful wrt
> > > serialization, especially if we want to reuse the ccw structure as
> > > well, for example. Nothing complicated, I'd just need some free time to
> > > do it :)
> > > 
> > > The more likely reason for get_config to fail is a device hotunplug,
> > > however. We'll get a seperate notification about that (via machine
> > > check + channel report), but it would be nice if we could stop poking
> > > the device immediately, as there's no use trying to do something with
> > > it anymore.
> > 
> > Normally, hotunplug requires guest cooperation.
> > IOW unplug request should send guest interrupt,
> > then block until guest confirms it's not using the
> > device anymore.
> > virtio pci already handles that fine, can't ccw
> > do something similar?
> 
> Hotunplug for channel devices does not require guest feedback. (In
> fact, I was surprised to hear that there is somthing like guest
> cooperation on other platforms.)

Consider a storage device. If you don't flush out caches
before removing the disk, you might lose a bunch of data.

> Basically, the guest is simply
> presented with the fact that the device is gone and has to deal with
> it. It does not matter whether the device was removed by operator
> request or due to a hardware failure.
> 
> (We do have support in the s390 channel device core to be able to deal
> with devices going away and coming back gracefully. ccw devices can be
> put into a special state where they retain their configuration so that
> they can be reactivated if they become available again. For example,
> dasd (disk) devices survive being detached and reattached just fine,
> even under I/O load.
> See the ->notify() callback of the ccw driver for
> details.)

How does guest distinguish between this and intentional permanent
removal?

-- 
MST

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
>> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
>> > On Thu, 26 Feb 2015 11:50:42 +1030
>> > Rusty Russell <rusty@rustcorp.com.au> wrote:
>> >
>> >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
>> >> >  Hi all,
>> >> >
>> >> > with the recent kernel 3.19, I get a kernel warning when I start my
>> >> > KVM guest on s390 with virtio balloon enabled:
>> >> 
>> >> The deeper problem is that virtio_ccw_get_config just silently fails on
>> >> OOM.
>> >> 
>> >> Neither get_config nor set_config are expected to fail.
>> >
>> > AFAIK this is currently not a problem. According to
>> > http://lwn.net/Articles/627419/ these kmalloc calls never
>> > fail because they allocate less than a page.
>> 
>> I strongly suggest you unlearn that fact.
>> The fix for this is in two parts:
>> 
>> 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
>>    a few times in low memory situations, but this isn't a high
>>    performance path.
>> 
>> 2) Handle get_config (and other) failure in some more elegant way.
>> 
>> Cheers,
>> Rusty.
>
> I agree, but I'd like to point out that even without kmalloc,
> on s390 get_config is blocking - it's waiting
> for a hardware interrupt.
>
> And it makes sense: config is not data path, I don't think
> we should spin there.
>
> So I think besides these two parts, we still need my two patches:
>     virtio-balloon: do not call blocking ops when !TASK_RUNNING

I prefer to annotate, over trying to fix this.

Because it's not important.  We might spin a few times, but it's very
unlikely, and it's certainly not performance critical.

Thanks,
Rusty.

Subject: virtio_balloon: annotate possible sleep waiting for event.

CCW (s390) does this.

Reported-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0413157f3b49..3f4d5acdbde0 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -340,6 +340,15 @@ static int balloon(void *_vballoon)
 		s64 diff;
 
 		try_to_freeze();
+
+		/*
+		 * Reading the config on the ccw backend involves an
+		 * allocation, so we may actually sleep and have an
+		 * extra iteration.  It's extremely unlikely, and this
+		 * isn't a fast path in any sense.
+		 */
+		sched_annotate_sleep();
+
 		wait_event_interruptible(vb->config_change,
 					 (diff = towards_target(vb)) != 0
 					 || vb->need_stats_update

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Michael S. Tsirkin]

On Wed, Mar 04, 2015 at 04:44:54PM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> >> > On Thu, 26 Feb 2015 11:50:42 +1030
> >> > Rusty Russell <rusty@rustcorp.com.au> wrote:
> >> >
> >> >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> >> >> >  Hi all,
> >> >> >
> >> >> > with the recent kernel 3.19, I get a kernel warning when I start my
> >> >> > KVM guest on s390 with virtio balloon enabled:
> >> >> 
> >> >> The deeper problem is that virtio_ccw_get_config just silently fails on
> >> >> OOM.
> >> >> 
> >> >> Neither get_config nor set_config are expected to fail.
> >> >
> >> > AFAIK this is currently not a problem. According to
> >> > http://lwn.net/Articles/627419/ these kmalloc calls never
> >> > fail because they allocate less than a page.
> >> 
> >> I strongly suggest you unlearn that fact.
> >> The fix for this is in two parts:
> >> 
> >> 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> >>    a few times in low memory situations, but this isn't a high
> >>    performance path.
> >> 
> >> 2) Handle get_config (and other) failure in some more elegant way.
> >> 
> >> Cheers,
> >> Rusty.
> >
> > I agree, but I'd like to point out that even without kmalloc,
> > on s390 get_config is blocking - it's waiting
> > for a hardware interrupt.
> >
> > And it makes sense: config is not data path, I don't think
> > we should spin there.
> >
> > So I think besides these two parts, we still need my two patches:
> >     virtio-balloon: do not call blocking ops when !TASK_RUNNING
> 
> I prefer to annotate, over trying to fix this.
> 
> Because it's not important.  We might spin a few times, but it's very
> unlikely, and it's certainly not performance critical.
> 
> Thanks,
> Rusty.
> 
> Subject: virtio_balloon: annotate possible sleep waiting for event.
> 
> CCW (s390) does this.
> 
> Reported-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 0413157f3b49..3f4d5acdbde0 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -340,6 +340,15 @@ static int balloon(void *_vballoon)
>  		s64 diff;
>  
>  		try_to_freeze();
> +
> +		/*
> +		 * Reading the config on the ccw backend involves an
> +		 * allocation, so we may actually sleep and have an
> +		 * extra iteration.  It's extremely unlikely,

Hmm, this part of the comment seems wrong to me.
Reading the config on the ccw backend always sleeps
because it's interrupt driven.

This is the relevant code:

static int ccw_io_helper(struct virtio_ccw_device *vcdev,
                         struct ccw1 *ccw, __u32 intparm)
{
        int ret;
        unsigned long flags;
        int flag = intparm & VIRTIO_CCW_INTPARM_MASK;

        do {
                spin_lock_irqsave(get_ccwdev_lock(vcdev->cdev), flags);
                ret = ccw_device_start(vcdev->cdev, ccw, intparm, 0, 0);
                if (!ret) {
                        if (!vcdev->curr_io)
                                vcdev->err = 0;
                        vcdev->curr_io |= flag;
                }
                spin_unlock_irqrestore(get_ccwdev_lock(vcdev->cdev), flags);
                cpu_relax();
        } while (ret == -EBUSY);
        wait_event(vcdev->wait_q, doing_io(vcdev, flag) == 0);
        return ret ? ret : vcdev->err;
}


>		 and this
> +		 * isn't a fast path in any sense.
> +		 */
> +		sched_annotate_sleep();
> +
>  		wait_event_interruptible(vb->config_change,
>  					 (diff = towards_target(vb)) != 0
>  					 || vb->need_stats_update


So the wait_event_interruptible always calls wait_event
which then becomes a busy wait on s390, which is not nice.

So I suspect
http://mid.gmane.org/1424874878-17155-1-git-send-email-mst@redhat.com
is better.

What do you think?

-- 
MST

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Cornelia Huck]

On Mon, 2 Mar 2015 21:44:10 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:


> > > Normally, hotunplug requires guest cooperation.
> > > IOW unplug request should send guest interrupt,
> > > then block until guest confirms it's not using the
> > > device anymore.
> > > virtio pci already handles that fine, can't ccw
> > > do something similar?
> > 
> > Hotunplug for channel devices does not require guest feedback. (In
> > fact, I was surprised to hear that there is somthing like guest
> > cooperation on other platforms.)
> 
> Consider a storage device. If you don't flush out caches
> before removing the disk, you might lose a bunch of data.

Yes, that is a problem. But hotunplug is indistinguishable from a hw
failure on s390, so there's not really much we can do here.

> 
> > Basically, the guest is simply
> > presented with the fact that the device is gone and has to deal with
> > it. It does not matter whether the device was removed by operator
> > request or due to a hardware failure.
> > 
> > (We do have support in the s390 channel device core to be able to deal
> > with devices going away and coming back gracefully. ccw devices can be
> > put into a special state where they retain their configuration so that
> > they can be reactivated if they become available again. For example,
> > dasd (disk) devices survive being detached and reattached just fine,
> > even under I/O load.
> > See the ->notify() callback of the ccw driver for
> > details.)
> 
> How does guest distinguish between this and intentional permanent
> removal?

It can't. It will get the same kind of notifications (and channel I/O
failures) for both. Only the admin has a chance of knowing, and they
may kill off a device in that state permanently (which, of course,
triggers the flush problems etc. which have just been delayed from the
initial detach).

Given that this is what the architecture gives us on all hypervisors
(LPAR and z/VM) and is for all I know decades old, it is what we have
to implement in qemu/kvm as well.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Cornelia Huck]

On Mon, 2 Mar 2015 21:44:10 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:


> > > Normally, hotunplug requires guest cooperation.
> > > IOW unplug request should send guest interrupt,
> > > then block until guest confirms it's not using the
> > > device anymore.
> > > virtio pci already handles that fine, can't ccw
> > > do something similar?
> > 
> > Hotunplug for channel devices does not require guest feedback. (In
> > fact, I was surprised to hear that there is somthing like guest
> > cooperation on other platforms.)
> 
> Consider a storage device. If you don't flush out caches
> before removing the disk, you might lose a bunch of data.

Yes, that is a problem. But hotunplug is indistinguishable from a hw
failure on s390, so there's not really much we can do here.

> 
> > Basically, the guest is simply
> > presented with the fact that the device is gone and has to deal with
> > it. It does not matter whether the device was removed by operator
> > request or due to a hardware failure.
> > 
> > (We do have support in the s390 channel device core to be able to deal
> > with devices going away and coming back gracefully. ccw devices can be
> > put into a special state where they retain their configuration so that
> > they can be reactivated if they become available again. For example,
> > dasd (disk) devices survive being detached and reattached just fine,
> > even under I/O load.
> > See the ->notify() callback of the ccw driver for
> > details.)
> 
> How does guest distinguish between this and intentional permanent
> removal?

It can't. It will get the same kind of notifications (and channel I/O
failures) for both. Only the admin has a chance of knowing, and they
may kill off a device in that state permanently (which, of course,
triggers the flush problems etc. which have just been delayed from the
initial detach).

Given that this is what the architecture gives us on all hypervisors
(LPAR and z/VM) and is for all I know decades old, it is what we have
to implement in qemu/kvm as well.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Cornelia Huck]

On Wed, 4 Mar 2015 11:25:56 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Mar 04, 2015 at 04:44:54PM +1030, Rusty Russell wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
> > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > >> > On Thu, 26 Feb 2015 11:50:42 +1030
> > >> > Rusty Russell <rusty@rustcorp.com.au> wrote:
> > >> >
> > >> >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
> > >> >> >  Hi all,
> > >> >> >
> > >> >> > with the recent kernel 3.19, I get a kernel warning when I start my
> > >> >> > KVM guest on s390 with virtio balloon enabled:
> > >> >> 
> > >> >> The deeper problem is that virtio_ccw_get_config just silently fails on
> > >> >> OOM.
> > >> >> 
> > >> >> Neither get_config nor set_config are expected to fail.
> > >> >
> > >> > AFAIK this is currently not a problem. According to
> > >> > http://lwn.net/Articles/627419/ these kmalloc calls never
> > >> > fail because they allocate less than a page.
> > >> 
> > >> I strongly suggest you unlearn that fact.
> > >> The fix for this is in two parts:
> > >> 
> > >> 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
> > >>    a few times in low memory situations, but this isn't a high
> > >>    performance path.
> > >> 
> > >> 2) Handle get_config (and other) failure in some more elegant way.
> > >> 
> > >> Cheers,
> > >> Rusty.
> > >
> > > I agree, but I'd like to point out that even without kmalloc,
> > > on s390 get_config is blocking - it's waiting
> > > for a hardware interrupt.
> > >
> > > And it makes sense: config is not data path, I don't think
> > > we should spin there.
> > >
> > > So I think besides these two parts, we still need my two patches:
> > >     virtio-balloon: do not call blocking ops when !TASK_RUNNING
> > 
> > I prefer to annotate, over trying to fix this.
> > 
> > Because it's not important.  We might spin a few times, but it's very
> > unlikely, and it's certainly not performance critical.
> > 
> > Thanks,
> > Rusty.
> > 
> > Subject: virtio_balloon: annotate possible sleep waiting for event.
> > 
> > CCW (s390) does this.
> > 
> > Reported-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > 
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index 0413157f3b49..3f4d5acdbde0 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -340,6 +340,15 @@ static int balloon(void *_vballoon)
> >  		s64 diff;
> >  
> >  		try_to_freeze();
> > +
> > +		/*
> > +		 * Reading the config on the ccw backend involves an
> > +		 * allocation, so we may actually sleep and have an
> > +		 * extra iteration.  It's extremely unlikely,
> 
> Hmm, this part of the comment seems wrong to me.
> Reading the config on the ccw backend always sleeps
> because it's interrupt driven.

(...)

> So I suspect
> http://mid.gmane.org/1424874878-17155-1-git-send-email-mst@redhat.com
> is better.
> 
> What do you think?

I'd prefer to fix this as well. While the I/O request completes
instantly on current qemu (the ssch backend handles the start function
immediately, not asynchronously as on real hardware), this (a) is an
implementation detail that may change and (b) doesn't account for the
need to deliver the interrupt to the guest - which might take non-zero
time.

Re: virtio balloon: do not call blocking ops when !TASK_RUNNING

[Rusty Russell]

Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> On Wed, 4 Mar 2015 11:25:56 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Wed, Mar 04, 2015 at 04:44:54PM +1030, Rusty Russell wrote:
>> > "Michael S. Tsirkin" <mst@redhat.com> writes:
>> > > On Mon, Mar 02, 2015 at 10:37:26AM +1030, Rusty Russell wrote:
>> > >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
>> > >> > On Thu, 26 Feb 2015 11:50:42 +1030
>> > >> > Rusty Russell <rusty@rustcorp.com.au> wrote:
>> > >> >
>> > >> >> Thomas Huth <thuth@linux.vnet.ibm.com> writes:
>> > >> >> >  Hi all,
>> > >> >> >
>> > >> >> > with the recent kernel 3.19, I get a kernel warning when I start my
>> > >> >> > KVM guest on s390 with virtio balloon enabled:
>> > >> >> 
>> > >> >> The deeper problem is that virtio_ccw_get_config just silently fails on
>> > >> >> OOM.
>> > >> >> 
>> > >> >> Neither get_config nor set_config are expected to fail.
>> > >> >
>> > >> > AFAIK this is currently not a problem. According to
>> > >> > http://lwn.net/Articles/627419/ these kmalloc calls never
>> > >> > fail because they allocate less than a page.
>> > >> 
>> > >> I strongly suggest you unlearn that fact.
>> > >> The fix for this is in two parts:
>> > >> 
>> > >> 1) Annotate using sched_annotate_sleep() and add a comment: we may spin
>> > >>    a few times in low memory situations, but this isn't a high
>> > >>    performance path.
>> > >> 
>> > >> 2) Handle get_config (and other) failure in some more elegant way.
>> > >> 
>> > >> Cheers,
>> > >> Rusty.
>> > >
>> > > I agree, but I'd like to point out that even without kmalloc,
>> > > on s390 get_config is blocking - it's waiting
>> > > for a hardware interrupt.
>> > >
>> > > And it makes sense: config is not data path, I don't think
>> > > we should spin there.
>> > >
>> > > So I think besides these two parts, we still need my two patches:
>> > >     virtio-balloon: do not call blocking ops when !TASK_RUNNING
>> > 
>> > I prefer to annotate, over trying to fix this.
>> > 
>> > Because it's not important.  We might spin a few times, but it's very
>> > unlikely, and it's certainly not performance critical.
>> > 
>> > Thanks,
>> > Rusty.
>> > 
>> > Subject: virtio_balloon: annotate possible sleep waiting for event.
>> > 
>> > CCW (s390) does this.
>> > 
>> > Reported-by: Thomas Huth <thuth@linux.vnet.ibm.com>
>> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
>> > 
>> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> > index 0413157f3b49..3f4d5acdbde0 100644
>> > --- a/drivers/virtio/virtio_balloon.c
>> > +++ b/drivers/virtio/virtio_balloon.c
>> > @@ -340,6 +340,15 @@ static int balloon(void *_vballoon)
>> >  		s64 diff;
>> >  
>> >  		try_to_freeze();
>> > +
>> > +		/*
>> > +		 * Reading the config on the ccw backend involves an
>> > +		 * allocation, so we may actually sleep and have an
>> > +		 * extra iteration.  It's extremely unlikely,
>> 
>> Hmm, this part of the comment seems wrong to me.
>> Reading the config on the ccw backend always sleeps
>> because it's interrupt driven.
>
> (...)
>
>> So I suspect
>> http://mid.gmane.org/1424874878-17155-1-git-send-email-mst@redhat.com
>> is better.
>> 
>> What do you think?
>
> I'd prefer to fix this as well. While the I/O request completes
> instantly on current qemu (the ssch backend handles the start function
> immediately, not asynchronously as on real hardware), this (a) is an
> implementation detail that may change and (b) doesn't account for the
> need to deliver the interrupt to the guest - which might take non-zero
> time.

Ah, I see.  My mistake.

I've thrown out my patch, applied that one.

Thanks,
Rusty.