On Tue, Jun 23, 2015 at 03:58:32PM +0100, Ian Campbell wrote: > There is an issue in libxl_set_memory_target whereby the target and > the max mem can get out of sync, this is because the call the > xc_domain_setmaxmem is not tied in any way to the xenstore transaction > which controls updates to the xenstore side of things. > > Consider a domain with 1M of RAM (==target and maxmem for the sake of > argument) and two simultaneous calls to libxl_set_memory_target, both > with relative=0 and enforce=1, one with target=3 and the other with > target=5. > > target=5 call target=3 call > > transaction start > transaction start > write target=5 to xenstore > write target=3 to xenstore > setmaxmem(5) > setmaxmem(3) > > transaction commit = success > transaction commit = EAGAIN > > At this point maxmem=3 while target=5. > > In reality the target=3 case will the retry and eventually (hopefully) > succeed with target=maxmem=3, however the bad state will persist for > some window which is undesirable. On failure other than EAGAIN all > bets are off anyway, but in that case we will likely stick in the bad > state until someone else sets the memory). > > To fix this we slightly abuse the userdata lock which is used to > protect updates to the domain's json configuration. Abused because > maxmem is not actually stored in there, but is kept by Xen. However > the lock protects some semantically similar things and is convenient > to use here too. > > libxl_domain_setmaxmem also takes the lock, since it reads > memory/target from xenstore before calling xc_domain_setmaxmem there > is a small (but perhaps not very interesting) race there too. > > There is on more use of xc_domain_setmaxmem in libxl__build_pre. > However taking a lock around this would be tricky since the xenstore > parts are not done until libxl__build_post. I think this one could be > argued to be OK since the domid is not "public" yet, that is it has > not been returned to the application yet (as the result of the create > operation). Toolstacks which go round fiddling with random domid's > which they find lying on the floor should be taught to do better. > > Add a doc note that taking the userdata lock requires the CTX_LOCK to > be held. > > Signed-off-by: Ian Campbell <ian.campb...@citrix.com> > --- > This applies on top of Wei's revert of "libxl_set_memory_target: > retain the same maxmem offset on top of the current target" > > I couldn't quite rule out some race (for transaction=EAGAIN, for > !EAGAIN there are obvious ones) which resulted in the incorrect state > being in place after both entities exit, but couldn't construct an > actual case.
Not sure I follow. With this patch you lock out other contenders to even start a transaction so the EAGAIN vs !EAGAIN should be moot. You can safely rollback in !EAGAIN case, right? Wei. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel