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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> @@ -821,9 +829,16 @@ static struct page_info *alloc_heap_pages(
>      pg = get_free_buddy(zone_lo, zone_hi, order, memflags, d);
>      if ( !pg )
>      {
> -        /* No suitable memory blocks. Fail the request. */
> -        spin_unlock(&heap_lock);
> -        return NULL;
> +        /* Try now getting a dirty buddy. */
> +        if ( !(memflags & MEMF_no_scrub) )
> +            pg = get_free_buddy(zone_lo, zone_hi, order,
> +                                memflags | MEMF_no_scrub, d);
> +        if ( !pg )
> +        {
> +            /* No suitable memory blocks. Fail the request. */
> +            spin_unlock(&heap_lock);
> +            return NULL;
> +        }
>      }

I'd appreciate if you avoided the re-indentation by simply
prefixing another if() to the one that's already there.

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

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

Jan


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