[Qemu-devel] [PATCH] tls.h: Enable TLS on FreeBSD

[Qemu-devel] [PATCH] tls.h: Enable TLS on FreeBSD

[Ed Maste]

Signed-off-by: Ed Maste <emaste@freebsd.org>
---
I have had this in a local tree for some time, and it is needed by the
BSD-user work that is now being proposed.

As an aside, an abstraction was recently proposed for Open vSwtich that
can use any of _Thread_local, __thread, or pthread_getspecific() which
may make a convenient reference for someone wishing to implement one of
the TODOs: http://openvswitch.org/pipermail/dev/2013-June/028665.html

 include/qemu/tls.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/tls.h b/include/qemu/tls.h
index b92ea9d..ae7d79d 100644
--- a/include/qemu/tls.h
+++ b/include/qemu/tls.h
@@ -38,7 +38,7 @@
  * TODO: proper implementations via Win32 .tls sections and
  * POSIX pthread_getspecific.
  */
-#ifdef __linux__
+#if defined(__linux__) || defined(__FreeBSD__)
 #define DECLARE_TLS(type, x) extern DEFINE_TLS(type, x)
 #define DEFINE_TLS(type, x)  __thread __typeof__(type) tls__##x
 #define tls_var(x)           tls__##x
-- 
1.7.11.5

Re: [Qemu-devel] [PATCH] tls.h: Enable TLS on FreeBSD

[Paolo Bonzini]

Il 24/06/2013 21:21, Ed Maste ha scritto:
> Signed-off-by: Ed Maste <emaste@freebsd.org>
> ---
> I have had this in a local tree for some time, and it is needed by the
> BSD-user work that is now being proposed.

At this time, qemu/tls.h is really just for cpu_single_env, so I think
this patch should be applied together with the bsd-user patches that
need it.

> As an aside, an abstraction was recently proposed for Open vSwtich that
> can use any of _Thread_local, __thread, or pthread_getspecific() which
> may make a convenient reference for someone wishing to implement one of
> the TODOs: http://openvswitch.org/pipermail/dev/2013-June/028665.html

I and Stefan Hajnoczi have almost the same idea implemented in QEMU
(except that get_foo() returns a pointer to the variable).  But
pthread_get/setspecific would be too slow for cpu_single_env, so we're
just switching to __thread for cpu_single_env (for Linux in our patches,
but you can add FreeBSD too once it's needed).

Paolo

Re: [Qemu-devel] [PATCH] tls.h: Enable TLS on FreeBSD

[Peter Maydell]

On 24 June 2013 22:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 24/06/2013 21:21, Ed Maste ha scritto:
>> Signed-off-by: Ed Maste <emaste@freebsd.org>
>> ---
>> I have had this in a local tree for some time, and it is needed by the
>> BSD-user work that is now being proposed.
>
> At this time, qemu/tls.h is really just for cpu_single_env, so I think
> this patch should be applied together with the bsd-user patches that
> need it.

This patch has arrived because I suggested splitting it out from
those ;-) System mode qemu is multi-threaded, so moving more host
OSes into the fold of "these variables behave like the linux host
which is where the bulk of testing happens" sounds like a good idea
to me.

>> As an aside, an abstraction was recently proposed for Open vSwtich that
>> can use any of _Thread_local, __thread, or pthread_getspecific() which
>> may make a convenient reference for someone wishing to implement one of
>> the TODOs: http://openvswitch.org/pipermail/dev/2013-June/028665.html
>
> I and Stefan Hajnoczi have almost the same idea implemented in QEMU
> (except that get_foo() returns a pointer to the variable).  But
> pthread_get/setspecific would be too slow for cpu_single_env, so we're
> just switching to __thread for cpu_single_env (for Linux in our patches,
> but you can add FreeBSD too once it's needed).

I would really like cpu_single_env (and the other thread variables)
to have consistent semantics everywhere (which means "always thread
local"), even if that means "sometimes suboptimal performance".
Reasoning about a variable that might or might not be per-thread
is just painful. Alternatively, we could just drop support for any
host OS that doesn't provide reasonably efficient per-thread data.

-- PMM

Re: [Qemu-devel] [PATCH] tls.h: Enable TLS on FreeBSD

[Paolo Bonzini]

Il 24/06/2013 23:30, Peter Maydell ha scritto:
> On 24 June 2013 22:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 24/06/2013 21:21, Ed Maste ha scritto:
>>> Signed-off-by: Ed Maste <emaste@freebsd.org>
>>> ---
>>> I have had this in a local tree for some time, and it is needed by the
>>> BSD-user work that is now being proposed.
>>
>> At this time, qemu/tls.h is really just for cpu_single_env, so I think
>> this patch should be applied together with the bsd-user patches that
>> need it.
> 
> This patch has arrived because I suggested splitting it out from
> those ;-) System mode qemu is multi-threaded, so moving more host
> OSes into the fold of "these variables behave like the linux host
> which is where the bulk of testing happens" sounds like a good idea
> to me.

Ah. :)

>>> As an aside, an abstraction was recently proposed for Open vSwtich that
>>> can use any of _Thread_local, __thread, or pthread_getspecific() which
>>> may make a convenient reference for someone wishing to implement one of
>>> the TODOs: http://openvswitch.org/pipermail/dev/2013-June/028665.html
>>
>> I and Stefan Hajnoczi have almost the same idea implemented in QEMU
>> (except that get_foo() returns a pointer to the variable).  But
>> pthread_get/setspecific would be too slow for cpu_single_env, so we're
>> just switching to __thread for cpu_single_env (for Linux in our patches,
>> but you can add FreeBSD too once it's needed).
> 
> I would really like cpu_single_env (and the other thread variables)
> to have consistent semantics everywhere (which means "always thread
> local"), even if that means "sometimes suboptimal performance".
> Reasoning about a variable that might or might not be per-thread
> is just painful. Alternatively, we could just drop support for any
> host OS that doesn't provide reasonably efficient per-thread data.

That is basically only OpenBSD.  If we get reasonable support for
FreeBSD, I'm perfectly fine with dropping OpenBSD.  But so far OpenBSD
was the only officially supported variant.

Paolo

Re: [Qemu-devel] [PATCH] tls.h: Enable TLS on FreeBSD

[Peter Maydell]

On 24 June 2013 22:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
> I and Stefan Hajnoczi have almost the same idea implemented in QEMU
> (except that get_foo() returns a pointer to the variable).  But
> pthread_get/setspecific would be too slow for cpu_single_env, so we're
> just switching to __thread for cpu_single_env (for Linux in our patches,
> but you can add FreeBSD too once it's needed).

By the way, what's the plan for Windows? Does that support
__thread too, or will there still need to be some windows
specific magic?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] tls.h: Enable TLS on FreeBSD

[Paolo Bonzini]

Il 25/06/2013 12:56, Peter Maydell ha scritto:
> On 24 June 2013 22:15, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> I and Stefan Hajnoczi have almost the same idea implemented in QEMU
>> (except that get_foo() returns a pointer to the variable).  But
>> pthread_get/setspecific would be too slow for cpu_single_env, so we're
>> just switching to __thread for cpu_single_env (for Linux in our patches,
>> but you can add FreeBSD too once it's needed).
> 
> By the way, what's the plan for Windows? Does that support
> __thread too, or will there still need to be some windows
> specific magic?

__thread is supported but slow, so there is a small amount of
Windows-specific magic (but it won't affect the users of tls.h).

Paolo

Re: [Qemu-devel] [Qemu-trivial] [PATCH] tls.h: Enable TLS on FreeBSD

[Michael Tokarev]

24.06.2013 23:21, Ed Maste wrote:
> Signed-off-by: Ed Maste <emaste@freebsd.org>
> ---
> I have had this in a local tree for some time, and it is needed by the
> BSD-user work that is now being proposed.

So I'm not applying this to -trivial, because it caused quite some
discussion.  If nevertheless it's okay to apply it, please let me know.

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH] tls.h: Enable TLS on FreeBSD

[Ed Maste]

On 28 June 2013 06:34, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 24.06.2013 23:21, Ed Maste wrote:
>> Signed-off-by: Ed Maste <emaste@freebsd.org>
>> ---
>> I have had this in a local tree for some time, and it is needed by the
>> BSD-user work that is now being proposed.
>
> So I'm not applying this to -trivial, because it caused quite some
> discussion.  If nevertheless it's okay to apply it, please let me know.
>
> /mjt

An objection seemed to be that it is not really needed yet, but as
pointed out by Peter this isn't really the case.  This patch brings us
(FreeBSD) in line with per-thread data semantics on Linux and is used
by qemu system emulation as well.

The other discussion was all around platforms unaffected by this
change (OpenBSD, Windows); I don't see it as an argument either for or
against this change.  So I think it's reasonable to go in.

Re: [Qemu-devel] [Qemu-trivial] [PATCH] tls.h: Enable TLS on FreeBSD

[Paolo Bonzini]

Il 28/06/2013 14:54, Ed Maste ha scritto:
> On 28 June 2013 06:34, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> 24.06.2013 23:21, Ed Maste wrote:
>>> Signed-off-by: Ed Maste <emaste@freebsd.org>
>>> ---
>>> I have had this in a local tree for some time, and it is needed by the
>>> BSD-user work that is now being proposed.
>>
>> So I'm not applying this to -trivial, because it caused quite some
>> discussion.  If nevertheless it's okay to apply it, please let me know.
>>
>> /mjt
> 
> An objection seemed to be that it is not really needed yet, but as
> pointed out by Peter this isn't really the case.  This patch brings us
> (FreeBSD) in line with per-thread data semantics on Linux and is used
> by qemu system emulation as well.

It brings it in line with Linux, even though FreeBSD has the same need
for TLS as OpenBSD and Windows (i.e. none).  In other words, we go from
two cases (need TLS and uses it, has no TLS and doesn't need it) to
three (we add "uses TLS with no need for it").

qemu/tls.h is currently a hack.  When it will grow to something useful
(soon), rest assured that FreeBSD will be supported properly.

Paolo

> The other discussion was all around platforms unaffected by this
> change (OpenBSD, Windows); I don't see it as an argument either for or
> against this change.  So I think it's reasonable to go in.
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH] tls.h: Enable TLS on FreeBSD

[Peter Maydell]

On 28 June 2013 14:03, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 28/06/2013 14:54, Ed Maste ha scritto:
>> An objection seemed to be that it is not really needed yet, but as
>> pointed out by Peter this isn't really the case.  This patch brings us
>> (FreeBSD) in line with per-thread data semantics on Linux and is used
>> by qemu system emulation as well.
>
> It brings it in line with Linux, even though FreeBSD has the same need
> for TLS as OpenBSD and Windows (i.e. none).  In other words, we go from
> two cases (need TLS and uses it, has no TLS and doesn't need it) to
> three (we add "uses TLS with no need for it").

I don't understand what you mean by "no need for it". We are
already multithreaded in system mode, so this is simply
making FreeBSD do the same thing as Linux. That seems like a
clear improvement to me.

-- PMM

Re: [Qemu-devel] [Qemu-trivial] [PATCH] tls.h: Enable TLS on FreeBSD

[Ed Maste]

On 28 June 2013 09:05, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 28 June 2013 14:03, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 28/06/2013 14:54, Ed Maste ha scritto:
>>> An objection seemed to be that it is not really needed yet, but as
>>> pointed out by Peter this isn't really the case.  This patch brings us
>>> (FreeBSD) in line with per-thread data semantics on Linux and is used
>>> by qemu system emulation as well.
>>
>> It brings it in line with Linux, even though FreeBSD has the same need
>> for TLS as OpenBSD and Windows (i.e. none).  In other words, we go from
>> two cases (need TLS and uses it, has no TLS and doesn't need it) to
>> three (we add "uses TLS with no need for it").
>
> I don't understand what you mean by "no need for it". We are
> already multithreaded in system mode, so this is simply
> making FreeBSD do the same thing as Linux. That seems like a
> clear improvement to me.

I agree; even if this is a hack, it's a well-tested one.  Indeed, even
on a FreeBSD host most users already have this change, as they have
built from ports or used prebuilt packages.

Re: [Qemu-devel] [Qemu-trivial] [PATCH] tls.h: Enable TLS on FreeBSD

[Paolo Bonzini]

Il 28/06/2013 15:05, Peter Maydell ha scritto:
>>> >> An objection seemed to be that it is not really needed yet, but as
>>> >> pointed out by Peter this isn't really the case.  This patch brings us
>>> >> (FreeBSD) in line with per-thread data semantics on Linux and is used
>>> >> by qemu system emulation as well.
>> >
>> > It brings it in line with Linux, even though FreeBSD has the same need
>> > for TLS as OpenBSD and Windows (i.e. none).  In other words, we go from
>> > two cases (need TLS and uses it, has no TLS and doesn't need it) to
>> > three (we add "uses TLS with no need for it").
> I don't understand what you mean by "no need for it". We are
> already multithreaded in system mode, so this is simply
> making FreeBSD do the same thing as Linux. 

cpu_single_env is protected by the BQL unless you're running on KVM.

Paolo

Re: [Qemu-devel] [Qemu-trivial] [PATCH] tls.h: Enable TLS on FreeBSD

[Peter Maydell]

On 28 June 2013 15:47, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 28/06/2013 15:05, Peter Maydell ha scritto:
>> I don't understand what you mean by "no need for it". We are
>> already multithreaded in system mode, so this is simply
>> making FreeBSD do the same thing as Linux.
>
> cpu_single_env is protected by the BQL unless you're running on KVM.

That doesn't make it magically not a per-thread variable.

-- PMM

Re: [Qemu-devel] [Qemu-trivial] [PATCH] tls.h: Enable TLS on FreeBSD

[Paolo Bonzini]

Il 28/06/2013 16:55, Peter Maydell ha scritto:
>>> >> I don't understand what you mean by "no need for it". We are
>>> >> already multithreaded in system mode, so this is simply
>>> >> making FreeBSD do the same thing as Linux.
>> >
>> > cpu_single_env is protected by the BQL unless you're running on KVM.
> That doesn't make it magically not a per-thread variable.

If it were possible to say "make cpu_single_env magically not per-thread
on Linux/TCG", I'd be all for it!

But I _can_ make cpu_single_env not per-thread on FreeBSD/TCG, because
no one needs per-thread cpu_single_env on FreeBSD.

If we could drop support for platforms that lack __thread (or some other
fast TLS mechanism), then it would be another story of course.  We would
not have to do things like this for qtest:

    while (1) {
        cpu_single_env = NULL;
        qemu_mutex_unlock_iothread();
        do {
            int sig;
            r = sigwait(&waitset, &sig);
        } while (r == -1 && (errno == EAGAIN || errno == EINTR));
        if (r == -1) {
            perror("sigwait");
            exit(1);
        }
        qemu_mutex_lock_iothread();
        cpu_single_env = env;
        qemu_wait_io_event_common(cpu);
    }

or similarly set cpu_single_env in cpu_exec.

And of course if bsd-user supported 1:1 mapping between guest and host
threads on FreeBSD, cpu_single_env would have to be thread-local.

Paolo

Re: [Qemu-devel] [Qemu-trivial] [PATCH] tls.h: Enable TLS on FreeBSD

[Ed Maste]

On 28 June 2013 11:11, Paolo Bonzini <pbonzini@redhat.com> wrote:
> And of course if bsd-user supported 1:1 mapping between guest and host
> threads on FreeBSD, cpu_single_env would have to be thread-local.

This is the case for the bsd-user work that Stacey posted at the
beginning of the week and is currently being revised after the initial
round of comments.

Can you comment on how quickly your tls work is expected to arrive?
We'll have to rebase on top of it, if it's in the next few days or so.
 If it's longer than that we'll need this change as a prerequisite for
the bsd-user work.

After Peter suggested splitting this change out I sent it to -trivial
because I already had it in a test tree, and it has no effect on
non-FreeBSD hosts.  If it's really objectionable right now we can
include it with v2 of the bsd-user series.

Re: [Qemu-devel] [Qemu-trivial] [PATCH] tls.h: Enable TLS on FreeBSD

[Paolo Bonzini]

Il 28/06/2013 17:35, Ed Maste ha scritto:
> On 28 June 2013 11:11, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> And of course if bsd-user supported 1:1 mapping between guest and host
>> threads on FreeBSD, cpu_single_env would have to be thread-local.
> 
> This is the case for the bsd-user work that Stacey posted at the
> beginning of the week and is currently being revised after the initial
> round of comments.
> 
> Can you comment on how quickly your tls work is expected to arrive?
> We'll have to rebase on top of it, if it's in the next few days or so.

Maybe a week or two.

To simplify rebase, if you want you can take the conflicting patch from
http://article.gmane.org/gmane.comp.emulators.qemu/216583/raw.

>  If it's longer than that we'll need this change as a prerequisite for
> the bsd-user work.
> 
> After Peter suggested splitting this change out I sent it to -trivial
> because I already had it in a test tree, and it has no effect on
> non-FreeBSD hosts.  If it's really objectionable right now we can
> include it with v2 of the bsd-user series.

Yes, that would be preferrable, either with or without the patch above.

Paolo

Re: [Qemu-devel] [Qemu-trivial] [PATCH] tls.h: Enable TLS on FreeBSD

[Stefan Hajnoczi]

On Fri, Jun 28, 2013 at 05:37:54PM +0200, Paolo Bonzini wrote:
> Il 28/06/2013 17:35, Ed Maste ha scritto:
> > On 28 June 2013 11:11, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> And of course if bsd-user supported 1:1 mapping between guest and host
> >> threads on FreeBSD, cpu_single_env would have to be thread-local.
> > 
> > This is the case for the bsd-user work that Stacey posted at the
> > beginning of the week and is currently being revised after the initial
> > round of comments.
> > 
> > Can you comment on how quickly your tls work is expected to arrive?
> > We'll have to rebase on top of it, if it's in the next few days or so.
> 
> Maybe a week or two.
> 
> To simplify rebase, if you want you can take the conflicting patch from
> http://article.gmane.org/gmane.comp.emulators.qemu/216583/raw.

I'll post the latest tweaked version of Paolo's improved tls.h that I'm
working with today.

Stefan