On 06/17/2015 01:08 PM, Peter Nørlund wrote:
The current multipath attempted to be quasi random, but in most cases it
behaved just like a round robin balancing. This patch refactors the
algorithm to be exactly that and in doing so, avoids the spin lock.
The new design paves the way for hash-based multipath, replacing the
modulo with thresholds, minimizing disruption in case of failing paths or
route replacements.
Signed-off-by: Peter Nørlund <pch@xxxxxxxxxxxx>
---
include/net/ip_fib.h | 6 +--
net/ipv4/Kconfig | 1 +
net/ipv4/fib_semantics.c | 116 ++++++++++++++++++++++++++---------------------
3 files changed, 68 insertions(+), 55 deletions(-)
diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index 54271ed..4be4f25 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -76,8 +76,8 @@ struct fib_nh {
unsigned int nh_flags;
unsigned char nh_scope;
#ifdef CONFIG_IP_ROUTE_MULTIPATH
- int nh_weight;
- int nh_power;
+ int nh_mp_weight;
+ atomic_t nh_mp_upper_bound;
#endif
#ifdef CONFIG_IP_ROUTE_CLASSID
__u32 nh_tclassid;
@@ -115,7 +115,7 @@ struct fib_info {
#define fib_advmss fib_metrics[RTAX_ADVMSS-1]
int fib_nhs;
#ifdef CONFIG_IP_ROUTE_MULTIPATH
- int fib_power;
+ int fib_mp_weight;
#endif
struct rcu_head rcu;
struct fib_nh fib_nh[0];
I could do without some of this renaming. For example you could
probably not bother with adding the _mp piece to the name. That way we
don't have to track all the nh_weight -> nh_mp_weight changes. Also
you could probably just use the name fib_weight since not including the
_mp was already the convention for the multipath portions of the
structure anyway.
This isn't really improving readability at all so I would say don't
bother renaming it.
diff --git a/net/ipv4/Kconfig b/net/ipv4/Kconfig
index d83071d..cb91f67 100644
--- a/net/ipv4/Kconfig
+++ b/net/ipv4/Kconfig
@@ -81,6 +81,7 @@ config IP_MULTIPLE_TABLES
config IP_ROUTE_MULTIPATH
bool "IP: equal cost multipath"
depends on IP_ADVANCED_ROUTER
+ select BITREVERSE
help
Normally, the routing tables specify a single action to be taken in
a deterministic manner for a given packet. If you say Y here
diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 28ec3c1..8c8df80 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c
@@ -15,6 +15,7 @@
#include <asm/uaccess.h>
#include <linux/bitops.h>
+#include <linux/bitrev.h>
#include <linux/types.h>
#include <linux/kernel.h>
#include <linux/jiffies.h>
@@ -57,7 +58,7 @@ static struct hlist_head fib_info_devhash[DEVINDEX_HASHSIZE];
#ifdef CONFIG_IP_ROUTE_MULTIPATH
-static DEFINE_SPINLOCK(fib_multipath_lock);
+static DEFINE_PER_CPU(u8, fib_mp_rr_counter);
#define for_nexthops(fi) { \
int nhsel; const struct fib_nh *nh; \
@@ -261,7 +262,7 @@ static inline int nh_comp(const struct fib_info *fi, const struct fib_info *ofi)
nh->nh_gw != onh->nh_gw ||
nh->nh_scope != onh->nh_scope ||
#ifdef CONFIG_IP_ROUTE_MULTIPATH
- nh->nh_weight != onh->nh_weight ||
+ nh->nh_mp_weight != onh->nh_mp_weight ||
#endif
#ifdef CONFIG_IP_ROUTE_CLASSID
nh->nh_tclassid != onh->nh_tclassid ||
@@ -449,6 +450,43 @@ static int fib_count_nexthops(struct rtnexthop *rtnh, int remaining)
return remaining > 0 ? 0 : nhs;
}
This is a good example. If we don't do the rename we don't have to
review changes like the one above which just add extra overhead to the
patch.
+static void fib_rebalance(struct fib_info *fi)
+{
+ int factor;
+ int total;
+ int w;
+
+ if (fi->fib_nhs < 2)
+ return;
+
+ total = 0;
+ for_nexthops(fi) {
+ if (!(nh->nh_flags & RTNH_F_DEAD))
+ total += nh->nh_mp_weight;
+ } endfor_nexthops(fi);
+
+ if (likely(total != 0)) {
+ factor = DIV_ROUND_UP(total, 8388608);
+ total /= factor;
+ } else {
+ factor = 1;
+ }
+
So where does the 8388608 value come from? Is it just here to help
restrict the upper_bound to a u8 value?
+ w = 0;
+ change_nexthops(fi) {
+ int upper_bound;
+
+ if (nexthop_nh->nh_flags & RTNH_F_DEAD) {
+ upper_bound = -1;
+ } else {
+ w += nexthop_nh->nh_mp_weight / factor;
+ upper_bound = DIV_ROUND_CLOSEST(256 * w, total);
+ }
This is doing some confusing stuff. I assume the whole point is to get
the value to convert the upper_bound into a u8 value based on the weight
where you end with 256 as the last value, do I have that right?
I'm not really sure that is the best way to go from a code standpoint
since fib_nhs is an int which technically means we could support more
than 256 next hops for a given route.
Also I assume that it is safe to divide by total because the assumption
is that if one or more routes are not dead then total has a non-zero
value, do I have that right? If so what is the point of setting the
factor to 1 above, unless you are just hacking around a divide by 0 error.
Maybe what you should do above is simply return if total == 0 since it
doesn't look like you can safely update anything otherwise.
+
+ atomic_set(&nexthop_nh->nh_mp_upper_bound, upper_bound);
+ } endfor_nexthops(fi);
+}
+
static int fib_get_nhs(struct fib_info *fi, struct rtnexthop *rtnh,
int remaining, struct fib_config *cfg)
{
@@ -461,7 +499,7 @@ static int fib_get_nhs(struct fib_info *fi, struct rtnexthop *rtnh,
nexthop_nh->nh_flags =
(cfg->fc_flags & ~0xFF) | rtnh->rtnh_flags;
nexthop_nh->nh_oif = rtnh->rtnh_ifindex;
- nexthop_nh->nh_weight = rtnh->rtnh_hops + 1;
+ nexthop_nh->nh_mp_weight = rtnh->rtnh_hops + 1;
attrlen = rtnh_attrlen(rtnh);
if (attrlen > 0) {
@@ -884,7 +922,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg)
fi->fib_net->ipv4.fib_num_tclassid_users++;
#endif
#ifdef CONFIG_IP_ROUTE_MULTIPATH
- nh->nh_weight = 1;
+ nh->nh_mp_weight = 1;
#endif
}
More unnecessary rename noise.
@@ -936,8 +974,15 @@ struct fib_info *fib_create_info(struct fib_config *cfg)
change_nexthops(fi) {
fib_info_update_nh_saddr(net, nexthop_nh);
+#ifdef CONFIG_IP_ROUTE_MULTIPATH
+ fi->fib_mp_weight += nexthop_nh->nh_mp_weight;
+#endif
} endfor_nexthops(fi)
+#ifdef CONFIG_IP_ROUTE_MULTIPATH
+ fib_rebalance(fi);
+#endif
+
link_it:
ofi = fib_find_info(fi);
if (ofi) {
@@ -1050,7 +1095,7 @@ int fib_dump_info(struct sk_buff *skb, u32 portid, u32 seq, int event,
goto nla_put_failure;
rtnh->rtnh_flags = nh->nh_flags & 0xFF;
- rtnh->rtnh_hops = nh->nh_weight - 1;
+ rtnh->rtnh_hops = nh->nh_mp_weight - 1;
rtnh->rtnh_ifindex = nh->nh_oif;
if (nh->nh_gw &&
@@ -1130,12 +1175,6 @@ int fib_sync_down_dev(struct net_device *dev, int force)
else if (nexthop_nh->nh_dev == dev &&
nexthop_nh->nh_scope != scope) {
nexthop_nh->nh_flags |= RTNH_F_DEAD;
-#ifdef CONFIG_IP_ROUTE_MULTIPATH
- spin_lock_bh(&fib_multipath_lock);
- fi->fib_power -= nexthop_nh->nh_power;
- nexthop_nh->nh_power = 0;
- spin_unlock_bh(&fib_multipath_lock);
-#endif
dead++;
}
#ifdef CONFIG_IP_ROUTE_MULTIPATH
@@ -1149,6 +1188,10 @@ int fib_sync_down_dev(struct net_device *dev, int force)
fi->fib_flags |= RTNH_F_DEAD;
ret++;
}
+
+#ifdef CONFIG_IP_ROUTE_MULTIPATH
+ fib_rebalance(fi);
+#endif
}
return ret;
@@ -1254,16 +1297,15 @@ int fib_sync_up(struct net_device *dev)
!__in_dev_get_rtnl(dev))
continue;
alive++;
- spin_lock_bh(&fib_multipath_lock);
- nexthop_nh->nh_power = 0;
nexthop_nh->nh_flags &= ~RTNH_F_DEAD;
- spin_unlock_bh(&fib_multipath_lock);
} endfor_nexthops(fi)
if (alive > 0) {
fi->fib_flags &= ~RTNH_F_DEAD;
ret++;
}
+
+ fib_rebalance(fi);
}
return ret;
@@ -1276,49 +1318,19 @@ int fib_sync_up(struct net_device *dev)
void fib_select_multipath(struct fib_result *res)
{
struct fib_info *fi = res->fi;
- int w;
-
- spin_lock_bh(&fib_multipath_lock);
- if (fi->fib_power <= 0) {
- int power = 0;
- change_nexthops(fi) {
- if (!(nexthop_nh->nh_flags & RTNH_F_DEAD)) {
- power += nexthop_nh->nh_weight;
- nexthop_nh->nh_power = nexthop_nh->nh_weight;
- }
- } endfor_nexthops(fi);
- fi->fib_power = power;
- if (power <= 0) {
- spin_unlock_bh(&fib_multipath_lock);
- /* Race condition: route has just become dead. */
- res->nh_sel = 0;
- return;
- }
- }
+ u8 w;
+ w = bitrev8(this_cpu_inc_return(fib_mp_rr_counter));
- /* w should be random number [0..fi->fib_power-1],
- * it is pretty bad approximation.
- */
-
- w = jiffies % fi->fib_power;
+ for_nexthops(fi) {
+ if (w >= atomic_read(&nh->nh_mp_upper_bound))
+ continue;
- change_nexthops(fi) {
- if (!(nexthop_nh->nh_flags & RTNH_F_DEAD) &&
- nexthop_nh->nh_power) {
- w -= nexthop_nh->nh_power;
- if (w <= 0) {
- nexthop_nh->nh_power--;
- fi->fib_power--;
- res->nh_sel = nhsel;
- spin_unlock_bh(&fib_multipath_lock);
- return;
- }
- }
+ res->nh_sel = nhsel;
+ return;
} endfor_nexthops(fi);
- /* Race condition: route has just become dead. */
+ /* Race condition: route has just become dead */
res->nh_sel = 0;
- spin_unlock_bh(&fib_multipath_lock);
}
#endif
Instead of doing bitrev8 why not just hash jiffies since that is
essentially what you are doing with this counter anyway. You could
probably even look at converting from a u8 to something like a u32 which
would give you a more pseudo random distribution.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
