From mboxrd@z Thu Jan 1 00:00:00 1970 From: Martin Schwidefsky Subject: Re: [PATCH net] bpf: do not use reciprocal divide Date: Wed, 15 Jan 2014 16:35:04 +0100 Message-ID: <20140115163504.23efebb7@mschwide> References: <20140113214249.GK6586@order.stressinduktion.org> <1389769361.31367.325.camel@edumazet-glaptop2.roam.corp.google.com> <20140115080007.GA6638@osiris> <20140115091322.3a7740a7@mschwide> <20140115105114.GB6638@osiris> <1389795716.31367.333.camel@edumazet-glaptop2.roam.corp.google.com> <1389795926.31367.334.camel@edumazet-glaptop2.roam.corp.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: Heiko Carstens , Hannes Frederic Sowa , netdev@vger.kernel.org, dborkman@redhat.com, darkjames-ws@darkjames.pl, Mircea Gherzan , Russell King , Matt Evans To: Eric Dumazet Return-path: Received: from e06smtp17.uk.ibm.com ([195.75.94.113]:34391 "EHLO e06smtp17.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752272AbaAOPfL (ORCPT ); Wed, 15 Jan 2014 10:35:11 -0500 Received: from /spool/local by e06smtp17.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 15 Jan 2014 15:35:10 -0000 Received: from b06cxnps4076.portsmouth.uk.ibm.com (d06relay13.portsmouth.uk.ibm.com [9.149.109.198]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 2DF332190056 for ; Wed, 15 Jan 2014 15:35:06 +0000 (GMT) Received: from d06av09.portsmouth.uk.ibm.com (d06av09.portsmouth.uk.ibm.com [9.149.37.250]) by b06cxnps4076.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s0FFYt3S49086586 for ; Wed, 15 Jan 2014 15:34:55 GMT Received: from d06av09.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av09.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s0FFZ65N010312 for ; Wed, 15 Jan 2014 08:35:07 -0700 In-Reply-To: <1389795926.31367.334.camel@edumazet-glaptop2.roam.corp.google.com> Sender: netdev-owner@vger.kernel.org List-ID: On Wed, 15 Jan 2014 06:25:26 -0800 Eric Dumazet wrote: > On Wed, 2014-01-15 at 06:21 -0800, Eric Dumazet wrote: > > On Wed, 2014-01-15 at 11:51 +0100, Heiko Carstens wrote: > > > On Wed, Jan 15, 2014 at 09:13:22AM +0100, Martin Schwidefsky wrote: > > > > On Wed, 15 Jan 2014 09:00:07 +0100 > > > > Heiko Carstens wrote: > > > > > > > > > On Tue, Jan 14, 2014 at 11:02:41PM -0800, Eric Dumazet wrote: > > > > > > diff --git a/arch/s390/net/bpf_jit_comp.c b/arch/s390/net/bpf_jit_comp.c > > > > > > index 16871da37371..e349dc7d0992 100644 > > > > > > --- a/arch/s390/net/bpf_jit_comp.c > > > > > > +++ b/arch/s390/net/bpf_jit_comp.c > > > > > > @@ -371,11 +371,11 @@ static int bpf_jit_insn(struct bpf_jit *jit, struct sock_filter *filter, > > > > > > /* dr %r4,%r12 */ > > > > > > EMIT2(0x1d4c); > > > > > > break; > > > > > > - case BPF_S_ALU_DIV_K: /* A = reciprocal_divide(A, K) */ > > > > > > - /* m %r4,(%r13) */ > > > > > > - EMIT4_DISP(0x5c40d000, EMIT_CONST(K)); > > > > > > - /* lr %r5,%r4 */ > > > > > > - EMIT2(0x1854); > > > > > > + case BPF_S_ALU_DIV_K: /* A /= K */ > > > > > > + /* lhi %r4,0 */ > > > > > > + EMIT4(0xa7480000); > > > > > > + /* d %r4,(%r13) */ > > > > > > + EMIT4_DISP(0x5d40d000, EMIT_CONST(K)); > > > > > > break; > > > > > > > > > > The s390 part looks good. > > > > > > > > Does it? The divide instruction is signed, for the special > > > > case of K==1 this can now cause an exception if the quotient > > > > gets too large. We should add a check for K==1 and do nothing > > > > in this case. With a divisor of at least 2 the result will > > > > stay in the limit. > > > > > > Indeed. That's quite subtle. > > > > net/core/filter.c does : > > > > A /= K; > > > > Why is this working in generic code (if K == 1), not in s390 one ? > > Note that I copied code found in BPF_S_ALU_MOD_K, so this one would need > a fix as well. Hmm, that is true BPF_S_ALU_DIV_X, BPF_S_ALU_MOD_K and BPF_S_ALU_MOD_X all suffer from the same problem. As the BPF jit is only used for 64-bit kernel the simplest solution is to replace the "dr" instruction with "dlr". -- blue skies, Martin. "Reality continues to ruin my life." - Calvin.