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=-4.0 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS,URIBL_BLOCKED autolearn=ham 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 40D52C282C0 for ; Wed, 23 Jan 2019 23:47:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 09EC1218A1 for ; Wed, 23 Jan 2019 23:47:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=netronome-com.20150623.gappssmtp.com header.i=@netronome-com.20150623.gappssmtp.com header.b="vDtHh837" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726366AbfAWXrP (ORCPT ); Wed, 23 Jan 2019 18:47:15 -0500 Received: from mail-qt1-f194.google.com ([209.85.160.194]:42499 "EHLO mail-qt1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726220AbfAWXrP (ORCPT ); Wed, 23 Jan 2019 18:47:15 -0500 Received: by mail-qt1-f194.google.com with SMTP id d19so4522694qtq.9 for ; Wed, 23 Jan 2019 15:47:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netronome-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :organization:mime-version:content-transfer-encoding; bh=ELIIn5aW5zym8qjVymQGr4AhcbRhpwQnhCklVDeB7AY=; b=vDtHh837ojzRVRMjvvmoAThm2PlM161vHDEJsKBxqydEI15M/aywG19yXgOo0EXTM3 +0ccS71x4Hmev5Q6a4WEOCRK5+8LtuP53tOgrMxjmiZFJk8VhsvmdwHNH32dAKw77MJR TSsxtY1aI8/SuXz2aK4eTWHkUbIdPLnxqv54nEcHjWVZ+Z5f0yGgTk7ZpVqYRCOvP0Oc y3tp0T1oa7thMn7WxGdKJNLpzCCBlyMbnzfzShYtB1IfHag9MBD+nIym0B3SIalks1R4 XpUcLrajo+KR0rOeo1gQ73OwZgNL4b6skmxTGrsHeYCU5JXuNuhL3azCaoPM65GWBZXZ wrhA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:organization:mime-version:content-transfer-encoding; bh=ELIIn5aW5zym8qjVymQGr4AhcbRhpwQnhCklVDeB7AY=; b=ikx//cgFghSt4wcEijAX3FMXL02WdGyAoPXHBM8tjPuTfNO+TAotrW9xV2s1AyHORC d3UcGf5Vm5cgWu90QDQYjC0bAIkgP26kqE53zCobKJVoM86VCvbootYkPqKA1oFS+d+k 3HFfDHh/Q6e9godDnTqN65aoIzQuPGPl8aa8kqR7WA2AnLgTgsXEG8GoNM+bEpd1o428 FrEDIyIS6Tkc/m7gJFz6P6QFBps359C7r0Y2jNifzrCvh7Ikh7P+tXi9e1d9G7Ovl4TI op2F5HheO/BWIJQwdQCEtIK5283l6yz+7VJ/fcdKijk+fmeNr8rubImixAJlcLoXGqQY Z1CQ== X-Gm-Message-State: AJcUukeXy+RIuYAKefFilKgaTXD4I2A/j69ueZO+WSExgQOviUq57gQQ XVxffSq7TDLSCpA4od58l95+5p0jmkg= X-Google-Smtp-Source: ALg8bN73mliBbFrUPLO2ENmLZa8bgbQZYb0mJDXoL0iiGyidOxBUPCaa9bUwLvL98BjtNgewUVUVAg== X-Received: by 2002:ac8:7950:: with SMTP id r16mr4613501qtt.12.1548287234200; Wed, 23 Jan 2019 15:47:14 -0800 (PST) Received: from cakuba.netronome.com ([66.60.152.14]) by smtp.gmail.com with ESMTPSA id o42sm81941520qtc.90.2019.01.23.15.47.13 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 23 Jan 2019 15:47:14 -0800 (PST) Date: Wed, 23 Jan 2019 15:47:09 -0800 From: Jakub Kicinski To: Alexei Starovoitov Cc: daniel@iogearbox.net, netdev@vger.kernel.org, oss-drivers@netronome.com Subject: Re: [PATCH bpf-next 2/3] selftests: bpf: break up test_verifier Message-ID: <20190123154709.6e649d79@cakuba.netronome.com> In-Reply-To: <20190123233203.jzxw2xkqwygraqui@ast-mbp.dhcp.thefacebook.com> References: <20190121184339.29485-1-jakub.kicinski@netronome.com> <20190121184339.29485-3-jakub.kicinski@netronome.com> <20190123233203.jzxw2xkqwygraqui@ast-mbp.dhcp.thefacebook.com> Organization: Netronome Systems, Ltd. MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, 23 Jan 2019 15:32:05 -0800, Alexei Starovoitov wrote: > On Mon, Jan 21, 2019 at 10:43:38AM -0800, Jakub Kicinski wrote: > > Break up the first 10 kLoC of test verifier test cases > > out into smaller files. Looks like git line counting > > gets a little flismy above 16 bit integers, so we need > > two commits to break up test_verifier. > > > > Signed-off-by: Jakub Kicinski > > Acked-by: Jiong Wang > > --- > > tools/testing/selftests/bpf/test_verifier.c | 10100 ---------------- > > tools/testing/selftests/bpf/verifier/and.c | 53 + > > .../selftests/bpf/verifier/array_access.c | 238 + > ... > > - { > > - "DIV64 by 0, zero check", > > - .insns = { > > - BPF_MOV32_IMM(BPF_REG_0, 42), > > - BPF_MOV32_IMM(BPF_REG_1, 0), > > - BPF_MOV32_IMM(BPF_REG_2, 1), > > - BPF_ALU64_REG(BPF_DIV, BPF_REG_2, BPF_REG_1), > > - BPF_EXIT_INSN(), > > - }, > > - .result = ACCEPT, > > - .retval = 42, > > - }, > ... > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/verifier/and.c > > @@ -0,0 +1,53 @@ > > +{ > > + "invalid and of negative number", > > + .insns = { > > + BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), > > + BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), > > + BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), > > + BPF_LD_MAP_FD(BPF_REG_1, 0), > > + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, > > + BPF_FUNC_map_lookup_elem), > > + BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4), > > + BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), > > + BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4), > > + BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2), > > + BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1), > > + BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, > > + offsetof(struct test_val, foo)), > > + BPF_EXIT_INSN(), > > + }, > > + .fixup_map_hash_48b = { 3 }, > > + .errstr = "R0 max value is outside of the array range", > > + .result = REJECT, > > + .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS, > > +}, > > I like the removal of one tab, but since we're refactoring the whole thing > can we remove another tab from instructions? > In many cases we wrap the lines which makes tests a bit harder to read/write > since jmp offsets don't easily add from current line number. > In addition I would propose to represet LD_MAP_FD as two lines too, > but that's optional. I'm thinking to try that for new tests. > So how about the following indent: > { > "invalid and of negative number", > .insns = { > BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), > BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), > BPF_LD_MAP_FD(BPF_REG_1, > 0), > BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem), > BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4), > BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), > BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4), > BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2), > BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1), > BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)), > BPF_EXIT_INSN(), > }, > .fixup_map_hash_48b = { 3 }, > .errstr = "R0 max value is outside of the array range", > .result = REJECT, > .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS, > }, > > Notice how calls and offsetof() fits into 80 char. > Another alternative is to switch to two spaces instead of tab: > { > "invalid and of negative number", > .insns = { > BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), > BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), > BPF_LD_MAP_FD(BPF_REG_1, > 0), > BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem), > BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 4), > BPF_LDX_MEM(BPF_B, BPF_REG_1, BPF_REG_0, 0), > BPF_ALU64_IMM(BPF_AND, BPF_REG_1, -4), > BPF_ALU64_IMM(BPF_LSH, BPF_REG_1, 2), > BPF_ALU64_REG(BPF_ADD, BPF_REG_0, BPF_REG_1), > BPF_ST_MEM(BPF_DW, BPF_REG_0, 0, offsetof(struct test_val, foo)), > BPF_EXIT_INSN(), > }, > .fixup_map_hash_48b = { 3 }, > .errstr = "R0 max value is outside of the array range", > .result = REJECT, > .flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS, > }, > > Thoughts? I'd vote for option (1). The two spaces don't seem to provide good enough visual separation, IMHO we'd either have to go to 4, or put the opening bracket on it's own line GNU style.