On 10.08.2011 11:50, Cherry G. Mathew wrote: > Module Name: src > Committed By: cherry > Date: Wed Aug 10 09:50:37 UTC 2011 > > Modified Files: > src/sys/arch/xen/include: xenpmap.h > src/sys/arch/xen/x86: x86_xpmap.c > > Log Message: > Introduce locking primitives for Xen pte operations, and xen helper calls for > MP related MMU ops
Hi Cherry, Sorry for this very late comment, as I was AFK most of the time for the last three weeks. I have two concerns with your patch: - first, you introduce locking primitives based on simple_lock(9). This mechanism ought to go in the not-so-distant future, so I would replace all these with a properly chosen mutex, likely at IPL_VM (depending on the priority of all consumers of the xpq_*() functions). This must be carefully reviewed first. - second, the lock is not placed at the correct abstraction level IMHO, it is way too high in the caller/callee hierarchy. It should remain hidden from most consumers of the xpq_queue API, and should only be used to protect the xpq_queue array together with its counters (and everything that isn't safe for all memory operations happening in xpq). Reason behind this is that your lock protects calls to hypervisor MMU operations, which are hypercalls (hence, a "slow" operation with regard to kernel). You are serializing lots of memory operations, something that should not happen from a performance point of view (some may take a faire amount of cycles to complete, like TLB flushes). I'd expect all Xen MMU hypercalls to be reentrant. -- Jean-Yves Migeon jeanyves.mig...@free.fr
