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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 08/06/2017 01:41 PM, Jan Beulich wrote:
>>>> 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.

I should move it to the inner scope --- I actually *want* to
reinitialize it on each iteration.

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

We want to make sure that there are in fact dirty pages in the
(sub-)buddy: first_dirty is only a hint there *may be* some. That's why
I have the word "indeed" in the comment there but it's probably worth
expanding on that.

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

Since I will be re-spinning this patch I will split this out.

-boris


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