| Message ID | 1460730231-1184-14-git-send-email-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 15/04/16 17:23, Alex Bennée wrote: > This makes multi-threading the default for 32 bit ARM on x86. It has > been tested with Debian Jessie as well as my extended KVM unit tests > which stress the SMC and TB invalidation code. Those tests can be found > at: > > https://github.com/stsquad/kvm-unit-tests/tree/mttcg/current-tests-v5 > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > cpus.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/cpus.c b/cpus.c > index 860e2a9..daa92c7 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -171,12 +171,24 @@ opts_init(tcg_register_config); > > static bool default_mttcg_enabled(void) > { > - /* > - * TODO: Check if we have a chance to have MTTCG working on this guest/host. > - * Basically is the atomic instruction implemented? Is there any > - * memory ordering issue? > + /* Checklist for enabling MTTCG on a given frontend/backend combination > + * > + * - Are atomics correctly modelled for an MTTCG environment - Are cross-CPU manipulations safe (e.g. TLB invalidation/flush from target helper) - Are TCG context manipulations safe (e.g. TB invalidation from target helper) > + * - If the backend is weakly ordered > + * - has the front-end implemented explicit memory ordering ops > + * - does the back-end generate code to ensure memory ordering > */ > +#if defined(__i386__) || defined(__x86_64__) > + /* x86 backend is strongly ordered which helps a lot */ > + #if defined(TARGET_ARM) > + return true; > + #else > + return false; > + #endif Is it okay to indent preprocessor lines this way? I think preprocessor lines are better to stand out from regular code and could be indented like this: #if defined(__foo__) # if defined(BAR) /* ... */ # else /* ... */ # endif #else /* ... */ #endif Kind regards, Sergey > +#else > + /* Until memory ordering implemented things will likely break */ > return false; > +#endif > } > > void qemu_tcg_configure(QemuOpts *opts)
Sergey Fedorov <serge.fdrv@gmail.com> writes: > On 15/04/16 17:23, Alex Bennée wrote: >> This makes multi-threading the default for 32 bit ARM on x86. It has >> been tested with Debian Jessie as well as my extended KVM unit tests >> which stress the SMC and TB invalidation code. Those tests can be found >> at: >> >> https://github.com/stsquad/kvm-unit-tests/tree/mttcg/current-tests-v5 >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> cpus.c | 20 ++++++++++++++++---- >> 1 file changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/cpus.c b/cpus.c >> index 860e2a9..daa92c7 100644 >> --- a/cpus.c >> +++ b/cpus.c >> @@ -171,12 +171,24 @@ opts_init(tcg_register_config); >> >> static bool default_mttcg_enabled(void) >> { >> - /* >> - * TODO: Check if we have a chance to have MTTCG working on this guest/host. >> - * Basically is the atomic instruction implemented? Is there any >> - * memory ordering issue? >> + /* Checklist for enabling MTTCG on a given frontend/backend combination >> + * >> + * - Are atomics correctly modelled for an MTTCG environment > > - Are cross-CPU manipulations safe (e.g. TLB invalidation/flush from > target helper) > - Are TCG context manipulations safe (e.g. TB invalidation from target > helper) OK > >> + * - If the backend is weakly ordered >> + * - has the front-end implemented explicit memory ordering ops >> + * - does the back-end generate code to ensure memory ordering >> */ >> +#if defined(__i386__) || defined(__x86_64__) >> + /* x86 backend is strongly ordered which helps a lot */ >> + #if defined(TARGET_ARM) >> + return true; >> + #else >> + return false; >> + #endif > > Is it okay to indent preprocessor lines this way? I think preprocessor > lines are better to stand out from regular code and could be indented > like this: > > #if defined(__foo__) > # if defined(BAR) > /* ... */ > # else > /* ... */ > # endif > #else > /* ... */ > #endif To be honest I was expecting more push-back on this because it is such an ugly way of solving the problem and expressing what a default on means. > > Kind regards, > Sergey > >> +#else >> + /* Until memory ordering implemented things will likely break */ >> return false; >> +#endif >> } >> >> void qemu_tcg_configure(QemuOpts *opts) -- Alex Bennée
On 06/06/16 11:58, Alex Bennée wrote: > Sergey Fedorov <serge.fdrv@gmail.com> writes: > >> On 15/04/16 17:23, Alex Bennée wrote: >>> diff --git a/cpus.c b/cpus.c >>> index 860e2a9..daa92c7 100644 >>> --- a/cpus.c >>> +++ b/cpus.c >>> @@ -171,12 +171,24 @@ opts_init(tcg_register_config); >>> >>> static bool default_mttcg_enabled(void) >>> { >>> - /* >>> - * TODO: Check if we have a chance to have MTTCG working on this guest/host. >>> - * Basically is the atomic instruction implemented? Is there any >>> - * memory ordering issue? >>> + /* Checklist for enabling MTTCG on a given frontend/backend combination >>> + * >>> + * - Are atomics correctly modelled for an MTTCG environment >> - Are cross-CPU manipulations safe (e.g. TLB invalidation/flush from >> target helper) >> - Are TCG context manipulations safe (e.g. TB invalidation from target >> helper) > OK > >>> + * - If the backend is weakly ordered >>> + * - has the front-end implemented explicit memory ordering ops >>> + * - does the back-end generate code to ensure memory ordering >>> */ >>> +#if defined(__i386__) || defined(__x86_64__) >>> + /* x86 backend is strongly ordered which helps a lot */ >>> + #if defined(TARGET_ARM) >>> + return true; >>> + #else >>> + return false; >>> + #endif >> Is it okay to indent preprocessor lines this way? I think preprocessor >> lines are better to stand out from regular code and could be indented >> like this: >> >> #if defined(__foo__) >> # if defined(BAR) >> /* ... */ >> # else >> /* ... */ >> # endif >> #else >> /* ... */ >> #endif > To be honest I was expecting more push-back on this because it is such > an ugly way of solving the problem and expressing what a default on > means. I could be okay as long as there are only a few options. We could also put here some generic tests like strong/weak ordering checks and introduce target- and host-specific functions which can tell us if we should ever try enabling MTTCG for them. Kind regards, Sergey
On 15 April 2016 at 15:23, Alex Bennée <alex.bennee@linaro.org> wrote: > This makes multi-threading the default for 32 bit ARM on x86. It has > been tested with Debian Jessie as well as my extended KVM unit tests > which stress the SMC and TB invalidation code. Those tests can be found > at: > > https://github.com/stsquad/kvm-unit-tests/tree/mttcg/current-tests-v5 > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > cpus.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/cpus.c b/cpus.c > index 860e2a9..daa92c7 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -171,12 +171,24 @@ opts_init(tcg_register_config); > > static bool default_mttcg_enabled(void) > { > - /* > - * TODO: Check if we have a chance to have MTTCG working on this guest/host. > - * Basically is the atomic instruction implemented? Is there any > - * memory ordering issue? > + /* Checklist for enabling MTTCG on a given frontend/backend combination > + * > + * - Are atomics correctly modelled for an MTTCG environment > + * - If the backend is weakly ordered > + * - has the front-end implemented explicit memory ordering ops > + * - does the back-end generate code to ensure memory ordering > */ > +#if defined(__i386__) || defined(__x86_64__) > + /* x86 backend is strongly ordered which helps a lot */ > + #if defined(TARGET_ARM) > + return true; > + #else > + return false; > + #endif > +#else > + /* Until memory ordering implemented things will likely break */ > return false; > +#endif No new per-host ifdef ladders, please (or per-target ifdef ladders, either). Have some #defines for "TCG backend supports MTTCG" and "TCG frontend supports MTTCG" which get set in some suitable per-host and per-target header, and only enable if they're both set. thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 15 April 2016 at 15:23, Alex Bennée <alex.bennee@linaro.org> wrote: >> This makes multi-threading the default for 32 bit ARM on x86. It has >> been tested with Debian Jessie as well as my extended KVM unit tests >> which stress the SMC and TB invalidation code. Those tests can be found >> at: >> >> https://github.com/stsquad/kvm-unit-tests/tree/mttcg/current-tests-v5 >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> cpus.c | 20 ++++++++++++++++---- >> 1 file changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/cpus.c b/cpus.c >> index 860e2a9..daa92c7 100644 >> --- a/cpus.c >> +++ b/cpus.c >> @@ -171,12 +171,24 @@ opts_init(tcg_register_config); >> >> static bool default_mttcg_enabled(void) >> { >> - /* >> - * TODO: Check if we have a chance to have MTTCG working on this guest/host. >> - * Basically is the atomic instruction implemented? Is there any >> - * memory ordering issue? >> + /* Checklist for enabling MTTCG on a given frontend/backend combination >> + * >> + * - Are atomics correctly modelled for an MTTCG environment >> + * - If the backend is weakly ordered >> + * - has the front-end implemented explicit memory ordering ops >> + * - does the back-end generate code to ensure memory ordering >> */ >> +#if defined(__i386__) || defined(__x86_64__) >> + /* x86 backend is strongly ordered which helps a lot */ >> + #if defined(TARGET_ARM) >> + return true; >> + #else >> + return false; >> + #endif >> +#else >> + /* Until memory ordering implemented things will likely break */ >> return false; >> +#endif > > No new per-host ifdef ladders, please (or per-target ifdef ladders, > either). Have some #defines for "TCG backend supports MTTCG" and > "TCG frontend supports MTTCG" which get set in some suitable per-host > and per-target header, and only enable if they're both set. Will do so. I guess the middling case of backend is strongly ordered enough to get away with partial barrier implementation at the front end should be skipped? We'll only turn on the frontend/backend support flags when: * All frontends fully express the ordering constraints to the TCG (e.g. all barriers and annotations complete) * The backend emits enough code to ensure any ordering constraint expressed in TCG ops can be satisfied. Are you happy to keep the commentary here with the default function as that is where people are likely to end up when searching? > > thanks > -- PMM -- Alex Bennée
On 6 June 2016 at 15:28, Alex Bennée <alex.bennee@linaro.org> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: >> No new per-host ifdef ladders, please (or per-target ifdef ladders, >> either). Have some #defines for "TCG backend supports MTTCG" and >> "TCG frontend supports MTTCG" which get set in some suitable per-host >> and per-target header, and only enable if they're both set. > > Will do so. I guess the middling case of backend is strongly ordered > enough to get away with partial barrier implementation at the front end > should be skipped? I don't mind if you have multiple ifdefs for "backend fully supports MTTCG" and "backend partially supports MTTCG" or whatever combination makes sense -- I haven't looked enough at the implementation to know what would be best. I just want to avoid ifdef ladders. > Are you happy to keep the commentary here with the default function as > that is where people are likely to end up when searching? Yes, that makes sense. Consider also a section in tcg/README documenting the requirements for a backend. thanks -- PMM
diff --git a/cpus.c b/cpus.c index 860e2a9..daa92c7 100644 --- a/cpus.c +++ b/cpus.c @@ -171,12 +171,24 @@ opts_init(tcg_register_config); static bool default_mttcg_enabled(void) { - /* - * TODO: Check if we have a chance to have MTTCG working on this guest/host. - * Basically is the atomic instruction implemented? Is there any - * memory ordering issue? + /* Checklist for enabling MTTCG on a given frontend/backend combination + * + * - Are atomics correctly modelled for an MTTCG environment + * - If the backend is weakly ordered + * - has the front-end implemented explicit memory ordering ops + * - does the back-end generate code to ensure memory ordering */ +#if defined(__i386__) || defined(__x86_64__) + /* x86 backend is strongly ordered which helps a lot */ + #if defined(TARGET_ARM) + return true; + #else + return false; + #endif +#else + /* Until memory ordering implemented things will likely break */ return false; +#endif } void qemu_tcg_configure(QemuOpts *opts)
This makes multi-threading the default for 32 bit ARM on x86. It has been tested with Debian Jessie as well as my extended KVM unit tests which stress the SMC and TB invalidation code. Those tests can be found at: https://github.com/stsquad/kvm-unit-tests/tree/mttcg/current-tests-v5 Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- cpus.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)