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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.05.17 at 16:53, <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 05/04/2017 06:17 AM, Jan Beulich wrote:
>>>>> On 14.04.17 at 17:37, <boris.ostrovsky@xxxxxxxxxx> 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.

Please do.

>>> +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().

I don't see why a page_list_del() couldn't be paired with a
page_list_add() here. Otherwise, just like it happened to me,
readers may wonder why this more involved function is being
called here.

Jan


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