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

[Keir Fraser <keir.xen@gmail.com>]

On 15/07/2013 09:24, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 12.07.13 at 16:30, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>> On 12.07.13 at 16:01, Keir Fraser <keir.xen@gmail.com> wrote:
>>> On 12/07/2013 14:41, "Jan Beulich" <JBeulich@suse.com> wrote:
>>> 
>>>>> 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).
>>> 
>>> Ah, I see. Well, locking only on changes to page-directory entries wouldn't
>>> be too bad, even if it were a single global lock? That would be a rare
>>> occurrence. It's reasonable to assume that callers will not conflict on the
>>> page-aligned regions they modify, so this would suffice?
>> 
>> Well, okay, I'll do it that way then. Are you okay with skipping the
>> locking during boot, just as done in __set_fixmap() in the current
>> version of the patch?

Yes.

> Further to this, there's a worrying comment prior to
> map_pages_to_xen(): "map_pages_to_xen() can be called with
> interrupts disabled:
>  * During early bootstrap; or
>  * alloc_xenheap_pages() via memguard_guard_range"
> 
> The former would be taken care of by only doing any locking
> post-boot, but the latter would also require to skip the locking
> when interrupts are disabled. Yet I wonder whether that part
> of the comment isn't stale - alloc_xenheap_pages() surely
> shouldn't be called with interrupts disabled? (I didn't get to
> try yet whether check_lock() would trigger on any such path.)

Agreed it must be stale!

 -- Keir

> Jan
>