Re: [Xen-devel] [PATCH] mm/page_alloc: add bootscrub=idle cmdline option

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.10.18 at 12:28, <sergey.dyasli@xxxxxxxxxx> wrote:
> Scrubbing RAM during boot may take a long time on machines with lots
> of RAM. Add 'idle' option which marks all pages dirty initially so they
> would eventually be scrubbed in idle-loop on every online CPU.
> 
> Performance of idle-loop scrubbing is worse than bootscrub but it's
> guaranteed that the allocator will return scrubbed pages by doing
> eager scrubbing during allocation (unless MEMF_no_scrub was provided).

I've commented on this before: Without clarifying what "performance"
means in this context, the statement's truth cannot be verified. To
certain (many?) people, performance in the context of booting a system
may mean the time it takes until the system is fully up. I think idle
scrubbing helps this rather than making it worse.

> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -227,7 +227,7 @@ that byte `0x12345678` is bad, you would place 
> `badpage=0x12345` on
>  Xen's command line.
>  
>  ### bootscrub
> -> `= <boolean>`
> +> `= <boolean> | idle`
>  
>  > Default: `true`

Any reason not to make "idle" the default? Iirc that was the plan anyway.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -161,8 +161,32 @@ string_param("badpage", opt_badpage);
>  /*
>   * no-bootscrub -> Free pages are not zeroed during boot.
>   */
> -static bool_t opt_bootscrub __initdata = 1;
> -boolean_param("bootscrub", opt_bootscrub);
> +enum {
> +    BOOTSCRUB_OFF = 0,
> +    BOOTSCRUB_ON,
> +    BOOTSCRUB_IDLE,
> +};
> +static int __read_mostly opt_bootscrub = BOOTSCRUB_ON;

Why "int" when you've just made yourself an enum?

> +static int __init parse_bootscrub_param(const char *s)
> +{
> +    if ( *s == '\0' )
> +        return 0;
> +
> +    if ( !strcmp(s, "idle") )
> +        opt_bootscrub = BOOTSCRUB_IDLE;
> +    else
> +        opt_bootscrub = parse_bool(s, NULL);
> +
> +    if ( opt_bootscrub < 0 )

Can you please follow the model used elsewhere, where parse_bool()
gets called first? Also can "bootscrub" alone please have the same
meaning as "bootscrub=yes"?

> +    {
> +        opt_bootscrub = BOOTSCRUB_ON;
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +custom_param("bootscrub", parse_bootscrub_param);

No blank line between function and its own custom_param()
please.

> @@ -1763,7 +1787,8 @@ static void init_heap_pages(
>              nr_pages -= n;
>          }
>  
> -        free_heap_pages(pg + i, 0, scrub_debug);
> +        free_heap_pages(pg + i, 0, scrub_debug ||
> +                                   opt_bootscrub == BOOTSCRUB_IDLE);

So this is the hunk explaining why the variable can't be __initdata.
The question though is whether the resulting post-init behavior
is actually correct; it's certainly odd for a boot related option to
have an effect post-boot as well.

> @@ -2039,8 +2064,11 @@ void __init heap_init_late(void)
>       */
>      setup_low_mem_virq();
>  
> -    if ( opt_bootscrub )
> +    if ( opt_bootscrub == BOOTSCRUB_ON )
>          scrub_heap_pages();
> +    else if ( opt_bootscrub == BOOTSCRUB_IDLE )
> +        printk("Scrubbing Free RAM on %d nodes in background\n",
> +               num_online_nodes());

switch()? And is the reference to the node count really meaningful
in the logged message?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel