Re: CVS commit: src/sys/arch/xen

[Jean-Yves Migeon <jeanyves.migeon%free.fr@localhost>]

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.migeon%free.fr@localhost