Re: [Xen-devel] [PATCH v2] add locking around certain calls to map_pages_to_xen()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.07.13 at 15:37, Keir Fraser <keir.xen@xxxxxxxxx> wrote:
> On 12/07/2013 13:44, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>>>>> On 12.07.13 at 14:15, Keir Fraser <keir.xen@xxxxxxxxx> wrote:
>>> On 12/07/2013 09:17, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>> 
>>>> While boot time calls don't need this, run time uses of the function
>>>> which may result in L2 page tables getting populated need to be
>>>> serialized to avoid two CPUs populating the same L2 (or L3) entry,
>>>> overwriting each other's results.
>>>> 
>>>> This fixes what would seem to be a regression from commit b0581b92
>>>> ("x86: make map_domain_page_global() a simple wrapper around vmap()"),
>>>> albeit that change only made more readily visible the already existing
>>>> issue.
>>>> 
>>>> The __init addition to memguard_init(), while seemingly unrelated,
>>>> helps making obvious that this function's use of map_pages_to_xen() is
>>>> a boot time only one.
>>> 
>>> Why can't the locking be implemented inside map_pages_to_xen()? The
>>> requirement is due to implementation details of that function after all.
>>> Pushing the synchronisation out to the callers is uglier and more fragile.
>> 
>> Not all use cases of the function require synchronization, so the
>> only kind of synchronization I would see validly adding there
>> instead of in the callers would be a mechanism marking a to-be-
>> populated non-leaf page table entry as "being processed" first,
>> and have racing invocations spin until that state clears. Afaict
>> that wouldn't cope with eventual (future) races through
>> destroy_xen_mappings() though, and hence I think the proposed
>> patch is the better alternative. But if you're fine with ignoring
>> that last aspect, I'm okay with going the alternative route.
> 
> Is it unsafe to just stick a lock around the guts of map_pages_to_xen(), or
> at least the parts that add new page tables?

I'm not certain about the safety of this, but clearly two CPUs
changing entirely different parts of the address space don't need
to lock out one another, so I rather view adding a global lock here
as being (potentially) harmful in terms of performance (and hence
the thought of locking at page table entry granularity instead).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel