Re: [Qemu-devel] [PATCH v2] memory: simple memory tree printer

[Avi Kivity <avi@redhat.com>]

On 09/25/2011 11:19 PM, Blue Swirl wrote:
> Add a monitor command 'info mtree' to show the memory hierarchy
> much like /proc/iomem in Linux.
>
>
> diff --git a/memory.c b/memory.c
> index ba74435..6b33fc4 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -17,6 +17,7 @@
>   #include "bitops.h"
>   #include "kvm.h"
>   #include<assert.h>
> +#include "monitor.h"

This is a unfortunate - now the monitor and memory.c are interdependent; 
this makes it harder to write unit tests (at least without ifdefs).

I guess we can disentangle it later using some kind of generic walker.

>   unsigned memory_region_transaction_depth = 0;
>
> @@ -1256,3 +1257,103 @@ void set_system_io_map(MemoryRegion *mr)
>       address_space_io.root = mr;
>       memory_region_update_topology();
>   }
> +
> +typedef struct MemoryRegionList MemoryRegionList;
> +typedef struct MemoryRegionListHead MemoryRegionListHead;
> +
> +struct MemoryRegionList {
> +    const MemoryRegion *mr;
> +    QLIST_ENTRY(MemoryRegionList) queue;
> +};
> +
> +struct MemoryRegionListHead {
> +    QLIST_HEAD(queue, MemoryRegionList) head;
> +};

Straight typedef of QLIST_HEAD(queue, MemoryRegionList) would be nicer.

> +
> +static void mtree_print_mr(Monitor *mon, const MemoryRegion *mr,
> +                           unsigned int level, target_phys_addr_t base,
> +                           MemoryRegionListHead *print_queue)
> +{
> +    const MemoryRegion *submr;
> +    unsigned int i;
> +
> +    for (i = 0; i<  level; i++) {
> +        monitor_printf(mon, "  ");
> +    }
> +
> +    if (mr->alias) {
> +        if (print_queue) {

print_queue is never NULL, why test?


> +            MemoryRegionList *ml;
> +            bool found = false;
> +
> +            /* check if the alias is already in the queue */
> +            QLIST_FOREACH(ml,&print_queue->head, queue) {
> +                if (ml->mr == mr->alias) {
> +                    found = true;
> +                }
> +            }
> +
> +            if (!found) {
> +                ml = g_malloc(sizeof(*ml));
> +                ml->mr = mr->alias;
> +                QLIST_INSERT_HEAD(&print_queue->head, ml, queue);
> +            }
> +        }
> +        monitor_printf(mon, TARGET_FMT_plx "-" TARGET_FMT_plx " :
> alias %s @%s "
> +                       TARGET_FMT_plx "-" TARGET_FMT_plx "\n",
> +                       base + mr->addr,
> +                       base + mr->addr + (target_phys_addr_t)mr->size - 1,
> +                       mr->name,
> +                       mr->alias->name,
> +                       mr->alias_offset + mr->alias->addr,
> +                       mr->alias_offset  + mr->alias->addr +
> +                       (target_phys_addr_t)mr->size - 1);

Adding mr->alias->addr doesn't help much - it doesn't give you an 
absolute address, just relative to the alias target's container.  If 
it's deep enough in the tree the address is meaningless.

> +
> +    } else {
> +        monitor_printf(mon, TARGET_FMT_plx "-" TARGET_FMT_plx " : %s\n",
> +                       base + mr->addr,
> +                       base + mr->addr + (target_phys_addr_t)mr->size - 1,
> +                       mr->name);
> +    }
> +    QTAILQ_FOREACH(submr,&mr->subregions, subregions_link) {
> +        mtree_print_mr(mon, submr, level + 1, base + mr->addr, print_queue);
> +    }
> +}
> +
> +void mtree_info(Monitor *mon)
> +{
> +    MemoryRegionListHead *ml_head, *ml_head2, *ml_tmp;
> +    MemoryRegionList *ml, *ml2;
> +
> +    ml_head = g_malloc(sizeof(*ml));

Wrong type.  g_new() is much better for this.

> +    QLIST_INIT(&ml_head->head);
> +
> +    monitor_printf(mon, "memory\n");
> +    mtree_print_mr(mon, address_space_memory.root, 0, 0, ml_head);
> +
> +    ml_head2 = g_malloc(sizeof(*ml));

Again.

> +
> +    /* print aliased regions */
> +    for (;;) {
> +        QLIST_INIT(&ml_head2->head);
> +
> +        QLIST_FOREACH_SAFE(ml,&ml_head->head, queue, ml2) {
> +            monitor_printf(mon, "%s\n", ml->mr->name);
> +            mtree_print_mr(mon, ml->mr, 0, 0, ml_head2);
> +            g_free(ml);
> +        }

I think you can eliminate more duplicates by adding a bool ->printed to 
the queue entry, and always using the same queue.  Iterate until no un 
->printed elements remain.

> +        if (QLIST_EMPTY(&ml_head->head)) {
> +            break;
> +        }
> +        ml_tmp = ml_head;
> +        ml_head = ml_head2;
> +        ml_head2 = ml_tmp;
> +    }
> +
> +#ifdef TARGET_I386
> +    monitor_printf(mon, "I/O\n");
> +    mtree_print_mr(mon, address_space_io.root, 0, 0, ml_head);
> +#endif
> +    g_free(ml_head2);
> +    g_free(ml_head);
> +}
> diff --git a/memory.h b/memory.h
> index 06b83ae..09d8e29 100644
> --- a/memory.h
> +++ b/memory.h
> @@ -500,6 +500,8 @@ void memory_region_transaction_begin(void);
>    */
>   void memory_region_transaction_commit(void);
>
> +void mtree_info(Monitor *mon);
> +
>   #endif
>

-- 
error compiling committee.c: too many arguments to function