| Message ID | 1372101677-46175-1-git-send-email-emaste@freebsd.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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.
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. >
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
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.
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
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
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
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.
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
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
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
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(-)