From mboxrd@z Thu Jan  1 00:00:00 1970
From: Samuel Thibault <samuel.thibault@ens-lyon.org>
Subject: Re: [PATCH MINI-OS] xenbus: notify the other end when
	necessary
Date: Mon, 26 Oct 2015 13:02:45 +0100
Message-ID: <20151026120245.GA19459@var.bordeaux.inria.fr>
References: <1445852868-16532-1-git-send-email-wei.liu2@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Return-path: <xen-devel-bounces@lists.xen.org>
Content-Disposition: inline
In-Reply-To: <1445852868-16532-1-git-send-email-wei.liu2@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Wei Liu <wei.liu2@citrix.com>
Cc: minios-devel@lists.xenproject.org, Xen-devel <xen-devel@lists.xenproject.org>, Ian Jackson <ian.jackson@eu.citrix.com>, Ian Campbell <ian.campbell@citrix.com>
List-Id: xen-devel@lists.xenproject.org

Hello,

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

Wei Liu, le Mon 26 Oct 2015 09:47:48 +0000, a =E9crit :
> 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 =3D 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 +=3D 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 +=3D msg.len + sizeof(msg);
                 wake_up(&req_info[msg.req_id].waitq);
             }
+
+            wmb();
         }
+        notify_remote_via_evtchn(start_info.store_evtchn);
     }
 }
 =