Re: [Xen-devel] [PATCH] xen/mm.h: add helper function to test-and-clear _PGC_allocated

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <JBeulich@xxxxxxxx>
> Sent: 10 July 2019 23:53
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Julien Grall <julien.grall@xxxxxxx>; 
> Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>; 
> Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Ian 
> Jackson
> <Ian.Jackson@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Konrad 
> Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tamas K Lengyel <tamas@xxxxxxxxxxxxx>; Tim 
> (Xen.org) <tim@xxxxxxx>; Wei Liu
> <wl@xxxxxxx>
> Subject: Re: [Xen-devel] [PATCH] xen/mm.h: add helper function to 
> test-and-clear _PGC_allocated
> 
> On 10.07.2019 18:17, Paul Durrant wrote:
> > @@ -418,13 +417,7 @@ static void hvm_free_ioreq_mfn(struct hvm_ioreq_server 
> > *s, bool buf)
> >       unmap_domain_page_global(iorp->va);
> >       iorp->va = NULL;
> >
> > -    /*
> > -     * Check whether we need to clear the allocation reference before
> > -     * dropping the explicit references taken by get_page_and_type().
> > -     */
> > -    if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
> > -        put_page(page);
> > -
> > +    clear_assignment_reference(page);
> >       put_page_and_type(page);
> >   }
> 
> Is there a specific reason you drop the comment? It doesn't become
> less relevant than when it was added, does it?

Not sure, since what's actually going on is now internal to the function. If I 
change the function name to clear_allocation_reference() then I think the 
comment probably becomes extraneous.

> 
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -658,4 +658,15 @@ static inline void 
> > share_xen_page_with_privileged_guests(
> >       share_xen_page_with_guest(page, dom_xen, flags);
> >   }
> >
> > +static inline void clear_assignment_reference(struct page_info *page)
> 
> I think the function should have 'page' in it's name. Perhaps
> page_deassign() / page_dealloc() are also misleading, but how
> about page_put_alloc() or page_put_alloc_ref()?
> 

Ok, I think page_put_alloc_ref() is most descriptive (particularly w.r.t. the 
above discussion).

> > +{
> > +    /*
> > +     * It is unsafe to clear _PGC_allocated without holding an additional
> > +     * reference.
> > +     */
> > +    ASSERT((page->count_info & PGC_count_mask) > 1);
> 
> While this isn't really in line with our goal of wanting to limit
> damage also in release builds, I agree that there's no really good
> alternative here. Crashing the owner of the page wouldn't help
> much, and bailing from the function wouldn't necessarily be better
> either. Hence I think this would better be BUG_ON().

Ok.

> 
> > +    if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
> > +        put_page(page);
> > +}
> 
> On the whole I have to admit I'm not entirely convinced the "open-
> coding" as you call it (to me it's not really open-coding as long as
> there is no helper) is such a bad thing here: Without the helper it
> is slightly more obvious at the use sites what's actually going on.
> But maybe that's indeed just me.

I still think a helper is better, but I'll add a comment to describe what it is 
doing.

  Paul

> 
> Jan

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