Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752037AbdF3QoQ (ORCPT ); Fri, 30 Jun 2017 12:44:16 -0400 Received: from dispatch1-us1.ppe-hosted.com ([67.231.154.164]:42246 "EHLO dispatch1-us1.ppe-hosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751203AbdF3QoO (ORCPT ); Fri, 30 Jun 2017 12:44:14 -0400 Subject: Re: [PATCH v3 net-next 00/12] bpf: rewrite value tracking in verifier To: Alexei Starovoitov , Daniel Borkmann References: <5953B436.6030506@iogearbox.net> <788035e1-1974-b48e-3008-d294194a8b05@solarflare.com> <595413AA.40502@iogearbox.net> <20170628213701.32krfuipzngsmt4k@ast-mbp> CC: , Alexei Starovoitov , , , iovisor-dev From: Edward Cree Message-ID: <91267d15-652a-16d9-4ee9-42958bd842aa@solarflare.com> Date: Fri, 30 Jun 2017 17:44:03 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <20170628213701.32krfuipzngsmt4k@ast-mbp> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.17.20.45] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-11.0.0.1191-8.100.1062-23166.003 X-TM-AS-Result: No--12.982300-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-MDID: 1498841053-1WzLW7mkpzhw Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2571 Lines: 42 On 28/06/17 22:37, Alexei Starovoitov wrote: > Increasing the limit is must have, since pruning suffered so much. > Going from 53k to 76k is pretty substantial. > What is the % increase for tests in selftests/ ? When I tried to measure the test_verifier tests, they changed hardly at all, only a couple of percent iirc. But that's with (a) only the accepted progs get measured, since rejected don't print the #insns line - and most of the tests in test_verifier are rejected; and (b) those test progs are pretty small, usually with only a couple of jumps and not much chance for pruning to occur. So it's really not a great test case for pruning effectiveness. I haven't measured the test_progs ones, because I *still* haven't gotten around to actually setting up a BPF toolchain (it doesn't help that I'm building everything on a test server that gets reimaged every night to run our nightly tests...). > I think we need to pin point exactly the reason. > Saying we just track more data is not enough. > We've tried v2 set on our load balancer and also saw ~20% increase. > I don't remember the absolute numbers. > These jumps don't make me comfortable with these extra tracking. > Can you try to roll back ptr&const and full negative/positive tracking > and see whether it gets back to what we had before? The ptr&const bit shouldn't be relevant unless your programs are actually doing that (i.e. ops on pointers other than +/-), which seems surprising. But if you really are, then it's not too hard to roll it back - just need to change how adjust_reg_min_max_vals() deals with EACCES. For a version without full negative/positive tracking, just take the first 3 patches; some of the selftests will fail but hopefully your progs will still be accepted. If not, we can try jbacik's patch (off-list response to v2). I will followup this email with a patch to apply on top of the first 3 that does that and rolls back ptr&const. > If tnum is causing it that would be reasonable trade off to make, > but if it's full neg/pos tracking that has no use today other than > (the whole thing is cleaner) I would rather drop it then. Well, the full neg/pos tracking was a result of needing to fix the bug I found with patch #1, and not wanting to confuse myself with the min/max range while doing my signed/unsigned tracking. But if we can make it work with jbacik's approach of 'remember whether our current bounds came from a signed or an unsigned compare', then we can drop or delay the full neg/pos tracking unless/until pruning is sorted out. -Ed