Re: [PATCH 1 of 3] x86/mm: Relieve contention for p2m lock in gva_to_gfn

["Andres Lagar-Cavilla" <andres@lagarcavilla.org>]

> At 15:34 -0400 on 24 Apr (1335281651), Andres Lagar-Cavilla wrote:
>>  xen/arch/x86/mm/hap/guest_walk.c |  6 +++++-
>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>
>>
>> We don't need to hold the p2m lock for the duration of the guest walk.
>> We need
>> to ensure livenes of the top level page.
>
> I'm not sure I want to start taking these s/gfn-lock/get-page/ changes
> piecemeal without a clear understanding of why the gfn-locking is not
> needed.  My understanding was that
>
>  - get_page protects against the page being freed and reused.
>  - gfn-lock protects against the GFN being reused, or the page
>    being reused within the VM.

Most cases gfn_lock protects against are already taken care of by the end
of a query. From there on, get_page will protect you against paging out.

gfn_lock will further protect the caller from the gfn being replaced. That
is the risk we are taking in these code paths, and the assumption is that
the VM would not try that on its own pagetables, GDTs, or mmio targets. If
so, Xen won't misbehave, yet the VM will shoot itself in the foot.
Deservedly.

gfn/p2m lock will also prevent modifications elsewhere in the p2m, and
that is good motivation for a fine-grained lock. These code paths don't
care about modifications elsewhere in the p2m.

I agree it's rather ad hoc, and I'm not happy about it. I would prefer a
proper construct that buries all details, or finding out if rwlocks are
feasible. I'm trying to find ways to relieve contention and address Yang's
problem before the window closes. It hasn't seem that successful anyways.

Andres
>
> Are there some cases where we only care about the first of these and
> some where we care about both?  In other words, can we replace the
> gfn-locking everywhere with get_page/put_page (i.e. get rid of put_gfn)?
>
> Also:
>
>> @@ -85,13 +86,16 @@ unsigned long hap_p2m_ga_to_gfn(GUEST_PA
>>
>>      /* Map the top-level table and call the tree-walker */
>>      ASSERT(mfn_valid(mfn_x(top_mfn)));
>> +    top_page = mfn_to_page(mfn_x(top_mfn));
>> +    ASSERT(get_page(top_page, p2m->domain));
>
> ASSERT(foo) is compiled out on non-debug builds, so that's definitely
> not what you want.
>
> I personally dislike this idiom with BUG_ON() as well, because even
> though BUG_ON(some side-effecting statement) may be correct, my eye
> tends to skip over it when skimming code.
>
> Cheers,
>
> Tim.
>