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

[Jan Beulich <JBeulich@xxxxxxxx>]

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?

> --- 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()?

> +{
> +    /*
> +     * 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().

> +    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.

Jan

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