Re: [Xen-devel] [PATCH v3] altp2m: Allow the hostp2m to be shared

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On May 25, 2016 05:27, "George Dunlap" <george.dunlap@xxxxxxxxxx> wrote:
>
> On Fri, Apr 29, 2016 at 6:42 PM, Tamas K Lengyel <tamas@xxxxxxxxxxxxx> wrote:
> > Don't propagate altp2m changes from ept_set_entry for memshare as memshare
> > already has the lock. We call altp2m propagate changes once memshare
> > successfully finishes. Allow the hostp2m entries to be of type
> > p2m_ram_shared when applying mem_access. Also, do not trigger PoD for hostp2m
> > when setting altp2m mem_access to be in-line with non-altp2m mem_access path.
>
> Hey Tamas,
>
> Sorry for the long delay in getting back to you on this.

No problem, thanks for taking a closer look!

>
> So the main issue here (correct me if I'm wrong) is the locking
> discipline: namely, men_sharing_share_pages():
> - Grabs the hostp2m lock
> - Grabs the appropriate domain memsharing locks
> - Calls set_shared_p2m_entry(), which ends up calling ept_set_entry(),
> which (when altp2m is active) grabs the altp2mlist and altp2m locks.
>
> This causes an ASSERT(), since the altp2mlist lock is ahead of the
> memsharing locks in the list.
>
> But having taken a closer look at the code, I'm not sure the change is
> quite correct.  Please correct me if I've misread something:
>
> mem_sharing_share_pages() is passed two <domain,gfn> pairs -- the
> <sd,sgfn> (which I assume stands for "shared gfn") and <cd,cgfn>
> (which I assume stands for "copy"); and it

Here s/c stands for source/client.

> 1) Looks up smfn and cmfn, which back sgfn and cmfn respectively
> 2) Looks up cmfn, which backs cgfn then replaces all gfn entries which
> point to cmfn with smfn (updating accounting as appropriate)

Hm, I might have missed that. Where does it do the lookup for all other cgfns backed by this cmfn?

> But this change will only call p2m_altp2m_propagate_change() for the
> original cgfn -- any other gfns which are backed by cmfn will not have
> the corresponding altp2m entries propagated properly.

Right, if there is some other place where it does sharing in the back we would have to propagate that change.

> This sort of mistake is easy to make, which is why I think we should
> try to always update the altp2ms in ept_set_entry() if we can, to
> minimize the opportunity for making this sort of mistake.
>
> Is there ever a reason to grab the altp2m lock and *then* grab the
> sharing lock?  Could we just move the sharing lock up between the p2m
> lock and the altp2mlist lock instead?
>

I can't think of a scenario where we would get to sharing from altp2m with altp2m locking first. Not sure what you mean by moving the sharing lock up though. The problem is that sharing already has the lock by the time altp2m tries to lock, so we could pass that info down to make altp2m aware it needs no locking. It would require extending a bunch of functions though with an extra input that is barely ever used..

Thanks,
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel