| Message ID | 20230406073519.75059-1-hbathini@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/bpf: populate extable entries only during the last pass | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
Le 06/04/2023 à 09:35, Hari Bathini a écrit : > Since commit 85e031154c7c ("powerpc/bpf: Perform complete extra passes > to update addresses"), two additional passes are performed to avoid > space and CPU time wastage on powerpc. But these extra passes led to > WARN_ON_ONCE() hits in bpf_add_extable_entry(). Fix it by not adding > extable entries during the extra pass. Are you sure this change is correct ? During the extra pass the code can get shrinked or expanded (within the limits of the size of the preliminary pass). Shouldn't extable entries be populated during the last pass ? Christophe > > Fixes: 85e031154c7c ("powerpc/bpf: Perform complete extra passes to update addresses") > Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> > --- > arch/powerpc/net/bpf_jit_comp32.c | 2 +- > arch/powerpc/net/bpf_jit_comp64.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c > index 7f91ea064c08..e788b1fbeee6 100644 > --- a/arch/powerpc/net/bpf_jit_comp32.c > +++ b/arch/powerpc/net/bpf_jit_comp32.c > @@ -977,7 +977,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * > if (size != BPF_DW && !fp->aux->verifier_zext) > EMIT(PPC_RAW_LI(dst_reg_h, 0)); > > - if (BPF_MODE(code) == BPF_PROBE_MEM) { > + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) { > int insn_idx = ctx->idx - 1; > int jmp_off = 4; > > diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c > index 8dd3cabaa83a..1cc2777ec846 100644 > --- a/arch/powerpc/net/bpf_jit_comp64.c > +++ b/arch/powerpc/net/bpf_jit_comp64.c > @@ -921,7 +921,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * > if (size != BPF_DW && insn_is_zext(&insn[i + 1])) > addrs[++i] = ctx->idx * 4; > > - if (BPF_MODE(code) == BPF_PROBE_MEM) { > + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) { > ret = bpf_add_extable_entry(fp, image, pass, ctx, ctx->idx - 1, > 4, dst_reg); > if (ret)
Hello Christophe, Thanks for the review. On 07/04/23 11:31 am, Christophe Leroy wrote: > > > Le 06/04/2023 à 09:35, Hari Bathini a écrit : >> Since commit 85e031154c7c ("powerpc/bpf: Perform complete extra passes >> to update addresses"), two additional passes are performed to avoid >> space and CPU time wastage on powerpc. But these extra passes led to >> WARN_ON_ONCE() hits in bpf_add_extable_entry(). Fix it by not adding >> extable entries during the extra pass. > > Are you sure this change is correct ? Actually, I was in two minds about that owing to commit 04c04205bc35 ("bpf powerpc: Remove extra_pass from bpf_jit_build_body()"). > During the extra pass the code can get shrinked or expanded (within the > limits of the size of the preliminary pass). Shouldn't extable entries > be populated during the last pass ? Unlikely, but the intention there was to eliminate a regression in case extra_pass ends up being 'false' always in any subsequent change. - Hari >> >> Fixes: 85e031154c7c ("powerpc/bpf: Perform complete extra passes to update addresses") >> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> >> --- >> arch/powerpc/net/bpf_jit_comp32.c | 2 +- >> arch/powerpc/net/bpf_jit_comp64.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c >> index 7f91ea064c08..e788b1fbeee6 100644 >> --- a/arch/powerpc/net/bpf_jit_comp32.c >> +++ b/arch/powerpc/net/bpf_jit_comp32.c >> @@ -977,7 +977,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * >> if (size != BPF_DW && !fp->aux->verifier_zext) >> EMIT(PPC_RAW_LI(dst_reg_h, 0)); >> >> - if (BPF_MODE(code) == BPF_PROBE_MEM) { >> + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) { >> int insn_idx = ctx->idx - 1; >> int jmp_off = 4; >> >> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c >> index 8dd3cabaa83a..1cc2777ec846 100644 >> --- a/arch/powerpc/net/bpf_jit_comp64.c >> +++ b/arch/powerpc/net/bpf_jit_comp64.c >> @@ -921,7 +921,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * >> if (size != BPF_DW && insn_is_zext(&insn[i + 1])) >> addrs[++i] = ctx->idx * 4; >> >> - if (BPF_MODE(code) == BPF_PROBE_MEM) { >> + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) { >> ret = bpf_add_extable_entry(fp, image, pass, ctx, ctx->idx - 1, >> 4, dst_reg); >> if (ret)
Hari Bathini wrote: > Hello Christophe, > > Thanks for the review. > > On 07/04/23 11:31 am, Christophe Leroy wrote: >> >> >> Le 06/04/2023 à 09:35, Hari Bathini a écrit : >>> Since commit 85e031154c7c ("powerpc/bpf: Perform complete extra passes >>> to update addresses"), two additional passes are performed to avoid >>> space and CPU time wastage on powerpc. But these extra passes led to >>> WARN_ON_ONCE() hits in bpf_add_extable_entry(). Fix it by not adding >>> extable entries during the extra pass. >> >> Are you sure this change is correct ? > > Actually, I was in two minds about that owing to commit 04c04205bc35 > ("bpf powerpc: Remove extra_pass from bpf_jit_build_body()"). Right, but Christophe's series adding complete passes during the extra_pass phase added 'extra_pass' parameter back to bpf_jit_build_body(). > >> During the extra pass the code can get shrinked or expanded (within the >> limits of the size of the preliminary pass). Shouldn't extable entries >> be populated during the last pass ? > > Unlikely, but the intention there was to eliminate a regression in case > extra_pass ends up being 'false' always in any subsequent change. But, the current approach risks generating incorrect offsets in the extable. The main motivation for the extra pass is to generate more compact code, so there is a good chance that offsets are going to change (especially with bpf subprogs). > > - Hari > >>> >>> Fixes: 85e031154c7c ("powerpc/bpf: Perform complete extra passes to update addresses") >>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> >>> --- >>> arch/powerpc/net/bpf_jit_comp32.c | 2 +- >>> arch/powerpc/net/bpf_jit_comp64.c | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c >>> index 7f91ea064c08..e788b1fbeee6 100644 >>> --- a/arch/powerpc/net/bpf_jit_comp32.c >>> +++ b/arch/powerpc/net/bpf_jit_comp32.c >>> @@ -977,7 +977,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * >>> if (size != BPF_DW && !fp->aux->verifier_zext) >>> EMIT(PPC_RAW_LI(dst_reg_h, 0)); >>> >>> - if (BPF_MODE(code) == BPF_PROBE_MEM) { >>> + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) { It is probably better to pass 'extra_pass' into bpf_add_extable_entry() to keep all those checks together. - Naveen
Hi Naveen, On 24/04/23 5:25 pm, Naveen N. Rao wrote: > Hari Bathini wrote: >> Hello Christophe, >> >> Thanks for the review. >> >> On 07/04/23 11:31 am, Christophe Leroy wrote: >>> >>> >>> Le 06/04/2023 à 09:35, Hari Bathini a écrit : >>>> Since commit 85e031154c7c ("powerpc/bpf: Perform complete extra passes >>>> to update addresses"), two additional passes are performed to avoid >>>> space and CPU time wastage on powerpc. But these extra passes led to >>>> WARN_ON_ONCE() hits in bpf_add_extable_entry(). Fix it by not adding >>>> extable entries during the extra pass. >>> >>> Are you sure this change is correct ? >> >> Actually, I was in two minds about that owing to commit 04c04205bc35 >> ("bpf powerpc: Remove extra_pass from bpf_jit_build_body()"). > > Right, but Christophe's series adding complete passes during the > extra_pass phase added 'extra_pass' parameter back to bpf_jit_build_body(). > >> >>> During the extra pass the code can get shrinked or expanded (within the >>> limits of the size of the preliminary pass). Shouldn't extable entries >>> be populated during the last pass ? >> >> Unlikely, but the intention there was to eliminate a regression in case >> extra_pass ends up being 'false' always in any subsequent change. > > But, the current approach risks generating incorrect offsets in the > extable. The main motivation for the extra pass is to generate more > compact code, so there is a good chance that offsets are going to change > (especially with bpf subprogs). > >> >> - Hari >> >>>> >>>> Fixes: 85e031154c7c ("powerpc/bpf: Perform complete extra passes to >>>> update addresses") >>>> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> >>>> --- >>>> arch/powerpc/net/bpf_jit_comp32.c | 2 +- >>>> arch/powerpc/net/bpf_jit_comp64.c | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/powerpc/net/bpf_jit_comp32.c >>>> b/arch/powerpc/net/bpf_jit_comp32.c >>>> index 7f91ea064c08..e788b1fbeee6 100644 >>>> --- a/arch/powerpc/net/bpf_jit_comp32.c >>>> +++ b/arch/powerpc/net/bpf_jit_comp32.c >>>> @@ -977,7 +977,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 >>>> *image, struct codegen_context * >>>> if (size != BPF_DW && !fp->aux->verifier_zext) >>>> EMIT(PPC_RAW_LI(dst_reg_h, 0)); >>>> - if (BPF_MODE(code) == BPF_PROBE_MEM) { >>>> + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) { > > It is probably better to pass 'extra_pass' into bpf_add_extable_entry() > to keep all those checks together. > Thanks for the review and also the suggestion (offline) to reset index during extra pass, for my concern about possible regression. Posted v2. - Hari
diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c index 7f91ea064c08..e788b1fbeee6 100644 --- a/arch/powerpc/net/bpf_jit_comp32.c +++ b/arch/powerpc/net/bpf_jit_comp32.c @@ -977,7 +977,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * if (size != BPF_DW && !fp->aux->verifier_zext) EMIT(PPC_RAW_LI(dst_reg_h, 0)); - if (BPF_MODE(code) == BPF_PROBE_MEM) { + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) { int insn_idx = ctx->idx - 1; int jmp_off = 4; diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c index 8dd3cabaa83a..1cc2777ec846 100644 --- a/arch/powerpc/net/bpf_jit_comp64.c +++ b/arch/powerpc/net/bpf_jit_comp64.c @@ -921,7 +921,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * if (size != BPF_DW && insn_is_zext(&insn[i + 1])) addrs[++i] = ctx->idx * 4; - if (BPF_MODE(code) == BPF_PROBE_MEM) { + if (BPF_MODE(code) == BPF_PROBE_MEM && !extra_pass) { ret = bpf_add_extable_entry(fp, image, pass, ctx, ctx->idx - 1, 4, dst_reg); if (ret)
Since commit 85e031154c7c ("powerpc/bpf: Perform complete extra passes to update addresses"), two additional passes are performed to avoid space and CPU time wastage on powerpc. But these extra passes led to WARN_ON_ONCE() hits in bpf_add_extable_entry(). Fix it by not adding extable entries during the extra pass. Fixes: 85e031154c7c ("powerpc/bpf: Perform complete extra passes to update addresses") Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> --- arch/powerpc/net/bpf_jit_comp32.c | 2 +- arch/powerpc/net/bpf_jit_comp64.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)