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

Re: [PATCH V4] xen/gnttab: Store frame GFN in struct page_info on Arm




On 22.12.21 12:01, Julien Grall wrote:

Hi Julien

Hi Jan,

On 14/12/2021 17:45, Jan Beulich wrote:
On 14.12.2021 17:26, Oleksandr wrote:
On 14.12.21 15:37, Jan Beulich wrote:
On 03.12.2021 21:33, Oleksandr Tyshchenko wrote:
@@ -2177,14 +2181,22 @@ void *alloc_xenheap_pages(unsigned int order, unsigned int memflags)
      void free_xenheap_pages(void *v, unsigned int order)
   {
+    struct page_info *pg;
+    unsigned int i;
+
       ASSERT(!in_irq());
          if ( v == NULL )
           return;
   +    pg = virt_to_page(v);
+
       memguard_guard_range(v, 1 << (order + PAGE_SHIFT));
... this really want to (logically) move into the new arch hooks.
That'll effectively mean to simply drop the Arm stubs afaict (and I
notice there's some dead code there on x86, which I guess I'll make
a patch to clean up). But first of all this suggests that you want
to call the hooks with base page and order, putting the loops there.

I see your point and agree ... However I see the on-list patches that
remove common memguard_* invocations and x86 bits.
So I assume, this request is not actual anymore, or I still need to pass
an order to new arch hooks? Please clarify.

Well, that patch (really just the Arm one) effectively takes care of
part of what I did say above. Irrespective I continue to think that
the hook should take a (page,order) tuple instead of getting invoked
once for every order-0 page. And the hook invocations should be placed
such that they could fulfill the (being removed) memguard function
(iirc that was already the case, at least mostly).

IIUC your suggestion, with your approach, alloc_xenheap_pages() would look like:

     for ( i = 0; i < (1u << order); i++ )
         pg[i].count_info |= PGC_xen_heap;

     arch_alloc_xenheap_pages(pg, 1U << order);

The Arm implementation for arch_alloc_xenheap_pages() would also contain a loop.
I have the same understanding (except passing an order instead of a number, but it doesn't really matter now)

#define arch_alloc_xenheap_pages(p, order) ({                   \
    unsigned int cnt_ = 1u << (order);                          \
    while (cnt_--)                                              \
        page_set_xenheap_gfn(&(p)[cnt_], INVALID_GFN); \
})

So yes if !CONFIG_SEPARATE_XENHEAP there will be two loops here.





This could turn out to be quite expensive with large allocation (1GB allocation would require 16MB of cache) because the cache may not have enough space contain all the pages of that range. So you would have to pull twice the page_info in the cache.

This is something we noticed when working on reducing the downtime for Live-Update. We were able to save 100s ms by reducing the number of loop on the page_info.

So I would prefer Oleksandr's approach here.

Cheers,

--
Regards,

Oleksandr Tyshchenko




 


Rackspace

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