Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.04.19 at 19:21, <tamas@xxxxxxxxxxxxx> wrote:
> Patch cf4b30dca0a "Add debug code to detect illegal page_lock and 
> put_page_type
> ordering" added extra sanity checking to page_lock/page_unlock for debug 
> builds
> with the assumption that no hypervisor path ever locks two pages at once.
> 
> This assumption doesn't hold during memory sharing so we introduce separate
> functions, page_lock_memshr and page_unlock_memshr, to be used exclusively
> in the memory sharing subsystem.

Completely bypassing these checks looks undesirable to me, to
be honest. Otoh as discussed mem-sharing is abusing the lock
anyway. So if this is to remain that way, I wonder whether the
better course of action wouldn't be to stop abusing the
functions, cloning them to continue using the PGT_locked flag.
I would then wonder whether e.g. the checking of PGT_validated
would actually be needed in the cloned functions - that's again
a flag which ought to have meaning mainly (exclusively?) for
handling of PV guests.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2030,12 +2030,11 @@ static inline bool 
> current_locked_page_ne_check(struct page_info *page) {
>  #define current_locked_page_ne_check(x) true
>  #endif
>  
> -int page_lock(struct page_info *page)
> +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
> +static int _page_lock(struct page_info *page)

As per above, personally I'm against introducing
page_{,un}lock_memshr(), as that makes the abuse even more
look like proper use. But if this was to be kept this way, may I
ask that you switch int -> bool in the return types at this occasion?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel