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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.02.2020 12:24, Julien Grall wrote:
> Hi Jan,
> 
> On 04/02/2020 15:13, Jan Beulich wrote:
>> 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.
> 
> You are right, assign_pages() could race with mark_page_offline(). We 
> would need to use a cmpxchg() loop to change type. If one of the page is 
> getting offlined, then we would need to revert all the changes and 
> return an error.

I'm not sure we need to go this far. The change of page state
happening behind assign_pages()' back is no different from it
happening after assign_pages() is done. All we need to make sure is
that we don't clobber the state change.

I'm also not sure a cmpxchg loop is needed here. Acquiring and
releasing the heap lock may do, too. You'll find an example of this
elsewhere in the same file, iirc.

Jan

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