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

[PATCH MINI-OS] 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>

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 J, this is a patch suitable for backporting to 4.6. It's good time
to branch mini-os now.
---
 xenbus/xenbus.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
index 4613ed6..7451161 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");
 
@@ -265,6 +269,9 @@ static void xenbus_thread_func(void *ign)
                 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] xenbus: notify the other end when necessary

[Samuel Thibault]

Hello,

Indeed, notification seems to have been missing since basically 2006...

Wei Liu, le Mon 26 Oct 2015 09:47:48 +0000, a écrit :
> diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
> index 4613ed6..7451161 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");
>  
> @@ -265,6 +269,9 @@ static void xenbus_thread_func(void *ign)
>                  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);
>          }
>      }
>  }

The wmb() position seems right, but the notification could be put a bit
later, after the exit of the while(1) loop.  That'd make it factorized
for all cases were the processing could want to stop, and make it quite
naturally enough just before the wait_event call, so something like
below (untested).

Samuel

diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
index 4613ed6..858aa98 100644
--- a/xenbus/xenbus.c
+++ b/xenbus/xenbus.c
@@ -265,7 +265,10 @@ static void xenbus_thread_func(void *ign)
                 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);
     }
 }
 

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

[Wei Liu]

On Mon, Oct 26, 2015 at 01:02:45PM +0100, Samuel Thibault wrote:
> Hello,
> 
> Indeed, notification seems to have been missing since basically 2006...
> 
> Wei Liu, le Mon 26 Oct 2015 09:47:48 +0000, a écrit :
> > diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
> > index 4613ed6..7451161 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");
> >  
> > @@ -265,6 +269,9 @@ static void xenbus_thread_func(void *ign)
> >                  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);
> >          }
> >      }
> >  }
> 
> The wmb() position seems right, but the notification could be put a bit
> later, after the exit of the while(1) loop.  That'd make it factorized
> for all cases were the processing could want to stop, and make it quite
> naturally enough just before the wait_event call, so something like
> below (untested).
> 
> Samuel
> 
> diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
> index 4613ed6..858aa98 100644
> --- a/xenbus/xenbus.c
> +++ b/xenbus/xenbus.c
> @@ -265,7 +265,10 @@ static void xenbus_thread_func(void *ign)
>                  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);

I am sure your patch works too but there is a subtle difference.  In my
patch, mini-os notifies remote whenever it consumes a message, which I
think it's slightly better because backend can start putting things in
the ring as mini-os processes them.

Wei.

>      }
>  }
>  

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

[Samuel Thibault]

Wei Liu, le Mon 26 Oct 2015 12:14:43 +0000, a écrit :
> In my patch, mini-os notifies remote whenever it consumes a message,
> which I think it's slightly better because backend can start putting
> things in the ring as mini-os processes them.

That makes more notifications, but that can lead to more pipelining
indeed.  That's what the Linux driver does, so let's do the same.

Also, I'm realizing: aren't we missing a full memory barrier between
the memcpy_from_ring call and xenstore_buf->rsp_cons += ? (in the two
places) We need to make sure to have finished copying from the ring
before writing the new rsp_cons.

Samuel

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

[Wei Liu]

On Mon, Oct 26, 2015 at 01:21:51PM +0100, Samuel Thibault wrote:
> Wei Liu, le Mon 26 Oct 2015 12:14:43 +0000, a écrit :
> > In my patch, mini-os notifies remote whenever it consumes a message,
> > which I think it's slightly better because backend can start putting
> > things in the ring as mini-os processes them.
> 
> That makes more notifications, but that can lead to more pipelining
> indeed.  That's what the Linux driver does, so let's do the same.
> 
> Also, I'm realizing: aren't we missing a full memory barrier between
> the memcpy_from_ring call and xenstore_buf->rsp_cons += ? (in the two
> places) We need to make sure to have finished copying from the ring
> before writing the new rsp_cons.
> 

You're right.

I think we should just turn that wmb() into two mb()s and place them
before xenstore_buf->rsp_cons +=.

Wei.

> Samuel

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

[Samuel Thibault]

Wei Liu, le Mon 26 Oct 2015 12:30:28 +0000, a écrit :
> On Mon, Oct 26, 2015 at 01:21:51PM +0100, Samuel Thibault wrote:
> > Wei Liu, le Mon 26 Oct 2015 12:14:43 +0000, a écrit :
> > > In my patch, mini-os notifies remote whenever it consumes a message,
> > > which I think it's slightly better because backend can start putting
> > > things in the ring as mini-os processes them.
> > 
> > That makes more notifications, but that can lead to more pipelining
> > indeed.  That's what the Linux driver does, so let's do the same.
> > 
> > Also, I'm realizing: aren't we missing a full memory barrier between
> > the memcpy_from_ring call and xenstore_buf->rsp_cons += ? (in the two
> > places) We need to make sure to have finished copying from the ring
> > before writing the new rsp_cons.
> > 
> 
> You're right.
> 
> I think we should just turn that wmb() into two mb()s and place them
> before xenstore_buf->rsp_cons +=.

We *also* need some barrier between rsp_cons += and the notification,
otherwise the notified domain may miss the rsp_cons update and thus
believe it was a spurious notification.

Samuel

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

[Wei Liu]

On Mon, Oct 26, 2015 at 01:33:12PM +0100, Samuel Thibault wrote:
> Wei Liu, le Mon 26 Oct 2015 12:30:28 +0000, a écrit :
> > On Mon, Oct 26, 2015 at 01:21:51PM +0100, Samuel Thibault wrote:
> > > Wei Liu, le Mon 26 Oct 2015 12:14:43 +0000, a écrit :
> > > > In my patch, mini-os notifies remote whenever it consumes a message,
> > > > which I think it's slightly better because backend can start putting
> > > > things in the ring as mini-os processes them.
> > > 
> > > That makes more notifications, but that can lead to more pipelining
> > > indeed.  That's what the Linux driver does, so let's do the same.
> > > 
> > > Also, I'm realizing: aren't we missing a full memory barrier between
> > > the memcpy_from_ring call and xenstore_buf->rsp_cons += ? (in the two
> > > places) We need to make sure to have finished copying from the ring
> > > before writing the new rsp_cons.
> > > 
> > 
> > You're right.
> > 
> > I think we should just turn that wmb() into two mb()s and place them
> > before xenstore_buf->rsp_cons +=.
> 
> We *also* need some barrier between rsp_cons += and the notification,
> otherwise the notified domain may miss the rsp_cons update and thus
> believe it was a spurious notification.
> 

Actually notify_remote_via_evtchn normally implies a mb(). But I think
I'd stay on the safe side. I will have a wmb() there. :-)

Thanks for your review. V2 coming soon.

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] xenbus: notify the other end when necessary

[Ian Campbell]

On Mon, 2015-10-26 at 09:47 +0000, Wei Liu wrote:
> 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.

Could any of this code benefit from using the various macros in ring.h
which produce or consume entries on the ring and return a _notify bit?

> 
> 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>
> 
> 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 J, this is a patch suitable for backporting to 4.6. It's good time
> to branch mini-os now.
> ---
>  xenbus/xenbus.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c
> index 4613ed6..7451161 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");
>  
> @@ -265,6 +269,9 @@ static void xenbus_thread_func(void *ign)
>                  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);
>          }
>      }
>  }

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

[Wei Liu]

On Mon, Nov 02, 2015 at 12:14:50PM +0000, Ian Campbell wrote:
> On Mon, 2015-10-26 at 09:47 +0000, Wei Liu wrote:
> > 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.
> 
> Could any of this code benefit from using the various macros in ring.h
> which produce or consume entries on the ring and return a _notify bit?
> 

The bug is it doesn't send notification at all. The existing code in
Linux doesn't seem to care about mitigating notifications -- xenbus is
supposed to exchange small amount of information anyway so whether a few
more notifications are sent is not essential. I think it would be better
if we follow something that works at this stage.

The use of ring macro can be considered an improvement but not
essential to fix the bug. It can be considered as improvement in the
future.

Wei.

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

[Ian Campbell]

On Mon, 2015-11-02 at 15:00 +0000, Wei Liu wrote:
> On Mon, Nov 02, 2015 at 12:14:50PM +0000, Ian Campbell wrote:
> > On Mon, 2015-10-26 at 09:47 +0000, Wei Liu wrote:
> > > 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.
> > 
> > Could any of this code benefit from using the various macros in ring.h
> > which produce or consume entries on the ring and return a _notify bit?
> > 
> 
> The bug is it doesn't send notification at all. The existing code in
> Linux doesn't seem to care about mitigating notifications -- xenbus is
> supposed to exchange small amount of information anyway so whether a few
> more notifications are sent is not essential. I think it would be better
> if we follow something that works at this stage.
> 
> The use of ring macro can be considered an improvement but not
> essential to fix the bug. It can be considered as improvement in the
> future.

My reason for suggesting it was more that we trust that those macros are
already correct, compared with needing to reason from scratch about the
open coded version used in this code which is what appeared to be going on.

Ian.

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

[Wei Liu]

On Mon, Nov 02, 2015 at 03:08:26PM +0000, Ian Campbell wrote:
> On Mon, 2015-11-02 at 15:00 +0000, Wei Liu wrote:
> > On Mon, Nov 02, 2015 at 12:14:50PM +0000, Ian Campbell wrote:
> > > On Mon, 2015-10-26 at 09:47 +0000, Wei Liu wrote:
> > > > 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.
> > > 
> > > Could any of this code benefit from using the various macros in ring.h
> > > which produce or consume entries on the ring and return a _notify bit?
> > > 
> > 
> > The bug is it doesn't send notification at all. The existing code in
> > Linux doesn't seem to care about mitigating notifications -- xenbus is
> > supposed to exchange small amount of information anyway so whether a few
> > more notifications are sent is not essential. I think it would be better
> > if we follow something that works at this stage.
> > 
> > The use of ring macro can be considered an improvement but not
> > essential to fix the bug. It can be considered as improvement in the
> > future.
> 
> My reason for suggesting it was more that we trust that those macros are
> already correct, compared with needing to reason from scratch about the
> open coded version used in this code which is what appeared to be going on.
> 

I don't think macros help here. Xenstore initialisation doesn't follow
conventional frontend and backend drivers, so the macros don't really
apply to it -- not without some refactoring.

Wei.

> Ian.

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

[Ian Campbell]

On Mon, 2015-11-02 at 15:53 +0000, Wei Liu wrote:
> On Mon, Nov 02, 2015 at 03:08:26PM +0000, Ian Campbell wrote:
> > On Mon, 2015-11-02 at 15:00 +0000, Wei Liu wrote:
> > > On Mon, Nov 02, 2015 at 12:14:50PM +0000, Ian Campbell wrote:
> > > > On Mon, 2015-10-26 at 09:47 +0000, Wei Liu wrote:
> > > > > 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.
> > > > 
> > > > Could any of this code benefit from using the various macros in
> > > > ring.h
> > > > which produce or consume entries on the ring and return a _notify
> > > > bit?
> > > > 
> > > 
> > > The bug is it doesn't send notification at all. The existing code in
> > > Linux doesn't seem to care about mitigating notifications -- xenbus
> > > is
> > > supposed to exchange small amount of information anyway so whether a
> > > few
> > > more notifications are sent is not essential. I think it would be
> > > better
> > > if we follow something that works at this stage.
> > > 
> > > The use of ring macro can be considered an improvement but not
> > > essential to fix the bug. It can be considered as improvement in the
> > > future.
> > 
> > My reason for suggesting it was more that we trust that those macros
> > are
> > already correct, compared with needing to reason from scratch about the
> > open coded version used in this code which is what appeared to be going
> > on.
> > 
> 
> I don't think macros help here. Xenstore initialisation doesn't follow
> conventional frontend and backend drivers, so the macros don't really
> apply to it -- not without some refactoring.

AH, ok then.