Re: [Xen-devel] [PATCH v6 1/8] mm: Place unscrubbed pages at the end of pagelist

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 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)
>{
>merge:

... 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;
>+                                break;
>+                            }

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 ...

>}
>else
>{
>+            struct page_info *successor = pg + mask;

... this, it would certainly help readability of the patch here if the 
introduction
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.

>--- a/xen/include/asm-x86/mm.h
>+++ b/xen/include/asm-x86/mm.h
>@@ -88,7 +88,15 @@ struct page_info
>/* Page is on a free list: ((count_info & PGC_count_mask) == 0). */
>struct {
>/* 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
here before).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel