Re: [Xen-devel] [PATCH v2] mm: disallow MEMF_no_refcount to be passed for domain-owned allocations

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.11.18 at 11:28, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 23/11/2018 09:45, Jan Beulich wrote:
>> When such pages get assigned to domains (and hence their ->tot_pages
>> not incremented accordingly) we would otherwise also need to suppress
>> decrementing the count when freeing those pages.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> v2: Add ASSERT_UNREACHABLE().
>>
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -2303,6 +2303,11 @@ struct page_info *alloc_domheap_pages(
>>  
>>      if ( memflags & MEMF_no_owner )
>>          memflags |= MEMF_no_refcount;
>> +    else if ( (memflags & MEMF_no_refcount) && d )
>> +    {
>> +        ASSERT_UNREACHABLE();
> 
> Sorry to do this, but on second thoughts, this path isn't actually
> unreachable.
> 
> Could I talk you into using ASSERT(!"Assigned domheap pages must be
> refcounted") instead, to give a slightly more clear error to developers
> who manage to hit it?

I think there are other places where we use ASSERT_UNREACHABLE()
when the path is reachable in the sense that one could construct a
suitable path, but with how things are (supposed to be) it cannot be
reached. I'm unconvinced the added string literal would be of overly
much help - once you see the line number, it is pretty easy to figure
out whats wrong.

But if you insist, I'll switch to the alternative way of expressing it
(although I'd then perhaps use
ASSERT(!(memflags & MEMF_no_refcount)) instead). Just let me
know.

Jan



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