Re: [Xen-devel] [PATCH 4/4] xen/arm: p2m: Remove translation table when it's empty

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Wed, 2015-11-18 at 15:49 +0000, Julien Grall wrote:
> Currently, the translation table is left in place even if no entries is
> inuse. Because of how the p2m code has been implemented, replacing a
> translation table by a block (i.e superpage) is not supported. Therefore,
> any mapping of a superpage size will be split in smaller chunks making
> the translation less efficient.
> 
> Replacing a table by a block when a new mapping is added would be too
> complicated because it requires to check if all the upper levels are not
> inuse and free them if necessary.
> 
> Instead, we will remove the empty translation table when the mapping are
> removed. To avoid going through all the table checking if no entry is
> inuse, a counter representing the number of entry currently inuse is
> kept per table translation and updated when an entry change state (i.e
> valid <-> invalid).
> 
> As Xen allocates a page for each translation table, it's possible to
> store the counter in the struct page_info.The field type_info has been
> choosen for this purpose as the p2m owns the page and nobody should used

"chosen"

> @@ -936,6 +938,16 @@ static int apply_one_level(struct domain *d,
> ÂÂÂÂÂBUG(); /* Should never get here */
> Â}
> Â
> +static void update_reference_mapping(struct page_info *page,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlpae_t old_entry,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlpae_t new_entry)
> +{
> +ÂÂÂÂif ( p2m_valid(old_entry) && !p2m_valid(new_entry) )
> +ÂÂÂÂÂÂÂÂpage->u.inuse.type_info--;
> +ÂÂÂÂelse if ( !p2m_valid(old_entry) && !p2m_valid(new_entry) )
> +ÂÂÂÂÂÂÂÂpage->u.inuse.type_info++;
> +}

Is there some suitable locking in place for page here?

I think the argument is that this page is part of the p2m and therefore the
p2m lock is the thing which protects it, and we certainly hold that
everywhere around here.

type_info is not actually a bare integer, it has some flags at the top (see
PGT_*) which are sometimes used (e.g. share_xen_page_with_guest).

I think for consistency we should probably add a PGT_p2m and use that (and
perhaps audit some of the other PGT_* which don't all seem to be completely
obviously not-x86).

Presumably we would then want {get,put}_page_type to actually do something
and to use them.

If we don't want to do that (I'm leaning towards we should, but I might be
convinceable otherwise) then I think we should avoid the use of type_info
as a bare counter, which would imply using another member of the inuse
union (p2m_refcount or some such).

> Â Â Â Â Â Â Âif ( ret != P2M_ONE_DESCEND ) break;
> @@ -1099,6 +1118,45 @@ static int apply_p2m_changes(struct domain *d,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> ÂÂÂÂÂÂÂÂÂÂÂÂÂ/* else: next level already valid */
> ÂÂÂÂÂÂÂÂÂ}
> +
> +ÂÂÂÂÂÂÂÂBUG_ON(level > 3);
> +
> +ÂÂÂÂÂÂÂÂif ( op == REMOVE )
> +ÂÂÂÂÂÂÂÂ{
> +ÂÂÂÂÂÂÂÂÂÂÂfor ( ; level > P2M_ROOT_LEVEL; level-- )
> +ÂÂÂÂÂÂÂÂÂÂÂÂ{

Something is up with the indentation here.

> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlpae_t old_entry;
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlpae_t *entry;
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂunsigned int offset;
> +
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpg = pages[level];
> +
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/*
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* No need to try the previous level if the current one
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* still contains some mappings
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*/
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif ( pg->u.inuse.type_info )
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> +
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂoffset = offsets[level - 1];
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂentry = &mappings[level - 1][offset];
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂold_entry = *entry;
> +
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpage_list_del(pg, &p2m->pages);
> +
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂp2m_remove_pte(entry, flush_pt);

This made me wonder how the existing pte clear path (which you refactored
into this function) gets away without freeing the page, are we just leaking
it onto the p2m->pages list?

p2m_put_l3_page (at the other call site) only takes care of foreign
mappings, which aren't on that list.

Maybe there is a latent bug here? And if so perhaps the fix involves making
p2m_remove_pte take care of various bits and bobs of the book keeping which
is done here?

> +
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂp2m->stats.mappings[level - 1]--;
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂupdate_reference_mapping(pages[level - 1], old_entry, 
> *entry);
> +
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/*
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* We can't free the page now because it may be present
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* in the guest TLB. Queue it and free it after the TLB
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* has been flushed.
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*/
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpage_list_add(pg, &free_pages);

You could have used page_list_move instead of del+add, but I can't quite
convince myself it matters.

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