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