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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
> >>> 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.

I don't have an answer to your question. I would prefer getting rid of
this whole thing myself. For now I'm just trying to make it usable
again.

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

Switching them to bool would be fine. Replacing them with something
saner is unfortunately out-of-scope at the moment. Unless someone has
a specific solution that can be put in place. I don't have one.

Thanks,
Tamas

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