Re: [PATCH 07/12] xen/common: add colored heap info debug-key

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

On Fri, Aug 26, 2022 at 4:13 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:

On 26.08.2022 14:51, Carlo Nonato wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -605,6 +605,27 @@ static struct page_info *alloc_col_domheap_page(struct domain *d,
>      return pg;
>  }
> 
> +static void dump_col_heap(unsigned char key)
> +{
> +    struct page_info *pg;

const and perhaps move into the loop's scope?

> +    unsigned long pages;
> +    unsigned int color;
> +
> +    printk("'%c' pressed -> dumping coloring heap info\n", key);
> +
> +    for ( color = 0; color < get_max_colors(); color++ )
> +    {
> +        printk("Heap[%u]: ", color);
> +        pages = 0;
> +        page_list_for_each( pg, colored_pages(color) )
> +        {
> +            BUG_ON(!(page_to_color(pg) == color));
> +            pages++;
> +        }

This is a very inefficient way for obtaining a count. On a large
system this loop is liable to take excessively long. I'm inclined
to say that even adding a call to process_pending_softirqs() isn't
going to make this work reasonably.

We can definitely add a dynamic array of counters that get updated when

inserting a page in the colored heap so that we don't need to compute

anything here.

I'm also not convinced of having BUG_ON() in keyhandler functions
which are supposed to only dump state.

You're right. I'll remove that. 

> @@ -2853,6 +2874,9 @@ static void cf_check dump_heap(unsigned char key)
>  static __init int cf_check register_heap_trigger(void)
>  {
>      register_keyhandler('H', dump_heap, "dump heap info", 1);
> +#ifdef CONFIG_CACHE_COLORING
> +    register_keyhandler('k', dump_col_heap, "dump coloring heap info", 1);
> +#endif

I question the consuming of a separate key for this purpose: What's
wrong with adding the functionality to dump_heap()?

We didn't want to weigh on that functionality so much, but probably

having a separate key is even worse. If it's not a problem I'll merge

it in the dump_heap() function.

Thanks.

- Carlo Nonato