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

[Oleksandr <olekstysh@xxxxxxxxx>]

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