On 8/28/24 11:43 AM, Alexei Starovoitov wrote:
On Wed, Aug 28, 2024 at 10:44 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
- for (i = 0; i < insn_cnt; i++, insn++) {
+ for (i = skip_cnt; i < insn_cnt; i++, insn++) {
Do we really need to add this argument?
- WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1));
+ WARN_ON(adjust_jmp_off(env->prog, subprog_start, 1, 0));
We can always do for (i = delta; ...
The above case of skip_cnt == 0 is lucky to work this way.
It would be less surprising to skip all insns in the patch.
Maybe I'm missing something.
For subprog_start case, tgt_idx (where the patch started) may not be 0. How
about this:
for (i = 0; i < insn_cnt; i++, insn++) {
if (tgt_idx <= i && i < tgt_idx + delta)
continue;
Yeah. Right. Same idea, but certainly your way is more correct
instead of my buggy proposal.
In that sense the "for (i = skip_cnt" approach
is also a bit buggy, if tgt_idx != 0.
Yep. Adding skip_cnt like this patch 2 is not right. I didn't think hard enough
what to do with the existing adjust_jmp_off().
I will remove skip_cnt in the next respin.
Thanks for the review!
