[PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Nikolay Igotti]

[-- 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 --]

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Laurent Vivier]

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)
> 

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Nikolay Igotti]

[-- 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 --]

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Emilio G. Cota]

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

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Nikolay Igotti]

[-- 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 --]

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Emilio G. Cota]

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

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Nikolay Igotti]

[-- 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 --]

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Alex Bennée]

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

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Alex Bennée]

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

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Emilio G. Cota]

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

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Peter Maydell]

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

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Nikolay Igotti]

[-- 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 --]

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Peter Maydell]

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

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Alex Bennée]

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

Re: [PATCH 3/3] plugins: avoid failing plugin when CPU is inited several times

[Nikolay Igotti]

[-- 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;
}