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

[Samuel Thibault <samuel.thibault@ens-lyon.org>]

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);
     }
 }