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

[Philippe Mathieu-Daudé]

On 8/31/21 10:27 PM, Peter Maydell wrote:
> On Mon, 23 Aug 2021 at 09:54, Philippe Mathieu-Daudé <philmd@redhat.com> 
> 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).
>>
> 
> 
>> 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;
>> +
>> +    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;
>>
>>      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,
>> +    };
> 
> Strictly speaking this is against our coding-style "no variable
> declarations in the middle of a block".

Right.

>> +
>> +    /* 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));
>> --
>> 2.31.1
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Side note: I wonder if it would be worth splitting mtree_info()
> into a function for ASes and a function for flatviews? The two
> cases share basically no code, and there's only one callsite.

I noticed that too but wanted to get the dedup reviewed first.
I'll respin including the split (which should remove the mid-block
declaration).

Thanks for the review,

Phil.