From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.9 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F41BAC3F2C6 for ; Wed, 4 Mar 2020 02:32:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C627E20842 for ; Wed, 4 Mar 2020 02:32:37 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=cs.washington.edu header.i=@cs.washington.edu header.b="M5O7QedO" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2387538AbgCDCcg (ORCPT ); Tue, 3 Mar 2020 21:32:36 -0500 Received: from mail-il1-f193.google.com ([209.85.166.193]:34179 "EHLO mail-il1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2387411AbgCDCcg (ORCPT ); Tue, 3 Mar 2020 21:32:36 -0500 Received: by mail-il1-f193.google.com with SMTP id n11so527884ild.1 for ; Tue, 03 Mar 2020 18:32:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cs.washington.edu; s=goo201206; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=n6mkcIQ2CwC1xTtJxH74NN6vQdrLEAX1li3E8fEudI8=; b=M5O7QedO8LmdTilKXhgDjj6pJDYijzuilBu5xyes4ICn+hClPJbwMCqeC/K5g1bE2g egpjdAJsAJmqcxtp4gn85lwUEg+JAOGc94aItnA3aqKydSvbr6rdf4OOqrfi7+KA5TI3 F0k1koSGJkFlWVrglQhyYi1JSodyDP31qeDsw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=n6mkcIQ2CwC1xTtJxH74NN6vQdrLEAX1li3E8fEudI8=; b=DuvrDCrI3hU5NgTs6JjLqoT7zOrbA7EjYTN2z8YhMJRhBOQleeOhuVE2xh1iuHxvfI psFy/iUrczNAEqCEPzj/DnJENVTZsD3+HWwsE+PTgoKxZ3uXYbBeWW269vDc0+drNjYL GdAVJQGYkzPVNV6zlddDgL2GrtsjJSmgXWmU2vyUjnJHwKBKgXTxikGKZbNwOCfJ8KlP j7PGp4VdAIeFYftP7mTa2oVX+TBqqA627o4CkKjFsOfNjJsS51LA1N888HzMhb27mOv7 Jv6CW9db8hGxfZ4D1QZjIKVXEKBiSAiBx8m0ZrukkSSpgmvAS8lcohVHgUr/gQd7SURA JxpA== X-Gm-Message-State: ANhLgQ1ujPt1APWco6YAfTFyOo1+WvHzPvn6tPAzWp7r+PwOo5quZCEe bv/7Yg3LAPZyY97Vbi3KsNQAQV7Dlk4hdFLF2cZNtw== X-Google-Smtp-Source: ADFU+vtZz9bK0eR23Fqfg2Flo7dTRJq/q3oOn/yBVOdWo5FSor2dKOCN3ztfEmpM0Yd68erGscs4Sdi+qrx0Fsa208s= X-Received: by 2002:a05:6e02:4c2:: with SMTP id f2mr687631ils.126.1583289154825; Tue, 03 Mar 2020 18:32:34 -0800 (PST) MIME-Version: 1.0 References: <20200303005035.13814-1-luke.r.nels@gmail.com> <20200303005035.13814-3-luke.r.nels@gmail.com> In-Reply-To: From: Luke Nelson Date: Tue, 3 Mar 2020 18:32:24 -0800 Message-ID: Subject: Re: [PATCH bpf-next v4 2/4] riscv, bpf: add RV32G eBPF JIT To: =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= Cc: bpf , Luke Nelson , Jonathan Corbet , Alexei Starovoitov , Daniel Borkmann , Martin KaFai Lau , Song Liu , Yonghong Song , Andrii Nakryiko , "David S. Miller" , Jakub Kicinski , Paul Walmsley , Palmer Dabbelt , Albert Ou , Xi Wang , Mauro Carvalho Chehab , Stephen Hemminger , Rob Herring , Greg Kroah-Hartman , Jonathan Cameron , Andy Shevchenko , linux-doc@vger.kernel.org, LKML , Netdev , linux-riscv@lists.infradead.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Bj=C3=B6rn, Thanks again for the comments, responses below: On Mon, Mar 2, 2020 at 11:48 PM Bj=C3=B6rn T=C3=B6pel wrote: > > Which are the tests that fail to JIT, and is that due to div/mod + > xadd? Yes, all of the cases that fail to JIT are because of unsupported div/mod or xadd. I'll make that clear in next revision. > > > Co-developed-by: Xi Wang > > Signed-off-by: Xi Wang > > Signed-off-by: Luke Nelson > > --- > > arch/riscv/Kconfig | 2 +- > > arch/riscv/net/Makefile | 7 +- > > arch/riscv/net/bpf_jit_comp32.c | 1466 +++++++++++++++++++++++++++++++ > > 3 files changed, 1473 insertions(+), 2 deletions(-) > > create mode 100644 arch/riscv/net/bpf_jit_comp32.c > [...] > > + > > +static const s8 *rv32_bpf_get_reg64(const s8 *reg, const s8 *tmp, > > + struct rv_jit_context *ctx) > > Really a nit, but you're using rv32 as prefix, and also as part of > many of the functions (e.g. emit_rv32). Everything is this file is > just for RV32, so maybe remove that implicit information from the > function name? Just a thought! :-) I got so used to reading these I never noticed how redundant they were :) I'll change the next revision to remove all of the "rv32"s in function names. > > + case BPF_LSH: > > + if (imm >=3D 32) { > > + emit(rv_slli(hi(rd), lo(rd), imm - 32), ctx); > > + emit(rv_addi(lo(rd), RV_REG_ZERO, 0), ctx); > > + } else if (imm =3D=3D 0) { > > + /* nop */ > > Can we get rid of this, and just do if/else if? imm =3D=3D 0 has been a tricky case for 32-bit JITs; see 6fa632e719ee ("bpf, x32: Fix bug with ALU64 {LSH, RSH, ARSH} BPF_K shift by 0"). We wanted to make the imm =3D=3D 0 case explicit and help future readers see that this case is handled correctly here. We could do the following if we really wanted to get rid of the check: if (imm >=3D 32) { ... } else if (imm !=3D 0) { ... } /* Do nothing for imm =3D=3D 0. */ Though it's unclear if this is easier to read. > > + case BPF_ARSH: > > + if (is_12b_int(imm)) { > > + emit(rv_srai(lo(rd), lo(rd), imm), ctx); > > + } else { > > + emit_imm(RV_REG_T0, imm, ctx); > > + emit(rv_sra(lo(rd), lo(rd), RV_REG_T0), ctx); > > + } > > + break; > > Again nit; I like "early exit" code if possible. Instead of: > > if (bleh) { > foo(); > } else { > bar(); > } > > do: > > if (bleh) { > foo() > return/break; > } > bar(); > > I find the latter easier to read -- but really a nit, and a matter of > style. There are number of places where that could be applied in the > file. I like "early exit" code, too, and agree that it's easier to read in general, especially when handling error conditions. But here we wanted to make it explicit that both branches are emitting equivalent instruction sequences (under different paths). Structured control flow seems a better fit for this particular context. > At this point of the series, let's introduce the shared code .c-file > containing implementation for bpf_int_jit_compile() (with build_body > part of that)and bpf_jit_needs_zext(). That will make it easier to > catch bugs in both JITs and to avoid code duplication! Also, when > adding the stronger invariant suggested by Palmer [1], we only need to > do it in one place. > > The pull out refactoring can be a separate commit. I think the idea of deduplicating bpf_int_jit_compile is good and will lead to more maintainable JITs. How does the following proposal for v5 sound? In patch 1 of this series: - Factor structs and common helpers to bpf_jit.h (just like v4). - Factor out bpf_int_jit_compile(), bpf_jit_needs_zext(), and build_body() to a new file bpf_jit_core.c and tweak the code as in v4. - Rename emit_insn() and build_{prologue,epilogue}() to bpf_jit_emit_insn() and bpf_jit_build_{prologue,epilogue}, since these functions are now extern rather than static. - Rename bpf_jit_comp.c to bpf_jit_comp64.c to be more explicit about its contents (as the next patch will add bpf_jit_comp32.c). Then patch 2 can reuse the new header and won't need to define its own bpf_int_jit_compile() etc. Thanks! Luke From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6A85AC3F2C6 for ; Wed, 4 Mar 2020 02:32:44 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3BB9E20842 for ; Wed, 4 Mar 2020 02:32:44 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="EFggucdU"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=cs.washington.edu header.i=@cs.washington.edu header.b="M5O7QedO" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3BB9E20842 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cs.washington.edu Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-riscv-bounces+infradead-linux-riscv=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Cc:List-Subscribe: List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: Content-Transfer-Encoding:Content-Type:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=n6mkcIQ2CwC1xTtJxH74NN6vQdrLEAX1li3E8fEudI8=; b=EFggucdUruh6jn I7IXingbUO0QOIBkHUxz0Keiase5PenpbtAGE5dKGwBJ7ldTgbujAK6+9jiLRAJn8xg4XArUsWMyc bwk0IfSHgPAHEq89ju1VFVZEs2UXSNQ0oUQzgb+sK8Vxj8TqHLwk5DHGLk8IFTpykGdRsDM7QaCYz iCO8qqFQsgKMqnw87CrY2b0X64AnfFwRFS8fAyjvu/I+yrTWuMro5Q4eRpdF3LjMQLVqBzEJfzYNm 6z5yRJeNS1MLFPmwlDOYV/GyBq584vacq25VMOtxbXUS0vY6hEJjsMwCnFT10bm7sVGk6jamhGx5o M5rquA62ZaZ6W1c7kczQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1j9Jpv-0001PW-GE; Wed, 04 Mar 2020 02:32:39 +0000 Received: from mail-il1-x142.google.com ([2607:f8b0:4864:20::142]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1j9Jpr-0001Ov-LU for linux-riscv@lists.infradead.org; Wed, 04 Mar 2020 02:32:37 +0000 Received: by mail-il1-x142.google.com with SMTP id a6so516843ilc.4 for ; Tue, 03 Mar 2020 18:32:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cs.washington.edu; s=goo201206; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=n6mkcIQ2CwC1xTtJxH74NN6vQdrLEAX1li3E8fEudI8=; b=M5O7QedO8LmdTilKXhgDjj6pJDYijzuilBu5xyes4ICn+hClPJbwMCqeC/K5g1bE2g egpjdAJsAJmqcxtp4gn85lwUEg+JAOGc94aItnA3aqKydSvbr6rdf4OOqrfi7+KA5TI3 F0k1koSGJkFlWVrglQhyYi1JSodyDP31qeDsw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=n6mkcIQ2CwC1xTtJxH74NN6vQdrLEAX1li3E8fEudI8=; b=bUyc9M8A/cwAhb+MiwvGr3yaFR0XgE3fV01QxSIvDsrBYT9cdFpl1Eg5oQBXf7NMEQ 8RGEq6v3ydlZLtXOY1z4WgctZxJ36guQ63s2yDDJyosQ2ZiPggWtsTqaOVQcqFba0aee UbD3sI75osflht4P9ApSq0CJ5DBGu6qQHvWSn42P5raMqhkWAbok3y3HHe8YZAc4oBxg /NYqDOkuUMNBrzi5EVckbaA62yM4JiefNzpp4SyzrONZUXJO4UWLPdDmFGWlobYzXRdB YEu+JmpOZhS9o/qGVlnPZEmRwl/WSD7IvQaukUlIVDkpazdaDMGK0L4B1xAGxcOz7PSu q3ww== X-Gm-Message-State: ANhLgQ188anP3t1ApDFaUkwOmNlwz7QVldqhIUiRSAfoRFbv85XoZxGP bM54w9hnkB0yzbIxaE3WRlbZ62PQ37srbv8/u7R89A== X-Google-Smtp-Source: ADFU+vtZz9bK0eR23Fqfg2Flo7dTRJq/q3oOn/yBVOdWo5FSor2dKOCN3ztfEmpM0Yd68erGscs4Sdi+qrx0Fsa208s= X-Received: by 2002:a05:6e02:4c2:: with SMTP id f2mr687631ils.126.1583289154825; Tue, 03 Mar 2020 18:32:34 -0800 (PST) MIME-Version: 1.0 References: <20200303005035.13814-1-luke.r.nels@gmail.com> <20200303005035.13814-3-luke.r.nels@gmail.com> In-Reply-To: From: Luke Nelson Date: Tue, 3 Mar 2020 18:32:24 -0800 Message-ID: Subject: Re: [PATCH bpf-next v4 2/4] riscv, bpf: add RV32G eBPF JIT To: =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200303_183235_708124_9567AF45 X-CRM114-Status: GOOD ( 23.61 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Song Liu , linux-doc@vger.kernel.org, Yonghong Song , Paul Walmsley , Alexei Starovoitov , Mauro Carvalho Chehab , linux-riscv@lists.infradead.org, Rob Herring , Daniel Borkmann , Jonathan Corbet , Jakub Kicinski , Andrii Nakryiko , Xi Wang , Albert Ou , Luke Nelson , Jonathan Cameron , Andy Shevchenko , Greg Kroah-Hartman , LKML , Martin KaFai Lau , Stephen Hemminger , Palmer Dabbelt , Netdev , bpf , "David S. Miller" Sender: "linux-riscv" Errors-To: linux-riscv-bounces+infradead-linux-riscv=archiver.kernel.org@lists.infradead.org Hi Bj=C3=B6rn, Thanks again for the comments, responses below: On Mon, Mar 2, 2020 at 11:48 PM Bj=C3=B6rn T=C3=B6pel wrote: > > Which are the tests that fail to JIT, and is that due to div/mod + > xadd? Yes, all of the cases that fail to JIT are because of unsupported div/mod or xadd. I'll make that clear in next revision. > > > Co-developed-by: Xi Wang > > Signed-off-by: Xi Wang > > Signed-off-by: Luke Nelson > > --- > > arch/riscv/Kconfig | 2 +- > > arch/riscv/net/Makefile | 7 +- > > arch/riscv/net/bpf_jit_comp32.c | 1466 +++++++++++++++++++++++++++++++ > > 3 files changed, 1473 insertions(+), 2 deletions(-) > > create mode 100644 arch/riscv/net/bpf_jit_comp32.c > [...] > > + > > +static const s8 *rv32_bpf_get_reg64(const s8 *reg, const s8 *tmp, > > + struct rv_jit_context *ctx) > > Really a nit, but you're using rv32 as prefix, and also as part of > many of the functions (e.g. emit_rv32). Everything is this file is > just for RV32, so maybe remove that implicit information from the > function name? Just a thought! :-) I got so used to reading these I never noticed how redundant they were :) I'll change the next revision to remove all of the "rv32"s in function names. > > + case BPF_LSH: > > + if (imm >=3D 32) { > > + emit(rv_slli(hi(rd), lo(rd), imm - 32), ctx); > > + emit(rv_addi(lo(rd), RV_REG_ZERO, 0), ctx); > > + } else if (imm =3D=3D 0) { > > + /* nop */ > > Can we get rid of this, and just do if/else if? imm =3D=3D 0 has been a tricky case for 32-bit JITs; see 6fa632e719ee ("bpf, x32: Fix bug with ALU64 {LSH, RSH, ARSH} BPF_K shift by 0"). We wanted to make the imm =3D=3D 0 case explicit and help future readers see that this case is handled correctly here. We could do the following if we really wanted to get rid of the check: if (imm >=3D 32) { ... } else if (imm !=3D 0) { ... } /* Do nothing for imm =3D=3D 0. */ Though it's unclear if this is easier to read. > > + case BPF_ARSH: > > + if (is_12b_int(imm)) { > > + emit(rv_srai(lo(rd), lo(rd), imm), ctx); > > + } else { > > + emit_imm(RV_REG_T0, imm, ctx); > > + emit(rv_sra(lo(rd), lo(rd), RV_REG_T0), ctx); > > + } > > + break; > > Again nit; I like "early exit" code if possible. Instead of: > > if (bleh) { > foo(); > } else { > bar(); > } > > do: > > if (bleh) { > foo() > return/break; > } > bar(); > > I find the latter easier to read -- but really a nit, and a matter of > style. There are number of places where that could be applied in the > file. I like "early exit" code, too, and agree that it's easier to read in general, especially when handling error conditions. But here we wanted to make it explicit that both branches are emitting equivalent instruction sequences (under different paths). Structured control flow seems a better fit for this particular context. > At this point of the series, let's introduce the shared code .c-file > containing implementation for bpf_int_jit_compile() (with build_body > part of that)and bpf_jit_needs_zext(). That will make it easier to > catch bugs in both JITs and to avoid code duplication! Also, when > adding the stronger invariant suggested by Palmer [1], we only need to > do it in one place. > > The pull out refactoring can be a separate commit. I think the idea of deduplicating bpf_int_jit_compile is good and will lead to more maintainable JITs. How does the following proposal for v5 sound? In patch 1 of this series: - Factor structs and common helpers to bpf_jit.h (just like v4). - Factor out bpf_int_jit_compile(), bpf_jit_needs_zext(), and build_body() to a new file bpf_jit_core.c and tweak the code as in v4. - Rename emit_insn() and build_{prologue,epilogue}() to bpf_jit_emit_insn() and bpf_jit_build_{prologue,epilogue}, since these functions are now extern rather than static. - Rename bpf_jit_comp.c to bpf_jit_comp64.c to be more explicit about its contents (as the next patch will add bpf_jit_comp32.c). Then patch 2 can reuse the new header and won't need to define its own bpf_int_jit_compile() etc. Thanks! Luke