[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()

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

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.


Julien Grall

Xen-devel mailing list



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