Re: [Xen-devel] [PATCH 5 of 9] Fine-grained concurrency control structure for the p2m

[Tim Deegan <tim@xxxxxxx>]

At 00:33 -0400 on 27 Oct (1319675630), Andres Lagar-Cavilla wrote:
> Introduce a fine-grained concurrency control structure for the p2m. This
> allows for locking 2M-aligned chunks of the p2m at a time, exclusively.
> Recursive locking is allowed. Global locking of the whole p2m is also
> allowed for certain operations. Simple deadlock detection heuristics are
> put in place.
> 
> Note the patch creates backwards-compatible shortcuts that will lock the
> p2m globally. So it should remain functionally identical to what is currently
> in place.
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>

Wow.  What a lot of code. :)  I took a look through, but I can't
guarantee to have got all the details.  Things I saw:

- You use atomic_t for the count but only ever update it under a
  lock. :)  If you just need to be sure of atomic writes, then 
  atomic_set will do that without using a locked increment/decrement.

- You allocate the bitmaps from xenheap - they should really be using
  p2m memory, so as to avoid changing the memory overhead of the domain 
  as it runs.   That will involve map_domain_page()ing the bitmaps as
  you go, but at least on x86_64 that's very cheap. 

- panic() on out-of-memory is pretty rude. 

But stepping back, I'm not sure that we need all this just yet.  I think
it would be worth doing the interface changes with a single p2m lock and
measuring how bad it is before getting stuck in to fine-grained locking
(fun though it might be).

I suspect that if this is a contention point, allowing multiple readers
will become important, especially if there are particular pages that
often get emulated access.

And also, I'd  like to get some sort of plan for handling long-lived
foreign mappings, if only to make sure that this phase-1 fix doesn't 
conflict wih it.

Oh, one more thing: 

> +/* Some deadlock book-keeping. Say CPU A holds a lock on range A, CPU B 
> holds a 
> + * lock on range B. Now, CPU A wants to lock range B and vice-versa. 
> Deadlock.
> + * We detect this by remembering the start of the current locked range.
> + * We keep a fairly small stack of guards (8), because we don't anticipate
> + * a great deal of recursive locking because (a) recursive locking is rare 
> + * (b) it is evil (c) only PoD seems to do it (is PoD therefore evil?) */

If PoD could ba adjusted not to do it, could we get rid of all the
recursive locking entirely?  That would simplify things a lot. 

Tim.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel