Re: [Xen-devel] [PATCH v4 5/7] mm: make MEMF_no_refcount pages safe to assign

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

> -----Original Message-----
[snip]
> > > @@ -2331,11 +2331,20 @@ struct page_info *alloc_domheap_pages(
> > >                                    memflags, d)) == NULL)) )
> > >           return NULL;
> > >
> > > -    if ( d && !(memflags & MEMF_no_owner) &&
> > > -         assign_pages(d, pg, order, memflags) )
> > > +    if ( d && !(memflags & MEMF_no_owner) )
> > >      {
> > > -        free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> > > -        return NULL;
> > > +        if ( assign_pages(d, pg, order, memflags) )
> > > +        {
> > > +            free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> > > +            return NULL;
> > > +        }
> > > +        if ( memflags & MEMF_no_refcount )
> > > +        {
> > > +            unsigned long i;
> > > +
> > > +            for ( i = 0; i < (1 << order); i++ )
> > > +                pg[i].count_info |= PGC_no_refcount;
> > > +        }
> >
> > I would seem to me that this needs doing the other way around:
> > First set PGC_no_refcount, then assign_pages(). After all, the
> > moment assign_pages() drops its lock, the domain could also
> > decide to get rid of (some of) the pages again.
> 
> True. Yes, this needs to be swapped.
> 
> > For this (and
> > also to slightly simplify things in free_domheap_pages())
> > perhaps it would be better not to add that ASSERT() to
> > free_heap_pages(). The function shouldn't really be concerned
> > of any refcounting, and hence could as well be ignorant to
> > PGC_no_refcount being set on a page.
> >
> 
> Not sure I understand here. What would you like to see free_heap_pages()
> assert?
> 

Oh, I misread. You want me to remove the ASSERT that I added... that's fine.

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