Re: [Xen-devel] [PATCH v2 7/9] mm: Keep pages available for allocation while scrubbing

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.04.17 at 18:50, <boris.ostrovsky@xxxxxxxxxx> wrote:
> Instead of scrubbing pages while holding heap lock we can mark
> buddy's head as being scrubbed and drop the lock temporarily.
> If someone (most likely alloc_heap_pages()) tries to access
> this chunk it will signal the scrubber to abort scrub by setting
> head's PAGE_SCRUB_ABORT bit. The scrubber checks this bit after
> processing each page and stops its work as soon as it sees it.

So if the scrubber managed to handle all but one page of, say, a
1Gb buddy, you'd re-do all of it synchronously? One more argument
to track dirty/scrubbed state per page instead of per buddy, I think.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -699,6 +699,18 @@ static void page_list_add_scrub(struct page_info *pg, 
> unsigned int node,
>          page_list_add(pg, &heap(node, zone, order));
>  }
>  
> +static void check_and_stop_scrub(struct page_info *head)
> +{
> +    if ( head->u.free.scrub_state & PAGE_SCRUBBING )
> +    {
> +        head->u.free.scrub_state |= PAGE_SCRUB_ABORT;
> +        smp_mb();
> +        spin_lock_kick();

I think the barrier would better be in the kicking construct than
explicit here.

> @@ -785,10 +797,15 @@ static struct page_info *alloc_heap_pages(
>              {
>                  if ( (pg = page_list_remove_head(&heap(node, zone, j))) )
>                  {
> -                    if ( (order == 0) || use_unscrubbed ||
> -                         !test_bit(_PGC_need_scrub, &pg->count_info) )
> +                    if ( !test_bit(_PGC_need_scrub, &pg[0].count_info) )

Any reason to change from -> to [0]. here?

> @@ -1074,12 +1096,34 @@ static unsigned int node_to_scrub(bool_t get_node)
>  }
>  
>  #define SCRUB_CHUNK_ORDER  8
> +
> +struct scrub_wait_state {
> +    struct page_info *pg;
> +    bool_t drop;
> +};
> +
> +static void scrub_continue(void *data)
> +{
> +    struct scrub_wait_state *st = (struct scrub_wait_state *)data;

Pointless cast.

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

Style.

> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -35,6 +35,10 @@ struct page_info
>          } inuse;
>          /* Page is on a free list: ((count_info & PGC_count_mask) == 0). */
>          struct {
> +#define PAGE_SCRUBBING      (1<<1)
> +#define PAGE_SCRUB_ABORT    (1<<2)

Any reason not to start from bit 0? I'm also not sure boolean flags
are the ideal solution here: You really only have three states afaict
(none, scrubbing, abort).

Jan


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