[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 1/2] xen/mm: fold PGC_broken into PGC_state bits
- To: David Woodhouse <dwmw2@xxxxxxxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>
- From: Julien Grall <julien@xxxxxxx>
- Date: Fri, 7 Feb 2020 20:27:42 +0000
- Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Ian Jackson <ian.jackson@xxxxxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Jeff Kubascik <jeff.kubascik@xxxxxxxxxxxxxxx>, Stewart Hildebrand <stewart.hildebrand@xxxxxxxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
- Delivery-date: Fri, 07 Feb 2020 20:27:57 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
Hi David,
Could you please send the series in a separate thread? So we don't mix
the two discussions (i.e merge and free on boot allocated page) together.
On 07/02/2020 15:57, David Woodhouse wrote:
From: David Woodhouse <dwmw@xxxxxxxxxxxx>
Only PGC_state_offlining and PGC_state_offlined are valid in conjunction
with PGC_broken. The other two states (free and inuse) were never valid
for a broken page.
By folding PGC_broken in, we can have three bits for PGC_state which
allows up to 8 states, of which 6 are currently used and 2 are available
for new use cases.
The idea looks good to me. I have a few mostly nitpicking comment below.
Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
---
xen/arch/x86/domctl.c | 2 +-
xen/common/page_alloc.c | 67 ++++++++++++++++++++++++----------------
xen/include/asm-arm/mm.h | 26 +++++++++++-----
xen/include/asm-x86/mm.h | 33 +++++++++++++-------
4 files changed, 82 insertions(+), 46 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 4fa9c91140..17a318e16d 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -415,7 +415,7 @@ long arch_do_domctl(
if ( page->u.inuse.type_info & PGT_pinned )
type |= XEN_DOMCTL_PFINFO_LPINTAB;
- if ( page->count_info & PGC_broken )
+ if ( page_is_broken(page) )
type = XEN_DOMCTL_PFINFO_BROKEN;
}
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 97902d42c1..4084503554 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1093,7 +1093,7 @@ static int reserve_offlined_page(struct page_info *head)
struct page_info *pg;
int next_order;
- if ( page_state_is(cur_head, offlined) )
+ if ( page_is_offlined(cur_head) )
{
cur_head++;
if ( first_dirty != INVALID_DIRTY_IDX && first_dirty )
@@ -1113,7 +1113,7 @@ static int reserve_offlined_page(struct page_info *head)
for ( i = (1 << cur_order), pg = cur_head + (1 << cur_order );
i < (1 << next_order);
i++, pg++ )
- if ( page_state_is(pg, offlined) )
+ if ( page_is_offlined(pg) )
break;
if ( i == ( 1 << next_order) )
{
@@ -1145,16 +1145,19 @@ static int reserve_offlined_page(struct page_info *head)
for ( cur_head = head; cur_head < head + ( 1UL << head_order); cur_head++ )
{
- if ( !page_state_is(cur_head, offlined) )
+ struct page_list_head *list;
We tend to add a newline after a series of declaration.
+ if ( page_state_is(cur_head, offlined) )
+ list = &page_offlined_list;
+ else if (page_state_is(cur_head, broken) )
+ list = &page_broken_list;
+ else
continue;
avail[node][zone]--;
total_avail_pages--;
ASSERT(total_avail_pages >= 0);
- page_list_add_tail(cur_head,
- test_bit(_PGC_broken, &cur_head->count_info) ?
- &page_broken_list : &page_offlined_list);
+ page_list_add_tail(cur_head, list);
count++;
}
@@ -1404,13 +1407,16 @@ static void free_heap_pages(
switch ( pg[i].count_info & PGC_state )
{
case PGC_state_inuse:
- BUG_ON(pg[i].count_info & PGC_broken);
pg[i].count_info = PGC_state_free;
break;
case PGC_state_offlining:
- pg[i].count_info = (pg[i].count_info & PGC_broken) |
- PGC_state_offlined;
+ pg[i].count_info = PGC_state_offlined;
+ tainted = 1;
+ break;
+
+ case PGC_state_broken_offlining:
+ pg[i].count_info = PGC_state_broken;
tainted = 1;
break;
@@ -1527,16 +1533,25 @@ static unsigned long mark_page_offline(struct page_info *pg, int broken)
do {
nx = x = y;
- if ( ((x & PGC_state) != PGC_state_offlined) &&
- ((x & PGC_state) != PGC_state_offlining) )
+ nx &= ~PGC_state;
+
+ switch ( x & PGC_state )
{
- nx &= ~PGC_state;
- nx |= (((x & PGC_state) == PGC_state_free)
- ? PGC_state_offlined : PGC_state_offlining);
- }
+ case PGC_state_inuse:
+ case PGC_state_offlining:
+ nx |= broken ? PGC_state_offlining : PGC_state_broken_offlining;
+ break;
+
+ case PGC_state_free:
+ nx |= broken ? PGC_state_broken : PGC_state_offlined;
- if ( broken )
- nx |= PGC_broken;
+ case PGC_state_broken_offlining:
+ nx |= PGC_state_broken_offlining;
+
+ case PGC_state_offlined:
+ case PGC_state_broken:
+ nx |= PGC_state_broken;
Shouldn't this be:
case PGC_state_offlined:
nx |= broken ? PGC_state_offlined : PGC_state_broken;
case PGC_state_broken:
nx |= PGC_state_broken;
There are also quite a difference with the default case now. Without
this patch, if you were to add a new state but not handling it here, you
would transition to PGC_state_offlining. With this patch, we will
transtion to 0 (i.e PGC_state_inuse for now).
PGC_state_* are not an enum, the compiler can't help to catch new state
that doesn't have a corresponding case. So I would suggest to add a
default matching the current behavior and adding an
ASSERT_UNREACHABLE(). Note that I am open to a different kind of
handling here.
+ }
if ( x == nx )
break;
@@ -1609,7 +1624,7 @@ int offline_page(mfn_t mfn, int broken, uint32_t *status)
* need to prevent malicious guest access the broken page again.
* Under such case, hypervisor shutdown guest, preventing recursive mce.
*/
- if ( (pg->count_info & PGC_broken) && (owner = page_get_owner(pg)) )
+ if ( page_is_broken(pg) && (owner = page_get_owner(pg)) )
{
*status = PG_OFFLINE_AGAIN;
domain_crash(owner);
@@ -1620,7 +1635,7 @@ int offline_page(mfn_t mfn, int broken, uint32_t *status)
old_info = mark_page_offline(pg, broken);
- if ( page_state_is(pg, offlined) )
+ if ( page_is_offlined(pg) )
{
reserve_heap_page(pg);
@@ -1699,14 +1714,14 @@ unsigned int online_page(mfn_t mfn, uint32_t *status)
do {
ret = *status = 0;
- if ( y & PGC_broken )
+ if ( (y & PGC_state) == PGC_state_broken ||
+ (y & PGC_state) == PGC_state_broken_offlining )
This is a bit a shame we can't use page_is_broken. Would it make sense
to introduce an helper that just take a count_info?
{
ret = -EINVAL;
*status = PG_ONLINE_FAILED |PG_ONLINE_BROKEN;
break;
}
-
- if ( (y & PGC_state) == PGC_state_offlined )
+ else if ( (y & PGC_state) == PGC_state_offlined )
{
page_list_del(pg, &page_offlined_list);
*status = PG_ONLINE_ONLINED;
@@ -1747,11 +1762,11 @@ int query_page_offline(mfn_t mfn, uint32_t *status)
pg = mfn_to_page(mfn);
- if ( page_state_is(pg, offlining) )
+ if ( page_is_offlining(pg) )
*status |= PG_OFFLINE_STATUS_OFFLINE_PENDING;
- if ( pg->count_info & PGC_broken )
+ if ( page_is_broken(pg) )
*status |= PG_OFFLINE_STATUS_BROKEN;
- if ( page_state_is(pg, offlined) )
+ if ( page_is_offlined(pg) )
*status |= PG_OFFLINE_STATUS_OFFLINED;
spin_unlock(&heap_lock);
@@ -2483,7 +2498,7 @@ __initcall(pagealloc_keyhandler_init);
void scrub_one_page(struct page_info *pg)
{
- if ( unlikely(pg->count_info & PGC_broken) )
+ if ( unlikely(page_is_broken(pg)) )
return;
#ifndef NDEBUG
diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 333efd3a60..c9466c8ca0 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -112,13 +112,25 @@ struct page_info
/* Page is broken? */
#define _PGC_broken PG_shift(7)
#define PGC_broken PG_mask(1, 7)
Shouldn't this be dropped now?
- /* Mutually-exclusive page states: { inuse, offlining, offlined, free }. */
-#define PGC_state PG_mask(3, 9)
-#define PGC_state_inuse PG_mask(0, 9)
-#define PGC_state_offlining PG_mask(1, 9)
-#define PGC_state_offlined PG_mask(2, 9)
-#define PGC_state_free PG_mask(3, 9)
-#define page_state_is(pg, st) (((pg)->count_info&PGC_state) == PGC_state_##st)
+ /*
+ * Mutually-exclusive page states:
+ * { inuse, offlining, offlined, free, broken_offlining, broken }
+ */
+#define PGC_state PG_mask(7, 9)
+#define PGC_state_inuse PG_mask(0, 9)
+#define PGC_state_offlining PG_mask(1, 9)
+#define PGC_state_offlined PG_mask(2, 9)
+#define PGC_state_free PG_mask(3, 9)
+#define PGC_state_broken_offlining PG_mask(4, 9)
+#define PGC_state_broken PG_mask(5, 9)
I agree that making all the value aligned make it nicer to read, but
this is increasing number of "unrelated" changes and makes the review
more difficult.
I would prefer if we leave the indentation alone for the current
#define. But I am not going to push for it :).
+
+#define page_state_is(pg, st) (((pg)->count_info&PGC_state) ==
PGC_state_##st)
+#define page_is_broken(pg) (page_state_is((pg), broken_offlining) || \
+ page_state_is((pg), broken))
+#define page_is_offlined(pg) (page_state_is((pg), broken) || \
+ page_state_is((pg), offlined))
+#define page_is_offlining(pg) (page_state_is((pg), broken_offlining) || \
+ page_state_is((pg), offlining))
/* Count of references to this frame. */
#define PGC_count_width PG_shift(9)
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 2ca8882ad0..3edadf7a7c 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -67,18 +67,27 @@
/* 3-bit PAT/PCD/PWT cache-attribute hint. */
#define PGC_cacheattr_base PG_shift(6)
#define PGC_cacheattr_mask PG_mask(7, 6)
- /* Page is broken? */
-#define _PGC_broken PG_shift(7)
-#define PGC_broken PG_mask(1, 7)
- /* Mutually-exclusive page states: { inuse, offlining, offlined, free }. */
-#define PGC_state PG_mask(3, 9)
-#define PGC_state_inuse PG_mask(0, 9)
-#define PGC_state_offlining PG_mask(1, 9)
-#define PGC_state_offlined PG_mask(2, 9)
-#define PGC_state_free PG_mask(3, 9)
-#define page_state_is(pg, st) (((pg)->count_info&PGC_state) == PGC_state_##st)
-
- /* Count of references to this frame. */
+ /*
+ * Mutually-exclusive page states:
+ * { inuse, offlining, offlined, free, broken_offlining, broken }
+ */
+#define PGC_state PG_mask(7, 9)
+#define PGC_state_inuse PG_mask(0, 9)
+#define PGC_state_offlining PG_mask(1, 9)
+#define PGC_state_offlined PG_mask(2, 9)
+#define PGC_state_free PG_mask(3, 9)
+#define PGC_state_broken_offlining PG_mask(4, 9)
+#define PGC_state_broken PG_mask(5, 9)
+
+#define page_state_is(pg, st) (((pg)->count_info&PGC_state) ==
PGC_state_##st)
+#define page_is_broken(pg) (page_state_is((pg), broken_offlining) || \
+ page_state_is((pg), broken))
+#define page_is_offlined(pg) (page_state_is((pg), broken) || \
+ page_state_is((pg), offlined))
+#define page_is_offlining(pg) (page_state_is((pg), broken_offlining) || \
+ page_state_is((pg), offlining))
+
+/* Count of references to this frame. */
#define PGC_count_width PG_shift(9)
#define PGC_count_mask ((1UL<<PGC_count_width)-1)
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
