Re: [PATCH v3] memory: Have 'info mtree' remove duplicated Address Space information

[David Hildenbrand <david@redhat.com>]

On 23.08.21 10:54, Philippe Mathieu-Daudé wrote:
> Per Peter Maydell [*]:
> 
>    'info mtree' monitor command was designed on the assumption that
>    there's really only one or two interesting address spaces, and
>    with more recent developments that's just not the case any more.
> 
> Similarly about how the FlatView are sorted using a GHashTable,
> sort the AddressSpace objects to remove the duplications (AS
> using the same root MemoryRegion).
> 
> This drastically reduce 'info mtree' on some boards.

s/reduce/reduces the output of/

> 
> Before:
> 
>    $ (echo info mtree; echo q) \
>      | qemu-system-aarch64 -S -monitor stdio -M raspi3b \
>      | wc -l
>    423
> 
> After:
> 
>    $ (echo info mtree; echo q) \
>      | qemu-system-aarch64 -S -monitor stdio -M raspi3b \
>      | wc -l
>    106
> 
>    (qemu) info mtree
>    address-space: I/O
>      0000000000000000-000000000000ffff (prio 0, i/o): io
> 
>    address-space: cpu-memory-0
>    address-space: cpu-memory-1
>    address-space: cpu-memory-2
>    address-space: cpu-memory-3
>    address-space: cpu-secure-memory-0
>    address-space: cpu-secure-memory-1
>    address-space: cpu-secure-memory-2
>    address-space: cpu-secure-memory-3

We can still distinguish from a completely empty AS, because we don't 
have an empty line here, correct?

[...]

> ---
>   softmmu/memory.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfedaf9c4df..459d6246672 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -3246,11 +3246,55 @@ static gboolean mtree_info_flatview_free(gpointer key, gpointer value,
>       return true;
>   }
>   
> +struct AddressSpaceInfo {
> +    MemoryRegionListHead *ml_head;
> +    bool owner;
> +    bool disabled;
> +};
> +
> +/* Returns negative value if a < b; zero if a = b; positive value if a > b. */
> +static gint address_space_compare_name(gconstpointer a, gconstpointer b)
> +{
> +    const AddressSpace *as_a = a;
> +    const AddressSpace *as_b = b;
> +
> +    return g_strcmp0(as_a->name, as_b->name);
> +}
> +static void mtree_print_as_name(gpointer data, gpointer user_data)
> +{
> +    AddressSpace *as = data;
> +
> +    qemu_printf("address-space: %s\n", as->name);
> +}
> +
> +static void mtree_print_as(gpointer key, gpointer value, gpointer user_data)
> +{
> +    MemoryRegion *mr = key;
> +    GSList *as_same_root_mr_list = value;
> +    struct AddressSpaceInfo *asi = user_data;

Reverse Christmas tree?

> +
> +    g_slist_foreach(as_same_root_mr_list, mtree_print_as_name, NULL);
> +    mtree_print_mr(mr, 1, 0, asi->ml_head, asi->owner, asi->disabled);
> +    qemu_printf("\n");
> +}
> +
> +static gboolean mtree_info_as_free(gpointer key, gpointer value,
> +                                   gpointer user_data)
> +{
> +    GSList *as_same_root_mr_list = value;
> +
> +    g_slist_free(as_same_root_mr_list);
> +
> +    return true;
> +}
> +
>   void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
>   {
>       MemoryRegionListHead ml_head;
>       MemoryRegionList *ml, *ml2;
>       AddressSpace *as;
> +    GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
> +    GSList *as_same_root_mr_list;

Reverse Christmas tree ?

>   
>       if (flatview) {
>           FlatView *view;
> @@ -3260,7 +3304,6 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
>               .owner = owner,
>           };
>           GArray *fv_address_spaces;
> -        GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
>           AccelClass *ac = ACCEL_GET_CLASS(current_accel());
>   
>           if (ac->has_memory) {
> @@ -3293,11 +3336,24 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
>       QTAILQ_INIT(&ml_head);
>   
>       QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> -        qemu_printf("address-space: %s\n", as->name);
> -        mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled);
> -        qemu_printf("\n");
> +        /* Create hashtable, key=AS root MR, value = list of AS */
> +        as_same_root_mr_list = g_hash_table_lookup(views, as->root);
> +        as_same_root_mr_list = g_slist_insert_sorted(as_same_root_mr_list, as,
> +                                                     address_space_compare_name);
> +        g_hash_table_insert(views, as->root, as_same_root_mr_list);
>       }
>   
> +    struct AddressSpaceInfo asi = {
> +        .ml_head = &ml_head,
> +        .owner = owner,
> +        .disabled = disabled,
> +    };
> +
> +    /* print address spaces */
> +    g_hash_table_foreach(views, mtree_print_as, &asi);
> +    g_hash_table_foreach_remove(views, mtree_info_as_free, 0);
> +    g_hash_table_unref(views);
> +
>       /* print aliased regions */
>       QTAILQ_FOREACH(ml, &ml_head, mrqueue) {
>           qemu_printf("memory-region: %s\n", memory_region_name(ml->mr));
> 

LGTM

-- 
Thanks,

David / dhildenb