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 <[email protected]> --- 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. */ typedef struct DynamicGDBFeatureInfo { GDBFeature desc; union { struct { - uint32_t *keys; + GPtrArray *regs; } cpregs; } data; } DynamicGDBFeatureInfo; diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c index d6e29c4cf46745dc7851bc5f8339c8d7c6857b8c..e66af8207b436fe8f1dd27441a587b1eb52ff928 100644 --- a/target/arm/gdbstub.c +++ b/target/arm/gdbstub.c @@ -238,19 +238,23 @@ static int arm_gdb_get_sysreg(CPUState *cs, GByteArray *buf, int reg) ARMCPU *cpu = ARM_CPU(cs); CPUARMState *env = &cpu->env; const ARMCPRegInfo *ri; - uint32_t key; - key = cpu->dyn_sysreg_feature.data.cpregs.keys[reg]; - ri = get_arm_cp_reginfo(cpu->cp_regs, key); + ri = g_ptr_array_index(cpu->dyn_sysreg_feature.data.cpregs.regs, reg); if (ri) { switch (cpreg_field_type(ri)) { case MO_64: if (ri->vhe_redir_to_el2 && (arm_hcr_el2_eff(env) & HCR_E2H) && arm_current_el(env) == 2) { - ri = get_arm_cp_reginfo(cpu->cp_regs, ri->vhe_redir_to_el2); + ri = g_ptr_array_index( + cpu->dyn_sysreg_feature.data.cpregs.regs, + ri->vhe_redir_to_el2 + ); } else if (ri->vhe_redir_to_el01) { - ri = get_arm_cp_reginfo(cpu->cp_regs, ri->vhe_redir_to_el01); + ri = g_ptr_array_index( + cpu->dyn_sysreg_feature.data.cpregs.regs, + ri->vhe_redir_to_el01 + ); } return gdb_get_reg64(buf, (uint64_t)read_raw_cp_reg(env, ri)); case MO_32: @@ -269,19 +273,18 @@ static int arm_gdb_set_sysreg(CPUState *cs, uint8_t *buf, int reg) static void arm_gen_one_feature_sysreg(GDBFeatureBuilder *builder, DynamicGDBFeatureInfo *dyn_feature, - ARMCPRegInfo *ri, uint32_t ri_key, + ARMCPRegInfo *ri, int bitsize, int n) { gdb_feature_builder_append_reg(builder, ri->name, bitsize, n, "int", "cp_regs"); - dyn_feature->data.cpregs.keys[n] = ri_key; + g_ptr_array_index(dyn_feature->data.cpregs.regs, n) = ri; } static void arm_register_sysreg_for_feature(gpointer key, gpointer value, gpointer p) { - uint32_t ri_key = (uintptr_t)key; ARMCPRegInfo *ri = value; RegisterSysregFeatureParam *param = p; ARMCPU *cpu = ARM_CPU(param->cs); @@ -292,7 +295,7 @@ static void arm_register_sysreg_for_feature(gpointer key, gpointer value, if (arm_feature(env, ARM_FEATURE_AARCH64)) { if (ri->state == ARM_CP_STATE_AA64) { arm_gen_one_feature_sysreg(¶m->builder, dyn_feature, - ri, ri_key, 64, param->n++); + ri, 64, param->n++); } } else { if (ri->state == ARM_CP_STATE_AA32) { @@ -302,10 +305,10 @@ static void arm_register_sysreg_for_feature(gpointer key, gpointer value, } if (ri->type & ARM_CP_64BIT) { arm_gen_one_feature_sysreg(¶m->builder, dyn_feature, - ri, ri_key, 64, param->n++); + ri, 64, param->n++); } else { arm_gen_one_feature_sysreg(¶m->builder, dyn_feature, - ri, ri_key, 32, param->n++); + ri, 32, param->n++); } } } @@ -323,7 +326,8 @@ static GDBFeature *arm_gen_dynamic_sysreg_feature(CPUState *cs, int base_reg) "org.qemu.gdb.arm.sys.regs", "system-registers.xml", base_reg); - cpu->dyn_sysreg_feature.data.cpregs.keys = g_new(uint32_t, num_regs); + cpu->dyn_sysreg_feature.data.cpregs.regs = g_ptr_array_new(); + g_ptr_array_set_size(cpu->dyn_sysreg_feature.data.cpregs.regs, num_regs); g_hash_table_foreach(cpu->cp_regs, arm_register_sysreg_for_feature, ¶m); gdb_feature_builder_end(¶m.builder); return &cpu->dyn_sysreg_feature.desc; --- base-commit: ac6721b88df944ade0048822b2b74210f543d656 change-id: 20260518-enhance_arm_gdb_get_sysreg_performance-4288474401d6 Best regards, -- Abdelrahman Elbehery <[email protected]>
