Re: [Xen-devel] [PATCH v4 3/8] mm: Scrub pages in alloc_heap_pages() if needed

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 06/09/2017 11:22 AM, Jan Beulich wrote:
>>>> On 19.05.17 at 17:50, <boris.ostrovsky@xxxxxxxxxx> wrote:
>> @@ -734,8 +735,15 @@ static struct page_info *get_free_buddy(unsigned int 
>> zone_lo,
>>  
>>              /* Find smallest order which can satisfy the request. */
>>              for ( j = order; j <= MAX_ORDER; j++ )
>> +            {
>>                  if ( (pg = page_list_remove_head(&heap(node, zone, j))) )
>> -                    return pg;
>> +                {
>> +                    if ( (order == 0) || use_unscrubbed ||
> Why is order 0 being special cased here? If this really is intended, a
> comment should be added.

That's because for a single page it's not worth skipping a dirty buddy.
(It is a pretty arbitrary number, could be <=1 or even <=2, presumably)

I'll add a comment.


>> @@ -855,10 +870,24 @@ static struct page_info *alloc_heap_pages(
>>      if ( d != NULL )
>>          d->last_alloc_node = node;
>>  
>> +    need_scrub &= !(memflags & MEMF_no_scrub);
> Can't this be done right away when need_scrub is being set?

No, because we use the earlier assignment to decide how we put
"sub-buddies" back to the heap (dirty or not). Here we use need_scrub to
decide whether to scrub the buddy.

This may change though with the changes that you suggested in the
comments to the first patch.

>
>>      for ( i = 0; i < (1 << order); i++ )
>>      {
>>          /* Reference count must continuously be zero for free pages. */
>> -        BUG_ON(pg[i].count_info != PGC_state_free);
>> +        BUG_ON((pg[i].count_info & ~PGC_need_scrub ) != PGC_state_free);
> Isn't this change needed in one of the earlier patches already?

At this patch level we are still scrubbing in free_heap_pages() so there
is never an unscrubbed page in the allocator. The next patch will switch
to scrubbing from idle loop.

> There also is a stray blank ahead of the first closing paren here.
>
>> +        if ( test_bit(_PGC_need_scrub, &pg[i].count_info) )
>> +        {
>> +            if ( need_scrub )
>> +                scrub_one_page(&pg[i]);
>> +            node_need_scrub[node]--;
>> +            /*
>> +             * Technically, we need to set first_dirty to INVALID_DIRTY_IDX
>> +             * on buddy's head. However, since we assign pg[i].count_info
>> +             * below, we can skip this.
>> +             */
> This comment is correct only with the current way struct page_info's
> fields are unionized. In fact I think the comment is unneeded - the
> buddy is being transitioned from free to allocated here, so the field
> loses its meaning.

That, actually, is exactly what I was trying to say. I can drop the
comment if you feel it is obvious why we don't need to set first_dirty.

-boris


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