Re: [PATCH v4 0/8] Add OPTPROBES feature on RISCV

From: Xim <chenguokai17@mails.ucas.ac.cn>
To: "Björn Töpel" <bjorn@kernel.org>
Cc: paul.walmsley@sifive.com, palmer@dabbelt.com,
	aou@eecs.berkeley.edu, rostedt@goodmis.org, mingo@redhat.com,
	sfr@canb.auug.org.au, linux-riscv@lists.infradead.org,
	linux-kernel@vger.kernel.org, liaochang1@huawei.com,
	Liao Chang <liaoclark@163.com>
Subject: Re: [PATCH v4 0/8] Add OPTPROBES feature on RISCV
Date: Tue, 8 Nov 2022 19:04:09 +0800	[thread overview]
Message-ID: <9A705974-A007-45E2-BC5D-A7E90821A258@mails.ucas.ac.cn> (raw)
In-Reply-To: <87y1sm1z8j.fsf@all.your.base.are.belong.to.us>

Hi Björn,

Thanks for your great review! Some explanations below.

> 2022年11月8日 00:54，Björn Töpel <bjorn@kernel.org> 写道：
> 
> Have you run the series on real hardware, or just qemu?

Currently only qemu tests are made, I will try to test it on a FPGA real hardware soon.

> AFAIU, the algorithm only tracks registers that are *in use*. You are
> already scanning the whole function (next patch). What about the caller
> saved registers that are *not* used by the function in the probe range?
> Can those, potentially unused, regs be used?

Great missing part! I have made a static analyzation right upon receiving this mail.
The result shows that this newly purposed idea reaches about the same
success rate on my test set (rv64 defconf with RVI only) while when combined,
they can reach a higher success rate, 1/3 above their baseline. A patch that
includes this strategy will be sent soon.
> 
>> +static void arch_find_register(unsigned long start, unsigned long end,
> 
> Nit; When I see "arch_" I think it's functionality that can be
> overridden per-arch. This is not the case, but just a helper for RV.

It can be explained from two aspects. First, it can be extended to most RISC
archs, which can be extracted into the common flow of Kprobe. Second, it is indeed
a internal helper for now, so I will correct the name in the next version.

>> static void find_free_registers(struct kprobe *kp, struct optimized_kprobe *op,
>> -				int *rd1, int *rd2)
>> +				int *rd, int *ra)
> 
> Nit; Please get rid of this code churn, just name the parameters
> correctly on introduction in the previous patch.

Will be fixed.

>> +	*rd = ((kw | ow) == 1UL) ? 0 : __builtin_ctzl((kw | ow) & ~1UL);
>> +	*ra = (kw == 1UL) ? 0 : __builtin_ctzl(kw & ~1UL);
> 
> Hmm, __builtin_ctzl is undefined for 0, right? Can that be triggered
> here?

Will be fixed.

Regards,
Guokai Chen

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv