[PATCH MINI-OS v3 0/2] Two mini-os xenbus patches

[PATCH MINI-OS v3 0/2] Two mini-os xenbus patches

[Wei Liu]

I broke down my previous patch to two patches. Both of them should be applied
to mini-os staging. The first patch should be applied to the (to be created)
stable-4.6 mini-os branch, along with a proper fix to oxenstored.

Wei Liu (2):
  xenbus: notify the other end when necessary
  xenbus: workaround oxenstored short-write

 xenbus/xenbus.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

-- 
2.1.4

[PATCH MINI-OS v3 1/2] 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 when it consumes a
message. A bunch of memory barriers are also added to ensure
correctness.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 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: [PATCH MINI-OS v3 1/2] xenbus: notify the other end when necessary

[Samuel Thibault]

Wei Liu, le Tue 27 Oct 2015 15:43:28 +0000, a écrit :
> 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 when it consumes a
> message. A bunch of memory barriers are also added to ensure
> correctness.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

And it should clearly be backported to stable branches.

Samuel

Re: [PATCH MINI-OS v3 1/2] xenbus: notify the other end when necessary

[Wei Liu]

On Tue, Oct 27, 2015 at 04:46:43PM +0100, Samuel Thibault wrote:
> Wei Liu, le Tue 27 Oct 2015 15:43:28 +0000, a écrit :
> > 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 when it consumes a
> > message. A bunch of memory barriers are also added to ensure
> > correctness.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 

Thanks for your help along the way!

> And it should clearly be backported to stable branches.
> 
> Samuel

Re: [PATCH MINI-OS v3 1/2] xenbus: notify the other end when necessary

[Ian Campbell]

On Tue, 2015-10-27 at 16:46 +0100, Samuel Thibault wrote:
> Wei Liu, le Tue 27 Oct 2015 15:43:28 +0000, a écrit :
> > 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 when it consumes a
> > message. A bunch of memory barriers are also added to ensure
> > correctness.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

Applied.

> And it should clearly be backported to stable branches.

I don't think we have a mini-os branch for stable, but I'll let Ian J
figure out the right way to achieve this.

Ian


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH MINI-OS v3 2/2] xenbus: workaround oxenstored short-write

[Wei Liu]

Oxenstored has a behaviour that it only writes a contiguous piece of
data. When it writes across ring boundary it will return a short-write
while there is still room. That leads to mini-os stalling when it sees
there is not enough data in the ring.

Given that oxenstored is the default xenstored implementation we think
it would be useful to workaround this for the benefit of running mini-os
(and unikernel based on it) on any Xen installation.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xenbus/xenbus.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
index 0ab387a..abf8b1b 100644
--- a/xenbus/xenbus.c
+++ b/xenbus/xenbus.c
@@ -205,8 +205,11 @@ 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)) {
+                /* Work around oxenstored bug */
+                notify_remote_via_evtchn(start_info.store_evtchn);
                 break;
+            }
             rmb();
             memcpy_from_ring(xenstore_buf->rsp,
                     &msg,
@@ -217,8 +220,11 @@ 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) {
+                /* Work around oxenstored bug */
+                notify_remote_via_evtchn(start_info.store_evtchn);
                 break;
+            }
 
             DEBUG("Message is good.\n");
 
-- 
2.1.4

Re: [PATCH MINI-OS v3 2/2] xenbus: workaround oxenstored short-write

[Samuel Thibault]

Wei Liu, le Tue 27 Oct 2015 15:43:29 +0000, a écrit :
> Oxenstored has a behaviour that it only writes a contiguous piece of
> data. When it writes across ring boundary it will return a short-write
> while there is still room. That leads to mini-os stalling when it sees
> there is not enough data in the ring.
> 
> Given that oxenstored is the default xenstored implementation we think
> it would be useful to workaround this for the benefit of running mini-os
> (and unikernel based on it) on any Xen installation.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

As discussed in the previous thread, this is rather a workaround. Better
be safe than sorry.

Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>

And could too be backported to stable branches.

> ---
>  xenbus/xenbus.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
> index 0ab387a..abf8b1b 100644
> --- a/xenbus/xenbus.c
> +++ b/xenbus/xenbus.c
> @@ -205,8 +205,11 @@ 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)) {
> +                /* Work around oxenstored bug */
> +                notify_remote_via_evtchn(start_info.store_evtchn);
>                  break;
> +            }
>              rmb();
>              memcpy_from_ring(xenstore_buf->rsp,
>                      &msg,
> @@ -217,8 +220,11 @@ 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) {
> +                /* Work around oxenstored bug */
> +                notify_remote_via_evtchn(start_info.store_evtchn);
>                  break;
> +            }
>  
>              DEBUG("Message is good.\n");
>  
> -- 
> 2.1.4
> 

-- 
Samuel
"c'est pas nous qui sommes à la rue, c'est la rue qui est à nous"

Re: [PATCH MINI-OS v3 2/2] xenbus: workaround oxenstored short-write

[Ian Campbell]

On Tue, 2015-10-27 at 16:47 +0100, Samuel Thibault wrote:
> Wei Liu, le Tue 27 Oct 2015 15:43:29 +0000, a écrit :
> > Oxenstored has a behaviour that it only writes a contiguous piece of
> > data. When it writes across ring boundary it will return a short-write
> > while there is still room. That leads to mini-os stalling when it sees
> > there is not enough data in the ring.
> > 
> > Given that oxenstored is the default xenstored implementation we think
> > it would be useful to workaround this for the benefit of running mini-
> > os
> > (and unikernel based on it) on any Xen installation.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> As discussed in the previous thread, this is rather a workaround.

Given that I'm in the process of applying "tools/ocaml/xb: Correct
calculations of data/space the ring" which I think is the real fix here am
unsure if we still want to take this workaround.

Any thoughts?


>  Better
> be safe than sorry.
> 
> Acked-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> 
> And could too be backported to stable branches.
> 
> > ---
> >  xenbus/xenbus.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
> > index 0ab387a..abf8b1b 100644
> > --- a/xenbus/xenbus.c
> > +++ b/xenbus/xenbus.c
> > @@ -205,8 +205,11 @@ 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)) {
> > +                /* Work around oxenstored bug */
> > +                notify_remote_via_evtchn(start_info.store_evtchn);
> >                  break;
> > +            }
> >              rmb();
> >              memcpy_from_ring(xenstore_buf->rsp,
> >                      &msg,
> > @@ -217,8 +220,11 @@ 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) {
> > +                /* Work around oxenstored bug */
> > +                notify_remote_via_evtchn(start_info.store_evtchn);
> >                  break;
> > +            }
> >  
> >              DEBUG("Message is good.\n");
> >  
> > -- 
> > 2.1.4
> > 
> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH MINI-OS v3 2/2] xenbus: workaround oxenstored short-write

[Wei Liu]

On Mon, Nov 16, 2015 at 11:30:14AM +0000, Ian Campbell wrote:
> On Tue, 2015-10-27 at 16:47 +0100, Samuel Thibault wrote:
> > Wei Liu, le Tue 27 Oct 2015 15:43:29 +0000, a écrit :
> > > Oxenstored has a behaviour that it only writes a contiguous piece of
> > > data. When it writes across ring boundary it will return a short-write
> > > while there is still room. That leads to mini-os stalling when it sees
> > > there is not enough data in the ring.
> > > 
> > > Given that oxenstored is the default xenstored implementation we think
> > > it would be useful to workaround this for the benefit of running mini-
> > > os
> > > (and unikernel based on it) on any Xen installation.
> > > 
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > As discussed in the previous thread, this is rather a workaround.
> 
> Given that I'm in the process of applying "tools/ocaml/xb: Correct
> calculations of data/space the ring" which I think is the real fix here am
> unsure if we still want to take this workaround.
> 
> Any thoughts?
> 

We should apply it for master but not 4.6 branch. Because other people
will grab mini-os and run it on older version of oxenstored.

Wei.