Re: [Xen-devel] [PATCH 3 of 4] Memory sharing: better handling of ENOMEM while unsharing

[Tim Deegan <tim@xxxxxxx>]

At 22:57 -0500 on 15 Feb (1329346626), Andres Lagar-Cavilla wrote:
>  xen/arch/x86/hvm/hvm.c            |  20 +++++++++++++-
>  xen/arch/x86/mm.c                 |   8 +++--
>  xen/arch/x86/mm/mem_sharing.c     |  52 
> ++++++++++++++++++++++++---------------
>  xen/arch/x86/mm/p2m.c             |  18 ++++++++++++-
>  xen/common/grant_table.c          |  11 ++++---
>  xen/common/memory.c               |   1 +
>  xen/include/asm-x86/mem_sharing.h |  15 +++++++++++
>  7 files changed, 94 insertions(+), 31 deletions(-)
> 
> 
> If unsharing fails with ENOMEM, we were:
>  - leaving the list of gfns backed by the shared page in an inconsistent state
>  - cycling forever on the hap page fault handler.
>  - Attempting to produce a mem event (which could sleep on a wait queue)
>    while holding locks.
>  - Not checking, for all callers, that unshare could have indeed failed.
> 
> Fix bugs above, and sanitize callers to place a ring event in an unlocked
> context, or without requiring to go to sleep on a wait queue.
> 
> A note on the rationale for unshare error handling:
>  1. Unshare can only fail with ENOMEM. Any other error conditions BUG_ON()'s

Please enforce this in the code -- either the function should just
return success/failure or (better, I think) the caller should ASSERT
that it doesn't see any other error codes.

>  2. We notify a potential dom0 helper through a mem_event ring. But we
>     allow the notification to not go to sleep. If the event ring is full
>     of ENOMEM warnings, then the helper will already have been kicked enough.

Could we just keep a count (or flag) to remind us that an ENOMEM is in
the pipe and avoid having an exception to the waiting rules?

>  3. We cannot "just" go to sleep until the unshare is resolved, because we
>     might be buried deep into locks (e.g. something -> copy_to_user ->
>     __hvm_copy)
>  4. So, we make sure we:
>     4.1. return an error
>     4.2. do not corrupt shared memory
>     4.3. do not corrupt guest memory
>     4.4. let the guest deal with the error if propagation will reach that far

Yep, the other cleanups look good to me. 

Cheers,

Tim.


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