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

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 30 January 2020 10:20
> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxxxxx>;
> Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; Julien Grall <julien@xxxxxxx>;
> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH v7 2/3] mm: make pages allocated with MEMF_no_refcount
> safe to assign
> 
> 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.

This is getting very very complicated now, which makes me think that my 
original approach using a 'normal' page and setting an initial max_pages in 
domain_create() was a better approach.

  Paul



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