| Message ID | 20181025172057.20414-20-cota@braap.org |
|---|---|
| State | New |
| Headers | show |
| Series | Plugin support | expand |
Emilio G. Cota <cota@braap.org> writes: > Signed-off-by: Emilio G. Cota <cota@braap.org> > --- > accel/tcg/translate-all.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c > index 3423cf74db..1690e3fd5b 100644 > --- a/accel/tcg/translate-all.c > +++ b/accel/tcg/translate-all.c > @@ -1233,6 +1233,8 @@ static gboolean tb_host_size_iter(gpointer key, gpointer value, gpointer data) > /* flush all the translation blocks */ > void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) > { > + bool did_flush = false; > + > mmap_lock(); > /* If it is already been done on request of another CPU, > * just retry. > @@ -1240,6 +1242,7 @@ void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) > if (tb_ctx.tb_flush_count != tb_flush_count.host_int) { > goto done; > } > + did_flush = true; > > if (DEBUG_TB_FLUSH_GATE) { > size_t nb_tbs = tcg_nb_tbs(); > @@ -1265,6 +1268,9 @@ void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) > > done: > mmap_unlock(); > + if (did_flush) { > + qemu_plugin_flush_cb(); > + } Are we introducing a race here? What is the purpose of letting the plugin know a flush has occurred? It shouldn't have any knowledge of the details of liveliness of the translated code and if it still exits or not. If all it wants to do is look at the counts then I think we can provide a simpler less abuse-able way to do this. > } > > void tb_flush(CPUState *cpu) -- Alex Bennée
On Fri, Nov 23, 2018 at 17:00:59 +0000, Alex Bennée wrote: > > Emilio G. Cota <cota@braap.org> writes: > > > Signed-off-by: Emilio G. Cota <cota@braap.org> > > --- > > accel/tcg/translate-all.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c > > index 3423cf74db..1690e3fd5b 100644 > > --- a/accel/tcg/translate-all.c > > +++ b/accel/tcg/translate-all.c > > @@ -1233,6 +1233,8 @@ static gboolean tb_host_size_iter(gpointer key, gpointer value, gpointer data) > > /* flush all the translation blocks */ > > void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) > > { > > + bool did_flush = false; > > + > > mmap_lock(); > > /* If it is already been done on request of another CPU, > > * just retry. > > @@ -1240,6 +1242,7 @@ void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) > > if (tb_ctx.tb_flush_count != tb_flush_count.host_int) { > > goto done; > > } > > + did_flush = true; > > > > if (DEBUG_TB_FLUSH_GATE) { > > size_t nb_tbs = tcg_nb_tbs(); > > @@ -1265,6 +1268,9 @@ void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) > > > > done: > > mmap_unlock(); > > + if (did_flush) { > > + qemu_plugin_flush_cb(); > > + } > > Are we introducing a race here? A race, how? We're in an async safe environment here, i.e. no other vCPU is running. > What is the purpose of letting the plugin know a flush has occurred? Plugins might allocate per-TB data that then they get passed each time the TB is executed (via the *userdata pointer). For example, in a simulator we'd allocate a per-TB struct that describes the guest instructions, after having disassembled them at translate time. It is therefore useful for plugins to know when all TB's have been flushed, so that they can then free that per-TB data. > It shouldn't have any knowledge of the details of liveliness of the > translated code and if it still exits or not. If all it wants to do is > look at the counts then I think we can provide a simpler less abuse-able > way to do this. I'm confused. What does "look at the counts" mean here? To reiterate, plugins should have a way to know when a TB doesn't exist any longer, so that they can reclaim memory. Thanks, Emilio
Emilio G. Cota <cota@braap.org> writes: > On Fri, Nov 23, 2018 at 17:00:59 +0000, Alex Bennée wrote: >> >> Emilio G. Cota <cota@braap.org> writes: >> >> > Signed-off-by: Emilio G. Cota <cota@braap.org> >> > --- >> > accel/tcg/translate-all.c | 6 ++++++ >> > 1 file changed, 6 insertions(+) >> > >> > diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c >> > index 3423cf74db..1690e3fd5b 100644 >> > --- a/accel/tcg/translate-all.c >> > +++ b/accel/tcg/translate-all.c >> > @@ -1233,6 +1233,8 @@ static gboolean tb_host_size_iter(gpointer key, gpointer value, gpointer data) >> > /* flush all the translation blocks */ >> > void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) >> > { >> > + bool did_flush = false; >> > + >> > mmap_lock(); >> > /* If it is already been done on request of another CPU, >> > * just retry. >> > @@ -1240,6 +1242,7 @@ void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) >> > if (tb_ctx.tb_flush_count != tb_flush_count.host_int) { >> > goto done; >> > } >> > + did_flush = true; >> > >> > if (DEBUG_TB_FLUSH_GATE) { >> > size_t nb_tbs = tcg_nb_tbs(); >> > @@ -1265,6 +1268,9 @@ void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) >> > >> > done: >> > mmap_unlock(); >> > + if (did_flush) { >> > + qemu_plugin_flush_cb(); >> > + } >> >> Are we introducing a race here? > > A race, how? We're in an async safe environment here, i.e. no other > vCPU is running. You are right, I was thrown by the mmap_lock/unlocks - but I guess even they aren't needed now if tb flush is always in an exclusive context. > >> What is the purpose of letting the plugin know a flush has occurred? > > Plugins might allocate per-TB data that then they get passed each > time the TB is executed (via the *userdata pointer). For example, > in a simulator we'd allocate a per-TB struct that describes the > guest instructions, after having disassembled them at translate time. > > It is therefore useful for plugins to know when all TB's have been > flushed, so that they can then free that per-TB data. Would they need a generation count propagated here or would it just be flush anything translation related at this point? >> It shouldn't have any knowledge of the details of liveliness of the >> translated code and if it still exits or not. If all it wants to do is >> look at the counts then I think we can provide a simpler less abuse-able >> way to do this. > > I'm confused. What does "look at the counts" mean here? > > To reiterate, plugins should have a way to know when a TB doesn't > exist any longer, so that they can reclaim memory. OK that makes sense. Could you expand the commit message just to explain the use case? Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -- Alex Bennée
On Mon, Nov 26, 2018 at 11:02:24 +0000, Alex Bennée wrote: > Emilio G. Cota <cota@braap.org> writes: > > On Fri, Nov 23, 2018 at 17:00:59 +0000, Alex Bennée wrote: > >> What is the purpose of letting the plugin know a flush has occurred? > > > > Plugins might allocate per-TB data that then they get passed each > > time the TB is executed (via the *userdata pointer). For example, > > in a simulator we'd allocate a per-TB struct that describes the > > guest instructions, after having disassembled them at translate time. > > > > It is therefore useful for plugins to know when all TB's have been > > flushed, so that they can then free that per-TB data. > > Would they need a generation count propagated here or would it just be > flush anything translation related at this point? The callback is guaranteed to be called in an exclusive context, so plugins can just rely on that; since no code execution can happen at that time, there's no need to pass further info, since the plugins can free that data right from the callback. > >> It shouldn't have any knowledge of the details of liveliness of the > >> translated code and if it still exits or not. If all it wants to do is > >> look at the counts then I think we can provide a simpler less abuse-able > >> way to do this. > > > > I'm confused. What does "look at the counts" mean here? > > > > To reiterate, plugins should have a way to know when a TB doesn't > > exist any longer, so that they can reclaim memory. > > OK that makes sense. Could you expand the commit message just to explain > the use case? > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> Updated the commit message with: translate-all: notify plugin code of tb_flush Plugins might allocate per-TB data that then they get passed each time a TB is executed (via the *userdata pointer). Notify plugin code every time a code cache flush occurs, so that plugins can then reclaim the memory of the per-TB data. Thanks, Emilio
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c index 3423cf74db..1690e3fd5b 100644 --- a/accel/tcg/translate-all.c +++ b/accel/tcg/translate-all.c @@ -1233,6 +1233,8 @@ static gboolean tb_host_size_iter(gpointer key, gpointer value, gpointer data) /* flush all the translation blocks */ void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) { + bool did_flush = false; + mmap_lock(); /* If it is already been done on request of another CPU, * just retry. @@ -1240,6 +1242,7 @@ void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) if (tb_ctx.tb_flush_count != tb_flush_count.host_int) { goto done; } + did_flush = true; if (DEBUG_TB_FLUSH_GATE) { size_t nb_tbs = tcg_nb_tbs(); @@ -1265,6 +1268,9 @@ void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count) done: mmap_unlock(); + if (did_flush) { + qemu_plugin_flush_cb(); + } } void tb_flush(CPUState *cpu)
Signed-off-by: Emilio G. Cota <cota@braap.org> --- accel/tcg/translate-all.c | 6 ++++++ 1 file changed, 6 insertions(+)