On 10.12.2025 13:44, Oleksii Kurochko wrote:
> On 12/10/25 8:06 AM, Jan Beulich wrote:
>> On 09.12.2025 18:09, Oleksii Kurochko wrote:
>>> On 12/9/25 2:47 PM, Jan Beulich wrote:
>>>> On 24.11.2025 13:33, Oleksii Kurochko wrote:
>>>>> + *md_pg = p2m_alloc_page(p2m);
>>>>> + if ( !*md_pg )
>>>>> + {
>>>>> + printk("%pd: can't allocate metadata page\n",
>>>>> p2m->domain);
>>>>> + domain_crash(p2m->domain);
>>>>> +
>>>>> + return;
>>>>> + }
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + if ( *md_pg )
>>>>> + metadata = __map_domain_page(*md_pg);
>>>>> +
>>>>> + if ( t >= p2m_first_external )
>>>>> + {
>>>>> + metadata[ctx->index].type = t;
>>>>> +
>>>>> + t = p2m_ext_storage;
>>>>> + }
>>>>> + else if ( metadata )
>>>>> + metadata[ctx->index].type = p2m_invalid;
>>>>> +
>>>>> + pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);
>>>>> +
>>>>> + unmap_domain_page(metadata);
>>>>> }
>>>> Just to mention (towards future work): Once a metadata page goes back to be
>>>> entirely zero-filled, it could as well be hooked off and returned to the
>>>> pool.
>>>> Not doing so may mean detaining an unused page indefinitely.
>>> Won’t that already happen when p2m_free_table() is called?
>> Well, that's when both page table and metadata table are freed. But what if a
>> leaf page table is moving back to holding all p2m_ram_rw mappings? Then the
>> metadata page is unused, but will remain allocated.
>
> Good point...
>
> This could be a rather expensive operation, since in the code:
> + else if ( metadata )
> + metadata[ctx->index].type = p2m_invalid;
> we would have to check all other metadata entries to determine whether they
> are
> (p2m_invalid) or not, and return the page to the pool.
>
> It would be nice to have something like metadata.used_entries_num, but the
> entire
> page is used for type entries.
> As an option, we could reserve 8 bits to store a counter of the number of used
> entries in the metadata page, and then use metadata[0].used_entries_num to
> check
> whether it is zero. If it is zero, we could simply return the metadata page
> to the
> pool in the “else if (metadata)” case mentioned above.
>
> How bad is this idea? Any better suggestions?
First, as said in my initial reply: This may not need taking care of right away.
It will need keeping in mind, of course.
As to suggestions - hardly any of the fields in struct page_info for the page
can be used when the page is a metadata one. Simply record the count there?
Finally, as to "rather expensive": Scanning a 4k page to hold all zeroes can't
be all that expensive? In any event that expensiveness needs weighing carefully
against the risk of getting the counter maintenance wrong.
Jan
