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

[Andreas Färber]

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?

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.

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.

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?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg