Re: [Xen-devel] [PATCH v4 6/8] mm: Keep heap accessible to others while scrubbing

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.05.17 at 17:50, <boris.ostrovsky@xxxxxxxxxx> wrote:
> @@ -1090,24 +1131,51 @@ bool scrub_free_pages(void)
>          do {
>              while ( !page_list_empty(&heap(node, zone, order)) )
>              {
> -                unsigned int i;
> +                unsigned int i, dirty_cnt;
> +                struct scrub_wait_state st;
>  
>                  /* Unscrubbed pages are always at the end of the list. */
>                  pg = page_list_last(&heap(node, zone, order));
>                  if ( pg->u.free.first_dirty == INVALID_DIRTY_IDX )
>                      break;
>  
> +                ASSERT(!pg->u.free.scrub_state);
> +                pg->u.free.scrub_state = PAGE_SCRUBBING;
> +
> +                spin_unlock(&heap_lock);
> +
> +                dirty_cnt = 0;
>                  for ( i = pg->u.free.first_dirty; i < (1U << order); i++)
>                  {
>                      cnt++;
>                      if ( test_bit(_PGC_need_scrub, &pg[i].count_info) )
>                      {
>                          scrub_one_page(&pg[i]);
> +                        /*
> +                         * We can modify count_info without holding heap
> +                         * lock since we effectively locked this buddy by
> +                         * setting its scrub_state.
> +                         */
>                          pg[i].count_info &= ~PGC_need_scrub;
> -                        node_need_scrub[node]--;
> +                        dirty_cnt++;
>                          cnt += 100; /* scrubbed pages add heavier weight. */
>                      }
>  
> +                    if ( pg->u.free.scrub_state & PAGE_SCRUB_ABORT )
> +                    {
> +                        /* Someone wants this chunk. Drop everything. */
> +
> +                        pg->u.free.first_dirty = (i == (1U << order)) ?

Similar like for the earlier patch, this condition is always false
(do to the condition in the loop header) afaict, and ...

> +                            INVALID_DIRTY_IDX : i + 1; 

... you'd again risk storing 1U << order into first_dirty here.

> @@ -1121,6 +1189,17 @@ bool scrub_free_pages(void)
>                      }
>                  }
>  
> +                st.pg = pg;
> +                st.first_dirty = (i == (1UL << order)) ?
> +                    INVALID_DIRTY_IDX : i + 1;

Same here then.

> +                st.drop = false;
> +                spin_lock_cb(&heap_lock, scrub_continue, &st);
> +
> +                node_need_scrub[node] -= dirty_cnt;
> +
> +                if ( st.drop )
> +                    goto out;
> +
>                  if ( i == (1U << order) )
>                  {
>                      page_list_del(pg, &heap(node, zone, order));
> @@ -1128,7 +1207,9 @@ bool scrub_free_pages(void)
>                  }
>                  else
>                      pg->u.free.first_dirty = i + 1;
> - 
> +

Please avoid adding the trailing blank in the first place (in the
earlier patch).

> @@ -1175,6 +1258,8 @@ static void free_heap_pages(
>          if ( page_state_is(&pg[i], offlined) )
>              tainted = 1;
>  
> +        pg[i].u.free.scrub_state = 0;

Is this really needed for every page in the buddy?

Jan


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