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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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.

>      }
>  }
>  

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