Re: [Xen-devel] [PATCH 7/8] xen/arm: Set foreign page type to p2m_map_foreign

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 12/05/2013 04:54 PM, Ian Campbell wrote:
> On Thu, 2013-12-05 at 16:41 +0000, Julien Grall wrote:
>
> > > >        switch ( space )
> > > >        {
> > > > @@ -1019,8 +1020,8 @@ static int xenmem_add_to_physmap_one(
> > > >            break;
> > > >        case XENMAPSPACE_gmfn_foreign:
> > > >        {
> > > > -        paddr_t maddr;
> > > >            struct domain *od;
> > > > +        struct page_info *page;
> > > >            od = rcu_lock_domain_by_any_id(foreign_domid);
> > > >            if ( od == NULL )
> > > >                return -ESRCH;
> > > > @@ -1032,15 +1033,18 @@ static int xenmem_add_to_physmap_one(
> > > >                return rc;
> > > >            }
> > > >
> > > > -        maddr = p2m_lookup(od, pfn_to_paddr(idx));
> > > > -        if ( maddr == INVALID_PADDR )
> > > > +        /* Take refcount to the foreign domain page.
> > > "on the foreign" (and maybe "domain's" or just "foreign page"
> > >
> > > > +         * Refcount will be release in XENMEM_remove_from_physmap */
> > > "will be released".
> > >
> > > The refcount will also be removed by p2m_teardown. That probably needs
> > > changing to do an actual walk of the p2m tearing things down, which is a
> > > pain.
> > >
> > > Stefano's now obsolete dma pinning series had a patch which added a
> > > fairly generic walker in it which might be reusable.
> > Do you have a link to this patch?
> I was afraid you'd say that...
> /scrobbles
>
> "xen/arm: introduce a generic p2m walker and use it in p2m_lookup". v6
> is msgid
> <1380298560-29352-2-git-send-email-stefano.stabellini@xxxxxxxxxxxxx>. I
> don't recall if that was the last one though.
Actually the generic walker will not be usefull for p2m_teardown. It 
seems to be an extension of p2m_lookup, so will only search a specific gfn.
> > > The walk might need to support continuations though.
> > >
> > > I wonder if this ref ought to be taken in create_p2m_entries for all
> > > entries and not just foreign ones, and then released in the appropriate
> > > places.
> > If I'm not mistaken, Xen already takes a ref when the page is allocated
> > for the domain. Why would we need to take another ref for these pages?
> There's a nice symmetry to all p2m entries taking a ref count, and it
> makes the teardown a bit simpler since you can just drop the ref every
> time without worrying about the type.
If I'm not mistaken, even x86 code doesn't have 2 ref for each page. Or 
I didn't see where the ref is taken/release ...
In any case, walk the whole p2m for looking for present page seems a bit 
tough and slow to release memory for a domain.
I'm wondering if we can have a list of foreign page, and browse it when 
the domain is destroyed.
--
Julien Grall

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