remove vcpu_info array v5

So here we go, trying to catch up on the PCI passthrough patch revision number, here's v5 of the struct vcpu_info patch.

In the end I decided to merge the contents of struct vcpu_info directly into CPU_COMMON as it was silly to create new files just to remove them in the next patch again.

This boots for me on ia64 and builds on x86_64, obviously it is 100% perfect <tm>!

Comments, bug reports, or upstream merge, welcome :-) Merge vcpu_info into CPUState.

Move contents of struct vcpu_info directly into CPU_COMMON. Rename struct qemu_kvm_work_item to qemu_work_item as it really isn't KVM specific.

#define CPU_TEMP_BUF_NLONGS 128

 #define CPU_COMMON                                                      \
     struct TranslationBlock *current_tb; /* currently executing TB  */  \

qemu/cpu-defs.h +1 -1

@@ -200,6 +204,15 @@
     /* user data */                                                     \
     void *opaque;                                                       \
                                                                         \
-    const char *cpu_model_str;
+    const char *cpu_model_str;                                          \

qemu/hw/acpi.c +6 -0

 #if defined(TARGET_I386) || defined(TARGET_X86_64)
+#ifdef USE_KVM
+static CPUState *qemu_kvm_cpu_env(int index)
+{
+    CPUState *penv;
+
+    penv = first_cpu;

Quoted 11 lines · Jes Sorensen

So here we go, trying to catch up on the PCI passthrough patch revision number, here's v5 of the struct vcpu_info patch.

In the end I decided to merge the contents of struct vcpu_info directly into CPU_COMMON as it was silly to create new files just to remove them in the next patch again.

This boots for me on ia64 and builds on x86_64, obviously it is 100% perfect <tm>!

Quoted 8 lines

Comments, bug reports, or upstream merge, welcome :-) Merge vcpu_info into CPUState.

Move contents of struct vcpu_info directly into CPU_COMMON. Rename struct qemu_kvm_work_item to qemu_work_item as it really isn't KVM specific.

Well, it is actually kvm specific, since qemu doesn't have vccpu threads.

libkvm/libkvm.c line 875

Quoted 3 lines

libkvm/libkvm.c +1 -1

-int kvm_run(kvm_context_t kvm, int vcpu)
+int kvm_run(kvm_context_t kvm, int vcpu, void *env)

That's a little ugly -- passing two arguments which describe the vcpu. How about passing env to kvm_create_vcpu() instead?

Quoted 9 lines

#define CPU_TEMP_BUF_NLONGS 128

 #define CPU_COMMON                                                      \
    struct TranslationBlock *current_tb; /* currently executing TB  */  \

qemu/cpu-defs.h +1 -1

@@ -200,6 +204,15 @@
    /* user data */                                                     \
    void *opaque;                                                       \
                                                                        \
-    const char *cpu_model_str;
+    const char *cpu_model_str;                                          \

qemu/cpu-defs.h line 217

Quoted 10 lines

qemu/cpu-defs.h +9 -0

+                                                                        \
+    int sipi_needed;                                                    \
+    int init;                                                           \
+    pthread_t thread;                                                   \
+    int signalled;                                                      \
+    int stop;                                                           \
+    int stopped;                                                        \
+    int created;                                                        \
+    struct qemu_work_item *queued_work_first, *queued_work_last;
 

struct kvm_stuff { ... } and put that in as a member, so it's easier to remember this is a kvm addition.

Quoted 12 lines

qemu/hw/acpi.c +11 -0

  #if defined(TARGET_I386) || defined(TARGET_X86_64)
+#ifdef USE_KVM
+static CPUState *qemu_kvm_cpu_env(int index)
+{
+    CPUState *penv;
+
+    penv = first_cpu;
+
+    while (penv) {
+        if (penv->cpu_index = index)
+            return penv;
+        penv = (CPUState *)penv->next_cpu;

qemu/hw/acpi.c line 740

Quoted 6 lines

qemu/hw/acpi.c +5 -0

+    }
+
+    return NULL;
+}
+#endif

This can't be the best way of determining the env pointer from a cpu number.

qemu/qemu-common.h line 148

Quoted 7 lines

qemu/qemu-common.h +2 -0

@@ -143,4 +143,12 @@
 void cpu_save(QEMUFile *f, void *opaque);
 int cpu_load(QEMUFile *f, void *opaque, int version_id);
 +/* work queue */
+struct qemu_work_item {
+    struct qemu_kvm_work_item *next;
 

Missed rename here.

qemu/qemu-common.h line 153

Quoted 6 lines

qemu/qemu-common.h +5 -0

+    void (*func)(void *data);
+    void *data;
+    int done;
+};
+

Please keep this in kvm specific files.

qemu/qemu-kvm.c line 31

Quoted 5 lines

qemu/qemu-kvm.c +0 -0

@@ -28,7 +28,6 @@
 #include <sys/syscall.h>
 #include <sys/mman.h>
 -#define bool _Bool
 

why?

Please separate into a patch that does the kvm_run int vcpu -> void *vcpu conversion, and then the vcpu_info -> env conversion.

--
error compiling committee.c: too many arguments to function

Quoted 2 lines

This can't be the best way of determining the env pointer from a cpu number.

By far it is not. But at least it does not depend on any kvm-specific data, and works
for both qemu and kvm. So it's better to be this way.

If we change this scheme to a better one, like a hash table, then it'll improve both qemu and kvm.

Quoted 9 lines · Glauber Costa

Quoted 4 lines

This can't be the best way of determining the env pointer from a cpu number.

By far it is not. But at least it does not depend on any kvm-specific data, and works
for both qemu and kvm. So it's better to be this way.

Quoted 4 lines

If we change this scheme to a better one, like a hash table, then it'll improve both qemu and kvm.

There are simpler data structures. An example is an array. Since we have an upper limit on the number of cpus (->max_cpus) there are no issues with scaling (unlike with the algorithm above).

Quoted 4 lines

There are simpler data structures. An example is an array. Since we have an upper limit on the number of cpus (->max_cpus) there are no issues with scaling (unlike with the algorithm above).

Yes, but I believe the whole point of jes patches is to remove those limits.

Quoted 11 lines · Glauber Costa

Quoted 7 lines

There are simpler data structures. An example is an array. Since we have
an upper limit on the number of cpus (->max_cpus) there are no issues with
scaling (unlike with the algorithm above).

Yes, but I believe the whole point of jes patches is to remove those limits.

So we could linear search huge lists? Or code hashes where arrays would do?
No, sir.

An array of pointers statically sized at MAX_CPUS is fine. If you insist, you can realloc() it on demand.

--
error compiling committee.c: too many arguments to function

Quoted 7 lines

Quoted 4 lines

Yes, but I believe the whole point of jes patches is to remove those limits.

So we could linear search huge lists? Or code hashes where arrays would do?
No, sir.

Quoted 3 lines

An array of pointers statically sized at MAX_CPUS is fine. If you insist, you can realloc() it on demand.

Yes, I agree. having MAX_CPUS is fine, and if we need to, we can increase that. but again: then the direction of the flow is better be have qemu to use a env array,
and then we can use the same method for kvm.

Quoted 6 lines · Glauber Costa

By far it is not. But at least it does not depend on any kvm-specific data, and works
for both qemu and kvm. So it's better to be this way.

If we change this scheme to a better one, like a hash table, then it'll improve both qemu and kvm.

There are simpler data structures. An example is an array. Since we have an upper limit on the number of cpus (->max_cpus) there are no issues with scaling (unlike with the algorithm above).

Quoted 3 lines · Jes Sorensen

Move contents of struct vcpu_info directly into CPU_COMMON. Rename struct qemu_kvm_work_item to qemu_work_item as it really isn't KVM specific.

Well, it is actually kvm specific, since qemu doesn't have vccpu threads.

libkvm/libkvm.c line 875

Quoted 3 lines

libkvm/libkvm.c +1 -1

-int kvm_run(kvm_context_t kvm, int vcpu)
+int kvm_run(kvm_context_t kvm, int vcpu, void *env)

That's a little ugly -- passing two arguments which describe the vcpu. How about passing env to kvm_create_vcpu() instead?

struct kvm_stuff { ... } and put that in as a member, so it's easier to remember this is a kvm addition.

Quoted 14 lines

#if defined(TARGET_I386) || defined(TARGET_X86_64)

qemu/hw/acpi.c +12 -0

+#ifdef USE_KVM
+static CPUState *qemu_kvm_cpu_env(int index)
+{
+    CPUState *penv;
+
+    penv = first_cpu;
+
+    while (penv) {
+        if (penv->cpu_index = index)
+            return penv;
+        penv = (CPUState *)penv->next_cpu;
+    }

qemu/hw/acpi.c line 740

Quoted 5 lines

qemu/hw/acpi.c +4 -0

+
+    return NULL;
+}
+#endif

This can't be the best way of determining the env pointer from a cpu number.

qemu/qemu-common.h line 148

Quoted 4 lines

qemu/qemu-common.h +3 -0

+/* work queue */
+struct qemu_work_item {
+    struct qemu_kvm_work_item *next;

Missed rename here.

qemu/qemu-common.h line 153

Quoted 6 lines

qemu/qemu-common.h +5 -0

+    void (*func)(void *data);
+    void *data;
+    int done;
+};
+

Please keep this in kvm specific files.

Quoted 2 lines

-#define bool _Bool

why?

Please separate into a patch that does the kvm_run int vcpu -> void *vcpu conversion, and then the vcpu_info -> env conversion.

Quoted 3 lines

Yes, but I believe the whole point of jes patches is to remove those limits.

So we could linear search huge lists? Or code hashes where arrays would do? No, sir.

An array of pointers statically sized at MAX_CPUS is fine. If you insist, you can realloc() it on demand.

Sorry, but this is riciculous. Please try and take a look at where the search is performed. It's *solely* in the ACPI hot plug code.

realloc() is not an option, unless of course you are in favor of putting a big lock around all access to such an array.

My patch gets rid of the pointless MAX_CPUS sized array, which doesn't buy us anything. In fact, most of the changes in my patch makes the code simpler, because it removes a stack of silly cases where qemu uses env->cpu_index to get into the array, just to hide CPUState from libkvm, just to have the callback in QEMU go from int vcpu back to CPUState.

Lets just do it right and get rid of this silliness.

Quoted 5 lines · Avi Kivity

Quoted 3 lines · Jes Sorensen

Move contents of struct vcpu_info directly into CPU_COMMON. Rename struct qemu_kvm_work_item to qemu_work_item as it really isn't KVM specific.

Quoted 2 lines

Well, it is actually kvm specific, since qemu doesn't have vccpu threads.

Hi Avi,

Anthony suggested I moved things directly in there, after my previous patch kept it as a struct.

Quoted 6 lines

libkvm/libkvm.c line 875

Quoted 3 lines

libkvm/libkvm.c +1 -1

-int kvm_run(kvm_context_t kvm, int vcpu)
+int kvm_run(kvm_context_t kvm, int vcpu, void *env)

That's a little ugly -- passing two arguments which describe the vcpu. How about passing env to kvm_create_vcpu() instead?

I am all in favor of doing this, but in order to do so, we have to teach libkvm about CPUState - are you ok with that?

Quoted 4 lines

struct kvm_stuff { ... } and put that in as a member, so it's easier to remember this is a kvm addition.

If we can agree, I really don't care too much. There's already plenty of stuff in CPU_COMMON that isn't used by specific users, so I didn't think this was an issue.

Quoted 3 lines

Quoted 1 line

Please keep this in kvm specific files.

Why? There really isn't anything specific about a work queue list like this. Making it an available functionality for QEMU shouldn't hurt.

Quoted 4 lines

Quoted 2 lines

-#define bool _Bool

why?

It was unused and didn't add any value.

Quoted 2 lines

Please separate into a patch that does the kvm_run int vcpu -> void *vcpu conversion, and then the vcpu_info -> env conversion.

As mentioned above, if I am going to go that way, we need to teach libkvm about 'env'. I am all in favor, but I am not going to do that unless I can get an ack that it's acceptable to do so.

Quoted 7 lines

My patch gets rid of the pointless MAX_CPUS sized array, which doesn't buy us anything. In fact, most of the changes in my patch makes the code simpler, because it removes a stack of silly cases where qemu uses env->cpu_index to get into the array, just to hide CPUState from libkvm, just to have the callback in QEMU go from int vcpu back to CPUState.

Quoted 1 line

Lets just do it right and get rid of this silliness.

I agree that vcpu_info should die. But we should still have a fast way of getting a cpu from a cpu_index. I don't see a problem with a static array of pointers, make it 16K in size if you want. Our real scaling limits are much lower; they involve the big qemu lock and the single iothread which will both limit scaling far below any static vcpu array.

Just do a linear search of the CPUState list and be done with it. This smells of premature optimization greatly. I would be amazed if walking the CPUState list is ever on the fast path or will ever be. Really, if you need to go from cpu_index => CPUState, it suggests you're doing something wrong.

Quoted 3 lines · Jes Sorensen

Sorry, but this is riciculous. Please try and take a look at where the search is performed. It's *solely* in the ACPI hot plug code.

There are other places we want a cpu from a cpu_index. Like the apic code (admittedly that's only using the userspace apic, which is non-default, but still).

Quoted 6 lines

My patch gets rid of the pointless MAX_CPUS sized array, which doesn't buy us anything. In fact, most of the changes in my patch makes the code simpler, because it removes a stack of silly cases where qemu uses env->cpu_index to get into the array, just to hide CPUState from libkvm, just to have the callback in QEMU go from int vcpu back to CPUState.

Quoted 1 line

Lets just do it right and get rid of this silliness.

I agree that vcpu_info should die. But we should still have a fast way of getting a cpu from a cpu_index. I don't see a problem with a static array of pointers, make it 16K in size if you want. Our real scaling limits are much lower; they involve the big qemu lock and the single iothread which will both limit scaling far below any static vcpu array.

If we have reasons where we actually rely on the cpu number, and thats not counting end-user pretty-number-print concerns, then I found that we practically never use the cpu number. If we really use it a lot, I agree we need a fast way, I just didn't hit it in my builds. What did I miss?

The code I mentioned only runs if the -no-kvm-irqchip option is passed. It's not the highest performing option...

So this isn't used a lot. But cpus definitely use cpu numbers (as apic ids), so qemu needs to be prepared to handle this. As to scalability, that takes will take a lot more work than removing/changing arbitrary limits. Look at qemu_mutex and weep.

Quoted 2 lines · Avi Kivity

The code I mentioned only runs if the -no-kvm-irqchip option is passed. It's not the highest performing option...

What I meant was that I was able to compile the code, and there was only one piece left that needed that function, which is why I moved it and made it static in the acpi code.

Quoted 4 lines

So this isn't used a lot. But cpus definitely use cpu numbers (as apic ids), so qemu needs to be prepared to handle this. As to scalability, that takes will take a lot more work than removing/changing arbitrary limits. Look at qemu_mutex and weep.

Well we have to start somewhere. Just because there's limits in other areas too, doesn't mean we should discard valid improvements elsewhere. I'll get to some of that at some point, but there's soo many cards used to build this house....

Quoted 4 lines · Jes Sorensen

What I meant was that I was able to compile the code, and there was only one piece left that needed that function, which is why I moved it and made it static in the acpi code.

That's because there is another static array in acpi.c...

Quoted 4 lines

Well we have to start somewhere. Just because there's limits in other areas too, doesn't mean we should discard valid improvements elsewhere. I'll get to some of that at some point, but there's soo many cards used to build this house....

Your patch builds the 16383rd floor while ignoring the 15th. Qemu scaling issues are much, much more painful than what you were addressing.

Quoted 9 lines · Avi Kivity

Quoted 5 lines · Jes Sorensen

What I meant was that I was able to compile the code, and there was only one piece left that needed that function, which is why I moved it and made it static in the acpi code.

That's because there is another static array in acpi.c...

Which array are you talking about? ACPI has it's own limitations, like we need ACPI 3.0 to go beyond 256 processors :-(

Quoted 7 lines

Quoted 4 lines

Well we have to start somewhere. Just because there's limits in other areas too, doesn't mean we should discard valid improvements elsewhere. I'll get to some of that at some point, but there's soo many cards used to build this house....

Your patch builds the 16383rd floor while ignoring the 15th. Qemu scaling issues are much, much more painful than what you were addressing.

Well I am not ignoring the 15th floor, I am just putting in expandable rails that can go all the way to the 16383rd. Next I'll get to look at putting in the button to make the elevator stop at the 15th+ floors :)

Cheers,
Jes

Quoted 7 lines · Avi Kivity

Quoted 5 lines · Jes Sorensen

What I meant was that I was able to compile the code, and there was only
one piece left that needed that function, which is why I moved it and made it static in the acpi code.

Quoted 1 line

That's because there is another static array in acpi.c...

Which array are you talking about? ACPI has it's own limitations, like we need ACPI 3.0 to go beyond 256 processors :-(

Quoted 5 lines

Quoted 4 lines

Well we have to start somewhere. Just because there's limits in other areas too, doesn't mean we should discard valid improvements elsewhere. I'll get to some of that at some point, but there's soo many cards used to build this house....

Quoted 3 lines

Your patch builds the 16383rd floor while ignoring the 15th. Qemu scaling issues are much, much more painful than what you were addressing.

Well I am not ignoring the 15th floor, I am just putting in expandable rails that can go all the way to the 16383rd. Next I'll get to look at putting in the button to make the elevator stop at the 15th+ floors :)

I think the main point here is that currently, one of the showstopers for scalability,
is the lack of fine grained locking. And we don't really know how the code will look like
after that. It can very well change completely, rendering this work as totally void.

Quoted 4 lines · Jes Sorensen

Quoted 1 line

That's a little ugly -- passing two arguments which describe the vcpu. How about passing env to kvm_create_vcpu() instead?

I am all in favor of doing this, but in order to do so, we have to teach libkvm about CPUState - are you ok with that?

No, libkvm is independent of qemu.

We can make kvm_create_vcpu(kvm_context_t kvm, int vcpu, void *env), and pass 'env' as the opaque to all the callbacks, instead of the vcpu number.

I pulled out the part of the patch that does the opague 'env' pointer. It should be pretty straight forward - the vcpu_info patch will go on top of this, but I'll work on that tomorrow.

Let me know what you think - this is about as small I can make this patch :-)

Jes Sorensen wrote:
It's good; applied, thanks.

There's a slight redundancy in that some callbacks are passed two opaques (per-cpu and per-vm), but it won't kill anyone.

(btw, this could have been done much more simply: declare a thread local variable to hold the current env)