Re: [PATCH v3 2/9] mm: Place unscrubbed pages at the end of pagelist

From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: tim@xen.org, sstabellini@kernel.org, wei.liu2@citrix.com,
	George.Dunlap@eu.citrix.com, andrew.cooper3@citrix.com,
	ian.jackson@eu.citrix.com, xen-devel@lists.xen.org
Subject: Re: [PATCH v3 2/9] mm: Place unscrubbed pages at the end of pagelist
Date: Thu, 4 May 2017 10:53:06 -0400	[thread overview]
Message-ID: <bebc27f6-770c-8e11-dfa7-8a44f494f184@oracle.com> (raw)
In-Reply-To: <590B1BD10200007800156B79@prv-mh.provo.novell.com>

On 05/04/2017 06:17 AM, Jan Beulich wrote:
>>>> On 14.04.17 at 17:37, <boris.ostrovsky@oracle.com> wrote:
>> @@ -678,6 +680,20 @@ static void check_low_mem_virq(void)
>>      }
>>  }
>>  
>> +/* Pages that need scrub are added to tail, otherwise to head. */
>> +static void page_list_add_scrub(struct page_info *pg, unsigned int node,
>> +                                unsigned int zone, unsigned int order,
>> +                                bool need_scrub)
>> +{
>> +    PFN_ORDER(pg) = order;
>> +    pg->u.free.dirty_head = need_scrub;
>> +
>> +    if ( need_scrub )
>> +        page_list_add_tail(pg, &heap(node, zone, order));
>> +    else
>> +        page_list_add(pg, &heap(node, zone, order));
>> +}
>> +
>>  /* Allocate 2^@order contiguous pages. */
>>  static struct page_info *alloc_heap_pages(
>>      unsigned int zone_lo, unsigned int zone_hi,
>> @@ -802,7 +818,7 @@ static struct page_info *alloc_heap_pages(
>>      while ( j != order )
>>      {
>>          PFN_ORDER(pg) = --j;
>> -        page_list_add_tail(pg, &heap(node, zone, j));
>> +        page_list_add(pg, &heap(node, zone, j));
> Don't you need to replicate pg->u.free.dirty_head (and hence use
> page_list_add_scrub()) here too?

I don't need to because we are still scrubbing from free_heap_pages()
and not from idle loop, thus we never have dirty pages in the heap. Next
patch will, in fact, start using page_list_add_scrub() here.

However, I can switch to it here for consistency.

>
>> +static void scrub_free_pages(unsigned int node)
>> +{
>> +    struct page_info *pg;
>> +    unsigned int zone, order;
>> +    unsigned long i;
> Here I would similarly appreciate if the local variables were moved
> into the scopes they're actually needed in.
>
>> +    ASSERT(spin_is_locked(&heap_lock));
>> +
>> +    if ( !node_need_scrub[node] )
>> +        return;
>> +
>> +    for ( zone = 0; zone < NR_ZONES; zone++ )
>> +    {
>> +        order = MAX_ORDER;
>> +        do {
>> +            while ( !page_list_empty(&heap(node, zone, order)) )
>> +            {
>> +                /* Unscrubbed pages are always at the end of the list. */
>> +                pg = page_list_last(&heap(node, zone, order));
>> +                if ( !pg->u.free.dirty_head )
>> +                    break;
>> +
>> +                for ( i = 0; i < (1UL << order); i++)
>> +                {
>> +                    if ( test_bit(_PGC_need_scrub, &pg[i].count_info) )
>> +                    {
>> +                        scrub_one_page(&pg[i]);
>> +                        pg[i].count_info &= ~PGC_need_scrub;
>> +                        node_need_scrub[node]--;
>> +                    }
>> +                }
>> +
>> +                page_list_del(pg, &heap(node, zone, order));
>> +                merge_and_free_buddy(pg, node, zone, order, false);
> Is there actually any merging involved here, i.e. can't you
> simply put back the buddy at the list head?

Yes, I can. I just wanted pages to enter heap only via
merge_and_free_buddy().

-boris

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