[PATCH MINI-OS v2] xenbus: notify the other end when necessary

[PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Wei Liu]

The xenbus thread didn't send notification to other end when it expected
more data or consumed responses, which led to stalling the ring from
time to time.

This is the culprit that guest was less responsive when using stubdom
because the device model was stalled.

Fix this by sending notification to the other end at the right places.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>

v2: add two more mb()s.

With this path I can migrate a guest with stubdom a few thousand times
without any issue, while before I could easily trigger time out
within a few iterations. This should make OSSTest stubdom test case more
reliable.

Ian, this is a patch suitable for backporting to 4.6. It's good time
branch mini-os now.
---
 xenbus/xenbus.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
index 4613ed6..bc669f2 100644
--- a/xenbus/xenbus.c
+++ b/xenbus/xenbus.c
@@ -205,8 +205,10 @@ static void xenbus_thread_func(void *ign)
             prod = xenstore_buf->rsp_prod;
             DEBUG("Rsp_cons %d, rsp_prod %d.\n", xenstore_buf->rsp_cons,
                     xenstore_buf->rsp_prod);
-            if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons < sizeof(msg))
+            if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons < sizeof(msg)) {
+                notify_remote_via_evtchn(start_info.store_evtchn);
                 break;
+            }
             rmb();
             memcpy_from_ring(xenstore_buf->rsp,
                     &msg,
@@ -217,8 +219,10 @@ static void xenbus_thread_func(void *ign)
                     xenstore_buf->rsp_prod - xenstore_buf->rsp_cons,
                     msg.req_id);
             if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons <
-                    sizeof(msg) + msg.len)
+                    sizeof(msg) + msg.len) {
+                notify_remote_via_evtchn(start_info.store_evtchn);
                 break;
+            }
 
             DEBUG("Message is good.\n");
 
@@ -237,6 +241,7 @@ static void xenbus_thread_func(void *ign)
 		event->path = data;
 		event->token = event->path + strlen(event->path) + 1;
 
+                mb();
                 xenstore_buf->rsp_cons += msg.len + sizeof(msg);
 
                 for (watch = watches; watch; watch = watch->next)
@@ -262,9 +267,13 @@ static void xenbus_thread_func(void *ign)
                     req_info[msg.req_id].reply,
                     MASK_XENSTORE_IDX(xenstore_buf->rsp_cons),
                     msg.len + sizeof(msg));
+                mb();
                 xenstore_buf->rsp_cons += msg.len + sizeof(msg);
                 wake_up(&req_info[msg.req_id].waitq);
             }
+
+            wmb();
+            notify_remote_via_evtchn(start_info.store_evtchn);
         }
     }
 }
-- 
2.1.4

Re: [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Samuel Thibault]

Wei Liu, le Mon 26 Oct 2015 12:47:56 +0000, a écrit :
> -            if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons < sizeof(msg))
> +            if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons < sizeof(msg)) {
> +                notify_remote_via_evtchn(start_info.store_evtchn);
>                  break;
> +            }

>              if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons <
> -                    sizeof(msg) + msg.len)
> +                    sizeof(msg) + msg.len) {
> +                notify_remote_via_evtchn(start_info.store_evtchn);

Actually thinking...  In principle we should not need these two
notifications: we have already notified last time we consumed a message.
Notifying again shouldn't be bringing anything new.  Do you actually see
a difference with these?

Samuel

Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Wei Liu]

On Mon, Oct 26, 2015 at 01:52:47PM +0100, Samuel Thibault wrote:
> Wei Liu, le Mon 26 Oct 2015 12:47:56 +0000, a écrit :
> > -            if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons < sizeof(msg))
> > +            if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons < sizeof(msg)) {
> > +                notify_remote_via_evtchn(start_info.store_evtchn);
> >                  break;
> > +            }
> 
> >              if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons <
> > -                    sizeof(msg) + msg.len)
> > +                    sizeof(msg) + msg.len) {
> > +                notify_remote_via_evtchn(start_info.store_evtchn);
> 
> Actually thinking...  In principle we should not need these two
> notifications: we have already notified last time we consumed a message.
> Notifying again shouldn't be bringing anything new.  Do you actually see
> a difference with these?
> 

Yes. The ring still gets stalled somehow without those two
notifications.

Wei.

> Samuel
> 
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@lists.xenproject.org
> http://lists.xenproject.org/cgi-bin/mailman/listinfo/minios-devel

Re: [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Ian Jackson]

Wei Liu writes ("[PATCH MINI-OS v2] xenbus: notify the other end when necessary"):
> The xenbus thread didn't send notification to other end when it expected
> more data or consumed responses, which led to stalling the ring from
> time to time.

Thanks for investigating this.  I haven't attempted to prove correct
(or incorrect) the ring handling after this patch.

But:

> This is the culprit that guest was less responsive when using stubdom
> because the device model was stalled.

ISTM with my stable tree maintainer hat on that this is a good
backport candidate.

Ian.

Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Samuel Thibault]

Wei Liu, le Mon 26 Oct 2015 13:55:31 +0000, a écrit :
> On Mon, Oct 26, 2015 at 01:52:47PM +0100, Samuel Thibault wrote:
> > Wei Liu, le Mon 26 Oct 2015 12:47:56 +0000, a écrit :
> > > -            if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons < sizeof(msg))
> > > +            if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons < sizeof(msg)) {
> > > +                notify_remote_via_evtchn(start_info.store_evtchn);
> > >                  break;
> > > +            }
> > 
> > >              if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons <
> > > -                    sizeof(msg) + msg.len)
> > > +                    sizeof(msg) + msg.len) {
> > > +                notify_remote_via_evtchn(start_info.store_evtchn);
> > 
> > Actually thinking...  In principle we should not need these two
> > notifications: we have already notified last time we consumed a message.
> > Notifying again shouldn't be bringing anything new.  Do you actually see
> > a difference with these?
> > 
> 
> Yes. The ring still gets stalled somehow without those two
> notifications.

Ok...  This is still very worrying.  The more I'm thinking about it,
the more I believe we shouldn't *have* to do this.  Which version of
the xenstore are you testing with, exactly?  I guess that while we
investigate this oddness we can apply this fix to stable trees, so as to
be safer than sorry.

Samuel

Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Ian Jackson]

Samuel Thibault writes ("Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary"):
> Ok...  This is still very worrying.  The more I'm thinking about it,
> the more I believe we shouldn't *have* to do this.  Which version of
> the xenstore are you testing with, exactly?  I guess that while we
> investigate this oddness we can apply this fix to stable trees, so as to
> be safer than sorry.

I would rather have only a proper fix for the stable trees, that we
have confidence in.  So I'll wait.

Would one of you let me know if you need me to stare hard at the ring
handling ?

Ian.

Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Samuel Thibault]

Ian Jackson, le Mon 26 Oct 2015 16:32:10 +0000, a écrit :
> Samuel Thibault writes ("Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary"):
> > Ok...  This is still very worrying.  The more I'm thinking about it,
> > the more I believe we shouldn't *have* to do this.  Which version of
> > the xenstore are you testing with, exactly?  I guess that while we
> > investigate this oddness we can apply this fix to stable trees, so as to
> > be safer than sorry.
> 
> I would rather have only a proper fix for the stable trees, that we
> have confidence in.  So I'll wait.

Well, at least the patch can not hurt.

> Would one of you let me know if you need me to stare hard at the ring
> handling ?

Let's check which version we need to stare hard at first :)

Samuel

Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Wei Liu]

On Mon, Oct 26, 2015 at 05:37:51PM +0100, Samuel Thibault wrote:
> Ian Jackson, le Mon 26 Oct 2015 16:32:10 +0000, a écrit :
> > Samuel Thibault writes ("Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary"):
> > > Ok...  This is still very worrying.  The more I'm thinking about it,
> > > the more I believe we shouldn't *have* to do this.  Which version of
> > > the xenstore are you testing with, exactly?  I guess that while we
> > > investigate this oddness we can apply this fix to stable trees, so as to
> > > be safer than sorry.
> > 
> > I would rather have only a proper fix for the stable trees, that we
> > have confidence in.  So I'll wait.
> 
> Well, at least the patch can not hurt.
> 
> > Would one of you let me know if you need me to stare hard at the ring
> > handling ?
> 
> Let's check which version we need to stare hard at first :)
> 

The oxenstored in staging. Sorry I haven't got around to investigate
more -- need to clear some patches in my inbox first.

Wei.

> Samuel

Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Samuel Thibault]

Also, just to make sure: you tested with the third and fourth hooks of
your v2 patch applied, only first and second hooks were removed?

Samuel

Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Wei Liu]

On Mon, Oct 26, 2015 at 05:43:53PM +0100, Samuel Thibault wrote:
> Also, just to make sure: you tested with the third and fourth hooks of
> your v2 patch applied, only first and second hooks were removed?
> 
> Samuel

See the patch below.

>From 0643a821ce2795e7e65e199e7caaa657f27bafcf Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Fri, 23 Oct 2015 20:01:06 +0100
Subject: [PATCH] Test patch

---
 xenbus/xenbus.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
index 4613ed6..0ab387a 100644
--- a/xenbus/xenbus.c
+++ b/xenbus/xenbus.c
@@ -237,6 +237,7 @@ static void xenbus_thread_func(void *ign)
 		event->path = data;
 		event->token = event->path + strlen(event->path) + 1;
 
+                mb();
                 xenstore_buf->rsp_cons += msg.len + sizeof(msg);
 
                 for (watch = watches; watch; watch = watch->next)
@@ -262,9 +263,13 @@ static void xenbus_thread_func(void *ign)
                     req_info[msg.req_id].reply,
                     MASK_XENSTORE_IDX(xenstore_buf->rsp_cons),
                     msg.len + sizeof(msg));
+                mb();
                 xenstore_buf->rsp_cons += msg.len + sizeof(msg);
                 wake_up(&req_info[msg.req_id].waitq);
             }
+
+            wmb();
+            notify_remote_via_evtchn(start_info.store_evtchn);
         }
     }
 }
-- 
2.1.4

Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Samuel Thibault]

Wei Liu, le Mon 26 Oct 2015 16:41:15 +0000, a écrit :
> The oxenstored in staging.

Ok.  For a first, one fishy thing at quick sight is that the only
occurence of rsp_cons (except at closure) is when writing, and not when
going to sleep.  One issue there is that ml_interface_write only writes
a contiguous piece of data, so when crossing the ring bound, it will
return a short write while there *is* room!

One quick test you could do is calling Xs_ring.write a second time in
tools/ocaml/libs/xb/xb.ml's write_mmap, something like below (untested,
and my caml is old, so it's ugly, but you get the idea).

Samuel

diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
index 50944b5..33298d2 100644
--- a/tools/ocaml/libs/xb/xb.ml
+++ b/tools/ocaml/libs/xb/xb.ml
@@ -91,10 +91,12 @@ let write_fd back con s len =
 	Unix.write back.fd s 0 len
 
 let write_mmap back con s len =
-	let ws = Xs_ring.write back.mmap s len in
-	if ws > 0 then
+	let ws = ref (Xs_ring.write back.mmap s len) in
+	if !ws < len then
+		ws := !ws + Xs_ring.write back.mmap (String.sub s !ws (len-!ws)) (len-!ws);
+	if !ws > 0 then
 		back.eventchn_notify ();
-	ws
+	!ws
 
 let write con s len =
 	match con.backend with

Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Wei Liu]

On Mon, Oct 26, 2015 at 06:11:04PM +0100, Samuel Thibault wrote:
> Wei Liu, le Mon 26 Oct 2015 16:41:15 +0000, a écrit :
> > The oxenstored in staging.
> 
> Ok.  For a first, one fishy thing at quick sight is that the only
> occurence of rsp_cons (except at closure) is when writing, and not when
> going to sleep.  One issue there is that ml_interface_write only writes
> a contiguous piece of data, so when crossing the ring bound, it will
> return a short write while there *is* room!
> 
> One quick test you could do is calling Xs_ring.write a second time in
> tools/ocaml/libs/xb/xb.ml's write_mmap, something like below (untested,
> and my caml is old, so it's ugly, but you get the idea).
> 
> Samuel
> 
> diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
> index 50944b5..33298d2 100644
> --- a/tools/ocaml/libs/xb/xb.ml
> +++ b/tools/ocaml/libs/xb/xb.ml
> @@ -91,10 +91,12 @@ let write_fd back con s len =
>  	Unix.write back.fd s 0 len
>  
>  let write_mmap back con s len =
> -	let ws = Xs_ring.write back.mmap s len in
> -	if ws > 0 then
> +	let ws = ref (Xs_ring.write back.mmap s len) in
> +	if !ws < len then
> +		ws := !ws + Xs_ring.write back.mmap (String.sub s !ws (len-!ws)) (len-!ws);
> +	if !ws > 0 then
>  		back.eventchn_notify ();
> -	ws
> +	!ws
>  
>  let write con s len =
>  	match con.backend with

I'm now running a slightly modified version of this diff (to fix
compilation) and a mini-os only notifies the other end when it consumes
message.

The result is looking good, but I will let it run overnight to be sure.

If the backend is to blame, I think we need to accept the fact it is now
the de facto behaviour of xenstore ring and work around it in the
frontend...

Wei.

Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Wei Liu]

On Mon, Oct 26, 2015 at 06:34:59PM +0000, Wei Liu wrote:
> On Mon, Oct 26, 2015 at 06:11:04PM +0100, Samuel Thibault wrote:
> > Wei Liu, le Mon 26 Oct 2015 16:41:15 +0000, a écrit :
> > > The oxenstored in staging.
> > 
> > Ok.  For a first, one fishy thing at quick sight is that the only
> > occurence of rsp_cons (except at closure) is when writing, and not when
> > going to sleep.  One issue there is that ml_interface_write only writes
> > a contiguous piece of data, so when crossing the ring bound, it will
> > return a short write while there *is* room!
> > 
> > One quick test you could do is calling Xs_ring.write a second time in
> > tools/ocaml/libs/xb/xb.ml's write_mmap, something like below (untested,
> > and my caml is old, so it's ugly, but you get the idea).
> > 
> > Samuel
> > 
> > diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
> > index 50944b5..33298d2 100644
> > --- a/tools/ocaml/libs/xb/xb.ml
> > +++ b/tools/ocaml/libs/xb/xb.ml
> > @@ -91,10 +91,12 @@ let write_fd back con s len =
> >  	Unix.write back.fd s 0 len
> >  
> >  let write_mmap back con s len =
> > -	let ws = Xs_ring.write back.mmap s len in
> > -	if ws > 0 then
> > +	let ws = ref (Xs_ring.write back.mmap s len) in
> > +	if !ws < len then
> > +		ws := !ws + Xs_ring.write back.mmap (String.sub s !ws (len-!ws)) (len-!ws);
> > +	if !ws > 0 then
> >  		back.eventchn_notify ();
> > -	ws
> > +	!ws
> >  
> >  let write con s len =
> >  	match con.backend with
> 
> I'm now running a slightly modified version of this diff (to fix
> compilation) and a mini-os only notifies the other end when it consumes
> message.
> 
> The result is looking good, but I will let it run overnight to be sure.
> 

The test ran overnight without issue.

I think it would be sensible to take my v2 patch for mini-os and work on
fixes for xenstored.

I haven't checked if cxenstored behaves the same, though.

Wei.

Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Samuel Thibault]

Wei Liu, le Tue 27 Oct 2015 10:32:18 +0000, a écrit :
> I haven't checked if cxenstored behaves the same, though.

It doesn't: it checks for room in rings before sleeping.

Samuel

Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Wei Liu]

On Tue, Oct 27, 2015 at 11:54:01AM +0100, Samuel Thibault wrote:
> Wei Liu, le Tue 27 Oct 2015 10:32:18 +0000, a écrit :
> > I haven't checked if cxenstored behaves the same, though.
> 
> It doesn't: it checks for room in rings before sleeping.
> 

Thanks for checking!

And I confirm with my test that cxenstored does the right thing.

Wei.

> Samuel

Re: [Minios-devel] [PATCH MINI-OS v2] xenbus: notify the other end when necessary

[Samuel Thibault]

Samuel Thibault, le Mon 26 Oct 2015 18:11:04 +0100, a écrit :
> diff --git a/tools/ocaml/libs/xb/xb.ml b/tools/ocaml/libs/xb/xb.ml
> index 50944b5..33298d2 100644
> --- a/tools/ocaml/libs/xb/xb.ml
> +++ b/tools/ocaml/libs/xb/xb.ml
> @@ -91,10 +91,12 @@ let write_fd back con s len =
>  	Unix.write back.fd s 0 len
>  
>  let write_mmap back con s len =
> -	let ws = Xs_ring.write back.mmap s len in
> -	if ws > 0 then
> +	let ws = ref (Xs_ring.write back.mmap s len) in
> +	if !ws < len then
> +		ws := !ws + Xs_ring.write back.mmap (String.sub s !ws (len-!ws)) (len-!ws);
> +	if !ws > 0 then
>  		back.eventchn_notify ();
> -	ws
> +	!ws
>  
>  let write con s len =
>  	match con.backend with

Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Not-tested-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Not-even-compiled-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Samuel