|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/3] x86/mm-locks: apply a bias to lock levels for current domain
On 12/19/18 2:30 PM, Jan Beulich wrote: >>>> On 19.12.18 at 13:38, <george.dunlap@xxxxxxxxxx> wrote: >> On 12/19/18 12:10 PM, Roger Pau Monné wrote: >>> On Wed, Dec 19, 2018 at 11:40:14AM +0000, George Dunlap wrote: >>>> On 12/18/18 4:05 PM, Roger Pau Monne wrote: >>>>> paging_log_dirty_op function takes mm locks from a subject domain and >>>>> then attempts to perform copy to operations against the caller >>>>> domain in order to copy the result of the hypercall into the caller >>>>> provided buffer. >>>>> >>>>> This works fine when the caller is a non-paging domain, but triggers a >>>>> lock order panic when the caller is a paging domain due to the fact >>>>> that at the point where the copy to operation is performed the subject >>>>> domain paging lock is locked, and the copy operation requires locking >>>>> the caller p2m lock which has a lower level. >>>>> >>>>> Fix this limitation by adding a bias to the level of the caller domain >>>>> mm locks, so that the lower caller domain mm lock always has a level >>>>> greater than the higher subject domain lock level. This allows locking >>>>> the subject domain mm locks and then locking the caller domain mm >>>>> locks, while keeping the same lock ordering and the changes mostly >>>>> confined to mm-locks.h. >>>>> >>>>> Note that so far only this flow (locking a subject domain locks and >>>>> then the caller domain ones) has been identified, but not all possible >>>>> code paths have been inspected. Hence this solution attempts to be a >>>>> non-intrusive fix for the problem at hand, without discarding further >>>>> changes in the future if other valid code paths are found that require >>>>> more complex lock level ordering. >>>>> >>>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx> >>>> >>>> As a quick fix I think this general approach is OK; the thing I don't >>>> like is that it's symmetric. We don't *expect* to ever have a situation >>>> where A grabs one of its own MM locks and then one of B's, *and* B then >>>> grabs one of its own locks and then A's; but it could happen. >>> >>> I have not identified such scenario ATM, but we cannot discard future >>> features needing such interlocking I guess. In any case, I think this >>> is something that would have to be solved when we came across such >>> scenario IMO. >> >> Right -- and the purpose of these macros is to make sure that we >> discover such potential deadlocks in testing rather than in production. >> >>>> Since we've generally identified dom0 which may be grabbing locks of a >>>> PVH stubdom, which may be grabbing logs of a normal domU, would it be >>>> possible / make sense instead to give a 2x bonus for dom0, and a 1x >>>> bonus for "is_priv_for" domains? >>> >>> Jan pointed out such case, but I'm not sure I can see how this is >>> supposedly to happen even given the scenario above, I have to admit >>> however I'm not that familiar with the mm code, so it's likely I'm >>> missing something. >>> >>> Hypercalls AFAIK have a single target (or subject) domain, so even if >>> there's a stubdomain relation I'm not sure I see why that would >>> require this kind of locking, any domain can perform hypercalls >>> against a single subject domain, and the hypervisor itself doesn't >>> even know about stubdomain relations. >> >> We're considering three potential cases: >> >> A. dom0 makes a hypercall w/ domU as a target. >> B. dom0 makes a hypercall w/ stubdom as a target. >> c. stubdom makes a hypercall w/ domU as a target. >> >> We could give only dom0 a bonus. In that case, A and B would work, but >> C might fail (since stubdom's lock values are the same as domU's). >> >> We could give both dom0 and stubdoms a bonus. In that case, A and C >> would work, but B might fail (since the stubdom's lock values are the >> same as dom0's). >> >> Or, we could do what I've proposed: give stubdom a bonus, and dom0 a >> double bonus. That way all 3 work, since dom0's lock values != >> stubdom's lock values, and stubdom's lock values != domU's lock values. >> >> On the other hand, starting simple and adding things in as you find you >> need them isn't a bad approach; so possibly just giving a bonus to dom0 >> is a good place to start. > > I'm not sure your reply is intentionally not matching the way Roger > has currently implemented it: There's no Dom0 bonus right now, but > instead a "current domain" one (covering Dom0 or stubdom, whenever > they act on their target domain(s)). It is intentional; my first response to Roger's patch said that I didn't like the "current domain" bonus, and suggested a fixed "dom0" and "stubdomain" bonus instead. Roger said he didn't understand the use case, so this is me explaining the situation and how my suggestion addresses it. > It was for this reason that I did > suggest (but you better express by your A/B/C scheme) the two > layers of what you call bonus - it is more obviously correct that way, > despite there not being any scenario that we can think of right now > where all 3 layers would actually get involved. OK, glad you like it. :-) "Bonus" is probably not the right word. -George _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |