[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 4/4] xen/arm: p2m: Remove translation table when it's empty
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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |