mbox series

[RFC,v1,0/2] tcg-cpus: split into 3 tcg variants

Message ID 20201014073605.6155-1-cfontana@suse.de
Headers show
Series tcg-cpus: split into 3 tcg variants | expand

Message

Claudio Fontana Oct. 14, 2020, 7:36 a.m. UTC
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

Comments

Philippe Mathieu-Daudé Oct. 14, 2020, 8 a.m. UTC | #1
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!
Alex Bennée Oct. 14, 2020, 10:14 a.m. UTC | #2
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
Claudio Fontana Oct. 14, 2020, 10:21 a.m. UTC | #3
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

> 

>
Philippe Mathieu-Daudé Oct. 14, 2020, 11:21 a.m. UTC | #4
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.