[PATCH net-next v2 0/3] reciprocal_divide updates

[PATCH net-next v2 0/3] reciprocal_divide updates

[Hannes Frederic Sowa]

This set is on top of Eric's BPF fix, that is:
  
  <http://patchwork.ozlabs.org/patch/311163/>

This fix is currently in the net repository; a merge of net to net-next
would be needed for this series. 

Included Patches:

Daniel Borkmann (2):
      random32: add prandom_u32_max and convert open coded users
      net: add trim helper and convert users

Hannes Frederic Sowa (1):
      reciprocal_divide: correction/update of the algorithm

Diffstat:
 drivers/net/bonding/bond_main.c     | 24 +++++++++++++++++-------
 drivers/net/bonding/bond_netlink.c  |  4 ----
 drivers/net/bonding/bond_options.c  | 15 ++++++++++----- 
 drivers/net/bonding/bond_sysfs.c    |  5 -----
 drivers/net/bonding/bonding.h       |  3 +++
 drivers/net/team/team_mode_random.c |  8 +-------
 include/linux/flex_array.h          |  3 ++-
 include/linux/kernel.h              | 19 +++++++++++++++++++
 include/linux/random.h              | 19 ++++++++++++++++++-
 include/linux/reciprocal_div.h      | 39 +++++++++++++++++++++------------------
 include/linux/slab_def.h            |  4 +++-
 include/net/codel.h                 |  4 +---
 include/net/red.h                   |  3 ++-
 lib/flex_array.c                    |  7 ++++++-
 lib/reciprocal_div.c                | 24 ++++++++++++++++++++----
 net/packet/af_packet.c              |  5 ++---
 net/sched/sch_choke.c               |  9 +--------
 net/sched/sch_netem.c               |  6 ++++--
 18 files changed, 130 insertions(+), 71 deletions(-)

Changelog:
v2)
* change function name to prandom_u32_max as suggested by Joe Perches
* first patch split into two patches, second one introduces helper
  function trim (suggested by Eric Dumazet to not go back to open-coded
  version and David Laight to rename reciprocal_divide())
* removed RECIPROCAL_VALUE_RESULT_TO_ZERO as it causes undefined behaviour
  and is broken (thanks Ben Hutchings!)
* reworded

[PATCH net-next v2 1/3] random32: add prandom_u32_max and convert open coded users

[Hannes Frederic Sowa]

From: Daniel Borkmann <dborkman@redhat.com>

Many functions have open coded a function that returns a random
number in range [0,N-1]. Under the assumption that we have a PRNG
such as taus113 with being well distributed in [0, ~0U] space,
we can implement such a function as uword t = (n*m')>>32, where
m' is a random number obtained from PRNG, n the right open interval
border and t our resulting random number, with n,m',t in u32 universe.

Lets go with Joe and simply call it prandom_u32_max(), although
technically we have an right open interval endpoint, but that we
have documented. Other users can further be migrated to the new
prandom_u32_max() function later on; for now, we need to make sure
to migrate reciprocal_divide() users for the reciprocal_divide()
follow-up fixup since their function signatures are going to change.

Joint work with Hannes Frederic Sowa.

Cc: Jakub Zawadzki <darkjames-ws@darkjames.pl>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 drivers/net/team/team_mode_random.c |  8 +-------
 include/linux/random.h              | 19 ++++++++++++++++++-
 net/packet/af_packet.c              |  2 +-
 net/sched/sch_choke.c               |  9 +--------
 4 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/net/team/team_mode_random.c b/drivers/net/team/team_mode_random.c
index 7f032e2..cd2f692 100644
--- a/drivers/net/team/team_mode_random.c
+++ b/drivers/net/team/team_mode_random.c
@@ -13,20 +13,14 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/skbuff.h>
-#include <linux/reciprocal_div.h>
 #include <linux/if_team.h>
 
-static u32 random_N(unsigned int N)
-{
-	return reciprocal_divide(prandom_u32(), N);
-}
-
 static bool rnd_transmit(struct team *team, struct sk_buff *skb)
 {
 	struct team_port *port;
 	int port_index;
 
-	port_index = random_N(team->en_port_count);
+	port_index = prandom_u32_max(team->en_port_count);
 	port = team_get_port_by_index_rcu(team, port_index);
 	if (unlikely(!port))
 		goto drop;
diff --git a/include/linux/random.h b/include/linux/random.h
index 4002b3d..f763a63 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -8,7 +8,6 @@
 
 #include <uapi/linux/random.h>
 
-
 extern void add_device_randomness(const void *, unsigned int);
 extern void add_input_randomness(unsigned int type, unsigned int code,
 				 unsigned int value);
@@ -38,6 +37,24 @@ struct rnd_state {
 u32 prandom_u32_state(struct rnd_state *state);
 void prandom_bytes_state(struct rnd_state *state, void *buf, int nbytes);
 
+/**
+ * prandom_u32_max - returns a random number in interval [0, ep_ro), the
+ *                   upper interval endpoint is right-open. This is useful
+ *                   when requesting a random index of an array containing
+ *                   ep_ro elements, for example.
+ *
+ * @ep_ro: right open interval endpoint
+ *
+ * Returns a pseduo-random number that is in interval [0, ep_ro). Note
+ * that the result depends on PRNG being well distributed in [0, ~0U]
+ * u32 space. Here we use maximally equidistributed combined Tausworthe
+ * generator, that is, prandom_u32().
+ */
+static inline u32 prandom_u32_max(u32 ep_ro)
+{
+	return (u32)(((u64) prandom_u32() * ep_ro) >> 32);
+}
+
 /*
  * Handle minimum values for seeds
  */
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 279467b..91daa01 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1247,7 +1247,7 @@ static unsigned int fanout_demux_rnd(struct packet_fanout *f,
 				     struct sk_buff *skb,
 				     unsigned int num)
 {
-	return reciprocal_divide(prandom_u32(), num);
+	return prandom_u32_max(num);
 }
 
 static unsigned int fanout_demux_rollover(struct packet_fanout *f,
diff --git a/net/sched/sch_choke.c b/net/sched/sch_choke.c
index ddd73cb..2aee028 100644
--- a/net/sched/sch_choke.c
+++ b/net/sched/sch_choke.c
@@ -14,7 +14,6 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/skbuff.h>
-#include <linux/reciprocal_div.h>
 #include <linux/vmalloc.h>
 #include <net/pkt_sched.h>
 #include <net/inet_ecn.h>
@@ -77,12 +76,6 @@ struct choke_sched_data {
 	struct sk_buff **tab;
 };
 
-/* deliver a random number between 0 and N - 1 */
-static u32 random_N(unsigned int N)
-{
-	return reciprocal_divide(prandom_u32(), N);
-}
-
 /* number of elements in queue including holes */
 static unsigned int choke_len(const struct choke_sched_data *q)
 {
@@ -233,7 +226,7 @@ static struct sk_buff *choke_peek_random(const struct choke_sched_data *q,
 	int retrys = 3;
 
 	do {
-		*pidx = (q->head + random_N(choke_len(q))) & q->tab_mask;
+		*pidx = (q->head + prandom_u32_max(choke_len(q))) & q->tab_mask;
 		skb = q->tab[*pidx];
 		if (skb)
 			return skb;
-- 
1.8.4.2

[PATCH net-next v2 2/3] net: add trim helper and convert users

[Hannes Frederic Sowa]

From: Daniel Borkmann <dborkman@redhat.com>

As David Laight suggests, we shouldn't necessarily call this
reciprocal_divide() when users didn't requested a reciprocal_value().
Lets make the name short and nifty, put this where we have other
generic helpers of similar kind and convert users.

Joint work with Hannes Frederic Sowa.

Cc: Jakub Zawadzki <darkjames-ws@darkjames.pl>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/linux/kernel.h | 19 +++++++++++++++++++
 include/net/codel.h    |  4 +---
 net/packet/af_packet.c |  3 +--
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index ecb8754..62ecbfa 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -193,6 +193,25 @@ extern int _cond_resched(void);
 		(__x < 0) ? -__x : __x;		\
 	})
 
+/**
+ * trim - perform a reciprocal multiplication in order to "clamp" a
+ *        value into range [0, ep_ro), where the upper interval
+ *        endpoint is right-open. This is useful, e.g. for accessing
+ *        a index of an array containing ep_ro elements, for example.
+ *        Think of it as sort of modulus, only that the result isn't
+ *        that of modulo. ;)
+ *        More: http://homepage.cs.uiowa.edu/~jones/bcd/divide.html
+ *
+ * @val: value to 'trim'
+ * @ep_ro: right open interval endpoint
+ *
+ * Returns a result based on val in interval [0, ep_ro).
+ */
+static inline u32 trim(u32 val, u32 ep_ro)
+{
+	return (u32)(((u64) val * ep_ro) >> 32);
+}
+
 #if defined(CONFIG_MMU) && \
 	(defined(CONFIG_PROVE_LOCKING) || defined(CONFIG_DEBUG_ATOMIC_SLEEP))
 void might_fault(void);
diff --git a/include/net/codel.h b/include/net/codel.h
index 3b04ff5..d302b4c 100644
--- a/include/net/codel.h
+++ b/include/net/codel.h
@@ -46,7 +46,6 @@
 #include <linux/skbuff.h>
 #include <net/pkt_sched.h>
 #include <net/inet_ecn.h>
-#include <linux/reciprocal_div.h>
 
 /* Controlling Queue Delay (CoDel) algorithm
  * =========================================
@@ -211,10 +210,9 @@ static codel_time_t codel_control_law(codel_time_t t,
 				      codel_time_t interval,
 				      u32 rec_inv_sqrt)
 {
-	return t + reciprocal_divide(interval, rec_inv_sqrt << REC_INV_SQRT_SHIFT);
+	return t + trim(interval, rec_inv_sqrt << REC_INV_SQRT_SHIFT);
 }
 
-
 static bool codel_should_drop(const struct sk_buff *skb,
 			      struct Qdisc *sch,
 			      struct codel_vars *vars,
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 91daa01..d327774 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -88,7 +88,6 @@
 #include <linux/virtio_net.h>
 #include <linux/errqueue.h>
 #include <linux/net_tstamp.h>
-#include <linux/reciprocal_div.h>
 #ifdef CONFIG_INET
 #include <net/inet_common.h>
 #endif
@@ -1220,7 +1219,7 @@ static unsigned int fanout_demux_hash(struct packet_fanout *f,
 				      struct sk_buff *skb,
 				      unsigned int num)
 {
-	return reciprocal_divide(skb->rxhash, num);
+	return trim(skb->rxhash, num);
 }
 
 static unsigned int fanout_demux_lb(struct packet_fanout *f,
-- 
1.8.4.2

[PATCH net-next v2 3/3] reciprocal_divide: correction/update of the algorithm

[Hannes Frederic Sowa]

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()")
Fixes: 704f15ddb5fc ("flex_array: avoid divisions when accessing elements")
Reported-by: Jakub Zawadzki <darkjames-ws@darkjames.pl>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Austin S Hemmelgarn <ahferroin7@gmail.com>
Cc: linux-kernel@vger.kernel.org
Cc: Jesse Gross <jesse@nicira.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Matt Mackall <mpm@selenic.com>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: Christoph Lameter <cl@linux-foundation.org>
Cc: Andy Gospodarek <andy@greyhouse.net>
Cc: Veaceslav Falico <vfalico@redhat.com>
Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Jakub Zawadzki <darkjames-ws@darkjames.pl>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 drivers/net/bonding/bond_main.c    | 24 ++++++++++++++++-------
 drivers/net/bonding/bond_netlink.c |  4 ----
 drivers/net/bonding/bond_options.c | 15 ++++++++++-----
 drivers/net/bonding/bond_sysfs.c   |  5 -----
 drivers/net/bonding/bonding.h      |  3 +++
 include/linux/flex_array.h         |  3 ++-
 include/linux/reciprocal_div.h     | 39 ++++++++++++++++++++------------------
 include/linux/slab_def.h           |  4 +++-
 include/net/red.h                  |  3 ++-
 lib/flex_array.c                   |  7 ++++++-
 lib/reciprocal_div.c               | 24 +++++++++++++++++++----
 net/sched/sch_netem.c              |  6 ++++--
 12 files changed, 88 insertions(+), 49 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index f2fe6cb..77e57da 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -79,7 +79,6 @@
 #include <net/pkt_sched.h>
 #include <linux/rculist.h>
 #include <net/flow_keys.h>
-#include <linux/reciprocal_div.h>
 #include "bonding.h"
 #include "bond_3ad.h"
 #include "bond_alb.h"
@@ -3551,8 +3550,9 @@ static void bond_xmit_slave_id(struct bonding *bond, struct sk_buff *skb, int sl
  */
 static u32 bond_rr_gen_slave_id(struct bonding *bond)
 {
-	int packets_per_slave = bond->params.packets_per_slave;
 	u32 slave_id;
+	struct reciprocal_value reciprocal_packets_per_slave;
+	int packets_per_slave = bond->params.packets_per_slave;
 
 	switch (packets_per_slave) {
 	case 0:
@@ -3562,8 +3562,10 @@ static u32 bond_rr_gen_slave_id(struct bonding *bond)
 		slave_id = bond->rr_tx_counter;
 		break;
 	default:
+		reciprocal_packets_per_slave =
+			bond->params.reciprocal_packets_per_slave;
 		slave_id = reciprocal_divide(bond->rr_tx_counter,
-					     packets_per_slave);
+					     reciprocal_packets_per_slave);
 		break;
 	}
 	bond->rr_tx_counter++;
@@ -4297,10 +4299,18 @@ static int bond_check_params(struct bond_params *params)
 	params->resend_igmp = resend_igmp;
 	params->min_links = min_links;
 	params->lp_interval = lp_interval;
-	if (packets_per_slave > 1)
-		params->packets_per_slave = reciprocal_value(packets_per_slave);
-	else
-		params->packets_per_slave = packets_per_slave;
+	params->packets_per_slave = packets_per_slave;
+	if (packets_per_slave > 0) {
+		params->reciprocal_packets_per_slave =
+			reciprocal_value(packets_per_slave);
+	} else {
+		/* reciprocal_packets_per_slave is unused if
+		 * packets_per_slave is 0 or 1, just initialize it
+		 */
+		params->reciprocal_packets_per_slave =
+			(struct reciprocal_value) { 0 };
+	}
+
 	if (primary) {
 		strncpy(params->primary, primary, IFNAMSIZ);
 		params->primary[IFNAMSIZ - 1] = 0;
diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
index 555c783..9b13791 100644
--- a/drivers/net/bonding/bond_netlink.c
+++ b/drivers/net/bonding/bond_netlink.c
@@ -19,7 +19,6 @@
 #include <linux/if_ether.h>
 #include <net/netlink.h>
 #include <net/rtnetlink.h>
-#include <linux/reciprocal_div.h>
 #include "bonding.h"
 
 static const struct nla_policy bond_policy[IFLA_BOND_MAX + 1] = {
@@ -416,9 +415,6 @@ static int bond_fill_info(struct sk_buff *skb,
 		goto nla_put_failure;
 
 	packets_per_slave = bond->params.packets_per_slave;
-	if (packets_per_slave > 1)
-		packets_per_slave = reciprocal_value(packets_per_slave);
-
 	if (nla_put_u32(skb, IFLA_BOND_PACKETS_PER_SLAVE,
 			packets_per_slave))
 		goto nla_put_failure;
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 945a666..85e4348 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -16,7 +16,6 @@
 #include <linux/netdevice.h>
 #include <linux/rwlock.h>
 #include <linux/rcupdate.h>
-#include <linux/reciprocal_div.h>
 #include "bonding.h"
 
 int bond_option_mode_set(struct bonding *bond, int mode)
@@ -671,11 +670,17 @@ int bond_option_packets_per_slave_set(struct bonding *bond,
 		pr_warn("%s: Warning: packets_per_slave has effect only in balance-rr mode\n",
 			bond->dev->name);
 
-	if (packets_per_slave > 1)
-		bond->params.packets_per_slave =
+	bond->params.packets_per_slave = packets_per_slave;
+	if (packets_per_slave > 0) {
+		bond->params.reciprocal_packets_per_slave =
 			reciprocal_value(packets_per_slave);
-	else
-		bond->params.packets_per_slave = packets_per_slave;
+	} else {
+		/* reciprocal_packets_per_slave is unused if
+		 * packets_per_slave is 0 or 1, just initialize it
+		 */
+		bond->params.reciprocal_packets_per_slave =
+			(struct reciprocal_value) { 0 };
+	}
 
 	return 0;
 }
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 011f163..c083e9a 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -39,7 +39,6 @@
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
 #include <linux/nsproxy.h>
-#include <linux/reciprocal_div.h>
 
 #include "bonding.h"
 
@@ -1374,10 +1373,6 @@ static ssize_t bonding_show_packets_per_slave(struct device *d,
 {
 	struct bonding *bond = to_bond(d);
 	unsigned int packets_per_slave = bond->params.packets_per_slave;
-
-	if (packets_per_slave > 1)
-		packets_per_slave = reciprocal_value(packets_per_slave);
-
 	return sprintf(buf, "%u\n", packets_per_slave);
 }
 
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 955dc48..502dda8 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -23,6 +23,8 @@
 #include <linux/netpoll.h>
 #include <linux/inetdevice.h>
 #include <linux/etherdevice.h>
+#include <linux/reciprocal_div.h>
+
 #include "bond_3ad.h"
 #include "bond_alb.h"
 
@@ -171,6 +173,7 @@ struct bond_params {
 	int resend_igmp;
 	int lp_interval;
 	int packets_per_slave;
+	struct reciprocal_value reciprocal_packets_per_slave;
 };
 
 struct bond_parm_tbl {
diff --git a/include/linux/flex_array.h b/include/linux/flex_array.h
index 6843cf1..b6efb0c 100644
--- a/include/linux/flex_array.h
+++ b/include/linux/flex_array.h
@@ -2,6 +2,7 @@
 #define _FLEX_ARRAY_H
 
 #include <linux/types.h>
+#include <linux/reciprocal_div.h>
 #include <asm/page.h>
 
 #define FLEX_ARRAY_PART_SIZE PAGE_SIZE
@@ -22,7 +23,7 @@ struct flex_array {
 			int element_size;
 			int total_nr_elements;
 			int elems_per_part;
-			u32 reciprocal_elems;
+			struct reciprocal_value reciprocal_elems;
 			struct flex_array_part *parts[];
 		};
 		/*
diff --git a/include/linux/reciprocal_div.h b/include/linux/reciprocal_div.h
index f9c90b3..8c5a3fb 100644
--- a/include/linux/reciprocal_div.h
+++ b/include/linux/reciprocal_div.h
@@ -4,29 +4,32 @@
 #include <linux/types.h>
 
 /*
- * This file describes reciprocical division.
+ * This algorithm is based on the paper "Division by Invariant
+ * Integers Using Multiplication" by Torbjörn Granlund and Peter
+ * L. Montgomery.
  *
- * This optimizes the (A/B) problem, when A and B are two u32
- * and B is a known value (but not known at compile time)
+ * The assembler implementation from Agner Fog, which this code is
+ * based on, can be found here:
+ * http://www.agner.org/optimize/asmlib.zip
  *
- * The math principle used is :
- *   Let RECIPROCAL_VALUE(B) be (((1LL << 32) + (B - 1))/ B)
- *   Then A / B = (u32)(((u64)(A) * (R)) >> 32)
- *
- * This replaces a divide by a multiply (and a shift), and
- * is generally less expensive in CPU cycles.
+ * This optimization for A/B is helpful if the divisor B is mostly
+ * runtime invariant. The reciprocal of B is calculated in the
+ * slow-path with reciprocal_value(). The fast-path can then just use
+ * a much faster multiplication operation with a variable dividend A
+ * to calculate the division A/B.
  */
 
-/*
- * Computes the reciprocal value (R) for the value B of the divisor.
- * Should not be called before each reciprocal_divide(),
- * or else the performance is slower than a normal divide.
- */
-extern u32 reciprocal_value(u32 B);
+struct reciprocal_value {
+	u32 m;
+	u8 sh1, sh2;
+};
 
+struct reciprocal_value reciprocal_value(u32 d);
 
-static inline u32 reciprocal_divide(u32 A, u32 R)
+static inline u32 reciprocal_divide(u32 a, struct reciprocal_value R)
 {
-	return (u32)(((u64)A * R) >> 32);
+	u32 t = (u32)(((u64)a * R.m) >> 32);
+	return (t + ((a - t) >> R.sh1)) >> R.sh2;
 }
-#endif
+
+#endif /* _LINUX_RECIPROCAL_DIV_H */
diff --git a/include/linux/slab_def.h b/include/linux/slab_def.h
index 09bfffb..96e8aba 100644
--- a/include/linux/slab_def.h
+++ b/include/linux/slab_def.h
@@ -1,6 +1,8 @@
 #ifndef _LINUX_SLAB_DEF_H
 #define	_LINUX_SLAB_DEF_H
 
+#include <linux/reciprocal_div.h>
+
 /*
  * Definitions unique to the original Linux SLAB allocator.
  */
@@ -12,7 +14,7 @@ struct kmem_cache {
 	unsigned int shared;
 
 	unsigned int size;
-	u32 reciprocal_buffer_size;
+	struct reciprocal_value reciprocal_buffer_size;
 /* 2) touched by every alloc & free from the backend */
 
 	unsigned int flags;		/* constant flags */
diff --git a/include/net/red.h b/include/net/red.h
index 168bb2f..76e0b5f 100644
--- a/include/net/red.h
+++ b/include/net/red.h
@@ -130,7 +130,8 @@ struct red_parms {
 	u32		qth_max;	/* Max avg length threshold: Wlog scaled */
 	u32		Scell_max;
 	u32		max_P;		/* probability, [0 .. 1.0] 32 scaled */
-	u32		max_P_reciprocal; /* reciprocal_value(max_P / qth_delta) */
+	/* reciprocal_value(max_P / qth_delta) */
+	struct reciprocal_value	max_P_reciprocal;
 	u32		qth_delta;	/* max_th - min_th */
 	u32		target_min;	/* min_th + 0.4*(max_th - min_th) */
 	u32		target_max;	/* min_th + 0.6*(max_th - min_th) */
diff --git a/lib/flex_array.c b/lib/flex_array.c
index 6948a66..2eed22f 100644
--- a/lib/flex_array.c
+++ b/lib/flex_array.c
@@ -90,8 +90,8 @@ struct flex_array *flex_array_alloc(int element_size, unsigned int total,
 {
 	struct flex_array *ret;
 	int elems_per_part = 0;
-	int reciprocal_elems = 0;
 	int max_size = 0;
+	struct reciprocal_value reciprocal_elems = { 0 };
 
 	if (element_size) {
 		elems_per_part = FLEX_ARRAY_ELEMENTS_PER_PART(element_size);
@@ -119,6 +119,11 @@ EXPORT_SYMBOL(flex_array_alloc);
 static int fa_element_to_part_nr(struct flex_array *fa,
 					unsigned int element_nr)
 {
+	/*
+	 * if element_size == 0 we don't get here, so we never touch
+	 * the zeroed fa->reciprocal_elems, which would yield invalid
+	 * results
+	 */
 	return reciprocal_divide(element_nr, fa->reciprocal_elems);
 }
 
diff --git a/lib/reciprocal_div.c b/lib/reciprocal_div.c
index 75510e9..4641524 100644
--- a/lib/reciprocal_div.c
+++ b/lib/reciprocal_div.c
@@ -1,11 +1,27 @@
+#include <linux/kernel.h>
 #include <asm/div64.h>
 #include <linux/reciprocal_div.h>
 #include <linux/export.h>
 
-u32 reciprocal_value(u32 k)
+/*
+ * For a description of the algorithm please have a look at
+ * include/linux/reciprocal_div.h
+ */
+
+struct reciprocal_value reciprocal_value(u32 d)
 {
-	u64 val = (1LL << 32) + (k - 1);
-	do_div(val, k);
-	return (u32)val;
+	struct reciprocal_value R;
+	u64 m;
+	int l;
+
+	l = fls(d - 1);
+	m = ((1ULL << 32) * ((1ULL << l) - d));
+	do_div(m, d);
+	++m;
+	R.m = (u32)m;
+	R.sh1 = min(l, 1);
+	R.sh2 = max(l - 1, 0);
+
+	return R;
 }
 EXPORT_SYMBOL(reciprocal_value);
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 3019c10..81fc5b0 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -91,7 +91,7 @@ struct netem_sched_data {
 	u64 rate;
 	s32 packet_overhead;
 	u32 cell_size;
-	u32 cell_size_reciprocal;
+	struct reciprocal_value cell_size_reciprocal;
 	s32 cell_overhead;
 
 	struct crndstate {
@@ -716,9 +716,11 @@ static void get_rate(struct Qdisc *sch, const struct nlattr *attr)
 	q->rate = r->rate;
 	q->packet_overhead = r->packet_overhead;
 	q->cell_size = r->cell_size;
+	q->cell_overhead = r->cell_overhead;
 	if (q->cell_size)
 		q->cell_size_reciprocal = reciprocal_value(q->cell_size);
-	q->cell_overhead = r->cell_overhead;
+	else
+		q->cell_size_reciprocal = (struct reciprocal_value) { 0 };
 }
 
 static int get_loss_clg(struct Qdisc *sch, const struct nlattr *attr)
-- 
1.8.4.2

Re: [PATCH net-next v2 3/3] reciprocal_divide: correction/update of the algorithm

[Eric Dumazet]

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.

Re: [PATCH net-next v2 3/3] reciprocal_divide: correction/update of the algorithm

[Hannes Frederic Sowa]

On Thu, Jan 16, 2014 at 06:33:37PM -0800, Eric Dumazet wrote:
> 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.

Sure, so did we state in the commit message.

> 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.

We can drop the the Fixes tags, no problem.

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

I really doubt it is helpful to have an implementation of reciprocal_divide
which has some known (and maybe unkown) problems in the long term.

This implementation still has an performance benefit compared to regular
division while calculating correct results in any case.

We clearly didn't intend stable inclusion, in fact this patch has been posted
for net-next inclusion as an improvment and not as a bugfix. The Fixes tags
where just lingering on this patch from my first attempt where the situation
was not that clear (at least for me).

Also I doubt the performance drop for SLAB will be that massive. Also it was
already replaced by SLUB as the default SLAB allocator, which doesn't use
reciprocal_divide.

Greetings,

  Hannes

Re: [PATCH net-next v2 3/3] reciprocal_divide: correction/update of the algorithm

[Eric Dumazet]

On Fri, 2014-01-17 at 05:29 +0100, Hannes Frederic Sowa wrote:

> Also I doubt the performance drop for SLAB will be that massive. Also it was
> already replaced by SLUB as the default SLAB allocator, which doesn't use
> reciprocal_divide.

Google servers use SLAB, not SLUB, for various reasons, and performance
is one of them.

RE: [PATCH net-next v2 2/3] net: add trim helper and convert users

[David Laight]

From: Hannes Frederic Sowa
...
> +/**
> + * trim - perform a reciprocal multiplication in order to "clamp" a
> + *        value into range [0, ep_ro), where the upper interval
> + *        endpoint is right-open. This is useful, e.g. for accessing
> + *        a index of an array containing ep_ro elements, for example.
> + *        Think of it as sort of modulus, only that the result isn't
> + *        that of modulo. ;)
> + *        More: http://homepage.cs.uiowa.edu/~jones/bcd/divide.html

It isn't appropriate to put urls into code comments (or commit messages).
They are very likely to get out of date.

	David

Re: [PATCH net-next v2 2/3] net: add trim helper and convert users

[Daniel Borkmann]

On 01/17/2014 10:52 AM, David Laight wrote:
> From: Hannes Frederic Sowa
> ...
>> +/**
>> + * trim - perform a reciprocal multiplication in order to "clamp" a
>> + *        value into range [0, ep_ro), where the upper interval
>> + *        endpoint is right-open. This is useful, e.g. for accessing
>> + *        a index of an array containing ep_ro elements, for example.
>> + *        Think of it as sort of modulus, only that the result isn't
>> + *        that of modulo. ;)
>> + *        More: http://homepage.cs.uiowa.edu/~jones/bcd/divide.html
>
> It isn't appropriate to put urls into code comments (or commit messages).
> They are very likely to get out of date.

Then please grep the git log for some references/urls, you'll find plenty.
This link is just a pointer for people interested to read some more
background; there's no need to overly elaborate in the kernel doc right
here.

Thanks !

> 	David
>
>
>

Re: [PATCH net-next v2 2/3] net: add trim helper and convert users

[Ben Hutchings]

On Fri, 2014-01-17 at 01:28 +0100, Hannes Frederic Sowa wrote:
[...]
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -193,6 +193,25 @@ extern int _cond_resched(void);
>  		(__x < 0) ? -__x : __x;		\
>  	})
>  
> +/**
> + * trim - perform a reciprocal multiplication in order to "clamp" a
> + *        value into range [0, ep_ro), where the upper interval
> + *        endpoint is right-open. This is useful, e.g. for accessing
> + *        a index of an array containing ep_ro elements, for example.
> + *        Think of it as sort of modulus, only that the result isn't
> + *        that of modulo. ;)
> + *        More: http://homepage.cs.uiowa.edu/~jones/bcd/divide.html
[...]

And you think trim() is an obvious name for that?!
How about: scale_u32_to_range().

Also the first physical line of a kernel-doc comment (after the name) is
a summary which is used, for example, in the summary line on a manual
page.  It seems like you have the summary and full description the wrong
way round here.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next v2 2/3] net: add trim helper and convert users

[Eric Dumazet]

On Fri, 2014-01-17 at 18:15 +0000, Ben Hutchings wrote:
> On Fri, 2014-01-17 at 01:28 +0100, Hannes Frederic Sowa wrote:
> [...]
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -193,6 +193,25 @@ extern int _cond_resched(void);
> >  		(__x < 0) ? -__x : __x;		\
> >  	})
> >  
> > +/**
> > + * trim - perform a reciprocal multiplication in order to "clamp" a
> > + *        value into range [0, ep_ro), where the upper interval
> > + *        endpoint is right-open. This is useful, e.g. for accessing
> > + *        a index of an array containing ep_ro elements, for example.
> > + *        Think of it as sort of modulus, only that the result isn't
> > + *        that of modulo. ;)
> > + *        More: http://homepage.cs.uiowa.edu/~jones/bcd/divide.html
> [...]
> 
> And you think trim() is an obvious name for that?!
> How about: scale_u32_to_range().
> 
> Also the first physical line of a kernel-doc comment (after the name) is
> a summary which is used, for example, in the summary line on a manual
> page.  It seems like you have the summary and full description the wrong
> way round here.

BTW the 'scaling' depends on u32 value being pretty much random.

If initial input is a small value like 0 .. 1000, then trim(x, 1000)
will return 0

I liked the reciprocal name because it was really expressing the
reciprocal idea.