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

[Julien Grall <julien.grall@citrix.com>]

Hi Ian,

On 25/11/15 12:40, Ian Campbell wrote:
> 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.

Correct. I can add a comment in the code to explain that.

> 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).

I think using correctly the field type_count would require lots of faff
on ARM as we don't use it for now.

So I would go with introducing a new member of inuse union.

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

Hmmm will give a look.

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

Because the existing pte clear path is only called on the leaf of the
page tables.

The intermediate table will left linked and empty.

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

Are you sure? AFAICT, page_list_move move the head of the list from one
variable to another. So all the list is moved.

Regards,

-- 
Julien Grall