From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH v2] add locking around certain calls to map_pages_to_xen() Date: Mon, 15 Jul 2013 09:36:11 +0100 Message-ID: References: <51E3CDD902000078000E4D78@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51E3CDD902000078000E4D78@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Andrew Cooper , xen-devel List-Id: xen-devel@lists.xenproject.org On 15/07/2013 09:24, "Jan Beulich" wrote: >>>> On 12.07.13 at 16:30, "Jan Beulich" wrote: >>>>> On 12.07.13 at 16:01, Keir Fraser wrote: >>> On 12/07/2013 14:41, "Jan Beulich" 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 >