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

["Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx>]

> 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.

I'll turn unshare into __unshare and make unshare a wrapper that asserts
the return value semantics of __unshare.

>
>>  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?

If we follow the "best effort ring" idea I just proposed for the previous
patch, it would make this one cleaner as well.

I don't think there's a point in remembering enomem's are on the pipe. The
helper will be woken up on enomem 1, it won't need over 64 notifications
to do something about it.

Andres

>
>>  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