Re: [Xen-devel] [PATCH] xen/mm: Avoid assuming PG_state_inuse == 0 in assign_pages()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.02.2020 14:51, Julien Grall wrote:
> 
> 
> On 04/02/2020 13:40, Jan Beulich wrote:
>> On 04.02.2020 14:33, Julien Grall wrote:
>>> At the moment, assign_pages() relies on PG_state_inuse to be 0. This
>>> makes the code slightly more difficult to understand.
>>
>> I can certainly see where you're coming from, but ...
>>
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -2286,10 +2286,11 @@ int assign_pages(
>>>       for ( i = 0; i < (1 << order); i++ )
>>>       {
>>>           ASSERT(page_get_owner(&pg[i]) == NULL);
>>> -        ASSERT(!pg[i].count_info);
>>> +        ASSERT(page_state_is(&pg[i], inuse));
>>> +        ASSERT(!(pg[i].count_info & (~PGC_state)));
>>
>> ... I think this one is better in its original form. An option
>> might be to put a BUILD_BUG_ON() next to it.
> 
> I want to avoid a BUILD_BUG_ON() if possible. I just realized, I could 
> simplify to "(pg[i].count_info != PGC_state_inuse)".
> 
> Would that be more suitable?

Yes, certainly.

However, isn't the ASSERT() itself wrong? We don't hold the heap lock
here, so mark_page_offline() could transition the page from inuse to
offlining (and possibly also set PGC_broken on it) at any point in
time. This wasn't obvious without the two PGC_inuse uses you add, but
becomes pretty apparent with them. Of course the simple assignment
that you adjust further down then also can't be a simple assignment
anymore.

Since this would move you quite far away from simply a cosmetic
patch (the problem as a whole looks to be wider than just the one
case above), I could understand if you didn't want to fix this at
this occasion. Yet then I think the patch description shouldn't give
the impression that all else is well, but instead at least outline
the issue (for someone else to pick up, possibly me).

>> (In no case, imo,
>> there should be parentheses around a negation expression.)
> 
> While I understand this would be valid, I find it a bit easier to read.

Quite the opposite for me, I'm afraid.

Jan

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