| Message ID | 20220826125111.152261-8-carlo.nonato@minervasys.tech (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Arm cache coloring | expand |
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. I'm also not convinced of having BUG_ON() in keyhandler functions which are supposed to only dump state. > @@ -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()? Jan
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
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 4ae3cfe9a7..be6bb2b9a1 100644 --- 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; + 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++; + } + printk("%lu pages\n", pages); + } +} + size_param("buddy-alloc-size", buddy_alloc_size); #else static void free_col_domheap_page(struct page_info *pg) @@ -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 return 0; } __initcall(register_heap_trigger);