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

[Blue Swirl <blauwirbel@gmail.com>]

On Mon, Sep 26, 2011 at 9:45 AM, Avi Kivity <avi@redhat.com> wrote:
> 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.

monitor.c could pass a pointer to a fprintf-like function and a void
*opaque for Monitor *mon.

>>  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.

OK.

>> +
>> +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?

Leftover from previous version. I'll remove the 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.

Right, it looked a bit nicer this way but I'll remove that.

>> +
>> +    } 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.

OK for both.

>> +
>> +    /* 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.

Good idea, that could avoid the forever loop.

>> +        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
>
>