| Message ID | 20201014073605.6155-1-cfontana@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | tcg-cpus: split into 3 tcg variants | expand |
On 10/14/20 9:36 AM, Claudio Fontana wrote: > The purpose of this series is to split the tcg-cpus into > 3 variants: > > tcg_cpus_mttcg (multithreaded tcg vcpus) > tcg_cpus_rr (single threaded round robin vcpus) > tcg_cpus_icount (same as RR, but using icount) Good idea!
Claudio Fontana <cfontana@suse.de> writes: > The purpose of this series is to split the tcg-cpus into > 3 variants: > > tcg_cpus_mttcg (multithreaded tcg vcpus) > tcg_cpus_rr (single threaded round robin vcpus) > tcg_cpus_icount (same as RR, but using icount) I've no objection to the cosmetic clean-up but I assume the 3 modes will still be available in TCG enabled binaries. > > Alex, I read the comment in tcg_start_vcpu_thread saying: > > /* > * Initialize TCG regions--once. Now is a good time, because: > * (1) TCG's init context, prologue and target globals have been set up. > * (2) qemu_tcg_mttcg_enabled() works now (TCG init code runs before the > * -accel flag is processed, so the check doesn't work then). > */ > > Is this actually current? Hmm probably not. Now everything is tied to the order of class initialisation and realisation. AIUI all properties set by the command line should be complete by the time an object realizes and parent classes should be processed before their children. > > I tried to refactor this (see patch 2), and it seems to work to do > the init of regions in tcg_init, and it seems that mttcg_enabled is known > already at that point.. > > Ciao, > > Claudio > > Claudio Fontana (2): > accel/tcg: split CpusAccel into three TCG variants > accel/tcg: split tcg_start_vcpu_thread > > accel/tcg/meson.build | 9 +- > accel/tcg/tcg-all.c | 13 +- > accel/tcg/tcg-cpus-icount.c | 145 +++++++++++ > accel/tcg/tcg-cpus-icount.h | 20 ++ > accel/tcg/tcg-cpus-mttcg.c | 142 ++++++++++ > accel/tcg/tcg-cpus-mttcg.h | 25 ++ > accel/tcg/tcg-cpus-rr.c | 305 ++++++++++++++++++++++ > accel/tcg/tcg-cpus-rr.h | 26 ++ > accel/tcg/tcg-cpus.c | 500 +----------------------------------- > accel/tcg/tcg-cpus.h | 9 +- > softmmu/icount.c | 2 +- > 11 files changed, 697 insertions(+), 499 deletions(-) > create mode 100644 accel/tcg/tcg-cpus-icount.c > create mode 100644 accel/tcg/tcg-cpus-icount.h > create mode 100644 accel/tcg/tcg-cpus-mttcg.c > create mode 100644 accel/tcg/tcg-cpus-mttcg.h > create mode 100644 accel/tcg/tcg-cpus-rr.c > create mode 100644 accel/tcg/tcg-cpus-rr.h
On 10/14/20 12:14 PM, Alex Bennée wrote: > > Claudio Fontana <cfontana@suse.de> writes: > >> The purpose of this series is to split the tcg-cpus into >> 3 variants: >> >> tcg_cpus_mttcg (multithreaded tcg vcpus) >> tcg_cpus_rr (single threaded round robin vcpus) >> tcg_cpus_icount (same as RR, but using icount) > > I've no objection to the cosmetic clean-up but I assume the 3 modes will > still be available in TCG enabled binaries. Hi Alex, yes, all three will be available in the code when enabling TCG. > >> >> Alex, I read the comment in tcg_start_vcpu_thread saying: >> >> /* >> * Initialize TCG regions--once. Now is a good time, because: >> * (1) TCG's init context, prologue and target globals have been set up. >> * (2) qemu_tcg_mttcg_enabled() works now (TCG init code runs before the >> * -accel flag is processed, so the check doesn't work then). >> */ >> >> Is this actually current? > > Hmm probably not. Now everything is tied to the order of class > initialisation and realisation. AIUI all properties set by the command > line should be complete by the time an object realizes and parent > classes should be processed before their children. This is great news, as it allows more refactoring as showed on patch 2, thanks a lot! Ciao, Claudio > >> >> I tried to refactor this (see patch 2), and it seems to work to do >> the init of regions in tcg_init, and it seems that mttcg_enabled is known >> already at that point.. >> >> Ciao, >> >> Claudio >> >> Claudio Fontana (2): >> accel/tcg: split CpusAccel into three TCG variants >> accel/tcg: split tcg_start_vcpu_thread >> >> accel/tcg/meson.build | 9 +- >> accel/tcg/tcg-all.c | 13 +- >> accel/tcg/tcg-cpus-icount.c | 145 +++++++++++ >> accel/tcg/tcg-cpus-icount.h | 20 ++ >> accel/tcg/tcg-cpus-mttcg.c | 142 ++++++++++ >> accel/tcg/tcg-cpus-mttcg.h | 25 ++ >> accel/tcg/tcg-cpus-rr.c | 305 ++++++++++++++++++++++ >> accel/tcg/tcg-cpus-rr.h | 26 ++ >> accel/tcg/tcg-cpus.c | 500 +----------------------------------- >> accel/tcg/tcg-cpus.h | 9 +- >> softmmu/icount.c | 2 +- >> 11 files changed, 697 insertions(+), 499 deletions(-) >> create mode 100644 accel/tcg/tcg-cpus-icount.c >> create mode 100644 accel/tcg/tcg-cpus-icount.h >> create mode 100644 accel/tcg/tcg-cpus-mttcg.c >> create mode 100644 accel/tcg/tcg-cpus-mttcg.h >> create mode 100644 accel/tcg/tcg-cpus-rr.c >> create mode 100644 accel/tcg/tcg-cpus-rr.h > >
On 10/14/20 12:14 PM, Alex Bennée wrote: > > Claudio Fontana <cfontana@suse.de> writes: > >> The purpose of this series is to split the tcg-cpus into >> 3 variants: >> >> tcg_cpus_mttcg (multithreaded tcg vcpus) >> tcg_cpus_rr (single threaded round robin vcpus) >> tcg_cpus_icount (same as RR, but using icount) > > I've no objection to the cosmetic clean-up but I assume the 3 modes will > still be available in TCG enabled binaries. Yes, I think so too, no point in disabling some. However it seems to me it is now easier to review for a newcomer. Code easily understandable/reviewable is easier to maintain and less bug prone :) Phil.
The purpose of this series is to split the tcg-cpus into 3 variants: tcg_cpus_mttcg (multithreaded tcg vcpus) tcg_cpus_rr (single threaded round robin vcpus) tcg_cpus_icount (same as RR, but using icount) Alex, I read the comment in tcg_start_vcpu_thread saying: /* * Initialize TCG regions--once. Now is a good time, because: * (1) TCG's init context, prologue and target globals have been set up. * (2) qemu_tcg_mttcg_enabled() works now (TCG init code runs before the * -accel flag is processed, so the check doesn't work then). */ Is this actually current? I tried to refactor this (see patch 2), and it seems to work to do the init of regions in tcg_init, and it seems that mttcg_enabled is known already at that point.. Ciao, Claudio Claudio Fontana (2): accel/tcg: split CpusAccel into three TCG variants accel/tcg: split tcg_start_vcpu_thread accel/tcg/meson.build | 9 +- accel/tcg/tcg-all.c | 13 +- accel/tcg/tcg-cpus-icount.c | 145 +++++++++++ accel/tcg/tcg-cpus-icount.h | 20 ++ accel/tcg/tcg-cpus-mttcg.c | 142 ++++++++++ accel/tcg/tcg-cpus-mttcg.h | 25 ++ accel/tcg/tcg-cpus-rr.c | 305 ++++++++++++++++++++++ accel/tcg/tcg-cpus-rr.h | 26 ++ accel/tcg/tcg-cpus.c | 500 +----------------------------------- accel/tcg/tcg-cpus.h | 9 +- softmmu/icount.c | 2 +- 11 files changed, 697 insertions(+), 499 deletions(-) create mode 100644 accel/tcg/tcg-cpus-icount.c create mode 100644 accel/tcg/tcg-cpus-icount.h create mode 100644 accel/tcg/tcg-cpus-mttcg.c create mode 100644 accel/tcg/tcg-cpus-mttcg.h create mode 100644 accel/tcg/tcg-cpus-rr.c create mode 100644 accel/tcg/tcg-cpus-rr.h