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: Fri, 12 Jul 2013 14:37:49 +0100 Message-ID: References: <51E0163402000078000E4767@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51E0163402000078000E4767@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 12/07/2013 13:44, "Jan Beulich" wrote: >>>> On 12.07.13 at 14:15, Keir Fraser wrote: >> On 12/07/2013 09:17, "Jan Beulich" 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? > Jan >