Re: [Qemu-devel] [RFC 19/48] translate-all: notify plugin code of tb_flush

[Alex Bennée]

Emilio G. Cota <address@hidden> writes:

> On Fri, Nov 23, 2018 at 17:00:59 +0000, Alex Bennée wrote:
>>
>> Emilio G. Cota <address@hidden> writes:
>>
>> > Signed-off-by: Emilio G. Cota <address@hidden>
>> > ---
>> >  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 <address@hidden>

--
Alex Bennée