Re: [PATCH] target/arm: replace cp_regs hashtable lookup with direct array index

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 6/2/26 08:19, abdelrahman elbehery wrote:
>> Ping
>> On Mon, 18 May 2026, 12:58 pm Abdelrahman Elbehery,
>> <elbeheri95@gmail.com <mailto:elbeheri95@gmail.com>> wrote:
>>     Profiling showed g_hash_table_lookup as a hot spot when running
>> TCG
>>     plugins that log all CPU registers (e.g. execlog with reg=*).
>>     Replace it with a direct array index to avoid lookup overhead.
>>     Signed-off-by: Abdelrahman Elbehery <elbeheri95@gmail.com
>> <mailto:elbeheri95@gmail.com>>
>>     ---
>>     While profiling (via callgrind) a TCG plugin that logs all the CPU registers
>>     (very identical to execlog with reg=*) i noticed that arm_gdb_get_sysreg is a hot spot,
>>     mostly while calling g_hash_table_lookup. Probably this is due to some hashtable collision
>>     that is handleded by glib internally.
>>     Currently each DynamicGDBFeatureInfo item holds keys for
>> cp_regs, and the keys are always
>>     used to retrieve the ARMCPRegInfo pointer.
>>     By replacing the keys array with a GPtrArray, we now just access
>> reg info directly from
>>     the given index.
>>     The benchmarking was done on c2d-highcpu-16 (16 vCPUs, 32 GB
>> memory) GCP
>>     Setup was to run qemu-system-aarch64 against a buildroot kernel
>> and rootfs
>>     with and without the patch mostly using:
>>          --plugin contrib/plugins/libstoptrigger.so,icount=2000000
>>          --plugin contrib/plugins/libexeclog.so,reg=*
>>          -M virt -cpu cortex-a710 -smp 1
>>     Benchmark showed ~15% performance gain when hashtable is not
>> used.
>>     For benchmarking, i used hyperfine with 3 warmup rounds and 10 iterations each.
>>     ---
>>       target/arm/cpu.h     |  6 +++---
>>       target/arm/gdbstub.c | 28 ++++++++++++++++------------
>>       2 files changed, 19 insertions(+), 15 deletions(-)
>>     diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>     index
>>     15a13b929276dad161032b61ba51ebbce7eeebc6..8816e94e3f8271ad403e77f6182fdb8cee31c527 100644
>>     --- a/target/arm/cpu.h
>>     +++ b/target/arm/cpu.h
>>     @@ -121,15 +121,15 @@
>>        * DynamicGDBFeatureInfo:
>>        * @desc: Contains the feature descriptions.
>>        * @data: A union with data specific to the set of registers
>>     - *    @cpregs_keys: Array that contains the corresponding Key of
>>     - *                  a given cpreg with the same order of the cpreg
>>     + *    @cpregs_regs: Array that contains the corresponding Register info pointer
>>     + *                  of a given cpreg with the same order of the cpreg
>>        *                  in the XML description.
>
> This will not work, because each ARMCPU has separate cpreg info
> structures, and these structures only have the lifetime of the ARMCPU
> struct.  We create and destroy these at runtime, so at some point you
> may end up with pointers to freed memory.

Shouldn't it be the same set for each CPU though?

>
>
> r~

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro