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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
