| Message ID | 20230912224654.6556-6-puranjay12@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: verifier: stop emitting zext for LDX | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | pending | PR summary |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for aarch64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | pending | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-0 | success | Logs for ShellCheck |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 9 this patch: 9 |
| netdev/cc_maintainers | success | CCed 19 of 19 maintainers |
| netdev/build_clang | success | Errors and warnings before: 9 this patch: 9 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 9 this patch: 9 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 23 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Sep 12, 2023 at 10:46:53PM +0000, Puranjay Mohan wrote: > The JITs should not depend on the verifier for zero extending the upper > 32 bits of the destination register when loading a byte, half-word, or > word. > > A following patch will make the verifier stop patching zext instructions > after LDX. This was introduced by: 163541e6ba34 ("arm: bpf: eliminate zero extension code-gen") along with an additional function. So three points: 1) the commit should probably explain why it has now become undesirable to access this verifier state, whereas it appears it was explicitly added to permit this optimisation. 2) you state that jits should not depend on this state, but the above commit adds more references than you're removing, so aren't there still references to the verifier remaining after this patch? I count a total of 10, and the patch below removes three. 3) what about the bpf_jit_needs_zext() function that was added to support the export of this zext state? Essentially, the logic stated in the commit message doesn't seem to be reflected by the proposed code change.
On Wed, Sep 13, 2023 at 1:04 AM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Tue, Sep 12, 2023 at 10:46:53PM +0000, Puranjay Mohan wrote: > > The JITs should not depend on the verifier for zero extending the upper > > 32 bits of the destination register when loading a byte, half-word, or > > word. > > > > A following patch will make the verifier stop patching zext instructions > > after LDX. > > This was introduced by: > > 163541e6ba34 ("arm: bpf: eliminate zero extension code-gen") > > along with an additional function. So three points: > > 1) the commit should probably explain why it has now become undesirable > to access this verifier state, whereas it appears it was explicitly > added to permit this optimisation. I added some details in the cover letter. For the complete discussion see: [1] > 2) you state that jits should not depend on this state, but the above > commit adds more references than you're removing, so aren't there still > references to the verifier remaining after this patch? I count a total > of 10, and the patch below removes three. The JITs should not depend on this state for LDX (loading a B/H/W. This patch removes the usage only for LDX. > 3) what about the bpf_jit_needs_zext() function that was added to > support the export of this zext state? That is still applicable, The verifier will still emit zext instructions for other instructions like BPF_ALU / BPF_ALU64 > > Essentially, the logic stated in the commit message doesn't seem to be > reflected by the proposed code change. I will try to provide more information. Currently I have asked Alexei if we really need this in [2]. I still think this optimization is useful and we should keep it. Thanks, Puranjay [1] https://lore.kernel.org/all/CANk7y0j2f-gPgZwd+YfTL71-6wfvky+f=kBC_ccqsS0EHAysyA@mail.gmail.com/ [2] https://lore.kernel.org/bpf/CANk7y0hK9sQJ-kRx3nQpVJSxpP=NzzFaLitOYq8=Pb6Dvk9fpg@mail.gmail.com/
On Tue, Sep 12, 2023 at 4:17 PM Puranjay Mohan <puranjay12@gmail.com> wrote: > > On Wed, Sep 13, 2023 at 1:04 AM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > > > On Tue, Sep 12, 2023 at 10:46:53PM +0000, Puranjay Mohan wrote: > > > The JITs should not depend on the verifier for zero extending the upper > > > 32 bits of the destination register when loading a byte, half-word, or > > > word. > > > > > > A following patch will make the verifier stop patching zext instructions > > > after LDX. > > > > This was introduced by: > > > > 163541e6ba34 ("arm: bpf: eliminate zero extension code-gen") > > > > along with an additional function. So three points: > > > > 1) the commit should probably explain why it has now become undesirable > > to access this verifier state, whereas it appears it was explicitly > > added to permit this optimisation. > > I added some details in the cover letter. > > For the complete discussion see: [1] > > > 2) you state that jits should not depend on this state, but the above > > commit adds more references than you're removing, so aren't there still > > references to the verifier remaining after this patch? I count a total > > of 10, and the patch below removes three. > > The JITs should not depend on this state for LDX (loading > a B/H/W. > This patch removes the usage only for LDX. > > > 3) what about the bpf_jit_needs_zext() function that was added to > > support the export of this zext state? > > That is still applicable, The verifier will still emit zext > instructions for other > instructions like BPF_ALU / BPF_ALU64 > > > > > Essentially, the logic stated in the commit message doesn't seem to be > > reflected by the proposed code change. > > I will try to provide more information. > Currently I have asked Alexei if we really need this in [2]. > I still think this optimization is useful and we should keep it. Right. subreg tracking is indeed functional for narrow loads. Let's drop this patch set.
diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c index 6a1c9fca5260..757a99febba5 100644 --- a/arch/arm/net/bpf_jit_32.c +++ b/arch/arm/net/bpf_jit_32.c @@ -1081,20 +1081,17 @@ static inline void emit_ldx_r(const s8 dst[], const s8 src, case BPF_B: /* Load a Byte */ emit(ARM_LDRB_I(rd[1], rm, off), ctx); - if (!ctx->prog->aux->verifier_zext) - emit_a32_mov_i(rd[0], 0, ctx); + emit_a32_mov_i(rd[0], 0, ctx); break; case BPF_H: /* Load a HalfWord */ emit(ARM_LDRH_I(rd[1], rm, off), ctx); - if (!ctx->prog->aux->verifier_zext) - emit_a32_mov_i(rd[0], 0, ctx); + emit_a32_mov_i(rd[0], 0, ctx); break; case BPF_W: /* Load a Word */ emit(ARM_LDR_I(rd[1], rm, off), ctx); - if (!ctx->prog->aux->verifier_zext) - emit_a32_mov_i(rd[0], 0, ctx); + emit_a32_mov_i(rd[0], 0, ctx); break; case BPF_DW: /* Load a Double Word */
The JITs should not depend on the verifier for zero extending the upper 32 bits of the destination register when loading a byte, half-word, or word. A following patch will make the verifier stop patching zext instructions after LDX. Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> --- arch/arm/net/bpf_jit_32.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)