| Message ID | 20181025172057.20414-11-cota@braap.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Plugin support | expand |
Emilio G. Cota <cota@braap.org> writes: > This will be used by plugin code to flush the code cache as well > as doing other bookkeeping in a safe work environment. This seems a little excessive given the plugin code could just call tb_flush() directly. Wouldn't calling tb_flush after scheduling the plugin_destroy be enough? If there is a race condition here maybe we could build some sort of awareness into tb_flush as to the current run state. But having two entry points to this rather fundamental action seems likely to either be misused or misunderstood. > > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > include/exec/exec-all.h | 1 + > accel/tcg/translate-all.c | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h > index 815e5b1e83..232e2f8966 100644 > --- a/include/exec/exec-all.h > +++ b/include/exec/exec-all.h > @@ -427,6 +427,7 @@ void tb_invalidate_phys_range(target_ulong start, target_ulong end); > void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs); > #endif > void tb_flush(CPUState *cpu); > +void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count); > void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr); > TranslationBlock *tb_htable_lookup(CPUState *cpu, target_ulong pc, > target_ulong cs_base, uint32_t flags, > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c > index c8b3e0a491..db2d28f8d3 100644 > --- a/accel/tcg/translate-all.c > +++ b/accel/tcg/translate-all.c > @@ -1230,7 +1230,7 @@ static gboolean tb_host_size_iter(gpointer key, gpointer value, gpointer data) > } > > /* flush all the translation blocks */ > -static void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) > +void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) > { > mmap_lock(); > /* If it is already been done on request of another CPU, -- Alex Bennée
On Thu, Nov 22, 2018 at 17:09:22 +0000, Alex Bennée wrote: > > Emilio G. Cota <cota@braap.org> writes: > > > This will be used by plugin code to flush the code cache as well > > as doing other bookkeeping in a safe work environment. > > This seems a little excessive given the plugin code could just call > tb_flush() directly. Wouldn't calling tb_flush after scheduling the > plugin_destroy be enough? > > If there is a race condition here maybe we could build some sort of > awareness into tb_flush as to the current run state. But having two > entry points to this rather fundamental action seems likely to either be > misused or misunderstood. We have to make sure that no callback left in the generated code is called once a plugin has been uninstalled. To me, using the same safe work window to both flush the TB and uninstall the plugin seems the simplest way to do this. Thanks, Emilio
Emilio G. Cota <cota@braap.org> writes: > On Thu, Nov 22, 2018 at 17:09:22 +0000, Alex Bennée wrote: >> >> Emilio G. Cota <cota@braap.org> writes: >> >> > This will be used by plugin code to flush the code cache as well >> > as doing other bookkeeping in a safe work environment. >> >> This seems a little excessive given the plugin code could just call >> tb_flush() directly. Wouldn't calling tb_flush after scheduling the >> plugin_destroy be enough? >> >> If there is a race condition here maybe we could build some sort of >> awareness into tb_flush as to the current run state. But having two >> entry points to this rather fundamental action seems likely to either be >> misused or misunderstood. > > We have to make sure that no callback left in the generated code is > called once a plugin has been uninstalled. To me, using the same safe > work window to both flush the TB and uninstall the plugin seems the > simplest way to do this. I still think making tb_flush() aware that it can run in an exclusive period would be a better solution than exposing two functions for the operation. So tb_flush could be something like: void tb_flush(CPUState *cpu) { if (tcg_enabled()) { unsigned tb_flush_count = atomic_mb_read(&tb_ctx.tb_flush_count); if (cpu_current_and_exclusive(cpu)) { do_tb_flush(RUN_ON_CPU_HOST_INT(tb_flush_count)) } else { async_safe_run_on_cpu(cpu, do_tb_flush, RUN_ON_CPU_HOST_INT(tb_flush_count)); } } } Or possibly push that logic down into async_safe_run_on_cpu()? -- Alex Bennée
On Mon, Nov 26, 2018 at 11:11:53 +0000, Alex Bennée wrote: > > Emilio G. Cota <cota@braap.org> writes: > > > On Thu, Nov 22, 2018 at 17:09:22 +0000, Alex Bennée wrote: > >> > >> Emilio G. Cota <cota@braap.org> writes: > >> > >> > This will be used by plugin code to flush the code cache as well > >> > as doing other bookkeeping in a safe work environment. > >> > >> This seems a little excessive given the plugin code could just call > >> tb_flush() directly. Wouldn't calling tb_flush after scheduling the > >> plugin_destroy be enough? > >> > >> If there is a race condition here maybe we could build some sort of > >> awareness into tb_flush as to the current run state. But having two > >> entry points to this rather fundamental action seems likely to either be > >> misused or misunderstood. > > > > We have to make sure that no callback left in the generated code is > > called once a plugin has been uninstalled. To me, using the same safe > > work window to both flush the TB and uninstall the plugin seems the > > simplest way to do this. > > I still think making tb_flush() aware that it can run in an exclusive > period would be a better solution than exposing two functions for the > operation. So tb_flush could be something like: > > void tb_flush(CPUState *cpu) > { > if (tcg_enabled()) { > unsigned tb_flush_count = atomic_mb_read(&tb_ctx.tb_flush_count); > if (cpu_current_and_exclusive(cpu)) { > do_tb_flush(RUN_ON_CPU_HOST_INT(tb_flush_count)) > } else { > async_safe_run_on_cpu(cpu, do_tb_flush, > RUN_ON_CPU_HOST_INT(tb_flush_count)); > } > } > } > > Or possibly push that logic down into async_safe_run_on_cpu()? The latter option would be much harder, because in async_safe_run_on_cpu we always queue the work and kick the CPU (which could be ourselves). IOW the job is always asynchronous, as the name implies. I've thus implemented the former in v2, as follows (I'm using a hole in struct CPUState to add the bool): @@ -1277,8 +1277,13 @@ void tb_flush(CPUState *cpu) { if (tcg_enabled()) { unsigned tb_flush_count = atomic_mb_read(&tb_ctx.tb_flush_count); - async_safe_run_on_cpu(cpu, do_tb_flush, - RUN_ON_CPU_HOST_INT(tb_flush_count)); + + if (cpu_in_exclusive_work_context(cpu)) { + do_tb_flush(cpu, RUN_ON_CPU_HOST_INT(tb_flush_count)); + } else { + async_safe_run_on_cpu(cpu, do_tb_flush, + RUN_ON_CPU_HOST_INT(tb_flush_count)); + } } } +++ b/cpus-common.c @@ -386,7 +386,9 @@ static void process_queued_cpu_work_locked(CPUState *cpu) qemu_mutex_unlock_iothread(); } start_exclusive(); + cpu->in_exclusive_work_context = true; wi->func(cpu, wi->data); + cpu->in_exclusive_work_context = false; end_exclusive(); I've also fixed a couple of unrelated bugs when uninstalling a plugin with memory callbacks enabled. Thanks, Emilio
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h index 815e5b1e83..232e2f8966 100644 --- a/include/exec/exec-all.h +++ b/include/exec/exec-all.h @@ -427,6 +427,7 @@ void tb_invalidate_phys_range(target_ulong start, target_ulong end); void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs); #endif void tb_flush(CPUState *cpu); +void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count); void tb_phys_invalidate(TranslationBlock *tb, tb_page_addr_t page_addr); TranslationBlock *tb_htable_lookup(CPUState *cpu, target_ulong pc, target_ulong cs_base, uint32_t flags, diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index c8b3e0a491..db2d28f8d3 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -1230,7 +1230,7 @@ static gboolean tb_host_size_iter(gpointer key, gpointer value, gpointer data) } /* flush all the translation blocks */ -static void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) +void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) { mmap_lock(); /* If it is already been done on request of another CPU,
This will be used by plugin code to flush the code cache as well as doing other bookkeeping in a safe work environment. Signed-off-by: Emilio G. Cota <cota@braap.org> --- include/exec/exec-all.h | 1 + accel/tcg/translate-all.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)