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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.07.13 at 16:30, "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>>> On 12.07.13 at 16:01, Keir Fraser <keir.xen@xxxxxxxxx> wrote:
>> On 12/07/2013 14:41, "Jan Beulich" <JBeulich@xxxxxxxx> 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?

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.)

Jan


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