On Fri, Aug 26, 2022 at 4:13 PM Jan Beulich <jbeulich@suse.com> 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