[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxxxxxxxxxxxxx
> http://lists.xenproject.org/cgi-bin/mailman/listinfo/minios-devel

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/minios-devel


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.