Mahmoud Mandour <ma.mandourr@gmail.com> writes:
> Since callbacks may be interleaved because of multithreaded execution,
> we should not make assumptions about `plugin_exit` either. The problem
> with `plugin_exit` is that it frees shared data structures (caches and
> `miss_ht` hash table). It should not be assumed that callbacks will not
> be called after it and hence use already-freed data structures.
What was your test case for this because I wonder if it would be worth
coming up with one for check-tcg?
I think just any ad-hoc multithreaded execution will evoke the race pretty much
consistently.
From what I remember the race is
in between preexit_cleanup and the eventual _exit/exit_group which nixes
all other child threads. Maybe we should be triggering a more graceful
unload here?
I think so. This remedies the bug for this particular plugin and I think there
would be a better solution of course. However, I just can't ever get plugin_exit
callback to be called more than once so I think that's probably not the problem?
The problem is that we *use* the data in translation/mem_access/exec callbacks
after a plugin_exit call is already called (this can be easily verified by having a
boolean set to true once plugin_exit is called and then g_assert this boolean is
false in the callbacks)
> This is mitigated in this commit by synchronizing the call to
> `plugin_exit` through locking to ensure execlusive access to data
> structures, and NULL-ifying those data structures so that subsequent
> callbacks can check whether the data strucutres are freed, and if so,
> immediately exit.
>
> It's okay to immediately exit and don't account for those callbacks
> since they won't be accounted for anyway since `plugin_exit` is already
> called once and reported the statistics.
>
> Signed-off-by: Mahmoud Mandour <ma.mandourr@gmail.com>
> ---
> contrib/plugins/cache.c | 31 ++++++++++++++++++++++++++++++-
> 1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/plugins/cache.c b/contrib/plugins/cache.c
> index 695fb969dc..a452aba01c 100644
> --- a/contrib/plugins/cache.c
> +++ b/contrib/plugins/cache.c
> @@ -363,6 +363,11 @@ static void vcpu_mem_access(unsigned int vcpu_index, qemu_plugin_meminfo_t info,
> effective_addr = hwaddr ? qemu_plugin_hwaddr_phys_addr(hwaddr) : vaddr;
>
> g_mutex_lock(&mtx);
> + if (dcache == NULL) {
> + g_mutex_unlock(&mtx);
> + return;
> + }
> +
> if (!access_cache(dcache, effective_addr)) {
> insn = (InsnData *) userdata;
> insn->dmisses++;
> @@ -380,6 +385,11 @@ static void vcpu_insn_exec(unsigned int vcpu_index, void *userdata)
> g_mutex_lock(&mtx);
> insn_addr = ((InsnData *) userdata)->addr;
>
> + if (icache == NULL) {
> + g_mutex_unlock(&mtx);
> + return;
> + }
> +
> if (!access_cache(icache, insn_addr)) {
> insn = (InsnData *) userdata;
> insn->imisses++;
> @@ -406,12 +416,24 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
> effective_addr = (uint64_t) qemu_plugin_insn_vaddr(insn);
> }
>
> + g_mutex_lock(&mtx);
> +
> + /*
> + * is the plugin_exit callback called? If so, any further callback
> + * registration is useless as it won't get accounted for after calling
> + * plugin_exit once already, and also will use miss_ht after it's freed
> + */
> + if (miss_ht == NULL) {
> + g_mutex_unlock(&mtx);
> + return;
> + }
> +
> /*
> * Instructions might get translated multiple times, we do not create
> * new entries for those instructions. Instead, we fetch the same
> * entry from the hash table and register it for the callback again.
> */
> - g_mutex_lock(&mtx);
> +
> data = "" GUINT_TO_POINTER(effective_addr));
> if (data == NULL) {
> data = "" 1);
> @@ -527,13 +549,20 @@ static void log_top_insns()
>
> static void plugin_exit(qemu_plugin_id_t id, void *p)
> {
> + g_mutex_lock(&mtx);
> log_stats();
> log_top_insns();
>
> cache_free(dcache);
> + dcache = NULL;
> +
> cache_free(icache);
> + icache = NULL;
>
> g_hash_table_destroy(miss_ht);
> + miss_ht = NULL;
> +
> + g_mutex_unlock(&mtx);
> }
>
> static void policy_init()
--
Alex Bennée