Re: [PATCH v2 0/7] accel/tcg: remove implied BQL from cpu_handle_interrupt/exception path

[Cornelia Huck]

On Wed, 19 Aug 2020 14:28:49 -0400
Robert Foley <robert.foley@linaro.org> wrote:

> The purpose of this change is to set the groundwork
> so that an arch could move towards removing
> the BQL from the cpu_handle_interrupt/exception paths.
> 
> The BQL is a bottleneck in scaling to more cores.
> And this cpu_handle_interrupt/exception path is one of
> the key BQL users as measured by the QEMU sync profiling (qsp).
> 
> As the first step in removing the BQL from this path, we will make
> changes to the core/common functions of cpu_handle_interrupt/exception
> to drop the holding of the BQL. The holding of the BQL is pushed down
> to the per-arch implementation code.

I have only skimmed the patches I was cc:ed on so far, but the series
seems sane to me in general.

> 
> This patch goes through several transitions of the code in order to
> maintain correctness (bisectability).  In order to maintain
> bisectability across these steps some patches need to touch many
> files across different arches, however most of the changes are trivial.
> 
> The general order of the changes is below where each step
> represents one patch.
> 
> 1) rename all *_do_interrupt functions to *_do_interrupt_locked

I'm wondering whether this renaming could be done in an automated way
(e.g. via Coccinelle). Reviewing the method for the renaming is often
easier than looking at a lot of similar code patterns.

> 
> 2) add a new function *_do_interrupt that takes the BQL and calls
> *_do_interrupt_locked, point ->do_interrupt to it, and remove 
> the BQL from cpu-exec.c's cpu_handle_exception.
> 
> 3) modify the BQL critical sections around
> ->cpu_exec_interrupt, so that the BQL critical section covers just the  
> call to ->cpu_exec_interrupt. 
> 
> 4/5) same as 1/2 for ->cpu_exec_interrupt.  This removes the BQL
> from cpu_handle_exception.

The method of doing this in steps looks fine, although the patches
produced are a bit unwieldy -- that's something we have to live with, I
guess.