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

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

Hi Ian,

On 15/12/15 10:25, Ian Campbell wrote:
> On Tue, 2015-12-01 at 17:52 +0000, Julien Grall wrote:
>> Currently, the translation table is left in place even if no entries is
> 
> "are in use" (in fact s/inuse/in use/ throughout.
> 
>> 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
>       
>       remapping ? To make it clear it doesn't apply to an initial mapping?

remapping is good here.

> 
>> 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
> 
>                          requires us to check
> 
>> inuse and free them if necessary.
>>
>> Instead, we will remove the empty translation table when the mapping are
> 
>                                                            mappings    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
> 
>                                                        changes
> 
>> valid <-> invalid).
>>
>> As Xen allocates a page for each translation table, it's possible to
>> store the counter in the struct page_info. A new field p2m_refcount as
> 
>                                                                       has
> 
>> been introduced in the inuse union for this purpose. This is fine as the
>> page is only used by the P2M code and nobody touch the other field of
> 
>                                                 touches
> 
>> the union type_info.
>>
>> For record, type_info has not been used because it would require
> 
>   For the record,
> 
>> more work to use it properly as Xen on ARM doesn't yet have the concept
>> of type.
>>
>> Once Xen has finished to remove a mapping and all the reference to each
> 
>                finished removing  a mapping...          references
> 
>> translation table has been updated, the level will be lookup backward to
> 
>                     have                                 looked at backwards?
> 
> I'm not sure about that second one, maybe the "the higher levels will be
> looked at in turn to check..." is better? Hrm, no that doesn't really work
> with the rest of the paragraph.
> 
>     Once Xen has finished to remove a mapping and all the references to each
>     translation table have been updated, then the higher levels will be
>     processed and freed as needed. This will allow to propagate the number
>     of references and free multiple translation table at different level in
>     one go.
> 
> ?

This is clearer than what I wrote. I'm happy if you replace it.

>> check if we need first need to free an unused translation table at an
>> higher level and then the lower levels. This will allow to propagate the
>> number of reference and free multiple translation table at different
>> level
>> in one go.
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> The code looks good, I can fixup the commit log if we can agree what to
> write for the second and final comments above (hopefully the rest are just
> uncontroversial grammar fixes).

I agree with all the comments. Thank you for fixing it.

Regards,

-- 
Julien Grall