On 23/04/15 17:11, Jan Beulich wrote: >>>> On 22.04.15 at 18:00, <david.vra...@citrix.com> wrote: >> >> + v->maptrack_head = lgt->maptrack_pages * MAPTRACK_PER_PAGE; >> + if (v->maptrack_tail == MAPTRACK_TAIL) >> + { >> + v->maptrack_tail = (lgt->maptrack_pages * MAPTRACK_PER_PAGE) >> + + MAPTRACK_PER_PAGE - 1; >> + new_mt[i - 1].ref = MAPTRACK_TAIL; >> } >> >> + spin_lock(&lgt->maptrack_lock); >> + lgt->maptrack[lgt->maptrack_pages++] = new_mt; >> spin_unlock(&lgt->maptrack_lock); > > The uses of ->maptrack_pages ahead of taking the lock can race > with updates inside the lock. And with locking elsewhere dropped > by the previous patch it looks like you can't update ->maptrack[] > like you do (you'd need a barrier between the pointer store and > the increment, and with that I think the lock would become > superfluous if it was needed only for this update).
This was my mistake. Malcolm's original patch had correct locking here. > Also note the coding style issues in the changes above^(and also > in code further down). > >> - return handle; >> + v->maptrack_limit = new_mt_limit; >> + >> + return __get_maptrack_handle(lgt, v); > > With the lock dropped, nothing guarantees this to succeed, which it > ought to unless the table size reached its allowed maximum. We've just added a new page of handles for this VCPU. This will succeed. David _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel