Date:	Thu, 16 Jan 2014 18:33:37 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Hannes Frederic Sowa <hannes@...essinduktion.org>
Cc:	netdev@...r.kernel.org, Austin S Hemmelgarn <ahferroin7@...il.com>,
	linux-kernel@...r.kernel.org, Jesse Gross <jesse@...ira.com>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	Stephen Hemminger <stephen@...workplumber.org>,
	Matt Mackall <mpm@...enic.com>,
	Pekka Enberg <penberg@...nel.org>,
	Christoph Lameter <cl@...ux-foundation.org>,
	Andy Gospodarek <andy@...yhouse.net>,
	Veaceslav Falico <vfalico@...hat.com>,
	Jay Vosburgh <fubar@...ibm.com>,
	Jakub Zawadzki <darkjames-ws@...kjames.pl>,
	Daniel Borkmann <dborkman@...hat.com>
Subject: Re: [PATCH net-next v2 3/3] reciprocal_divide: correction/update of
 the algorithm

On Fri, 2014-01-17 at 01:28 +0100, Hannes Frederic Sowa wrote:
> Jakub Zawadzki noticed that some divisions by reciprocal_divide()
> were not correct [1][2], which he could also show with BPF code after
> divisions are transformed into reciprocal_value() for runtime invariant
> which can be passed to reciprocal_divide() later on; reverse in BPF dump
> ended up with a different, off-by-one K.
> 
> This has been fixed by Eric Dumazet in commit aee636c4809fa5 ("bpf: do not
> use reciprocal divide"). This follow-up patch improves reciprocal_value()
> and reciprocal_divide() to work in all cases, so future use is safe.
> 
> Known problems with the old implementation were that division by 1 always
> returned 0 and some off-by-ones when the dividend and divisor where
> very large.  This seemed to not be problematic with its current users
> in networking, mm/slab.c and lib/flex_array.c, but future users would
> need to check for this specifically and it might not be obvious at first.
> 
> In order to fix that, we propose an extension from the original
> implementation from commit 6a2d7a955d8d resp. [3][4], by using
> the algorithm proposed in "Division by Invariant Integers Using
> Multiplication" [5], Torbjörn Granlund and Peter L. Montgomery, that is,
> pseudocode for q = n/d where q,n,d is in u32 universe:
> 
> 1) Initialization:
> 
>   int l = ceil(log_2 d)
>   uword m' = floor((1<<32)*((1<<l)-d)/d)+1
>   int sh_1 = min(l,1)
>   int sh_2 = max(l-1,0)
> 
> 2) For q = n/d, all uword:
> 
>   uword t = (n*m')>>32
>   q = (t+((n-t)>>sh_1))>>sh_2
> 
> The assembler implementation from Agner Fog [6] also helped a lot
> while implementing. We have tested the implementation on x86_64,
> ppc64, i686, s390x; on x86_64/haswell we're still half the latency
> compared to normal divide.
> 
> Joint work with Daniel Borkmann.
> 
>   [1] http://www.wireshark.org/~darkjames/reciprocal-buggy.c
>   [2] http://www.wireshark.org/~darkjames/set-and-dump-filter-k-bug.c
>   [3] https://gmplib.org/~tege/division-paper.pdf
>   [4] http://homepage.cs.uiowa.edu/~jones/bcd/divide.html
>   [5] http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.1.2556
>   [6] http://www.agner.org/optimize/asmlib.zip
> 
> Fixes: 6a2d7a955d8d ("SLAB: use a multiply instead of a divide in obj_to_index()")


I already demonstrated this slab patch was fine.

The current algo works well (no off-by-one error) when the dividend is
a multiple of the divisor.

You are adding extra overhead, while we know its not necessary.

By using "Fixes: ... " you are asking a backport to stable branches,
which seems really silly in this case, especially with this monolithic
patch changing 12 files in different subsystems.

If you believe flex_array has a problem, please fix flex_array only,
by a small patch (Maybe a revert ?)

Then, introduce your new helpers if we really think they are needed.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists