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

[Rusty Russell <rusty@rustcorp.com.au>]

"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