On Fri, Aug 26, 2022 at 4:13 PM Jan Beulich <[email protected]> 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
