From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933376AbcI3Rkc (ORCPT ); Fri, 30 Sep 2016 13:40:32 -0400 Received: from mout.kundenserver.de ([212.227.126.130]:62494 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932430AbcI3RkY (ORCPT ); Fri, 30 Sep 2016 13:40:24 -0400 From: Arnd Bergmann To: Eric Dumazet Subject: Re: [PATCH 3/3] netfilter: xt_hashlimit: uses div_u64 for division Date: Fri, 30 Sep 2016 19:39:54 +0200 User-Agent: KMail/1.12.2 (Linux/4.7.0-rc7+; KDE/4.3.2; x86_64; ; ) Cc: Pablo Neira Ayuso , Patrick McHardy , Jozsef Kadlecsik , "David S. Miller" , Joshua Hunt , Vishwanath Pai , netfilter-devel@vger.kernel.org, coreteam@netfilter.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org References: <20160930160559.4102745-1-arnd@arndb.de> <20160930160559.4102745-3-arnd@arndb.de> <1475253522.28155.195.camel@edumazet-glaptop3.roam.corp.google.com> In-Reply-To: <1475253522.28155.195.camel@edumazet-glaptop3.roam.corp.google.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201609301939.55089.arnd@arndb.de> X-Provags-ID: V03:K0:A1xyQl0bTWYICicCQAmm21YrZtqq4z4zMqU5//LAGeEXv85ZuXx nrvTBsX5r+CvmjSMk8SPtkOeMDexSuYREJuIv1jwAyvf9ic9tm19/ijBN3TtBy7Y51VkMv6 +48g/gcCjZpkIsmQd/r+L3yA43PYKjeFBr+N3Rv43edmpASvo5Lk7tY2kCHYf9OjGGNhDZQ 1ZOl5CBap0c6V2epDewUA== X-UI-Out-Filterresults: notjunk:1;V01:K0:wFzkTf96q4o=:mCcZjXg3V1LBAsZS4FJqCY X38/NSQ1OPYEPJrfX48AQUgcqz0D9SaGu5lhEZ/LSrWN4o9NI0JQ7Fw3/HEBOddKfGR0UFYFv wZfhsUaHSoSbydaOW4KbDnXwUhox4euLQMw2PHRKndXRHY+6RPloy+TjPdgI9mSXd1dmRfjkQ 8XHWuoxl7nL5xVWUwPCLI4FTMxwGqCe/T8lh+d7tn/d+ozskbrcBTfoZt8bOu4RMMbulgUuJU 6He6pZ1Rw9PjrzQ0CX2q5VMB+FIbXYqhYsJpSn7H1+VrWwOB4SijQJawsTgU/+xrVr0S9C1oc XKo6569ZiNO+JG7OXofWSrPqMXbkfXca/oDnDHDwCOj19gL3LVm26Bl87P2Z9OZa4cGBdMLHa qrHCu4+34FzjRHSULcVjm+8ljiWNNz7fIUirzmMmPqNesy7JVD+6K/eHt9fbF82g88QCdfC1V C95SMXBXKZJCUgDlqiEEdoj1aVfuYeX01Y1PgKgI7jix7APWk9IKRM/6byN8EIG/COKgq7Gl7 lK3bL4pKSxLgMZ94wPtMzlTaKj8epCeLko9QpayFDwd5PQijgdYXvRPo64PAdigCmYmsiY3l7 p9tplC0ly/Hfu3gITPzepGyO30z//nalz4YbMuzCop1Km+FGCOeymbe8nGtIIfJMT66ixj+tA LvtzBMXxcOsgXmhyOoYY5xm7FF8qDxO/GkNWEqqAgcTDzyqPBKWBHjI/m5KGn4KJwzJPFN0EZ 7Uz3klpWj7jK0yXZ Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Friday 30 September 2016, Eric Dumazet wrote: > On Fri, 2016-09-30 at 18:05 +0200, Arnd Bergmann wrote: > > net/netfilter/xt_hashlimit.c | 17 ++++++++++------- > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c > > index 44a095ecc7b7..3d5525df6eb3 100644 > > --- a/net/netfilter/xt_hashlimit.c > > +++ b/net/netfilter/xt_hashlimit.c > > @@ -464,20 +464,23 @@ static u32 xt_hashlimit_len_to_chunks(u32 len) > > static u64 user2credits(u64 user, int revision) > > { > > if (revision == 1) { > > + u32 user32 = user; /* use 32-bit division */ > > + > > This looks dangerous to me. Have you really tried all possible cases ? Yes, I'm pretty certain about that: The 11d5f15723c9 patch that introduced this kept the existing implementation for the revision==1 case, except for changing the types. > Caller (even if using revision == 1) does > user2credits(cfg->avg * cfg->burst, revision); > > Since this is not a fast path, I would prefer to keep the 64bit divide. > > Vishwanath version looks safer. Ok, fair enough. I couldn't tell how much of a fast path this was, and it's more a general issue that I see with other developers blindly using div_u64() whenever getting this link error. Since I already had the patch by the time I saw the other one (which is also at v3 and got comments), I just sent it out along with the other two patches I had for netfilter. I also ended up introducing a typo in a last-minute change, so I'll let Vishwanath and you work out the best implementation and withdraw my version. Arnd