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

[Julien Grall <julien@xxxxxxx>]

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.
> 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).
I am happy to have a look at it.

Cheers,

--
Julien Grall

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