[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


  • To: Jan Beulich <JBeulich@xxxxxxxx>
  • From: George Dunlap <george.dunlap@xxxxxxxxxx>
  • Date: Wed, 19 Dec 2018 14:37:15 +0000
  • Autocrypt: addr=george.dunlap@xxxxxxxxxx; prefer-encrypt=mutual; keydata= mQINBFPqG+MBEACwPYTQpHepyshcufo0dVmqxDo917iWPslB8lauFxVf4WZtGvQSsKStHJSj 92Qkxp4CH2DwudI8qpVbnWCXsZxodDWac9c3PordLwz5/XL41LevEoM3NWRm5TNgJ3ckPA+J K5OfSK04QtmwSHFP3G/SXDJpGs+oDJgASta2AOl9vPV+t3xG6xyfa2NMGn9wmEvvVMD44Z7R W3RhZPn/NEZ5gaJhIUMgTChGwwWDOX0YPY19vcy5fT4bTIxvoZsLOkLSGoZb/jHIzkAAznug Q7PPeZJ1kXpbW9EHHaUHiCD9C87dMyty0N3TmWfp0VvBCaw32yFtM9jUgB7UVneoZUMUKeHA fgIXhJ7I7JFmw3J0PjGLxCLHf2Q5JOD8jeEXpdxugqF7B/fWYYmyIgwKutiGZeoPhl9c/7RE Bf6f9Qv4AtQoJwtLw6+5pDXsTD5q/GwhPjt7ohF7aQZTMMHhZuS52/izKhDzIufl6uiqUBge 0lqG+/ViLKwCkxHDREuSUTtfjRc9/AoAt2V2HOfgKORSCjFC1eI0+8UMxlfdq2z1AAchinU0 eSkRpX2An3CPEjgGFmu2Je4a/R/Kd6nGU8AFaE8ta0oq5BSFDRYdcKchw4TSxetkG6iUtqOO ZFS7VAdF00eqFJNQpi6IUQryhnrOByw+zSobqlOPUO7XC5fjnwARAQABtCRHZW9yZ2UgVy4g RHVubGFwIDxkdW5sYXBnQHVtaWNoLmVkdT6JAkAEEwEKACoCGwMFCwkIBwMFFQoJCAsFFgID AQACHgECF4ACGQEFAlpk2IEFCQo9I54ACgkQpjY8MQWQtG1A1BAAnc0oX3+M/jyv4j/ESJTO U2JhuWUWV6NFuzU10pUmMqpgQtiVEVU2QbCvTcZS1U/S6bqAUoiWQreDMSSgGH3a3BmRNi8n HKtarJqyK81aERM2HrjYkC1ZlRYG+jS8oWzzQrCQiTwn3eFLJrHjqowTbwahoiMw/nJ+OrZO /VXLfNeaxA5GF6emwgbpshwaUtESQ/MC5hFAFmUBZKAxp9CXG2ZhTP6ROV4fwhpnHaz8z+BT NQz8YwA4gkmFJbDUA9I0Cm9D/EZscrCGMeaVvcyldbMhWS+aH8nbqv6brhgbJEQS22eKCZDD J/ng5ea25QnS0fqu3bMrH39tDqeh7rVnt8Yu/YgOwc3XmgzmAhIDyzSinYEWJ1FkOVpIbGl9 uR6seRsfJmUK84KCScjkBhMKTOixWgNEQ/zTcLUsfTh6KQdLTn083Q5aFxWOIal2hiy9UyqR VQydowXy4Xx58rqvZjuYzdGDdAUlZ+D2O3Jp28ez5SikA/ZaaoGI9S1VWvQsQdzNfD2D+xfL qfd9yv7gko9eTJzv5zFr2MedtRb/nCrMTnvLkwNX4abB5+19JGneeRU4jy7yDYAhUXcI/waS /hHioT9MOjMh+DoLCgeZJYaOcgQdORY/IclLiLq4yFnG+4Ocft8igp79dbYYHkAkmC9te/2x Kq9nEd0Hg288EO+5AQ0EVFpnOgEIAM6XPDYOTqW64Yma5+vV6947NvKfm+GvtATrwuPDX6za L2cOHhXiiM5iP7ehJCZEqgSMaG1kaQZMBsHhDbKp3dKooJrA8ODeyfV8dIfQEQ6olsV+I6+7 vcWriPgkSdawTTt1Vd9EHQAsEOC6oUf1gPiI3YcjB8I9xCRhOtTXT/4dM32i2AG7xIOO/0z0 4RbJuJvEXem1+0ZK6zoAWy/wDp2DjBIr8n2WSl9b74hHpgLy33ZNpWbe1Zul/32ym1fLT1Lm RC8zXnSb00wUt/5dRVc/TlHCw3loRhHZcalx9LGFoRPfj10wH8+ScSh/izHrcBDPA27jqAyK ZiBmSq2ftn0AEQEAAYkDRAQYAQoADwIbAgUCWmTW+QUJB+ujPwEpwF0gBBkBCgAGBQJUWmc6 AAoJELIVx6fHhBvtxesIALSpB4RaYtr2gQA9r7lTrC8bW3+aLbaBk3q7NBcfV9og6gN6Gvs8 8RITq25H+8gJNOdpKt3hQM816o6pUXTth7FYPUsNxAbo+dGoLkMhfVEYTcFpJoyXakUk/zL5 yF7CzXXI/wYMFvFoixNwdkjWJUgL1cuGh56BaLzi9hzwXjOIANV+jBuZu9xXDXWATy2YAsLB N4F5lW15eOHQ4QsfCtzX/iPjK8Q2MhdE75AsiCTjeQHntSmvi0/YwRyzSh2A8z5D6gRM4nTT HMuCROcs+KYLUUhbZs5l1OP5Srp7NFLYsqw2Zb49FG83IDmiMRsD99rGYCMxm0t1JJJ4UrzL hKgJEKY2PDEFkLRtji8P/RTPQdWZmdN29QhJ92ws/IuYmEOrwlAmvQGZWxADe+9VIoQeQaSA e/i8yuC9nbPJhl5DyrbmOv9A3EnAXvxyt1c1jpznWg3m0xuB214G7iN5l5g71tOajy9ZhId8 HKRwnmefRcT153tE0Kfw1ILgpslhUasrGuuICsMUAeNPCgdT3siIXDTD5kY/M0m7sHYdM+Ik DzK4vYhB89lZY4k87SrNEAs2YRu8nub27iRB+mb+qjSRWCVlQ1OWQ8gq2BmSoNch1zF3ukB0 KHIclPZ9EI8JpQ6qVbP6RkNPf7AdtIZrI+5eIjsVNvqhCXfaXxfB4fwHmMcbMT5f3s6CFH3M TVm/j7CpXCt8PQOZIWlDrdRhW9ywFPcKWwfUI37WAbHxJI4tzZAUytHi0TlpcQpPHXbbw10s ME4mbMuOlW/Rt01sc2d5SuZkG2/rw7E4TBq6VA3ZbSztvA6ZW6IZX/oX9dFyhw28gHG7+yRw WSNLkCgnO2rXhPJTNfOAn4bdBcQ8Adb9QbWdtqt0xpe6/NjAWGJMBmvXMiiDAKcyS3o8EXK2 CKtRdNjWisu3q/6KPQup7UxP1fMQ0dN9qGz6Cuw1tBKaTDRLS80c8i0WEHcHDSkEIx63sny1 GhyT0XIEmJfhdw99RvEh5S3CkxYnUpHay6KaHJgNKL5L2+oxzpIWA1S6uQENBFRaur0BCADt onSLWlBKZRHpldkPZgQPGJrYHJHS5mhNLs3Q1i/U6NTy/qnTXu7QVyjn5CiO799n3tJweGnn EZUCTmTFkEUNPii8l3Sch5KvdttbB83MbHXBrO193Ne3qfcwEqvsCGKgHWb6+6TfWt51R2eF u283s7jQwL5+BKTn/6NEbFjcg5U+ihArNQ7sznUag6DjCX2JrcfYTM6gaE3a+lNtPyoJwv3Z llnCQFGV2gBaftzWEQpJO5Pd/VWlKaGOdfQni68pnVXZHuuigolgUFzJILTBrxpOYC0C8uB9 yl76V6A62CoMrMu43jnHMSPKMKIjnbW3zPE0w8lj0WII82/SwKQPABEBAAGJAiUEGAEKAA8C GwwFAlpk1zMFCQfrT/YACgkQpjY8MQWQtG2/tg//YY59ZOVnER5btfVhrh+qtCoJtS0U+z55 0s/dOIoBzRJTAeWu8EY8OZHTcFN7EZtp55h3jiR/JGI9h59UIF+UqkLMrFkx1jhLHhnqF8nc fc2WZLd6ECTPvTVdVYytGzl8KoYkMhFFs+f/ZeOuxUv5OBSeQhzUbpr4S2tJdhxBLuacauOt x0GRw7eGBP/WO+Hlzp2AgeJ62MUA/xklxGb1q8hFq3g6Ghas6tUyrcx4RYEBu8hVBHqcS0VF LWLBKU+kZLNpeCwqht4VQ9FERSIk8rsScd1Qtk2uCx94cULYmiKbl6qtg+M+t4erwsdsMX2X P1kRxm6+DQJQfNZd+UP1B8jKHFbmC49JZRdK8FOAI4imealjUhHbxKS+N3072WMUIQwo0Eym 29/KJruT+JDn9R0+7PpJkCkbYiwZah8ytew+Cv9fNAA8O2t4J5q+UbpnGT9zRkkmQOoz+bza kKTbuIKqzxVjUCkHFvBwYmBYKukqC0EFm0cSQx700WCdprO6AnvO9IIeA9cBRaky3sl4lao3 XRDRjWj/GZQg8OhFPNjfAZ+S1yo0dRlqNlCtwo65B6U7d2GGb64UtjDthGBHFo8ruiwCxf5U us+iynkGfrfQHUFHCC5a8fSMal7+hrwKASyWNY4xgavv5ET61l6aGkJ+xV1hnzKlPjZGPXp8 q5c=
  • Cc: Wei Liu <wei.liu2@xxxxxxxxxx>, George Dunlap <George.Dunlap@xxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Tim Deegan <tim@xxxxxxx>, xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Roger Pau Monne <roger.pau@xxxxxxxxxx>
  • Delivery-date: Wed, 19 Dec 2018 14:37:23 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Openpgp: preference=signencrypt

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

 


Rackspace

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