[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 3/7] xen/p2m: put reference for level 2 superpage


  • To: Julien Grall <julien@xxxxxxx>
  • From: Luca Fancellu <Luca.Fancellu@xxxxxxx>
  • Date: Tue, 28 May 2024 08:29:55 +0000
  • Accept-language: en-GB, en-US
  • Arc-authentication-results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=arm.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com])
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=7VOcNoNlr9GAIkTXpODPS2/BPKKMfCbMqMjV39EpSNY=; b=VEnG7OTTCNJgeZTVptpzZcEcbiDb0Zuxu+NPjqF+HSMvSJN0GhEj16HqPlxwDd3AnFJ9Kxi5cYse2IeP3Z288+pJ43UqdtLQmm4wakgaA+m0fw8sGR8KB6nkRz15My+vgqlwPMGesZY7qkItgfjm+RcINxusykpPn7BjPDTM4L0/6N3fzm1BWoLeHL3Fbp0lERE8DXDRwN3aZEf2DJf2UlmyQOmp+mB3r9BQpZHE+735BiqLhFCaH5f3GfCRtAQsng+gtoAbOA61YD1Qy1S1D/ls4RNhENccTXygQzxEHV+hWE+svBUJiDNzh+EaMWrP3gJT9rKQWUEloYJ9Gt46XA==
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=7VOcNoNlr9GAIkTXpODPS2/BPKKMfCbMqMjV39EpSNY=; b=ablMDDMXWswRaltQCxH9973/+TrQeFpi9ygWJdrCGWfe7fJ5AyJ0kgbNMtiHRxd0VAFduLM536gOa5s/XTGeN7yvw7+ACiMrdbBfzVxdl4mTFPJ4ZHXfQcs2QMHy553nNc2sLj/EMBu3R/0VU9tcKAuorPy2PH6NFYA9MciL8MiGhhtXxID4iEj01m2C1j/4vy1ItxAm7CgWr58uJYJ8/X2pUjCcbOKfviYThUwKVKYLFo3bJMq+c7h37WFfSSTay4hvYShDEDozCxH1U/i8PWA+afuaYghKtjno962TJtVL9+fEV7vQRlA/eHufIKyxfPUELq6WwQZKHRbCD9Z9kg==
  • Arc-seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=dLiDz2ODCF8zLR9Ju5g/3D5keVH5g6q4VFOS27mZs3wxDGnA8civ9qyTyfnOBGcsZXEZHFD0WqqhnIO2fRszrXrkWmXdUELhlDzdDJpJvATC+rekFy6CDLWgKQnuGCAIp4M50PTiDjj7iEn160KRNz8hfugsWTX0zqIqCz8+Tu5qU1t4oz8w6xMo7lJhGW2+Eh3S1DWRiZiLGf3pqqCHzYnLQ4tmXwYepJtq2RbrwigPMkQ4NsF2AH77+uNoppKkeCiySQLUouLHr+T86pHgqjioOE1HyQG2iZecpz8opMSl9c4mtubuH4SnrabblXChj+qUF4W+Sv1VinQTZls7wQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=fFCJwbcePffJiGLnHGHoRXj1Xy5LAy58IiLWfiKkggaC3utWqv50fT2Bf+M8lJM2G4nQcqAN+gGH7Hxzx+XeTytxrxzosoX+CZwtg6wO5SbGKkqgeG62PyqB9GJ5WOCOgUzcHRjd1p1q9FWIIolwEuGhOs17qy9u5bcr1y8IGZYj6thN7jCjNpM6eurHptKnl7boFc8IE8XTXWsumhdqxhmZ+lWd6gATEvmGE7or3C9GB0OWnSC2Egja9i82sfnLwRigMGl0xpim54jhNU50BTcDsWSN1GwAauXlanlhV52pe127BMggfnj6OILlna+HmDpgQ8ruVlFrTJiBtwoqJg==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Penny Zheng <Penny.Zheng@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Michal Orzel <michal.orzel@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Tue, 28 May 2024 08:30:32 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHarde5MtnBBoqXhEOFOHlm9nE79rGmV9OAgAX+4YA=
  • Thread-topic: [PATCH v4 3/7] xen/p2m: put reference for level 2 superpage

Hi Julien,

> On 24 May 2024, at 13:56, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Luca,
> 
> On 24/05/2024 13:40, Luca Fancellu wrote:
>> From: Penny Zheng <Penny.Zheng@xxxxxxx>
>> We are doing foreign memory mapping for static shared memory, and
>> there is a great possibility that it could be super mapped.
>> But today, p2m_put_l3_page could not handle superpages.
>> This commits implements a new function p2m_put_l2_superpage to handle
>> 2MB superpages, specifically for helping put extra references for
>> foreign superpages.
>> Modify relinquish_p2m_mapping as well to take into account preemption
>> when type is foreign memory and order is above 9 (2MB).
>> Currently 1GB superpages are not handled because Xen is not preemptible
>> and therefore some work is needed to handle such superpages, for which
>> at some point Xen might end up freeing memory and therefore for such a
>> big mapping it could end up in a very long operation.
>> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
>> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
>> ---
>> v4 changes:
>>  - optimised the path to call put_page() on the foreign mapping as
>>    Julien suggested. Add a comment in p2m_put_l2_superpage to state
>>    that any changes needs to take into account some change in the
>>    relinquish code. (Julien)
>> v3 changes:
>>  - Add reasoning why we don't support now 1GB superpage, remove level_order
>>    variable from p2m_put_l2_superpage, update TODO comment inside
>>    p2m_free_entry, use XEN_PT_LEVEL_ORDER(2) instead of value 9 inside
>>    relinquish_p2m_mapping. (Michal)
>> v2:
>>  - Do not handle 1GB super page as there might be some issue where
>>    a lot of calls to put_page(...) might be issued which could lead
>>    to free memory that is a long operation.
>> v1:
>>  - patch from 
>> https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-Penny.Zheng@xxxxxxx/
>> ---
>>  xen/arch/arm/mmu/p2m.c | 82 +++++++++++++++++++++++++++++++-----------
>>  1 file changed, 62 insertions(+), 20 deletions(-)
>> diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c
>> index 41fcca011cf4..986c5a03c54b 100644
>> --- a/xen/arch/arm/mmu/p2m.c
>> +++ b/xen/arch/arm/mmu/p2m.c
>> @@ -753,34 +753,66 @@ static int p2m_mem_access_radix_set(struct p2m_domain 
>> *p2m, gfn_t gfn,
>>      return rc;
>>  }
>>  -/*
>> - * Put any references on the single 4K page referenced by pte.
>> - * TODO: Handle superpages, for now we only take special references for leaf
>> - * pages (specifically foreign ones, which can't be super mapped today).
>> - */
>> -static void p2m_put_l3_page(const lpae_t pte)
>> +static void p2m_put_foreign_page(struct page_info *pg)
>>  {
>> -    mfn_t mfn = lpae_get_mfn(pte);
>> -
>> -    ASSERT(p2m_is_valid(pte));
>> -
>>      /*
>> -     * TODO: Handle other p2m types
>> -     *
>>       * It's safe to do the put_page here because page_alloc will
>>       * flush the TLBs if the page is reallocated before the end of
>>       * this loop.
>>       */
>> -    if ( p2m_is_foreign(pte.p2m.type) )
>> +    put_page(pg);
>> +}
>> +
>> +/* Put any references on the single 4K page referenced by mfn. */
>> +static void p2m_put_l3_page(mfn_t mfn, p2m_type_t type)
>> +{
>> +    /* TODO: Handle other p2m types */
>> +    if ( p2m_is_foreign(type) )
>>      {
>>          ASSERT(mfn_valid(mfn));
>> -        put_page(mfn_to_page(mfn));
>> +        p2m_put_foreign_page(mfn_to_page(mfn));
>>      }
>>      /* Detect the xenheap page and mark the stored GFN as invalid. */
>> -    else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) )
>> +    else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) )
>>          page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN);
>>  }
>>  +/* Put any references on the superpage referenced by mfn. */
>> +static void p2m_put_l2_superpage(mfn_t mfn, p2m_type_t type)
>> +{
>> +    struct page_info *pg;
>> +    unsigned int i;
>> +
>> +    /*
>> +     * TODO: Handle other p2m types, but be aware that any changes to handle
>> +     * different types should require an update on the relinquish code to 
>> handle
>> +     * preemption.
>> +     */
>> +    if ( !p2m_is_foreign(type) )
>> +        return;
>> +
>> +    ASSERT(mfn_valid(mfn));
>> +
>> +    pg = mfn_to_page(mfn);
>> +
>> +    for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++, pg++ )
>> +        p2m_put_foreign_page(pg);
>> +}
>> +
>> +/* Put any references on the page referenced by pte. */
>> +static void p2m_put_page(const lpae_t pte, unsigned int level)
>> +{
>> +    mfn_t mfn = lpae_get_mfn(pte);
>> +
>> +    ASSERT(p2m_is_valid(pte));
>> +
>> +    /* We have a second level 2M superpage */
>> +    if ( p2m_is_superpage(pte, level) && (level == 2) )
> 
> AFAICT, p2m_put_page() can only be called if the pte points to a superpage or 
> page:
> 
>    if ( p2m_is_superpage(entry, level) || (level == 3) )
>    {
>       ...
>       p2m_put_page()
> 
>    }
> 
> So do we actually need to check p2m_is_superpage()?
> 
>> +        return p2m_put_l2_superpage(mfn, pte.p2m.type);
>> +    else if ( level == 3 )
>> +        return p2m_put_l3_page(mfn, pte.p2m.type);
>> +}
>> +
>>  /* Free lpae sub-tree behind an entry */
>>  static void p2m_free_entry(struct p2m_domain *p2m,
>>                             lpae_t entry, unsigned int level)
>> @@ -809,9 +841,16 @@ static void p2m_free_entry(struct p2m_domain *p2m,
>>  #endif
>>            p2m->stats.mappings[level]--;
>> -        /* Nothing to do if the entry is a super-page. */
>> -        if ( level == 3 )
>> -            p2m_put_l3_page(entry);
>> +        /*
>> +         * TODO: Currently we don't handle 1GB super-page, Xen is not
>> +         * preemptible and therefore some work is needed to handle such
>> +         * superpages, for which at some point Xen might end up freeing 
>> memory
>> +         * and therefore for such a big mapping it could end up in a very 
>> long
>> +         * operation.
>> +         */
>> +        if ( level >= 2 )
> 
> The code in p2m_put_page() can properly handle level 1. So I would rather 
> prefer if we remove this check (again in order to reduce the amount of work 
> in the P2M code).
> 
> This TODO could be moved there.
> 
>> +            p2m_put_page(entry, level);
>> +
>>          return;
>>      }
>>  @@ -1558,9 +1597,12 @@ int relinquish_p2m_mapping(struct domain *d)
>>            count++;
>>          /*
>> -         * Arbitrarily preempt every 512 iterations.
>> +         * Arbitrarily preempt every 512 iterations or when type is foreign
>> +         * mapping and the order is above 9 (2MB).
> 
> Let's avoid mentioning 2MB. This is only valid when using 4KB pages.
> 
> This is also talking about a specific when the code below is not 4KB 
> specific. I would rework anything after the 'or' to:
> 
> 'we have a level-2 foreign mapping'
> 
> The rest of the LGTM to me but I don't feel confortable to do all those 
> changes on commit. So this will want a respin.
> 
> For simplicity, you can only resend this patch.

Thanks, I’ll address your comment and push the new patch asap.

Cheers,
Luca


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.