Re: [Qemu-devel] [PATCH qom-cpu 31/59] monitor: Simplify do_info_numa()

[Markus Armbruster]

Andreas Färber <address@hidden> writes:

> Am 10.06.2013 09:20, schrieb Markus Armbruster:
>> Andreas Färber <address@hidden> writes:
>> 
>>> Use new qemu_for_each_cpu().
>>>
>>> Signed-off-by: Andreas Färber <address@hidden>
>>> ---
>>>  monitor.c | 27 +++++++++++++++++++--------
>>>  1 file changed, 19 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/monitor.c b/monitor.c
>>> index 9be515c..f37bf3d 100644
>>> --- a/monitor.c
>>> +++ b/monitor.c
>>> @@ -1803,21 +1803,32 @@ static void do_info_mtree(Monitor *mon, const QDict 
>>> *qdict)
>>>      mtree_info((fprintf_function)monitor_printf, mon);
>>>  }
>>>  
>>> +typedef struct DoInfoNUMAData {
>>> +    Monitor *mon;
>>> +    int numa_node;
>>> +} DoInfoNUMAData;
>>> +
>>> +static void do_info_numa_one(CPUState *cpu, void *data)
>>> +{
>>> +    DoInfoNUMAData *s = data;
>>> +
>>> +    if (cpu->numa_node == s->numa_node) {
>>> +        monitor_printf(s->mon, " %d", cpu->cpu_index);
>>> +    }
>>> +}
>>> +
>>>  static void do_info_numa(Monitor *mon, const QDict *qdict)
>>>  {
>>>      int i;
>>> -    CPUArchState *env;
>>> -    CPUState *cpu;
>>> +    DoInfoNUMAData s = {
>>> +        .mon = mon,
>>> +    };
>>>  
>>>      monitor_printf(mon, "%d nodes\n", nb_numa_nodes);
>>>      for (i = 0; i < nb_numa_nodes; i++) {
>>>          monitor_printf(mon, "node %d cpus:", i);
>>> -        for (env = first_cpu; env != NULL; env = env->next_cpu) {
>>> -            cpu = ENV_GET_CPU(env);
>>> -            if (cpu->numa_node == i) {
>>> -                monitor_printf(mon, " %d", cpu->cpu_index);
>>> -            }
>>> -        }
>>> +        s.numa_node = i;
>>> +        qemu_for_each_cpu(do_info_numa_one, &s);
>>>          monitor_printf(mon, "\n");
>>>          monitor_printf(mon, "node %d size: %" PRId64 " MB\n", i,
>>>              node_mem[i] >> 20);
>> 
>> This again demonstrates the relative clunkiness of higher order
>> functions in C.  Control flow jumps back and forth in the source
>> (lambda, how I miss you), you need an extra type, and you need to go
>> around the type system.
>> 
>> In my experience, loops are a much more natural fit for C.
>> 
>> Would it be possible to have a function cpu_next(CPUState *)?  So you
>> can simply do:
>> 
>>         for (cpu = cpu_next(NULL); cpu; cpu = cpu_next(cpu) {
>>             if (cpu->numa_node == i) {
>>                 monitor_printf(mon, " %d", cpu->cpu_index);
>>             }
>>         }
>> 
>> Simple and type safe.
>> 
>> Precedence: bdrv_next().
>
> I can see your point, but I don't like the suggested alternative.
>
> Are you generally opposed to qemu_for_each_cpu() for iteration?
> http://git.qemu.org/?p=qemu.git;a=commit;h=d6b9e0d60cc511eca210834428bb74508cff3d33
>
> Or is it just the need to pass in more than one value via struct, as
> done in this patch among others, that you dislike? I.e., are you okay
> with patch 03/59 where each loop iteration is independent?

"Dislike" is more accurate than "oppose".

I dislike the notational overhead and weak typing of higher order
functions in C in general.  The additional type merely adds a little to
the notational overhead.

Higher order functions can still be sensible in C when the iterator has
complex state.  For instance, when the iterator is most easily expressed
as recursive function.

> Before this function came up, I had a macro in mind to abstract the
> loop. Then however I thought such a loop would be duplicating the
> qemu/queue.h macros we already have with their _SAFE_ variants, so I was
> hoping to replace the CPU_COMMON next_cpu field with one of those macros
> in CPUState.
>
> Unfortunately at least two places did CPUArchState** pointer magic, so
> that I couldn't just change first_cpu and leave next_cpu for later.

I agree that we better not reinvent queue.h.

> Now, I don't see a huge benefit of doing
> for (cpu = cpu_next(NULL); cpu != NULL; cpu = cpu_next(cpu)) {...}
> over
> for (cpu = first_cpu; cpu != NULL; cpu = cpu->next_cpu) {...}
> with the latter being much clearer to read IMO.

You're right.  The iterator is too trivial to warrant encapsulation.
But making the encapsulation less trivial by using a higher order
function doesn't change that one bit :)

What can change it is a desire to make the type of first_cpu abstract,
because then you can't dereference cpu->next_cpu, or a desire to give
first_cpu internal linkage.  That's what motivated bdrv_next().

> If having patch 57/59 larger is not a concern (which getting rid of
> open-coded CPU loops tried to address), then I can just convert the
> loops in place alongside first_cpu / next_cpu.
> Then still the question remains of whether you'd want to inline and drop
> qemu_for_each_cpu() afterwards, or whether we can establish some rules
> of thumb when to use which?

To be honest, I see no value in qemu_for_each_cpu().

At a glance, PATCH 57 looks fairly mechanical to me.  Is it?  Would it
remain so if refrains from converting simple loops to
qemu_for_each_cpu()?