* [Qemu-devel] [PATCH v3] introduce on_vcpu
@ 2009-07-16 21:55 Glauber Costa
2009-08-27 17:15 ` [Qemu-devel] " Jan Kiszka
0 siblings, 1 reply; 16+ messages in thread
From: Glauber Costa @ 2009-07-16 21:55 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka, aliguori
on_vcpu is a qemu-kvm function that will make sure that a specific
piece of code will run on a requested cpu. We don't need that because
we're restricted to -smp 1 right now, but those days are likely to end soon.
So for the benefit of having qemu-kvm share more code with us, I'm
introducing our own version of on_vcpu(). Right now, we either run
a function on the current cpu, or abort the execution, because it would
mean something is seriously wrong.
As an example code, I "ported" kvm_update_guest_debug to use it,
with some slight differences from qemu-kvm.
This is probably 0.12 material
Signed-off-by: Glauber Costa <glommer@redhat.com>
CC: Jan Kiszka <jan.kiszka@siemens.com>
---
kvm-all.c | 35 +++++++++++++++++++++++++++++------
1 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/kvm-all.c b/kvm-all.c
index 61194b8..07a1cdb 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -155,6 +155,15 @@ static void kvm_reset_vcpu(void *opaque)
}
}
+static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
+{
+ if (env == cpu_single_env) {
+ func(data);
+ return;
+ }
+ abort();
+}
+
int kvm_init_vcpu(CPUState *env)
{
KVMState *s = kvm_state;
@@ -892,18 +901,32 @@ int kvm_sw_breakpoints_active(CPUState *env)
return !TAILQ_EMPTY(&env->kvm_state->kvm_sw_breakpoints);
}
+struct kvm_set_guest_debug_data {
+ struct kvm_guest_debug dbg;
+ CPUState *env;
+ int err;
+};
+
+static void kvm_invoke_set_guest_debug(void *data)
+{
+ struct kvm_set_guest_debug_data *dbg_data = data;
+ dbg_data->err = kvm_vcpu_ioctl(dbg_data->env, KVM_SET_GUEST_DEBUG, &dbg_data->dbg);
+}
+
int kvm_update_guest_debug(CPUState *env, unsigned long reinject_trap)
{
- struct kvm_guest_debug dbg;
+ struct kvm_set_guest_debug_data data;
- dbg.control = 0;
+ data.dbg.control = 0;
if (env->singlestep_enabled)
- dbg.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
+ data.dbg.control = KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_SINGLESTEP;
- kvm_arch_update_guest_debug(env, &dbg);
- dbg.control |= reinject_trap;
+ kvm_arch_update_guest_debug(env, &data.dbg);
+ data.dbg.control |= reinject_trap;
+ data.env = env;
- return kvm_vcpu_ioctl(env, KVM_SET_GUEST_DEBUG, &dbg);
+ on_vcpu(env, kvm_invoke_set_guest_debug, &data);
+ return data.err;
}
int kvm_insert_breakpoint(CPUState *current_env, target_ulong addr,
--
1.6.2.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-07-16 21:55 [Qemu-devel] [PATCH v3] introduce on_vcpu Glauber Costa
@ 2009-08-27 17:15 ` Jan Kiszka
2009-08-27 17:40 ` Glauber Costa
2009-08-28 1:18 ` Glauber Costa
0 siblings, 2 replies; 16+ messages in thread
From: Jan Kiszka @ 2009-08-27 17:15 UTC (permalink / raw)
To: Glauber Costa; +Cc: aliguori, qemu-devel
Glauber Costa wrote:
> on_vcpu is a qemu-kvm function that will make sure that a specific
> piece of code will run on a requested cpu. We don't need that because
> we're restricted to -smp 1 right now, but those days are likely to end soon.
>
> So for the benefit of having qemu-kvm share more code with us, I'm
> introducing our own version of on_vcpu(). Right now, we either run
> a function on the current cpu, or abort the execution, because it would
> mean something is seriously wrong.
>
> As an example code, I "ported" kvm_update_guest_debug to use it,
> with some slight differences from qemu-kvm.
>
> This is probably 0.12 material
>
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> CC: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> kvm-all.c | 35 +++++++++++++++++++++++++++++------
> 1 files changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/kvm-all.c b/kvm-all.c
> index 61194b8..07a1cdb 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -155,6 +155,15 @@ static void kvm_reset_vcpu(void *opaque)
> }
> }
>
> +static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
> +{
> + if (env == cpu_single_env) {
> + func(data);
> + return;
> + }
> + abort();
Sorry, missed this before it went in: This abort fires already now when
kvm_update_guest_debug is invoked by the gdbstub (where cpu_single_env
is 0). This completely breaks guest debugging int kvm mode.
Moreover, if you enable I/O thread support, you already have a need for
true on_vcpu, don't you? Or is locking around the I/O thread still
broken in kvm mode? Anyway, please fix.
Thanks,
Jan
--
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-27 17:15 ` [Qemu-devel] " Jan Kiszka
@ 2009-08-27 17:40 ` Glauber Costa
2009-08-27 17:45 ` Jan Kiszka
2009-08-28 1:18 ` Glauber Costa
1 sibling, 1 reply; 16+ messages in thread
From: Glauber Costa @ 2009-08-27 17:40 UTC (permalink / raw)
To: Jan Kiszka; +Cc: aliguori, qemu-devel
On Thu, Aug 27, 2009 at 07:15:28PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > on_vcpu is a qemu-kvm function that will make sure that a specific
> > piece of code will run on a requested cpu. We don't need that because
> > we're restricted to -smp 1 right now, but those days are likely to end soon.
> >
> > So for the benefit of having qemu-kvm share more code with us, I'm
> > introducing our own version of on_vcpu(). Right now, we either run
> > a function on the current cpu, or abort the execution, because it would
> > mean something is seriously wrong.
> >
> > As an example code, I "ported" kvm_update_guest_debug to use it,
> > with some slight differences from qemu-kvm.
> >
> > This is probably 0.12 material
> >
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > CC: Jan Kiszka <jan.kiszka@siemens.com>
> > ---
> > kvm-all.c | 35 +++++++++++++++++++++++++++++------
> > 1 files changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 61194b8..07a1cdb 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -155,6 +155,15 @@ static void kvm_reset_vcpu(void *opaque)
> > }
> > }
> >
> > +static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
> > +{
> > + if (env == cpu_single_env) {
> > + func(data);
> > + return;
> > + }
> > + abort();
>
> Sorry, missed this before it went in: This abort fires already now when
> kvm_update_guest_debug is invoked by the gdbstub (where cpu_single_env
> is 0). This completely breaks guest debugging int kvm mode.
>
> Moreover, if you enable I/O thread support, you already have a need for
> true on_vcpu, don't you? Or is locking around the I/O thread still
> broken in kvm mode? Anyway, please fix.
It is, but I just sent two patches that would leave us in a better shape.
Not sure what was made of them.
Anyway, I still have something almost ready for on_vcpu.
Actually, I want to hear input on it: I was thinking the best architecture
would be to drop it completely, and do automatically whenever we call vcpu_ioctl.
My only concern then would be speed. In this case, we could make this non-blocking,
and introduce explicit flush requests for remote cpus.
What do you think about it ?
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-27 17:40 ` Glauber Costa
@ 2009-08-27 17:45 ` Jan Kiszka
0 siblings, 0 replies; 16+ messages in thread
From: Jan Kiszka @ 2009-08-27 17:45 UTC (permalink / raw)
To: Glauber Costa; +Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, Avi Kivity
Glauber Costa wrote:
> On Thu, Aug 27, 2009 at 07:15:28PM +0200, Jan Kiszka wrote:
>> Glauber Costa wrote:
>>> on_vcpu is a qemu-kvm function that will make sure that a specific
>>> piece of code will run on a requested cpu. We don't need that because
>>> we're restricted to -smp 1 right now, but those days are likely to end soon.
>>>
>>> So for the benefit of having qemu-kvm share more code with us, I'm
>>> introducing our own version of on_vcpu(). Right now, we either run
>>> a function on the current cpu, or abort the execution, because it would
>>> mean something is seriously wrong.
>>>
>>> As an example code, I "ported" kvm_update_guest_debug to use it,
>>> with some slight differences from qemu-kvm.
>>>
>>> This is probably 0.12 material
>>>
>>> Signed-off-by: Glauber Costa <glommer@redhat.com>
>>> CC: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>> kvm-all.c | 35 +++++++++++++++++++++++++++++------
>>> 1 files changed, 29 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index 61194b8..07a1cdb 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -155,6 +155,15 @@ static void kvm_reset_vcpu(void *opaque)
>>> }
>>> }
>>>
>>> +static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
>>> +{
>>> + if (env == cpu_single_env) {
>>> + func(data);
>>> + return;
>>> + }
>>> + abort();
>> Sorry, missed this before it went in: This abort fires already now when
>> kvm_update_guest_debug is invoked by the gdbstub (where cpu_single_env
>> is 0). This completely breaks guest debugging int kvm mode.
>>
>> Moreover, if you enable I/O thread support, you already have a need for
>> true on_vcpu, don't you? Or is locking around the I/O thread still
>> broken in kvm mode? Anyway, please fix.
> It is, but I just sent two patches that would leave us in a better shape.
> Not sure what was made of them.
>
> Anyway, I still have something almost ready for on_vcpu.
> Actually, I want to hear input on it: I was thinking the best architecture
> would be to drop it completely, and do automatically whenever we call vcpu_ioctl.
> My only concern then would be speed. In this case, we could make this non-blocking,
> and introduce explicit flush requests for remote cpus.
>
> What do you think about it ?
Is on_vcpu only used for issuing vcpu_ioctl? Then combining both is
surely a reasonable step.
If you are concerned about performance, I think crafting a first version
over qemu-kvm, probably also merging it into that branch first makes
some sense. Though I think we should get along with a simple test for
the caller context in vcpu_ioctl for the fast path.
Jan
--
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-27 17:15 ` [Qemu-devel] " Jan Kiszka
2009-08-27 17:40 ` Glauber Costa
@ 2009-08-28 1:18 ` Glauber Costa
2009-08-28 1:38 ` Anthony Liguori
1 sibling, 1 reply; 16+ messages in thread
From: Glauber Costa @ 2009-08-28 1:18 UTC (permalink / raw)
To: Jan Kiszka; +Cc: aliguori, qemu-devel, avi
On Thu, Aug 27, 2009 at 07:15:28PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > on_vcpu is a qemu-kvm function that will make sure that a specific
> > piece of code will run on a requested cpu. We don't need that because
> > we're restricted to -smp 1 right now, but those days are likely to end soon.
> >
> > So for the benefit of having qemu-kvm share more code with us, I'm
> > introducing our own version of on_vcpu(). Right now, we either run
> > a function on the current cpu, or abort the execution, because it would
> > mean something is seriously wrong.
> >
> > As an example code, I "ported" kvm_update_guest_debug to use it,
> > with some slight differences from qemu-kvm.
> >
> > This is probably 0.12 material
> >
> > Signed-off-by: Glauber Costa <glommer@redhat.com>
> > CC: Jan Kiszka <jan.kiszka@siemens.com>
> > ---
> > kvm-all.c | 35 +++++++++++++++++++++++++++++------
> > 1 files changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 61194b8..07a1cdb 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -155,6 +155,15 @@ static void kvm_reset_vcpu(void *opaque)
> > }
> > }
> >
> > +static void on_vcpu(CPUState *env, void (*func)(void *data), void *data)
> > +{
> > + if (env == cpu_single_env) {
> > + func(data);
> > + return;
> > + }
> > + abort();
>
> Sorry, missed this before it went in: This abort fires already now when
> kvm_update_guest_debug is invoked by the gdbstub (where cpu_single_env
> is 0). This completely breaks guest debugging int kvm mode.
To begin with, I believe checking for cpu_single_env is totally evil.
It is unpredictable at best, since there are many places (like you just found out)
that will clear it.
qemu-kvm uses a TLS variable for that, to guarantee that we're in the same thread
as our calling context. I like this idea. if we have io-thread disabled, we're always
in the same thread, and will always execute the function directly as we used to do
before on_vcpu().
I do however remember anthony bending towards issuing a gettid() instead of using
a TLS var. I'm fine with both. Anthony, avi, you guys have a word here?
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-28 1:18 ` Glauber Costa
@ 2009-08-28 1:38 ` Anthony Liguori
2009-08-28 1:58 ` Glauber Costa
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Anthony Liguori @ 2009-08-28 1:38 UTC (permalink / raw)
To: Glauber Costa; +Cc: Jan Kiszka, qemu-devel, avi
Glauber Costa wrote:
> qemu-kvm uses a TLS variable for that, to guarantee that we're in the same thread
> as our calling context. I like this idea. if we have io-thread disabled, we're always
> in the same thread, and will always execute the function directly as we used to do
> before on_vcpu().
>
> I do however remember anthony bending towards issuing a gettid() instead of using
> a TLS var. I'm fine with both. Anthony, avi, you guys have a word here?
>
Since we already keep the tid in the vcpu structure, it seems to make
more sense to ask "am I this vcpu thread" by doing gettid() == env->tid
than by maintaining a new global tls variable.
--
Regards,
Anthony Liguori
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-28 1:38 ` Anthony Liguori
@ 2009-08-28 1:58 ` Glauber Costa
2009-08-28 6:18 ` Gleb Natapov
2009-08-29 1:22 ` Jamie Lokier
2 siblings, 0 replies; 16+ messages in thread
From: Glauber Costa @ 2009-08-28 1:58 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Jan Kiszka, qemu-devel, avi
On Thu, Aug 27, 2009 at 08:38:56PM -0500, Anthony Liguori wrote:
> Glauber Costa wrote:
>> qemu-kvm uses a TLS variable for that, to guarantee that we're in the same thread
>> as our calling context. I like this idea. if we have io-thread disabled, we're always
>> in the same thread, and will always execute the function directly as we used to do
>> before on_vcpu().
>>
>> I do however remember anthony bending towards issuing a gettid() instead of using
>> a TLS var. I'm fine with both. Anthony, avi, you guys have a word here?
>>
>
> Since we already keep the tid in the vcpu structure, it seems to make
> more sense to ask "am I this vcpu thread" by doing gettid() == env->tid
> than by maintaining a new global tls variable.
Actually, we don't. qemu-kvm does, but not qemu.
There's, however, a 'host_tid' field used by linux-user that I intend to hijack
for that. It is one of the patches I have queued up.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-28 1:38 ` Anthony Liguori
2009-08-28 1:58 ` Glauber Costa
@ 2009-08-28 6:18 ` Gleb Natapov
2009-08-29 1:22 ` Jamie Lokier
2 siblings, 0 replies; 16+ messages in thread
From: Gleb Natapov @ 2009-08-28 6:18 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Jan Kiszka, Glauber Costa, qemu-devel, avi
On Thu, Aug 27, 2009 at 08:38:56PM -0500, Anthony Liguori wrote:
> Glauber Costa wrote:
> >qemu-kvm uses a TLS variable for that, to guarantee that we're in the same thread
> >as our calling context. I like this idea. if we have io-thread disabled, we're always
> >in the same thread, and will always execute the function directly as we used to do
> >before on_vcpu().
> >
> >I do however remember anthony bending towards issuing a gettid() instead of using
> >a TLS var. I'm fine with both. Anthony, avi, you guys have a word here?
>
> Since we already keep the tid in the vcpu structure, it seems to
> make more sense to ask "am I this vcpu thread" by doing gettid() ==
> env->tid than by maintaining a new global tls variable.
>
What is the problem with tls variables? Why do unneeded system call?
--
Gleb.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-28 1:38 ` Anthony Liguori
2009-08-28 1:58 ` Glauber Costa
2009-08-28 6:18 ` Gleb Natapov
@ 2009-08-29 1:22 ` Jamie Lokier
2009-08-31 11:35 ` Glauber Costa
2 siblings, 1 reply; 16+ messages in thread
From: Jamie Lokier @ 2009-08-29 1:22 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Jan Kiszka, Glauber Costa, qemu-devel, avi
Anthony Liguori wrote:
> Glauber Costa wrote:
> Since we already keep the tid in the vcpu structure, it seems to make
> more sense to ask "am I this vcpu thread" by doing gettid() == env->tid
> than by maintaining a new global tls variable.
Note that a tls variable will be much faster than gettid(). Don't
know if you're talking about a hot path.
-- Jamie
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-29 1:22 ` Jamie Lokier
@ 2009-08-31 11:35 ` Glauber Costa
2009-08-31 12:04 ` Jamie Lokier
2009-09-01 0:55 ` Paolo Bonzini
0 siblings, 2 replies; 16+ messages in thread
From: Glauber Costa @ 2009-08-31 11:35 UTC (permalink / raw)
To: Jamie Lokier; +Cc: Jan Kiszka, Anthony Liguori, qemu-devel, avi
On Sat, Aug 29, 2009 at 02:22:27AM +0100, Jamie Lokier wrote:
> Anthony Liguori wrote:
> > Glauber Costa wrote:
> > Since we already keep the tid in the vcpu structure, it seems to make
> > more sense to ask "am I this vcpu thread" by doing gettid() == env->tid
> > than by maintaining a new global tls variable.
>
> Note that a tls variable will be much faster than gettid(). Don't
> know if you're talking about a hot path.
just to be sure, TLS is not supported on all our linux target hosts, right?
We can probably wrap it into a function that uses gettid on linux (or whatever
in other platforms), and uses a TLS variable where available. (and if needed).
I can agree with anthony that although TLS is in fact faster, we might not need it.
I doubt that anything that communicates using signals will be the hot path for anything.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-31 11:35 ` Glauber Costa
@ 2009-08-31 12:04 ` Jamie Lokier
2009-08-31 12:14 ` Glauber Costa
2009-08-31 12:57 ` Glauber Costa
2009-09-01 0:55 ` Paolo Bonzini
1 sibling, 2 replies; 16+ messages in thread
From: Jamie Lokier @ 2009-08-31 12:04 UTC (permalink / raw)
To: Glauber Costa; +Cc: Jan Kiszka, Anthony Liguori, qemu-devel, avi
Glauber Costa wrote:
> On Sat, Aug 29, 2009 at 02:22:27AM +0100, Jamie Lokier wrote:
> > Anthony Liguori wrote:
> > > Glauber Costa wrote:
> > > Since we already keep the tid in the vcpu structure, it seems to make
> > > more sense to ask "am I this vcpu thread" by doing gettid() == env->tid
> > > than by maintaining a new global tls variable.
> >
> > Note that a tls variable will be much faster than gettid(). Don't
> > know if you're talking about a hot path.
> just to be sure, TLS is not supported on all our linux target hosts, right?
>
> We can probably wrap it into a function that uses gettid on linux (or whatever
> in other platforms), and uses a TLS variable where available. (and if needed).
>
> I can agree with anthony that although TLS is in fact faster, we might not need it.
> I doubt that anything that communicates using signals will be the hot path for anything.
I was going to say just use pthread_self()! It's fast like TLS on all
hosts, and more portable then gettid().
But then you mentioned signals. I'm not sure if the code in question
is inside signal handlers.
pthread_self() is not officially permitted inside a signal handler.
And indeed, gives the wrong answer with some Linuxes when
sigaltstack() is used.
But if you were going to use gettid(), that means you are only
targetting Linuxes which use NPTL threads anyway, because gettid() is
not available elsewhere. As far as I know, pthread_self() is safe in
signal handlers on all versions of NPTL threads, and unlike gettid(),
should be fast.
-- Jamie
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-31 12:04 ` Jamie Lokier
@ 2009-08-31 12:14 ` Glauber Costa
2009-08-31 12:21 ` Jan Kiszka
2009-08-31 12:57 ` Glauber Costa
1 sibling, 1 reply; 16+ messages in thread
From: Glauber Costa @ 2009-08-31 12:14 UTC (permalink / raw)
To: Jamie Lokier; +Cc: Jan Kiszka, Anthony Liguori, qemu-devel, avi
On Mon, Aug 31, 2009 at 01:04:52PM +0100, Jamie Lokier wrote:
> Glauber Costa wrote:
> > On Sat, Aug 29, 2009 at 02:22:27AM +0100, Jamie Lokier wrote:
> > > Anthony Liguori wrote:
> > > > Glauber Costa wrote:
> > > > Since we already keep the tid in the vcpu structure, it seems to make
> > > > more sense to ask "am I this vcpu thread" by doing gettid() == env->tid
> > > > than by maintaining a new global tls variable.
> > >
> > > Note that a tls variable will be much faster than gettid(). Don't
> > > know if you're talking about a hot path.
> > just to be sure, TLS is not supported on all our linux target hosts, right?
> >
> > We can probably wrap it into a function that uses gettid on linux (or whatever
> > in other platforms), and uses a TLS variable where available. (and if needed).
> >
> > I can agree with anthony that although TLS is in fact faster, we might not need it.
> > I doubt that anything that communicates using signals will be the hot path for anything.
>
> I was going to say just use pthread_self()! It's fast like TLS on all
> hosts, and more portable then gettid().
>
> But then you mentioned signals. I'm not sure if the code in question
> is inside signal handlers.
Signals are just used to wake up the other cpu. I think it is pretty valid
to rule out usage insigne signal handlers (mention in comments, etc).
I'll propose that switch on qemu-kvm, which already uses tls variables, and see
what the response is.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-31 12:14 ` Glauber Costa
@ 2009-08-31 12:21 ` Jan Kiszka
2009-08-31 22:25 ` Jamie Lokier
0 siblings, 1 reply; 16+ messages in thread
From: Jan Kiszka @ 2009-08-31 12:21 UTC (permalink / raw)
To: Glauber Costa; +Cc: Anthony Liguori, qemu-devel@nongnu.org, avi@redhat.com
Glauber Costa wrote:
> On Mon, Aug 31, 2009 at 01:04:52PM +0100, Jamie Lokier wrote:
>> Glauber Costa wrote:
>>> On Sat, Aug 29, 2009 at 02:22:27AM +0100, Jamie Lokier wrote:
>>>> Anthony Liguori wrote:
>>>>> Glauber Costa wrote:
>>>>> Since we already keep the tid in the vcpu structure, it seems to make
>>>>> more sense to ask "am I this vcpu thread" by doing gettid() == env->tid
>>>>> than by maintaining a new global tls variable.
>>>> Note that a tls variable will be much faster than gettid(). Don't
>>>> know if you're talking about a hot path.
>>> just to be sure, TLS is not supported on all our linux target hosts, right?
>>>
>>> We can probably wrap it into a function that uses gettid on linux (or whatever
>>> in other platforms), and uses a TLS variable where available. (and if needed).
>>>
>>> I can agree with anthony that although TLS is in fact faster, we might not need it.
>>> I doubt that anything that communicates using signals will be the hot path for anything.
>> I was going to say just use pthread_self()! It's fast like TLS on all
>> hosts, and more portable then gettid().
>>
>> But then you mentioned signals. I'm not sure if the code in question
>> is inside signal handlers.
> Signals are just used to wake up the other cpu. I think it is pretty valid
> to rule out usage insigne signal handlers (mention in comments, etc).
>
> I'll propose that switch on qemu-kvm, which already uses tls variables, and see
> what the response is.
>
To my experience, TLS can cause a lot of problems, but only when used
close to inline assembly (gcc is still horribly broken then, clobbering
or "optimizing" register content, specifically on ARM). I do not expect
problems for our standard use cases.
But in case someone still does not feel well about it:
pthread_get/set_specific can serve as a "safer" alternative that is also
syscall-free (where possible).
Jan
--
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-31 12:21 ` Jan Kiszka
@ 2009-08-31 22:25 ` Jamie Lokier
0 siblings, 0 replies; 16+ messages in thread
From: Jamie Lokier @ 2009-08-31 22:25 UTC (permalink / raw)
To: Jan Kiszka
Cc: Glauber Costa, avi@redhat.com, Anthony Liguori,
qemu-devel@nongnu.org
Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Mon, Aug 31, 2009 at 01:04:52PM +0100, Jamie Lokier wrote:
> >> Glauber Costa wrote:
> >>> On Sat, Aug 29, 2009 at 02:22:27AM +0100, Jamie Lokier wrote:
> >>>> Anthony Liguori wrote:
> >>>>> Glauber Costa wrote:
> >>>>> Since we already keep the tid in the vcpu structure, it seems to make
> >>>>> more sense to ask "am I this vcpu thread" by doing gettid() == env->tid
> >>>>> than by maintaining a new global tls variable.
> >>>> Note that a tls variable will be much faster than gettid(). Don't
> >>>> know if you're talking about a hot path.
> >>> just to be sure, TLS is not supported on all our linux target hosts, right?
> >>>
> >>> We can probably wrap it into a function that uses gettid on linux (or whatever
> >>> in other platforms), and uses a TLS variable where available. (and if needed).
> >>>
> >>> I can agree with anthony that although TLS is in fact faster, we might not need it.
> >>> I doubt that anything that communicates using signals will be the hot path for anything.
> >> I was going to say just use pthread_self()! It's fast like TLS on all
> >> hosts, and more portable then gettid().
> >>
> >> But then you mentioned signals. I'm not sure if the code in question
> >> is inside signal handlers.
> > Signals are just used to wake up the other cpu. I think it is pretty valid
> > to rule out usage insigne signal handlers (mention in comments, etc).
> >
> > I'll propose that switch on qemu-kvm, which already uses tls variables, and see
> > what the response is.
> >
>
> To my experience, TLS can cause a lot of problems, but only when used
> close to inline assembly (gcc is still horribly broken then, clobbering
> or "optimizing" register content, specifically on ARM). I do not expect
> problems for our standard use cases.
>
> But in case someone still does not feel well about it:
> pthread_get/set_specific can serve as a "safer" alternative that is also
> syscall-free (where possible).
Just so y'all know, pthread_get/set_specific are also unsafe inside
signal handlers for the same reason as pthread_self is unsafe.
-- Jamie
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-31 12:04 ` Jamie Lokier
2009-08-31 12:14 ` Glauber Costa
@ 2009-08-31 12:57 ` Glauber Costa
1 sibling, 0 replies; 16+ messages in thread
From: Glauber Costa @ 2009-08-31 12:57 UTC (permalink / raw)
To: Jamie Lokier; +Cc: Jan Kiszka, Anthony Liguori, qemu-devel, avi
On Mon, Aug 31, 2009 at 01:04:52PM +0100, Jamie Lokier wrote:
> Glauber Costa wrote:
> > On Sat, Aug 29, 2009 at 02:22:27AM +0100, Jamie Lokier wrote:
> > > Anthony Liguori wrote:
> > > > Glauber Costa wrote:
> > > > Since we already keep the tid in the vcpu structure, it seems to make
> > > > more sense to ask "am I this vcpu thread" by doing gettid() == env->tid
> > > > than by maintaining a new global tls variable.
> > >
> > > Note that a tls variable will be much faster than gettid(). Don't
> > > know if you're talking about a hot path.
> > just to be sure, TLS is not supported on all our linux target hosts, right?
> >
> > We can probably wrap it into a function that uses gettid on linux (or whatever
> > in other platforms), and uses a TLS variable where available. (and if needed).
> >
> > I can agree with anthony that although TLS is in fact faster, we might not need it.
> > I doubt that anything that communicates using signals will be the hot path for anything.
>
> I was going to say just use pthread_self()! It's fast like TLS on all
> hosts, and more portable then gettid().
>
Just noted one thing we are missing, here.
The use we're looking at with qemu is just to ask a question like "are we running on
the current thread"?
However, by looking at qemu-kvm's tree, we can see that this is not always the case.
We use that to answer questions like "what is the env in this thread, so I can inject
an interrupt?". And it kinds of smell like a possible hot path.
So although we could use pthread_getspecific() like Jan suggested, it is not as fast as
a TLS variable. I would then, rather, write a wrapper that does a TLS read when available,
and uses pthread_getspecific() otherwise.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] Re: [PATCH v3] introduce on_vcpu
2009-08-31 11:35 ` Glauber Costa
2009-08-31 12:04 ` Jamie Lokier
@ 2009-09-01 0:55 ` Paolo Bonzini
1 sibling, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2009-09-01 0:55 UTC (permalink / raw)
To: Glauber Costa; +Cc: Jan Kiszka, Anthony Liguori, qemu-devel, avi
On 08/31/2009 01:35 PM, Glauber Costa wrote:
> On Sat, Aug 29, 2009 at 02:22:27AM +0100, Jamie Lokier wrote:
>> Anthony Liguori wrote:
>>> Glauber Costa wrote:
>>> Since we already keep the tid in the vcpu structure, it seems to make
>>> more sense to ask "am I this vcpu thread" by doing gettid() == env->tid
>>> than by maintaining a new global tls variable.
>>
>> Note that a tls variable will be much faster than gettid(). Don't
>> know if you're talking about a hot path.
> just to be sure, TLS is not supported on all our linux target hosts, right?
I think it is.
~/devel/gcc/gcc pbonzini$ grep -l SYMBOL_REF_TLS_MODEL config/*/*.c
config/alpha/alpha.c
config/arm/arm.c
config/frv/frv.c
config/i386/i386.c
config/ia64/ia64.c
config/m68k/m68k.c
config/mips/mips.c
config/pa/pa.c
config/rs6000/rs6000.c
config/s390/s390.c
config/sh/sh.c
config/sparc/sparc.c
config/xtensa/xtensa.c
Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2009-09-01 0:55 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-07-16 21:55 [Qemu-devel] [PATCH v3] introduce on_vcpu Glauber Costa
2009-08-27 17:15 ` [Qemu-devel] " Jan Kiszka
2009-08-27 17:40 ` Glauber Costa
2009-08-27 17:45 ` Jan Kiszka
2009-08-28 1:18 ` Glauber Costa
2009-08-28 1:38 ` Anthony Liguori
2009-08-28 1:58 ` Glauber Costa
2009-08-28 6:18 ` Gleb Natapov
2009-08-29 1:22 ` Jamie Lokier
2009-08-31 11:35 ` Glauber Costa
2009-08-31 12:04 ` Jamie Lokier
2009-08-31 12:14 ` Glauber Costa
2009-08-31 12:21 ` Jan Kiszka
2009-08-31 22:25 ` Jamie Lokier
2009-08-31 12:57 ` Glauber Costa
2009-09-01 0:55 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).