[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH qom-cpu 31/59] monitor: Simplify do_info_numa()
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH qom-cpu 31/59] monitor: Simplify do_info_numa() |
Date: |
Tue, 11 Jun 2013 09:41:21 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
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()?
- Re: [Qemu-devel] [PATCH qom-cpu 25/59] cpu: Change qemu_init_vcpu() argument to CPUState, (continued)
[Qemu-devel] [PATCH qom-cpu 27/59] cpu: Turn cpu_unassigned_access() into a CPUState hook, Andreas Färber, 2013/06/09
[Qemu-devel] [PATCH qom-cpu 29/59] cputlb: Simplify cpu_tlb_reset_dirty_all(), Andreas Färber, 2013/06/09
[Qemu-devel] [PATCH qom-cpu 30/59] exec: Simplify tcg_commit(), Andreas Färber, 2013/06/09
[Qemu-devel] [PATCH qom-cpu 31/59] monitor: Simplify do_info_numa(), Andreas Färber, 2013/06/09
[Qemu-devel] [PATCH qom-cpu 28/59] cpu: Replace cpu_single_env with CPUState cpu_single_cpu, Andreas Färber, 2013/06/09
[Qemu-devel] [PATCH qom-cpu 32/59] kvm: Simplify kvm_{insert, remove, remove_all}_breakpoint[s](), Andreas Färber, 2013/06/09
[Qemu-devel] [PATCH qom-cpu 33/59] kvm: Simplify kvm_remove_all_breakpoints() further, Andreas Färber, 2013/06/09
[Qemu-devel] [PATCH qom-cpu 34/59] kvm: Change kvm_remove_all_breakpoints() argument to CPUState, Andreas Färber, 2013/06/09
[Qemu-devel] [PATCH qom-cpu 35/59] linux-user: Simplify start_exclusive(), Andreas Färber, 2013/06/09
[Qemu-devel] [PATCH qom-cpu 36/59] linux-user/elfload: Abstract fill_note_info() with qemu_for_each_cpu(), Andreas Färber, 2013/06/09
[Qemu-devel] [PATCH qom-cpu 38/59] translate-all: Abstract tb_flush() with qemu_for_each_cpu(), Andreas Färber, 2013/06/09
[Qemu-devel] [PATCH qom-cpu 37/59] target-i386: Abstract cpu_x86_inject_mce() with qemu_for_each_cpu(), Andreas Färber, 2013/06/09