Re: virtio balloon: do not call blocking ops when !TASK_RUNNING
On Thu, 26 Feb 2015 11:50:42 +1030 Rusty Russell wrote: > Thomas Huth 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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: virtio balloon: do not call blocking ops when !TASK_RUNNING
Thomas Huth 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 >[<00174a1e>] 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: 021d ti: 021d8000 task.ti: > 021d8000 > [0.839707] Krnl PSW : 0704c0018000 0015bf8e > (__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: 000d 021d 0071 0001 > [0.839718]00675ace 01998c50 > > [0.839720]00982134 0058f824 00a008a8 > > [0.839722]04d9 007ea992 0015bf8a > 021dbc28 > [0.839731] Krnl Code: 0015bf7e: c0200033e838 larl > %r2,7d8fee >0015bf84: c0e50028cd62 brasl %r14,675a48 > #0015bf8a: a7f40001 brc 15,15bf8c > >0015bf8e: 9201a000 mvi 0(%r10),1 >0015bf92: a7f4ffe2 brc 15,15bf56 >0015bf96: 0707 bcr 0,%r7 >0015bf98: ebdff0800024 stmg%r13,%r15,128(%r15) >0015bf9e: a7f13fe0 tmll%r15,16352 > [0.839749] Call Trace: > [0.839751] ([<0015bf8a>] __might_sleep+0x8a/0x98) > [0.839756] [<0028a562>] __kmalloc+0x272/0x350 > [0.839759] [<0058f824>] virtio_ccw_get_config+0x3c/0x100 > [0.839762] [<0049fcb0>] balloon+0x1b8/0x330 > [0.839765] [<001529c8>] kthread+0x120/0x138 > [0.839767] [<00683c22>] kernel_thread_starter+0x6/0xc > [0.839770] [<00683c1c>] kernel_thread_starter+0x0/0xc > [0.839772] no locks held by vballoon/46. > [0.839773] Last Breaking-Event-Address: > [0.839776] [<0015bf8a>] __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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost: drop hard-coded num_buffers size
From: "Michael S. Tsirkin" Date: Tue, 24 Feb 2015 17:31:10 +0100 > The 2 that we use for copy_to_iter comes from sizeof(u16), > it used to be that way before the iov iter update. > Fix it up, making it obvious the size of stack access > is right. > > Signed-off-by: Michael S. Tsirkin Michael, what tree do you want these two patches to go through? ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio-balloon: do not call blocking ops when !TASK_RUNNING
On Wed, 25 Feb 2015 16:11:27 +0100 Cornelia Huck wrote: > On Wed, 25 Feb 2015 15:36:02 +0100 > "Michael S. Tsirkin" wrote: > > > 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: sta...@vger.kernel.org > > Reported-by: Thomas Huth > > Signed-off-by: Michael S. Tsirkin > > --- > > > > changes from v1: > > remove wait_event_interruptible > > noticed by Cornelia Huck > > > > drivers/virtio/virtio_balloon.c | 19 ++- > > 1 file changed, 14 insertions(+), 5 deletions(-) > > > > I was able to reproduce Thomas' original problem and can confirm that > it is gone with this patch. > > Reviewed-by: Cornelia Huck Right, I just applied the patch on my system, too, and the problem is indeed gone! Thanks for the quick fix! Tested-by: Thomas Huth ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2] virtio-balloon: do not call blocking ops when !TASK_RUNNING
On Wed, 25 Feb 2015 15:36:02 +0100 "Michael S. Tsirkin" wrote: > 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: sta...@vger.kernel.org > Reported-by: Thomas Huth > Signed-off-by: Michael S. Tsirkin > --- > > changes from v1: > remove wait_event_interruptible > noticed by Cornelia Huck > > drivers/virtio/virtio_balloon.c | 19 ++- > 1 file changed, 14 insertions(+), 5 deletions(-) > I was able to reproduce Thomas' original problem and can confirm that it is gone with this patch. Reviewed-by: Cornelia Huck ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2] virtio-balloon: do not call blocking ops when !TASK_RUNNING
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: sta...@vger.kernel.org
Reported-by: Thomas Huth
Signed-off-by: Michael S. Tsirkin
---
changes from v1:
remove wait_event_interruptible
noticed by Cornelia Huck
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
#include
#include
+#include
/*
* 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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-balloon: do not call blocking ops when !TASK_RUNNING
On Wed, Feb 25, 2015 at 03:32:08PM +0100, Cornelia Huck wrote:
> On Wed, 25 Feb 2015 15:14:36 +0100
> "Michael S. Tsirkin" wrote:
>
> > 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: sta...@vger.kernel.org
> > Reported-by: Thomas Huth
> > Signed-off-by: Michael S. Tsirkin
> > ---
> > drivers/virtio/virtio_balloon.c | 14 ++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_balloon.c
> > b/drivers/virtio/virtio_balloon.c
> > index 0413157..2f19f65 100644
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -29,6 +29,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > /*
> > * Balloon device works in 4K page units. So each page is pointed to by
> > @@ -334,12 +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();
> > +
> > + 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);
> > +
> > wait_event_interruptible(vb->config_change,
> > (diff = towards_target(vb)) != 0
> > || vb->need_stats_update
>
> Forgot to remove the wait_event_interruptible()?
Ugh. Forgot to commit :(
Will resend.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-balloon: do not call blocking ops when !TASK_RUNNING
On Wed, 25 Feb 2015 15:14:36 +0100
"Michael S. Tsirkin" wrote:
> 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: sta...@vger.kernel.org
> Reported-by: Thomas Huth
> Signed-off-by: Michael S. Tsirkin
> ---
> drivers/virtio/virtio_balloon.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 0413157..2f19f65 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
>
> /*
> * Balloon device works in 4K page units. So each page is pointed to by
> @@ -334,12 +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();
> +
> + 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);
> +
> wait_event_interruptible(vb->config_change,
>(diff = towards_target(vb)) != 0
>|| vb->need_stats_update
Forgot to remove the wait_event_interruptible()?
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] vhost: drop hard-coded num_buffers size
The 2 that we use for copy_to_iter comes from sizeof(u16),
it used to be that way before the iov iter update.
Fix it up, making it obvious the size of stack access
is right.
Signed-off-by: Michael S. Tsirkin
---
drivers/vhost/net.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index ca70434..2bbfc25 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -621,7 +621,8 @@ static void handle_rx(struct vhost_net *net)
num_buffers = cpu_to_vhost16(vq, headcount);
if (likely(mergeable) &&
- copy_to_iter(&num_buffers, 2, &fixup) != 2) {
+ copy_to_iter(&num_buffers, sizeof num_buffers,
+&fixup) != sizeof num_buffers) {
vq_err(vq, "Failed num_buffers write");
vhost_discard_vq_desc(vq, headcount);
break;
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] vhost: cleanup iterator update logic
Recent iterator-related changes in vhost made it
harder to follow the logic fixing up the header.
In fact, the fixup always happens at the same
offset: sizeof(virtio_net_hdr): sometimes the
fixup iterator is updated by copy_to_iter,
sometimes-by iov_iter_advance.
Rearrange code to make this obvious.
Signed-off-by: Michael S. Tsirkin
---
drivers/vhost/net.c | 22 --
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index afa06d2..ca70434 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -591,11 +591,6 @@ static void handle_rx(struct vhost_net *net)
* TODO: support TSO.
*/
iov_iter_advance(&msg.msg_iter, vhost_hlen);
- } else {
- /* It'll come from socket; we'll need to patch
-* ->num_buffers over if VIRTIO_NET_F_MRG_RXBUF
-*/
- iov_iter_advance(&fixup, sizeof(hdr));
}
err = sock->ops->recvmsg(NULL, sock, &msg,
sock_len, MSG_DONTWAIT | MSG_TRUNC);
@@ -609,11 +604,18 @@ static void handle_rx(struct vhost_net *net)
continue;
}
/* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
- if (unlikely(vhost_hlen) &&
- copy_to_iter(&hdr, sizeof(hdr), &fixup) != sizeof(hdr)) {
- vq_err(vq, "Unable to write vnet_hdr at addr %p\n",
- vq->iov->iov_base);
- break;
+ if (unlikely(vhost_hlen)) {
+ if (copy_to_iter(&hdr, sizeof(hdr),
+&fixup) != sizeof(hdr)) {
+ vq_err(vq, "Unable to write vnet_hdr "
+ "at addr %p\n", vq->iov->iov_base);
+ break;
+ }
+ } else {
+ /* Header came from socket; we'll need to patch
+* ->num_buffers over if VIRTIO_NET_F_MRG_RXBUF
+*/
+ iov_iter_advance(&fixup, sizeof(hdr));
}
/* TODO: Should check and handle checksum. */
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: virtio balloon: do not call blocking ops when !TASK_RUNNING
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 >[<00174a1e>] 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: 021d ti: 021d8000 task.ti: > 021d8000 > [0.839707] Krnl PSW : 0704c0018000 0015bf8e > (__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: 000d 021d 0071 0001 > [0.839718]00675ace 01998c50 > > [0.839720]00982134 0058f824 00a008a8 > > [0.839722]04d9 007ea992 0015bf8a > 021dbc28 > [0.839731] Krnl Code: 0015bf7e: c0200033e838 larl > %r2,7d8fee >0015bf84: c0e50028cd62 brasl %r14,675a48 > #0015bf8a: a7f40001 brc 15,15bf8c > >0015bf8e: 9201a000 mvi 0(%r10),1 >0015bf92: a7f4ffe2 brc 15,15bf56 >0015bf96: 0707 bcr 0,%r7 >0015bf98: ebdff0800024 stmg%r13,%r15,128(%r15) >0015bf9e: a7f13fe0 tmll%r15,16352 > [0.839749] Call Trace: > [0.839751] ([<0015bf8a>] __might_sleep+0x8a/0x98) > [0.839756] [<0028a562>] __kmalloc+0x272/0x350 > [0.839759] [<0058f824>] virtio_ccw_get_config+0x3c/0x100 > [0.839762] [<0049fcb0>] balloon+0x1b8/0x330 > [0.839765] [<001529c8>] kthread+0x120/0x138 > [0.839767] [<00683c22>] kernel_thread_starter+0x6/0xc > [0.839770] [<00683c1c>] kernel_thread_starter+0x0/0xc > [0.839772] no locks held by vballoon/46. > [0.839773] Last Breaking-Event-Address: > [0.839776] [<0015bf8a>] __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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio-balloon: do not call blocking ops when !TASK_RUNNING
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: sta...@vger.kernel.org
Reported-by: Thomas Huth
Signed-off-by: Michael S. Tsirkin
---
drivers/virtio/virtio_balloon.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0413157..2f19f65 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
/*
* Balloon device works in 4K page units. So each page is pointed to by
@@ -334,12 +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();
+
+ 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);
+
wait_event_interruptible(vb->config_change,
(diff = towards_target(vb)) != 0
|| vb->need_stats_update
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: Qemu and virtio 1.0
On Wed, 25 Feb 2015 14:50:22 +1030
Rusty Russell wrote:
> OK, I am trying to experiment with virtio 1.0 support using the
> latest kernel and MST's qemu tree:
>
> https://git.kernel.org/cgit/virt/kvm/mst/qemu.git/?h=virtio-1.0
>
> The first issue is that the device config endian was wrong (see
> attached patch).
>
> I'm now setting up a BE guest on my x86 laptop, and a BE and LE guest
> on a BE powerpc machine, to check that all combinations work correctly.
> If others test too, that would be appreciated!
My virtio-1 work had been taking the back seat recently, but I plan to
look into it again soon.
>
> Cheers,
> Rusty.
>
> From 95ac91554ed602f856a2a5fcc25eaffcad1b1c8d Mon Sep 17 00:00:00 2001
> From: Rusty Russell
> Date: Tue, 24 Feb 2015 14:47:44 +1030
> Subject: [PATCH] virtio_config_write*/virtio_config_read*: Don't endian swap
> for virtio 1.0.
Ah, virtio-ccw doesn't use these config space accessors, that's why I
did not look at them.
>
> Signed-off-by: Rusty Russell
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 079944c..882a31b 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -662,7 +662,12 @@ uint32_t virtio_config_readw(VirtIODevice *vdev,
> uint32_t addr)
>
> k->get_config(vdev, vdev->config);
>
> -val = lduw_p(vdev->config + addr);
> +/* Virtio 1.0 is always LE */
> +if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +val = lduw_le_p(vdev->config + addr);
> +} else {
> +val = lduw_p(vdev->config + addr);
> +}
Can't you use the virtio_* helpers for that?
> return val;
> }
>
It seems virtio-pci does some conditional swapping based on
virtio-endianness already, which should account for virtio-1.
virtio-mmio does not seem to do so.
But:
Device code accessing config space already does use the virtio
accessors - or virtio-ccw would not work as it simply copies the whole
config space. So it seems this change simply undos the endian swap in
virtio-pci (while probably breaking virtio-mmio). Can we simply get rid
of the endian swap in virtio-pci instead, or have I been throuroughly
confused?
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: virtio balloon: do not call blocking ops when !TASK_RUNNING
On Wed, 25 Feb 2015 11:13:18 +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 >[<00174a1e>] 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: 021d ti: 021d8000 task.ti: > 021d8000 > [0.839707] Krnl PSW : 0704c0018000 0015bf8e > (__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: 000d 021d 0071 0001 > [0.839718]00675ace 01998c50 > > [0.839720]00982134 0058f824 00a008a8 > > [0.839722]04d9 007ea992 0015bf8a > 021dbc28 > [0.839731] Krnl Code: 0015bf7e: c0200033e838 larl > %r2,7d8fee >0015bf84: c0e50028cd62 brasl %r14,675a48 > #0015bf8a: a7f40001 brc 15,15bf8c > >0015bf8e: 9201a000 mvi 0(%r10),1 >0015bf92: a7f4ffe2 brc 15,15bf56 >0015bf96: 0707 bcr 0,%r7 >0015bf98: ebdff0800024 stmg%r13,%r15,128(%r15) >0015bf9e: a7f13fe0 tmll%r15,16352 > [0.839749] Call Trace: > [0.839751] ([<0015bf8a>] __might_sleep+0x8a/0x98) > [0.839756] [<0028a562>] __kmalloc+0x272/0x350 > [0.839759] [<0058f824>] virtio_ccw_get_config+0x3c/0x100 > [0.839762] [<0049fcb0>] balloon+0x1b8/0x330 > [0.839765] [<001529c8>] kthread+0x120/0x138 > [0.839767] [<00683c22>] kernel_thread_starter+0x6/0xc > [0.839770] [<00683c1c>] kernel_thread_starter+0x0/0xc > [0.839772] no locks held by vballoon/46. > [0.839773] Last Breaking-Event-Address: > [0.839776] [<0015bf8a>] __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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock
* Christian Borntraeger wrote: > > By all means! > > > > You'll first need to cherry-pick these commits: > > > 927609d622a3 kernel: tighten rules for ACCESS ONCE > > c5b19946eb76 kernel: Fix sparse warning for ACCESS_ONCE > > dd36929720f4 kernel: make READ_ONCE() valid on const arguments > > If you go before 3.19, you will also need > >230fa253df63 kernel: Provide READ_ONCE and ASSIGN_ONCE >43239cbe79fc kernel: Change ASSIGN_ONCE(val, x) to WRITE_ONCE(x, val) The affected spinlock code went over several iterations post v3.18, which I think makes the spinlock change too risky and complex to backport so far back. So it's not necessay to backport these READ_ONCE() changes. > > That's the minimum set you will need for backporting, > > due to overlapping changes to the ACCESS_ONCE() > > definition. > > > > and then apply this commit: > > > > d6abfdb20223 x86/spinlocks/paravirt: Fix memory corruption on unlock > > the alternative might be to replace READ_ONCE with > ACCESS_ONCE when doing the backport. Doing changes to patches when doing a backport is a big no-no IMHO. Either there is a clean sequence of upstream commit IDs to cherry-pick, or it should not be backported in most cases. Thanks, Ingo ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock
Am 25.02.2015 um 11:08 schrieb Ingo Molnar: > > * Greg KH wrote: > It's: d6abfdb20223 x86/spinlocks/paravirt: Fix memory corruption on unlock >>> >>> Yes, This is the original patch. Please note I have taken out the >>> READ_ONCE changes from the original patch to avoid build warnings >>> mentioned below. >>> (Those READ_ONCE changes were cosmetic and was not present in the >>> previous versions) >>> You'll also need this fix from Linus to avoid (harmless) build warnings: dd36929720f4 kernel: make READ_ONCE() valid on const arguments >>> >>> So this may not be absolutely necessary with the current patch. >> >> I'd prefer to be as close as possible to the upstream >> patch. So if applying both of these patches will work, >> I'd much rather do that. Changing patches when >> backporting them to stable for no good reason than to >> clean things up, just confuses everyone involved. >> >> Let's keep our messy history :) > > By all means! > > You'll first need to cherry-pick these commits: > > 927609d622a3 kernel: tighten rules for ACCESS ONCE > c5b19946eb76 kernel: Fix sparse warning for ACCESS_ONCE > dd36929720f4 kernel: make READ_ONCE() valid on const arguments If you go before 3.19, you will also need 230fa253df63 kernel: Provide READ_ONCE and ASSIGN_ONCE 43239cbe79fc kernel: Change ASSIGN_ONCE(val, x) to WRITE_ONCE(x, val) > > That's the minimum set you will need for backporting, due > to overlapping changes to the ACCESS_ONCE() definition. > > and then apply this commit: > > d6abfdb20223 x86/spinlocks/paravirt: Fix memory corruption on unlock the alternative might be to replace READ_ONCE with ACCESS_ONCE when doing the backport. This depends on how important you consider backporting the ACCESS_ONCE fixes. Christian ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
virtio balloon: do not call blocking ops when !TASK_RUNNING
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 [<00174a1e>] 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: 021d ti: 021d8000 task.ti: 021d8000 [0.839707] Krnl PSW : 0704c0018000 0015bf8e (__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: 000d 021d 0071 0001 [0.839718]00675ace 01998c50 [0.839720]00982134 0058f824 00a008a8 [0.839722]04d9 007ea992 0015bf8a 021dbc28 [0.839731] Krnl Code: 0015bf7e: c0200033e838larl %r2,7d8fee 0015bf84: c0e50028cd62 brasl %r14,675a48 #0015bf8a: a7f40001 brc 15,15bf8c >0015bf8e: 9201a000 mvi 0(%r10),1 0015bf92: a7f4ffe2 brc 15,15bf56 0015bf96: 0707 bcr 0,%r7 0015bf98: ebdff0800024 stmg%r13,%r15,128(%r15) 0015bf9e: a7f13fe0 tmll%r15,16352 [0.839749] Call Trace: [0.839751] ([<0015bf8a>] __might_sleep+0x8a/0x98) [0.839756] [<0028a562>] __kmalloc+0x272/0x350 [0.839759] [<0058f824>] virtio_ccw_get_config+0x3c/0x100 [0.839762] [<0049fcb0>] balloon+0x1b8/0x330 [0.839765] [<001529c8>] kthread+0x120/0x138 [0.839767] [<00683c22>] kernel_thread_starter+0x6/0xc [0.839770] [<00683c1c>] kernel_thread_starter+0x0/0xc [0.839772] no locks held by vballoon/46. [0.839773] Last Breaking-Event-Address: [0.839776] [<0015bf8a>] __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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH for stable] x86/spinlocks/paravirt: Fix memory corruption on unlock
* Greg KH wrote: > > >It's: > > > > > > d6abfdb20223 x86/spinlocks/paravirt: Fix memory corruption on unlock > > > > Yes, This is the original patch. Please note I have taken out the > > READ_ONCE changes from the original patch to avoid build warnings > > mentioned below. > > (Those READ_ONCE changes were cosmetic and was not present in the > > previous versions) > > > > > > > >You'll also need this fix from Linus to avoid (harmless) > > >build warnings: > > > > > > dd36929720f4 kernel: make READ_ONCE() valid on const arguments > > > > So this may not be absolutely necessary with the current patch. > > I'd prefer to be as close as possible to the upstream > patch. So if applying both of these patches will work, > I'd much rather do that. Changing patches when > backporting them to stable for no good reason than to > clean things up, just confuses everyone involved. > > Let's keep our messy history :) By all means! You'll first need to cherry-pick these commits: 927609d622a3 kernel: tighten rules for ACCESS ONCE c5b19946eb76 kernel: Fix sparse warning for ACCESS_ONCE dd36929720f4 kernel: make READ_ONCE() valid on const arguments That's the minimum set you will need for backporting, due to overlapping changes to the ACCESS_ONCE() definition. and then apply this commit: d6abfdb20223 x86/spinlocks/paravirt: Fix memory corruption on unlock I've double checked that these commits will cherry-pick fine on top of v3.19, in that order, and that an x86-64 defconfig+kvmconfig+PARAVIRT_SPINLOCK=y kernel builds fine without warnings. I've not boot tested the changes, so if anything breaks it's all your fault - while if it works just fine then I'll be glad to take credit for that. Thanks, Ingo ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
