[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.