Re: [PATCH v2 07/10] xen/arm: Introduce relinquish_p2m_mapping to remove refcount every mapped page

[Julien Grall <julien.grall@linaro.org>]

On 12/09/2013 04:28 PM, Ian Campbell wrote:
> On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote:
>> This function will be called when the domain relinquishes its memory.
>> It removes refcount on every mapped page to a valid MFN.
>>
>> Currently, Xen doesn't take refcount on every new mapping but only for foreign
>> mapping. Restrict the function only on foreign mapping.
>
> Skimming the remainder of the patch's titles and recalling a previous
> conversation the intention is not to extend this for 4.4, correct?

Right, it's too big for Xen 4.4.

>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>      Changes in v2:
>>          - Introduce the patch
>> ---
>>   xen/arch/arm/domain.c        |    5 +++++
>>   xen/arch/arm/p2m.c           |   47 ++++++++++++++++++++++++++++++++++++++++++
>>   xen/include/asm-arm/domain.h |    1 +
>>   xen/include/asm-arm/p2m.h    |   15 ++++++++++++++
>>   4 files changed, 68 insertions(+)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 1590708..e7c2f67 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -717,6 +717,11 @@ int domain_relinquish_resources(struct domain *d)
>>           if ( ret )
>>               return ret;
>>
>> +    case RELMEM_mapping:
>
> Something somewhere should be setting d->arch.relmem = RELMEM_mapping at
> the appropriate time. (immediately above I think?)
>
> You also want a "Fallthrough" comment just above.

Oops, I will update the patch for the next version.

>> +        ret = relinquish_p2m_mapping(d);
>> +        if ( ret )
>> +            return ret;
>> +
>>           d->arch.relmem = RELMEM_done;
>>           /* Fallthrough */
>>
>> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
>> index f0bbaca..dbd6a06 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -6,6 +6,7 @@
>>   #include <xen/bitops.h>
>>   #include <asm/flushtlb.h>
>>   #include <asm/gic.h>
>> +#include <asm/event.h>
>>
>>   /* First level P2M is 2 consecutive pages */
>>   #define P2M_FIRST_ORDER 1
>> @@ -320,6 +321,16 @@ static int create_p2m_entries(struct domain *d,
>>               flush_tlb_all_local();
>>       }
>>
>> +    if ( (t == p2m_ram_rw) || (t == p2m_ram_ro) || (t == p2m_map_foreign))
>> +    {
>> +        unsigned long sgfn = paddr_to_pfn(start_gpaddr);
>> +        unsigned long egfn = paddr_to_pfn(end_gpaddr);
>> +
>> +        p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn);
>> +        /* Use next_gfn_to_relinquish to store the lowest gfn mapped */
>> +        p2m->next_gfn_to_relinquish = MIN(p2m->next_gfn_to_relinquish, sgfn);
>> +    }
>> +
>>       rc = 0;
>>
>>   out:
>> @@ -503,12 +514,48 @@ int p2m_init(struct domain *d)
>>
>>       p2m->first_level = NULL;
>>
>> +    p2m->max_mapped_gfn = 0;
>> +    p2m->next_gfn_to_relinquish = ULONG_MAX;
>> +
>>   err:
>>       spin_unlock(&p2m->lock);
>>
>>       return rc;
>>   }
>>
>> +int relinquish_p2m_mapping(struct domain *d)
>> +{
>> +    struct p2m_domain *p2m = &d->arch.p2m;
>> +    unsigned long gfn, count = 0;
>> +    int rc = 0;
>> +
>> +    for ( gfn = p2m->next_gfn_to_relinquish;
>> +          gfn < p2m->max_mapped_gfn; gfn++ )
>
> I know that Tim has been keen to get rid of these sorts of loops on x86,
> and with good reason I think.
>
>> +    {
>> +        p2m_type_t t;
>> +        paddr_t p = p2m_lookup(d, gfn, &t);
>
> This does the full walk for each address, even though 2/3 of the levels
> are more than likely identical to the previous gfn.
>
> It would be better to do a full walk, which sadly will look a lot like
> p2m_lookup, no avoiding that I think.
>
> You can still resume the walk based on next_gfn_to_relinquish and bound
> it on max_mapped_gfn, although I don't think it is strictly necessary.
>> +        unsigned long mfn = p >> PAGE_SHIFT;
>> +
>> +        if ( mfn_valid(mfn) && p2m_is_foreign(t) )
>
> I think it would be worth reiterating in a comment that we only take a
> ref for foreign mappings right now.

Will do.

>> +        {
>> +            put_page(mfn_to_page(mfn));
>> +            guest_physmap_remove_page(d, gfn, mfn, 0);
>
> You should unmap it and then put it I think.
>
> Is this going to do yet another lookup/walk?
>
> The REMOVE case of create_p2m_entries is so trivial you could open code
> it here, or if you wanted to you could refactor it into a helper.
>
> I am wondering if the conditional put page ought to be at the point of
> removal (i.e. in the helper) rather than here. (I think Tim made a
> similar comment on the x86 version of the remove_from_physmap pvh
> patches, you probably need to match the generic change which that
> implies)
>
> BTW, if you do the clear (but not the put_page) for every entry then the
> present bit (or pte.bits == 0) might be a useful proxy for
> next_lN_entry? I suppose even walking, say, 4GB of mostly empty P2M is
> not going to be super cheap.

Following the discution we had IRL, I will try to extend 
create_p2m_entries by adding RELINQUISH option.

>> +        }
>> +
>> +        count++;
>> +
>> +        /* Preempt every 2MiB. Arbitrary */
>> +        if ( (count == 512) && hypercall_preempt_check() )
>> +        {
>> +            p2m->next_gfn_to_relinquish = gfn + 1;
>> +            rc = -EAGAIN;
>
> I think I'm just failing to find it, but where is the call to
> hypercall_create_continuation? I suppose it is somewhere way up the
> stack?

Actually rc = -EAGAIN will be catched by xc_domain_destroy in libxc. The 
function will call again the hypercall if needed.

I'm not sure why... do you have any idea why x86 also uses this trick?

> I'm not sure the count == 512 is needed -- hypercall_preempt_check
> should be sufficient?

On ARM hypercall_preempt_check is a bit complex. Checking every loop 
seems a bit overkill.

-- 
Julien Grall