Cc: Aleksandar Markovic <amarkovic@wavecomp.com>
Cc: Alexander Graf <agraf@suse.de>
Cc: Alistair Francis <alistair23@gmail.com>
Cc: Andrzej Zaborowski <balrogg@gmail.com>
Cc: Anthony Green <green@moxielogic.com>
Cc: Artyom Tarasenko <atar4qemu@gmail.com>
Cc: Aurelien Jarno <aurelien@aurel32.net>
Cc: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: Chris Wulff <crwulff@gmail.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: David Hildenbrand <david@redhat.com>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Fabien Chouteau <chouteau@adacore.com>
Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
Cc: James Hogan <jhogan@kernel.org>
Cc: Laurent Vivier <laurent@vivier.eu>
Cc: Marek Vasut <marex@denx.de>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: Michael Clark <mjc@sifive.com>
Cc: Michael Walle <michael@walle.cc>
Cc: Palmer Dabbelt <palmer@sifive.com>
Cc: Pavel Dovgalyuk <dovgaluk@ispras.ru>
Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
Cc: qemu-ppc@nongnu.org
Cc: qemu-s390x@nongnu.org
Cc: Richard Henderson <rth@twiddle.net>
Cc: Sagar Karandikar <sagark@eecs.berkeley.edu>
Cc: Stafford Horne <shorne@gmail.com>

I'm calling this series a v3 because it supersedes the two series
I previously sent about using atomics for interrupt_request:
  https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg02013.html
The approach in that series cannot work reliably; using (locked) atomics
to set interrupt_request but not using (locked) atomics to read it
can lead to missed updates.

This series takes a different approach: it serializes access to many
CPUState fields, including .interrupt_request, with a per-CPU lock.

Protecting more fields of CPUState with the lock then allows us to
substitute the BQL for the per-CPU lock in many places, notably
the execution loop in cpus.c. This leads to better scalability
for MTTCG, since CPUs don't have to acquire a contended lock
(the BQL) every time they stop executing code.

Some hurdles that remain:

1. I am not happy with the shutdown path via pause_all_vcpus.
   What happens if
   (a) A CPU is added while we're calling pause_all_vcpus?
   (b) Some CPUs are trying to run exclusive work while we
       call pause_all_vcpus?
   Am I being overly paranoid here?

2. I have done very light testing with x86_64 KVM, and no
   testing with other accels (hvf, hax, whpx). check-qtest
   works, except for an s390x test that to me is broken
   in master -- I reported the problem here:
     https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg03728.html

3. This might break record-replay. Furthermore, a quick test with icount
   on aarch64 seems to work, but I haven't tested icount extensively.

4. Some architectures still need the BQL in cpu_has_work.
   This leads to some contortions to avoid deadlock, since
   in this series cpu_has_work is called with the CPU lock held.

5. The interrupt handling path remains with the BQL held, mostly
   because the ISAs I routinely work with need the BQL anyway
   when handling the interrupt. We can complete the pushdown
   of the BQL to .do_interrupt/.exec_interrupt later on; this
   series is already way too long.

Points (1)-(3) makes this series an RFC and not a proper patch series.
I'd appreciate feedback on this approach and/or testing.

Note that this series fixes a bug by which cpu_has_work is
called without the BQL (from cpu_handle_halt). After
this series, cpu_has_work is called with the CPU lock,
and only the targets that need the BQL in cpu_has_work
acquire it.

For some performance numbers, see the last patch.

The series is checkpatch-clean; only one warning due to the
use of __COVERITY__ in cpus.c.

You can fetch this series from:

  https://github.com/cota/qemu/tree/cpu-lock-v3

Note that it applies on top of tcg-next + my dynamic TLB series,
which I'm using in the faint hope that the ubuntu experiments might
run a bit faster.

Thanks!

		Emilio

On 19/10/2018 03:05, Emilio G. Cota wrote:
> I'm calling this series a v3 because it supersedes the two series
> I previously sent about using atomics for interrupt_request:
>   https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg02013.html
> The approach in that series cannot work reliably; using (locked) atomics
> to set interrupt_request but not using (locked) atomics to read it
> can lead to missed updates.

The idea here was that changes to protected fields are all followed by
kick.  That may not have been the case, granted, but I wonder if the
plan is unworkable.

Paolo

On Fri, Oct 19, 2018 at 08:59:24 +0200, Paolo Bonzini wrote:
> On 19/10/2018 03:05, Emilio G. Cota wrote:
> > I'm calling this series a v3 because it supersedes the two series
> > I previously sent about using atomics for interrupt_request:
> >   https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg02013.html
> > The approach in that series cannot work reliably; using (locked) atomics
> > to set interrupt_request but not using (locked) atomics to read it
> > can lead to missed updates.
> 
> The idea here was that changes to protected fields are all followed by
> kick.  That may not have been the case, granted, but I wonder if the
> plan is unworkable.

I suspect that the cpu->interrupt_request+kick mechanism is not the issue,
otherwise master should not work--we do atomic_read(cpu->interrupt_request)
and only if that read != 0 we take the BQL.

My guess is that the problem is with other reads of cpu->interrupt_request,
e.g. those in cpu_has_work. Currently those reads happen with the
BQL held, and updates to cpu->interrupt_request take the BQL. If we drop
the BQL from the setters to instead use locked atomics (like in the
aforementioned series), those BQL-protected readers might miss updates.

Given that we need a per-CPU lock anyway to remove the BQL from the
CPU loop, extending this lock to protect cpu->interrupt_request is
a simple solution that keeps the current logic and allows for
greater scalability.

Thanks,

		Emilio

On 19/10/2018 16:50, Emilio G. Cota wrote:
> On Fri, Oct 19, 2018 at 08:59:24 +0200, Paolo Bonzini wrote:
>> On 19/10/2018 03:05, Emilio G. Cota wrote:
>>> I'm calling this series a v3 because it supersedes the two series
>>> I previously sent about using atomics for interrupt_request:
>>>   https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg02013.html
>>> The approach in that series cannot work reliably; using (locked) atomics
>>> to set interrupt_request but not using (locked) atomics to read it
>>> can lead to missed updates.
>>
>> The idea here was that changes to protected fields are all followed by
>> kick.  That may not have been the case, granted, but I wonder if the
>> plan is unworkable.
> 
> I suspect that the cpu->interrupt_request+kick mechanism is not the issue,
> otherwise master should not work--we do atomic_read(cpu->interrupt_request)
> and only if that read != 0 we take the BQL.
> 
> My guess is that the problem is with other reads of cpu->interrupt_request,
> e.g. those in cpu_has_work. Currently those reads happen with the
> BQL held, and updates to cpu->interrupt_request take the BQL. If we drop
> the BQL from the setters to instead use locked atomics (like in the
> aforementioned series), those BQL-protected readers might miss updates.

cpu_has_work is only needed to handle the processor's halted state (or
is it?).  If it is, OR+kick should work.

> Given that we need a per-CPU lock anyway to remove the BQL from the
> CPU loop, extending this lock to protect cpu->interrupt_request is
> a simple solution that keeps the current logic and allows for
> greater scalability.

Sure, I was just curious what the problem was.  KVM uses OR+kick with no
problems.

Paolo

On Fri, Oct 19, 2018 at 18:01:18 +0200, Paolo Bonzini wrote:
> On 19/10/2018 16:50, Emilio G. Cota wrote:
> > On Fri, Oct 19, 2018 at 08:59:24 +0200, Paolo Bonzini wrote:
> >> On 19/10/2018 03:05, Emilio G. Cota wrote:
> >>> I'm calling this series a v3 because it supersedes the two series
> >>> I previously sent about using atomics for interrupt_request:
> >>>   https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg02013.html
> >>> The approach in that series cannot work reliably; using (locked) atomics
> >>> to set interrupt_request but not using (locked) atomics to read it
> >>> can lead to missed updates.
> >>
> >> The idea here was that changes to protected fields are all followed by
> >> kick.  That may not have been the case, granted, but I wonder if the
> >> plan is unworkable.
> > 
> > I suspect that the cpu->interrupt_request+kick mechanism is not the issue,
> > otherwise master should not work--we do atomic_read(cpu->interrupt_request)
> > and only if that read != 0 we take the BQL.
> > 
> > My guess is that the problem is with other reads of cpu->interrupt_request,
> > e.g. those in cpu_has_work. Currently those reads happen with the
> > BQL held, and updates to cpu->interrupt_request take the BQL. If we drop
> > the BQL from the setters to instead use locked atomics (like in the
> > aforementioned series), those BQL-protected readers might miss updates.
> 
> cpu_has_work is only needed to handle the processor's halted state (or
> is it?).  If it is, OR+kick should work.
> 
> > Given that we need a per-CPU lock anyway to remove the BQL from the
> > CPU loop, extending this lock to protect cpu->interrupt_request is
> > a simple solution that keeps the current logic and allows for
> > greater scalability.
> 
> Sure, I was just curious what the problem was.  KVM uses OR+kick with no
> problems.

I never found exactly where things break. The hangs happen
pretty early when booting a large (-smp > 16) x86_64 Ubuntu guest.
Booting never completes (ssh unresponsive) if I don't have the
console output (I suspect the console output slows things down
enough to hide some races). I only see a few threads busy:
a couple of vCPU threads, and the I/O thread.

I didn't have time to debug any further, so I moved on
to an alternative approach.

So it is possible that it was my implementation, and not the approach,
what was at fault :-)

Thanks,

		E.

On Fri, Oct 19, 2018 at 15:29:32 -0400, Emilio G. Cota wrote:
> On Fri, Oct 19, 2018 at 18:01:18 +0200, Paolo Bonzini wrote:
> > > Given that we need a per-CPU lock anyway to remove the BQL from the
> > > CPU loop, extending this lock to protect cpu->interrupt_request is
> > > a simple solution that keeps the current logic and allows for
> > > greater scalability.
> > 
> > Sure, I was just curious what the problem was.  KVM uses OR+kick with no
> > problems.
> 
> I never found exactly where things break. The hangs happen
> pretty early when booting a large (-smp > 16) x86_64 Ubuntu guest.
> Booting never completes (ssh unresponsive) if I don't have the
> console output (I suspect the console output slows things down
> enough to hide some races). I only see a few threads busy:
> a couple of vCPU threads, and the I/O thread.
> 
> I didn't have time to debug any further, so I moved on
> to an alternative approach.
> 
> So it is possible that it was my implementation, and not the approach,
> what was at fault :-)

I've just observed a similar hang after adding the "BQL
pushdown" patches on top of this series. So it's likely that the
hangs come from those patches, and not from the work on
cpu->interrupt_request. I just confirmed with the prior
series, and removing the pushdown patches fixes the hangs there
as well.

Thanks,

		Emilio

On 20/10/2018 01:46, Emilio G. Cota wrote:
>> So it is possible that it was my implementation, and not the approach,
>> what was at fault :-)
> I've just observed a similar hang after adding the "BQL
> pushdown" patches on top of this series. So it's likely that the
> hangs come from those patches, and not from the work on
> cpu->interrupt_request. I just confirmed with the prior
> series, and removing the pushdown patches fixes the hangs there
> as well.

Oh well, not a big deal.  You already wrote these patches and I don't
have much time for MTTCG anyway, so I am okay with sticking with them.
Thanks!

Paolo