>>> Boris Ostrovsky <boris.ostrov...@oracle.com> 08/04/17 7:03 PM >>>
>@@ -873,6 +916,8 @@ static int reserve_offlined_page(struct page_info *head)
>while ( cur_order < head_order )
>+ unsigned int idx = INVALID_DIRTY_IDX;
Is it correct for the variable to live in this scope, rather than ...
>@@ -892,8 +937,28 @@ static int reserve_offlined_page(struct page_info *head)
... in this one? Of course it's less the variable scope itself, but the initial
value at the point here.
>/* We don't consider merging outside the head_order. */
>- page_list_add_tail(cur_head, &heap(node, zone, cur_order));
>- PFN_ORDER(cur_head) = cur_order;
>+ /* See if any of the pages indeed need scrubbing. */
>+ if ( first_dirty != INVALID_DIRTY_IDX )
>+ if ( (1U << cur_order) > first_dirty )
>+ for ( i = first_dirty ; i < (1U << cur_order); i++ )
>+ if ( test_bit(_PGC_need_scrub,
>+ &cur_head[i].count_info) )
>+ idx = i;
Why again do you need to look through all the pages here, rather than
simply marking the chunks as needing scrubbing simply based on first_dirty?
It seems to me that I've asked this before, which is a good indication that
such special behavior would better have a comment attached.
>@@ -977,35 +1096,49 @@ static void free_heap_pages(
>if ( (page_to_mfn(pg) & mask) )
>+ struct page_info *predecessor = pg - mask;
For this and ...
>+ struct page_info *successor = pg + mask;
... this, it would certainly help readability of the patch here if the
of the new local variables was broken out in a prereq patch. But yes, I should
have asked for this earlier on, so I'm not going to insist.
>@@ -88,7 +88,15 @@ struct page_info
>/* Page is on a free list: ((count_info & PGC_count_mask) == 0). */
>/* Do TLBs need flushing for safety before next page use? */
>- bool_t need_tlbflush;
>+ unsigned long need_tlbflush:1;
>+ * Index of the first *possibly* unscrubbed page in the buddy.
>+ * One more than maximum possible order to accommodate
"One more bit than ..." (I think I did point out the ambiguity of the wording
Xen-devel mailing list