Re: [Xen-devel] [PATCH v7 2/3] mm: make pages allocated with MEMF_no_refcount safe to assign

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.01.2020 18:10, Paul Durrant wrote:
> NOTE: steal_page() is also modified to decrement extra_pages in the case of
>       a PGC_extra page being stolen from a domain.

I don't think stealing of such pages should be allowed. If anything,
the replacement page then again should be an "extra" one, which I
guess would be quite ugly to arrange for. But such "extra" pages
aren't supposed to be properly exposed (and hence played with) to
the domain in the first place.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2256,6 +2256,7 @@ int assign_pages(
>  {
>      int rc = 0;
>      unsigned long i;
> +    unsigned int extra_pages = 0;
>  
>      spin_lock(&d->page_alloc_lock);
>  
> @@ -2267,13 +2268,19 @@ int assign_pages(
>          goto out;
>      }
>  
> +    for ( i = 0; i < (1 << order); i++ )
> +        if ( pg[i].count_info & PGC_extra )
> +            extra_pages++;

Perhaps assume (and maybe ASSERT()) that all pages in the batch
are the same in this regard? Then you could ...

>      if ( !(memflags & MEMF_no_refcount) )
>      {
> -        if ( unlikely((d->tot_pages + (1 << order)) > d->max_pages) )
> +        unsigned int max_pages = d->max_pages - d->extra_pages - extra_pages;
> +
> +        if ( unlikely((d->tot_pages + (1 << order)) > max_pages) )
>          {
>              gprintk(XENLOG_INFO, "Over-allocation for domain %u: "
>                      "%u > %u\n", d->domain_id,
> -                    d->tot_pages + (1 << order), d->max_pages);
> +                    d->tot_pages + (1 << order), max_pages);
>              rc = -E2BIG;
>              goto out;
>          }
> @@ -2282,13 +2289,17 @@ int assign_pages(
>              get_knownalive_domain(d);
>      }
>  
> +    d->extra_pages += extra_pages;

... arrange things like this, I think:

    if ( pg[i].count_info & PGC_extra )
        d->extra_pages += 1U << order;
    else if ( !(memflags & MEMF_no_refcount) )
    {
        unsigned int max_pages = d->max_pages - d->extra_pages;
        ...

This would, afaict, then also eliminate the need to mask off
MEMF_no_refcount in alloc_domheap_pages(), ...


>      for ( i = 0; i < (1 << order); i++ )
>      {
> +        unsigned long count_info = pg[i].count_info;
> +
>          ASSERT(page_get_owner(&pg[i]) == NULL);
> -        ASSERT(!pg[i].count_info);
> +        ASSERT(!(count_info & ~PGC_extra));

... resulting in my prior comment on this one still applying.

Besides the changes you've made, what about the code handling
XENMEM_set_pod_target? What about p2m-pod.c? And
pv_shim_setup_dom()? I'm also not fully sure whether
getdomaininfo() shouldn't subtract extra_pages, but I think
this is the only way to avoid having an externally visible
effect. There may be more. Perhaps it's best to introduce a
domain_tot_pages() inline function returning the difference,
and us it almost everywhere where ->tot_pages is used right
now.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel