* [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times @ 2020-04-20 9:15 Nikolay Igotti 2020-04-20 9:51 ` Laurent Vivier 2020-04-20 10:11 ` Peter Maydell 0 siblings, 2 replies; 15+ messages in thread From: Nikolay Igotti @ 2020-04-20 9:15 UTC (permalink / raw) To: laurent, riku.voipio; +Cc: qemu-devel [-- Attachment #1: Type: text/plain, Size: 1022 bytes --] In linux-user multithreaded scenarious CPU could be inited many times with the same id, so avoid assertions on already present hashtable entry. Signed-off-by: Nikolay Igotti <igotti@gmail.com> --- plugins/core.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/plugins/core.c b/plugins/core.c index 51bfc94787..889cc6441a 100644 --- a/plugins/core.c +++ b/plugins/core.c @@ -196,13 +196,10 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum qemu_plugin_event ev, void qemu_plugin_vcpu_init_hook(CPUState *cpu) { - bool success; - qemu_rec_mutex_lock(&plugin.lock); plugin_cpu_update__locked(&cpu->cpu_index, NULL, NULL); - success = g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index, + g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index, &cpu->cpu_index); - g_assert(success); qemu_rec_mutex_unlock(&plugin.lock); plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_INIT); -- 2.24.2 (Apple Git-127) [-- Attachment #2: Type: text/html, Size: 9929 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-04-20 9:15 [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times Nikolay Igotti @ 2020-04-20 9:51 ` Laurent Vivier 2020-04-20 10:04 ` Nikolay Igotti 2020-04-20 10:11 ` Peter Maydell 1 sibling, 1 reply; 15+ messages in thread From: Laurent Vivier @ 2020-04-20 9:51 UTC (permalink / raw) To: Nikolay Igotti, riku.voipio; +Cc: Emilio G. Cota, Alex Bennée, qemu-devel Nikolay, the patch seems corrupted by your mailer. CC: Alex and Emilio as this code is theirs. Thanks, Laurent Le 20/04/2020 à 11:15, Nikolay Igotti a écrit : > In linux-user multithreaded scenarious CPU could be inited many times > with the same id, > > so avoid assertions on already present hashtable entry. > > > Signed-off-by: Nikolay Igotti <igotti@gmail.com <mailto:igotti@gmail.com>> > > --- > > plugins/core.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > diff --git a/plugins/core.c b/plugins/core.c > > index 51bfc94787..889cc6441a 100644 > > --- a/plugins/core.c > > +++ b/plugins/core.c > > @@ -196,13 +196,10 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum > qemu_plugin_event ev, > > > > void qemu_plugin_vcpu_init_hook(CPUState *cpu) > > { > > - bool success; > > - > > qemu_rec_mutex_lock(&plugin.lock); > > plugin_cpu_update__locked(&cpu->cpu_index, NULL, NULL); > > - success = g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index, > > + g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index, > > &cpu->cpu_index); > > - g_assert(success); > > qemu_rec_mutex_unlock(&plugin.lock); > > > > plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_INIT); > > -- > > 2.24.2 (Apple Git-127) > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-04-20 9:51 ` Laurent Vivier @ 2020-04-20 10:04 ` Nikolay Igotti 2020-05-09 23:00 ` Emilio G. Cota 0 siblings, 1 reply; 15+ messages in thread From: Nikolay Igotti @ 2020-04-20 10:04 UTC (permalink / raw) To: Laurent Vivier; +Cc: Alex Bennée, riku.voipio, Emilio G. Cota, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2736 bytes --] OK, maybe this version will work better (gmail web interface may be not the best one for sending patches): ---- In linux-user multithreaded scenarious CPU could be inited many times with the same id, so avoid assertions on already present hashtable entry. Signed-off-by: Nikolay Igotti <igotti@gmail.com> --- plugins/core.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/plugins/core.c b/plugins/core.c index 51bfc94787..889cc6441a 100644 --- a/plugins/core.c +++ b/plugins/core.c @@ -196,13 +196,10 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum qemu_plugin_event ev, void qemu_plugin_vcpu_init_hook(CPUState *cpu) { - bool success; - qemu_rec_mutex_lock(&plugin.lock); plugin_cpu_update__locked(&cpu->cpu_index, NULL, NULL); - success = g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index, + g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index, &cpu->cpu_index); - g_assert(success); qemu_rec_mutex_unlock(&plugin.lock); plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_INIT); -- 2.24.2 (Apple Git-127) On Mon, Apr 20, 2020 at 12:51 PM Laurent Vivier <laurent@vivier.eu> wrote: > Nikolay, > > the patch seems corrupted by your mailer. > > CC: Alex and Emilio as this code is theirs. > > Thanks, > Laurent > > Le 20/04/2020 à 11:15, Nikolay Igotti a écrit : > > In linux-user multithreaded scenarious CPU could be inited many times > > with the same id, > > > > so avoid assertions on already present hashtable entry. > > > > > > Signed-off-by: Nikolay Igotti <igotti@gmail.com <mailto:igotti@gmail.com > >> > > > > --- > > > > plugins/core.c | 5 +---- > > > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > > > diff --git a/plugins/core.c b/plugins/core.c > > > > index 51bfc94787..889cc6441a 100644 > > > > --- a/plugins/core.c > > > > +++ b/plugins/core.c > > > > @@ -196,13 +196,10 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum > > qemu_plugin_event ev, > > > > > > > > void qemu_plugin_vcpu_init_hook(CPUState *cpu) > > > > { > > > > - bool success; > > > > - > > > > qemu_rec_mutex_lock(&plugin.lock); > > > > plugin_cpu_update__locked(&cpu->cpu_index, NULL, NULL); > > > > - success = g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index, > > > > + g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index, > > > > &cpu->cpu_index); > > > > - g_assert(success); > > > > qemu_rec_mutex_unlock(&plugin.lock); > > > > > > > > plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_INIT); > > > > -- > > > > 2.24.2 (Apple Git-127) > > > > [-- Attachment #2: Type: text/html, Size: 3795 bytes --] ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-04-20 10:04 ` Nikolay Igotti @ 2020-05-09 23:00 ` Emilio G. Cota 2020-05-11 15:53 ` Nikolay Igotti 0 siblings, 1 reply; 15+ messages in thread From: Emilio G. Cota @ 2020-05-09 23:00 UTC (permalink / raw) To: Nikolay Igotti; +Cc: riku.voipio, Alex Bennée, Laurent Vivier, qemu-devel On Mon, Apr 20, 2020 at 13:04:51 +0300, Nikolay Igotti wrote: > In linux-user multithreaded scenarious CPU could be inited many times with > the same id, > so avoid assertions on already present hashtable entry. > > Signed-off-by: Nikolay Igotti <igotti@gmail.com> > --- > plugins/core.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/plugins/core.c b/plugins/core.c > index 51bfc94787..889cc6441a 100644 > --- a/plugins/core.c > +++ b/plugins/core.c > @@ -196,13 +196,10 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum > qemu_plugin_event ev, > > void qemu_plugin_vcpu_init_hook(CPUState *cpu) > { > - bool success; > - > qemu_rec_mutex_lock(&plugin.lock); > plugin_cpu_update__locked(&cpu->cpu_index, NULL, NULL); > - success = g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index, > + g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index, > &cpu->cpu_index); > - g_assert(success); > qemu_rec_mutex_unlock(&plugin.lock); Do you have a reproducer for this? I'd expect (1) the g_hash_table_remove call in qemu_plugin_vcpu_exit_hook to clear this entry upon CPU exit, and (2) no two live CPUs to have the same cpu_index. But maybe assumption (2) is wrong, or simply (1) does not get called for some exiting CPUs, in which case the right fix would be to make sure that it does get called on CPU exit. Thanks, Emilio ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-05-09 23:00 ` Emilio G. Cota @ 2020-05-11 15:53 ` Nikolay Igotti 2020-05-12 0:55 ` Emilio G. Cota 0 siblings, 1 reply; 15+ messages in thread From: Nikolay Igotti @ 2020-05-11 15:53 UTC (permalink / raw) To: Emilio G. Cota; +Cc: riku.voipio, Alex Bennée, Laurent Vivier, qemu-devel [-- Attachment #1: Type: text/plain, Size: 1779 bytes --] Attached to the mail counter.c when running with attached test.c compiled to Linux standalone binary shows failing assert, unless the patch is applied. вс, 10 мая 2020 г. в 02:00, Emilio G. Cota <cota@braap.org>: > On Mon, Apr 20, 2020 at 13:04:51 +0300, Nikolay Igotti wrote: > > In linux-user multithreaded scenarious CPU could be inited many times > with > > the same id, > > so avoid assertions on already present hashtable entry. > > > > Signed-off-by: Nikolay Igotti <igotti@gmail.com> > > --- > > plugins/core.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/plugins/core.c b/plugins/core.c > > index 51bfc94787..889cc6441a 100644 > > --- a/plugins/core.c > > +++ b/plugins/core.c > > @@ -196,13 +196,10 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum > > qemu_plugin_event ev, > > > > void qemu_plugin_vcpu_init_hook(CPUState *cpu) > > { > > - bool success; > > - > > qemu_rec_mutex_lock(&plugin.lock); > > plugin_cpu_update__locked(&cpu->cpu_index, NULL, NULL); > > - success = g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index, > > + g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index, > > &cpu->cpu_index); > > - g_assert(success); > > qemu_rec_mutex_unlock(&plugin.lock); > > Do you have a reproducer for this? I'd expect (1) the g_hash_table_remove > call in qemu_plugin_vcpu_exit_hook to clear this entry upon CPU exit, > and (2) no two live CPUs to have the same cpu_index. But maybe assumption > (2) is wrong, or simply (1) does not get called for some exiting CPUs, > in which case the right fix would be to make sure that it does get called > on CPU exit. > > Thanks, > > Emilio > [-- Attachment #2: Type: text/html, Size: 2443 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-05-11 15:53 ` Nikolay Igotti @ 2020-05-12 0:55 ` Emilio G. Cota 2020-05-12 8:38 ` Nikolay Igotti 2020-05-12 20:11 ` Alex Bennée 0 siblings, 2 replies; 15+ messages in thread From: Emilio G. Cota @ 2020-05-12 0:55 UTC (permalink / raw) To: Nikolay Igotti; +Cc: riku.voipio, Alex Bennée, Laurent Vivier, qemu-devel On Mon, May 11, 2020 at 18:53:19 +0300, Nikolay Igotti wrote: > Attached to the mail counter.c when running with attached test.c compiled > to Linux standalone binary shows failing assert, unless the patch is > applied. I didn't get the attachment. Can you paste the code at the end of your reply? Thanks, Emilio ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-05-12 0:55 ` Emilio G. Cota @ 2020-05-12 8:38 ` Nikolay Igotti 2020-05-12 19:35 ` Alex Bennée 2020-05-12 20:11 ` Alex Bennée 1 sibling, 1 reply; 15+ messages in thread From: Nikolay Igotti @ 2020-05-12 8:38 UTC (permalink / raw) To: Emilio G. Cota Cc: Riku Voipio, Alex Bennée, Laurent Vivier, QEMU Developers [-- Attachment #1: Type: text/plain, Size: 4628 bytes --] --- counter.c #include <assert.h> #include <pthread.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <unistd.h> #include <glib.h> #include <qemu-plugin.h> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; // Files with descriptors after this one are intercepted for instruction counting marks. #define CATCH_BASE 0xcafebabe static uint64_t insn_count = 0; static pthread_t counting = false; static pthread_t counting_for = 0; static bool on_every_close = false; static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata) { if (counting && pthread_self() == counting_for) insn_count++; } static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) { size_t n = qemu_plugin_tb_n_insns(tb); size_t i; for (i = 0; i < n; i++) { struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i); // TODO: do this call only on first insn in bb. qemu_plugin_register_vcpu_insn_exec_cb( insn, vcpu_insn_exec_before, QEMU_PLUGIN_CB_NO_REGS, NULL); } } static void print_insn_count(void) { g_autofree gchar *out = g_strdup_printf("executed %" PRIu64 " instructions\n", insn_count); qemu_plugin_outs(out); } static void vcpu_syscall(qemu_plugin_id_t id, unsigned int vcpu_index, int64_t num, uint64_t a1, uint64_t a2, uint64_t a3, uint64_t a4, uint64_t a5, uint64_t a6, uint64_t a7, uint64_t a8) { // We put our listener on fd reads in range [CATCH_BASE, CATCH_BASE + 1] if (num == 0) { // sys_read switch (a1) { case CATCH_BASE + 0: counting = true; counting_for = pthread_self(); insn_count = 0; break; case CATCH_BASE + 1: { counting = false; counting_for = 0; if (a3 == 8) { // In case of user emulation in QEMU, addresses are 1:1 translated, so we can tell the caller // number of executed instructions by just writing into the buffer argument of read. *(uint64_t*)a2 = insn_count; } print_insn_count(); break; } default: break; } } if (num == 3 && on_every_close) { // sys_close print_insn_count(); } } QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info, int argc, char **argv) { int i; for (i = 0; i < argc; i++) { if (!strcmp(argv[i], "on_every_close")) { on_every_close = true; counting = true; counting_for = pthread_self(); } } qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans); qemu_plugin_register_vcpu_syscall_cb(id, vcpu_syscall); return 0; } --- test.c #include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <unistd.h> #include <pthread.h> #define CATCH_BASE 0xcafebabe static void start_counting() { char buf; int rv = read(CATCH_BASE, &buf, 1); (void)rv; } static void end_counting() { uint64_t counter = 0; int rv = read(CATCH_BASE + 1, &counter, sizeof(counter)); (void)rv; printf("We got %lld from TCG\n", counter); } int global = 0; typedef struct { int delay; } ThreadArg; static void* thread_fn(void* varg) { ThreadArg* arg = varg; usleep(arg->delay); free(arg); return NULL; } int main(int argc, char** argv) { int i; int repeat = 100; #define THREAD_NUM 10 pthread_t threads[THREAD_NUM]; if (argc > 1) { repeat = atoi(argv[1]); } for (i = 0; i < THREAD_NUM; i++) { ThreadArg* arg = calloc(sizeof(ThreadArg), 1); arg->delay = i * 100; pthread_create(threads + i, NULL, thread_fn, arg); } start_counting(); for (i = 0; i < repeat; i++) { global += i; } end_counting(); for (i = 0; i < THREAD_NUM; i++) { pthread_join(threads[i], NULL); } return 0; } On Tue, May 12, 2020 at 3:55 AM Emilio G. Cota <cota@braap.org> wrote: > On Mon, May 11, 2020 at 18:53:19 +0300, Nikolay Igotti wrote: > > Attached to the mail counter.c when running with attached test.c compiled > > to Linux standalone binary shows failing assert, unless the patch is > > applied. > > I didn't get the attachment. Can you paste the code at the end of your > reply? > > Thanks, > Emilio > [-- Attachment #2: Type: text/html, Size: 6069 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-05-12 8:38 ` Nikolay Igotti @ 2020-05-12 19:35 ` Alex Bennée 0 siblings, 0 replies; 15+ messages in thread From: Alex Bennée @ 2020-05-12 19:35 UTC (permalink / raw) To: Nikolay Igotti Cc: Riku Voipio, Emilio G. Cota, Laurent Vivier, QEMU Developers Nikolay Igotti <igotti@gmail.com> writes: > --- counter.c > > #include <assert.h> > #include <pthread.h> > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > #include <unistd.h> > > #include <glib.h> > > #include <qemu-plugin.h> > > QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; > > // Files with descriptors after this one are intercepted for instruction > counting marks. > #define CATCH_BASE 0xcafebabe > > static uint64_t insn_count = 0; > static pthread_t counting = false; > static pthread_t counting_for = 0; > static bool on_every_close = false; > > static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata) > { > if (counting && pthread_self() == counting_for) > insn_count++; > } > > static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) > { > size_t n = qemu_plugin_tb_n_insns(tb); > size_t i; > > for (i = 0; i < n; i++) { > struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i); > > // TODO: do this call only on first insn in bb. > qemu_plugin_register_vcpu_insn_exec_cb( > insn, vcpu_insn_exec_before, QEMU_PLUGIN_CB_NO_REGS, NULL); > } > } > > static void print_insn_count(void) { > g_autofree gchar *out = g_strdup_printf("executed %" PRIu64 " > instructions\n", insn_count); > qemu_plugin_outs(out); > } > > static void vcpu_syscall(qemu_plugin_id_t id, unsigned int vcpu_index, > int64_t num, uint64_t a1, uint64_t a2, > uint64_t a3, uint64_t a4, uint64_t a5, > uint64_t a6, uint64_t a7, uint64_t a8) > { > // We put our listener on fd reads in range [CATCH_BASE, CATCH_BASE + 1] > if (num == 0) { // sys_read > switch (a1) > { > case CATCH_BASE + 0: > counting = true; > counting_for = pthread_self(); > insn_count = 0; > break; > case CATCH_BASE + 1: { > counting = false; > counting_for = 0; > if (a3 == 8) { > // In case of user emulation in QEMU, addresses are 1:1 > translated, so we can tell the caller > // number of executed instructions by just writing into > the buffer argument of read. > *(uint64_t*)a2 = insn_count; Hmm this was certainly unintentional - is it the host or guest address you are messing with here? I wouldn't count on it pointing where you think and relying on it to pass information back to the instrumented guest. Anyway I have a replication - we are trying to insert the same id into the plugin cpu index hash table twice. > } > print_insn_count(); > break; > } > default: > break; > } > } > if (num == 3 && on_every_close) { // sys_close > print_insn_count(); > } > } > > QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, > const qemu_info_t *info, > int argc, char **argv) > { > int i; > for (i = 0; i < argc; i++) { > if (!strcmp(argv[i], "on_every_close")) { > on_every_close = true; > counting = true; > counting_for = pthread_self(); > } > } > > qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans); > qemu_plugin_register_vcpu_syscall_cb(id, vcpu_syscall); > return 0; > } > > --- test.c > > #include <stdint.h> > #include <stdio.h> > #include <stdlib.h> > #include <unistd.h> > #include <pthread.h> > > #define CATCH_BASE 0xcafebabe > > static void start_counting() { > char buf; > int rv = read(CATCH_BASE, &buf, 1); > (void)rv; > } > > static void end_counting() { > uint64_t counter = 0; > int rv = read(CATCH_BASE + 1, &counter, sizeof(counter)); > (void)rv; > printf("We got %lld from TCG\n", counter); > } > > int global = 0; > > typedef struct { > int delay; > } ThreadArg; > > static void* thread_fn(void* varg) { > ThreadArg* arg = varg; > usleep(arg->delay); > free(arg); > return NULL; > } > > int main(int argc, char** argv) { > int i; > int repeat = 100; > #define THREAD_NUM 10 > pthread_t threads[THREAD_NUM]; > > if (argc > 1) { > repeat = atoi(argv[1]); > } > > for (i = 0; i < THREAD_NUM; i++) { > ThreadArg* arg = calloc(sizeof(ThreadArg), 1); > arg->delay = i * 100; > pthread_create(threads + i, NULL, thread_fn, arg); > } > > start_counting(); > for (i = 0; i < repeat; i++) { > global += i; > } > end_counting(); > > for (i = 0; i < THREAD_NUM; i++) { > pthread_join(threads[i], NULL); > } > > return 0; > } > > On Tue, May 12, 2020 at 3:55 AM Emilio G. Cota <cota@braap.org> wrote: > >> On Mon, May 11, 2020 at 18:53:19 +0300, Nikolay Igotti wrote: >> > Attached to the mail counter.c when running with attached test.c compiled >> > to Linux standalone binary shows failing assert, unless the patch is >> > applied. >> >> I didn't get the attachment. Can you paste the code at the end of your >> reply? >> >> Thanks, >> Emilio >> -- Alex Bennée ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-05-12 0:55 ` Emilio G. Cota 2020-05-12 8:38 ` Nikolay Igotti @ 2020-05-12 20:11 ` Alex Bennée 2020-05-24 14:11 ` Emilio G. Cota 1 sibling, 1 reply; 15+ messages in thread From: Alex Bennée @ 2020-05-12 20:11 UTC (permalink / raw) To: Emilio G. Cota Cc: qemu-devel, Paolo Bonzini, riku.voipio, Laurent Vivier, Nikolay Igotti Emilio G. Cota <cota@braap.org> writes: > On Mon, May 11, 2020 at 18:53:19 +0300, Nikolay Igotti wrote: >> Attached to the mail counter.c when running with attached test.c compiled >> to Linux standalone binary shows failing assert, unless the patch is >> applied. > > I didn't get the attachment. Can you paste the code at the end of your > reply? So the problem is we are recycling cpu_index: 21:01:54 [alex@zen:~/l/q/b/all.plugin] plugins/next|●2✚3…(+8 insertions(+)) 127 + ./x86_64-linux-user/qemu-x86_64 ./tests/tcg/x86_64-linux-user/threadcount cpu_list_add: 0x5605598579f0 cpu_get_free_index: 0 cpu_list_add: 0x5605598c6010 cpu_get_free_index: 1 cpu_list_add: 0x5605598ec330 cpu_get_free_index: 2 cpu_list_add: 0x560559912a30 cpu_get_free_index: 3 cpu_list_add: 0x560559939130 cpu_get_free_index: 4 cpu_list_add: 0x56055995d360 cpu_get_free_index: 1 ** ERROR:/home/alex/lsrc/qemu.git/plugins/core.c:205:qemu_plugin_vcpu_init_hook: assertion failed: (success) qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x7fa29c702613 However I'm confused as to how. The cpu_index is really just a function of the number of CPUs in the cpus list and I can't see where we free them up. We never call cpu_common_unrealizefn so the list should never shrink. We should just end up with a slowly growing list of now dead CPU states. I'm giving up for now but will have a look again in the morning if no one else has figured out what is going on by then. > > Thanks, > Emilio -- Alex Bennée ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-05-12 20:11 ` Alex Bennée @ 2020-05-24 14:11 ` Emilio G. Cota 0 siblings, 0 replies; 15+ messages in thread From: Emilio G. Cota @ 2020-05-24 14:11 UTC (permalink / raw) To: Alex Bennée Cc: qemu-devel, Paolo Bonzini, riku.voipio, Laurent Vivier, Nikolay Igotti Hi Alex, On Tue, May 12, 2020 at 21:11:46 +0100, Alex Bennée wrote: > > Emilio G. Cota <cota@braap.org> writes: > > > On Mon, May 11, 2020 at 18:53:19 +0300, Nikolay Igotti wrote: > >> Attached to the mail counter.c when running with attached test.c compiled > >> to Linux standalone binary shows failing assert, unless the patch is > >> applied. > > > > I didn't get the attachment. Can you paste the code at the end of your > > reply? > > So the problem is we are recycling cpu_index: > > 21:01:54 [alex@zen:~/l/q/b/all.plugin] plugins/next|●2✚3…(+8 insertions(+)) 127 + ./x86_64-linux-user/qemu-x86_64 ./tests/tcg/x86_64-linux-user/threadcount > cpu_list_add: 0x5605598579f0 > cpu_get_free_index: 0 > cpu_list_add: 0x5605598c6010 > cpu_get_free_index: 1 > cpu_list_add: 0x5605598ec330 > cpu_get_free_index: 2 > cpu_list_add: 0x560559912a30 > cpu_get_free_index: 3 > cpu_list_add: 0x560559939130 > cpu_get_free_index: 4 > cpu_list_add: 0x56055995d360 > cpu_get_free_index: 1 > ** > ERROR:/home/alex/lsrc/qemu.git/plugins/core.c:205:qemu_plugin_vcpu_init_hook: assertion failed: (success) > qemu:handle_cpu_signal received signal outside vCPU context @ pc=0x7fa29c702613 > > However I'm confused as to how. The cpu_index is really just a function > of the number of CPUs in the cpus list and I can't see where we free > them up. We never call cpu_common_unrealizefn so the list should never > shrink. We should just end up with a slowly growing list of now dead CPU > states. > > I'm giving up for now but will have a look again in the morning if no > one else has figured out what is going on by then. I just got this fixed, only to realise that you already fixed it weeks ago :-) (I didn't see your fixes because I'm not subscribed to the list.) My changes are here https://github.com/cota/qemu/tree/cpu_index Please feel free to take anything from there that you find valuable, e.g. the use of a bitmap for tracking cpu_index'es and the addition of a unit test (note that the test is fixed in the last commit). Thanks, Emilio ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-04-20 9:15 [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times Nikolay Igotti 2020-04-20 9:51 ` Laurent Vivier @ 2020-04-20 10:11 ` Peter Maydell 2020-04-20 10:18 ` Nikolay Igotti 2020-04-20 15:07 ` Alex Bennée 1 sibling, 2 replies; 15+ messages in thread From: Peter Maydell @ 2020-04-20 10:11 UTC (permalink / raw) To: Nikolay Igotti; +Cc: Riku Voipio, Laurent Vivier, QEMU Developers On Mon, 20 Apr 2020 at 10:16, Nikolay Igotti <igotti@gmail.com> wrote: > > In linux-user multithreaded scenarious CPU could be inited many times with the same id, > > so avoid assertions on already present hashtable entry. > > > Signed-off-by: Nikolay Igotti <igotti@gmail.com> Wouldn't it be better to make sure we remove the entry from the hashtable when the old thread that was using that CPU ID exits, or is that not feasible ? thanks -- PMM ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-04-20 10:11 ` Peter Maydell @ 2020-04-20 10:18 ` Nikolay Igotti 2020-04-20 10:30 ` Peter Maydell 2020-04-20 15:07 ` Alex Bennée 1 sibling, 1 reply; 15+ messages in thread From: Nikolay Igotti @ 2020-04-20 10:18 UTC (permalink / raw) To: Peter Maydell; +Cc: Riku Voipio, Laurent Vivier, QEMU Developers [-- Attachment #1: Type: text/plain, Size: 655 bytes --] Maybe, I've tried least intrusive change as not entirely familiar with plugin login in QEMU. On Mon, Apr 20, 2020 at 1:11 PM Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 20 Apr 2020 at 10:16, Nikolay Igotti <igotti@gmail.com> wrote: > > > > In linux-user multithreaded scenarious CPU could be inited many times > with the same id, > > > > so avoid assertions on already present hashtable entry. > > > > > > Signed-off-by: Nikolay Igotti <igotti@gmail.com> > > Wouldn't it be better to make sure we remove the entry from > the hashtable when the old thread that was using that CPU > ID exits, or is that not feasible ? > > thanks > -- PMM > [-- Attachment #2: Type: text/html, Size: 1131 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-04-20 10:18 ` Nikolay Igotti @ 2020-04-20 10:30 ` Peter Maydell 0 siblings, 0 replies; 15+ messages in thread From: Peter Maydell @ 2020-04-20 10:30 UTC (permalink / raw) To: Nikolay Igotti Cc: Riku Voipio, Alex Bennée, Laurent Vivier, QEMU Developers On Mon, 20 Apr 2020 at 11:18, Nikolay Igotti <igotti@gmail.com> wrote: > > Maybe, I've tried least intrusive change as not entirely familiar with plugin login in QEMU. Me neither, but having the hashtable continuing to contain stale information about a CPU that's gone away seems like it might cause problems somehow. (There might be similar situations with CPU hotplug/unplug in system emulation mode.) Just noticed Alex wasn't on cc, so I've added him (I'm afraid you'll have to look back up the thread for context, Alex...) thanks -- PMM ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-04-20 10:11 ` Peter Maydell 2020-04-20 10:18 ` Nikolay Igotti @ 2020-04-20 15:07 ` Alex Bennée 2020-04-20 16:14 ` Nikolay Igotti 1 sibling, 1 reply; 15+ messages in thread From: Alex Bennée @ 2020-04-20 15:07 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-devel, Riku Voipio, Laurent Vivier, Nikolay Igotti Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 20 Apr 2020 at 10:16, Nikolay Igotti <igotti@gmail.com> wrote: >> >> In linux-user multithreaded scenarious CPU could be inited many times with the same id, >> >> so avoid assertions on already present hashtable entry. >> >> >> Signed-off-by: Nikolay Igotti <igotti@gmail.com> > > Wouldn't it be better to make sure we remove the entry from > the hashtable when the old thread that was using that CPU > ID exits, or is that not feasible ? I'm fairly sure that is exactly what should be happening via qemu_plugin_vcpu_exit_hook(cpu) which should be a result of the object_unref(OBJECT(cpu)) in thread exit. Is there a test vase? > > thanks > -- PMM -- Alex Bennée ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times 2020-04-20 15:07 ` Alex Bennée @ 2020-04-20 16:14 ` Nikolay Igotti 0 siblings, 0 replies; 15+ messages in thread From: Nikolay Igotti @ 2020-04-20 16:14 UTC (permalink / raw) To: Alex Bennée; +Cc: Peter Maydell, Riku Voipio, Laurent Vivier, qemu-devel [-- Attachment #1.1: Type: text/plain, Size: 952 bytes --] Sure, attached plugin and testcase where it fails. On Mon, Apr 20, 2020 at 6:08 PM Alex Bennée <alex.bennee@linaro.org> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > > On Mon, 20 Apr 2020 at 10:16, Nikolay Igotti <igotti@gmail.com> wrote: > >> > >> In linux-user multithreaded scenarious CPU could be inited many times > with the same id, > >> > >> so avoid assertions on already present hashtable entry. > >> > >> > >> Signed-off-by: Nikolay Igotti <igotti@gmail.com> > > > > Wouldn't it be better to make sure we remove the entry from > > the hashtable when the old thread that was using that CPU > > ID exits, or is that not feasible ? > > I'm fairly sure that is exactly what should be happening via > qemu_plugin_vcpu_exit_hook(cpu) which should be a result of the > object_unref(OBJECT(cpu)) in thread exit. > > Is there a test vase? > > > > > thanks > > -- PMM > > > -- > Alex Bennée > [-- Attachment #1.2: Type: text/html, Size: 1566 bytes --] [-- Attachment #2: counter.c --] [-- Type: application/octet-stream, Size: 2993 bytes --] #include <assert.h> #include <pthread.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #include <unistd.h> #include <glib.h> #include <qemu-plugin.h> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; // Files with descriptors after this one are intercepted for instruction counting marks. #define CATCH_BASE 0xcafebabe static uint64_t insn_count = 0; static pthread_t counting = false; static pthread_t counting_for = 0; static bool on_every_close = false; static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata) { if (counting && pthread_self() == counting_for) insn_count++; } static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) { size_t n = qemu_plugin_tb_n_insns(tb); size_t i; for (i = 0; i < n; i++) { struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i); // TODO: do this call only on first insn in bb. qemu_plugin_register_vcpu_insn_exec_cb( insn, vcpu_insn_exec_before, QEMU_PLUGIN_CB_NO_REGS, NULL); } } static void print_insn_count(void) { g_autofree gchar *out = g_strdup_printf("executed %" PRIu64 " instructions\n", insn_count); qemu_plugin_outs(out); } static void vcpu_syscall(qemu_plugin_id_t id, unsigned int vcpu_index, int64_t num, uint64_t a1, uint64_t a2, uint64_t a3, uint64_t a4, uint64_t a5, uint64_t a6, uint64_t a7, uint64_t a8) { // We put our listener on fd reads in range [CATCH_BASE, CATCH_BASE + 1] if (num == 0) { // sys_read switch (a1) { case CATCH_BASE + 0: counting = true; counting_for = pthread_self(); insn_count = 0; break; case CATCH_BASE + 1: { counting = false; counting_for = 0; if (a3 == 8) { // In case of user emulation in QEMU, addresses are 1:1 translated, so we can tell the caller // number of executed instructions by just writing into the buffer argument of read. *(uint64_t*)a2 = insn_count; } print_insn_count(); break; } default: break; } } if (num == 3 && on_every_close) { // sys_close print_insn_count(); } } QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info, int argc, char **argv) { int i; for (i = 0; i < argc; i++) { if (!strcmp(argv[i], "on_every_close")) { on_every_close = true; counting = true; counting_for = pthread_self(); } } qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans); qemu_plugin_register_vcpu_syscall_cb(id, vcpu_syscall); return 0; } [-- Attachment #3: test.c --] [-- Type: application/octet-stream, Size: 1182 bytes --] #include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <unistd.h> #include <pthread.h> #define CATCH_BASE 0xcafebabe static void start_counting() { char buf; int rv = read(CATCH_BASE, &buf, 1); (void)rv; } static void end_counting() { uint64_t counter = 0; int rv = read(CATCH_BASE + 1, &counter, sizeof(counter)); (void)rv; printf("We got %lld from TCG\n", counter); } int global = 0; typedef struct { int delay; } ThreadArg; static void* thread_fn(void* varg) { ThreadArg* arg = varg; usleep(arg->delay); free(arg); return NULL; } int main(int argc, char** argv) { int i; int repeat = 100; #define THREAD_NUM 10 pthread_t threads[THREAD_NUM]; if (argc > 1) { repeat = atoi(argv[1]); } for (i = 0; i < THREAD_NUM; i++) { ThreadArg* arg = calloc(sizeof(ThreadArg), 1); arg->delay = i * 100; pthread_create(threads + i, NULL, thread_fn, arg); } start_counting(); for (i = 0; i < repeat; i++) { global += i; } end_counting(); for (i = 0; i < THREAD_NUM; i++) { pthread_join(threads[i], NULL); } return 0; } ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-05-24 14:11 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-20 9:15 [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times Nikolay Igotti 2020-04-20 9:51 ` Laurent Vivier 2020-04-20 10:04 ` Nikolay Igotti 2020-05-09 23:00 ` Emilio G. Cota 2020-05-11 15:53 ` Nikolay Igotti 2020-05-12 0:55 ` Emilio G. Cota 2020-05-12 8:38 ` Nikolay Igotti 2020-05-12 19:35 ` Alex Bennée 2020-05-12 20:11 ` Alex Bennée 2020-05-24 14:11 ` Emilio G. Cota 2020-04-20 10:11 ` Peter Maydell 2020-04-20 10:18 ` Nikolay Igotti 2020-04-20 10:30 ` Peter Maydell 2020-04-20 15:07 ` Alex Bennée 2020-04-20 16:14 ` Nikolay Igotti
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.