Hello, Only few notes below. If all goes well, v4 can be the last version... :) On Wed, 21 Mar 2018, Inju Song wrote: > Implements the Google's Maglev hashing algorithm as a IPVS scheduler. > > Basically it provides consistent hashing but offers some special > features about disruption and load balancing. > > 1) minimal disruption: when the set of destinations changes, > a connection will likely be sent to the same destination > as it was before. > > 2) load balancing: each destination will receive an almost > equal number of connections. > > Seel also for detail: [3.4 Consistent Hasing] in > https://www.usenix.org/system/files/conference/nsdi16/nsdi16-paper-eisenbud.pdf Above commit message is properly indented but comments in patches 1 and 3 should not be indented, there should not be spaces at left. > Signed-off-by: Inju Song <inju.song@xxxxxxxxxxxxx> > --- > net/netfilter/ipvs/ip_vs_mh.c | 532 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 532 insertions(+) > create mode 100644 net/netfilter/ipvs/ip_vs_mh.c > > diff --git a/net/netfilter/ipvs/ip_vs_mh.c b/net/netfilter/ipvs/ip_vs_mh.c > new file mode 100644 > index 0000000..e531100 > --- /dev/null > +++ b/net/netfilter/ipvs/ip_vs_mh.c > @@ -0,0 +1,532 @@ > + IP_VS_DBG_BUF(6, > + "MH: selected unavailable server %s:%d (offset %d), reselecting", > + IP_VS_DBG_ADDR(dest->af, &dest->addr), > + ntohs(dest->port), roffset); As some vars are unsigned int, make sure all %d in debug messages are changed to %u, for consistency. This also includes all ports. I guess, these %d come from the SH scheduler. > +static void ip_vs_mh_state_free(struct rcu_head *head) > +{ > + struct ip_vs_mh_state *s; > + > + s = container_of(head, struct ip_vs_mh_state, rcu_head); > + kfree(s->lookup); > + kfree(s); > +} > + > +static int ip_vs_mh_init_svc(struct ip_vs_service *svc) > +{ > + struct ip_vs_mh_state *s; > + > + /* allocate the MH table for this service */ > + s = kzalloc(sizeof(*s), GFP_KERNEL); > + if (!s) > + return -ENOMEM; > + > + s->lookup = kcalloc(IP_VS_MH_TAB_SIZE, sizeof(struct ip_vs_mh_lookup), > + GFP_KERNEL); > + if (!s->lookup) { > + kfree(s); > + return -ENOMEM; > + } > + > + generate_hash_secret(&s->hash1, &s->hash2); > + s->gcd = ip_vs_mh_gcd_weight(svc); > + s->rshift = ip_vs_mh_shift_weight(svc, s->gcd); > + > + svc->sched_data = s; > + IP_VS_DBG(6, > + "MH lookup table (memory=%zdbytes) allocated for current service\n", > + sizeof(struct ip_vs_mh_lookup) * IP_VS_MH_TAB_SIZE); > + > + /* permutate & populate with current dests */ > + return ip_vs_mh_reassign(s, svc); Looks like we must not set svc->sched_data on error. We can do it like this: int ret; ... IP_VS_DBG() ret = ip_vs_mh_reassign(s, svc); if (ret < 0) { ip_vs_mh_reset(s); /* This is here for completeness */ ip_vs_mh_state_free(&s->rcu_head); return ret; } /* No more failures, attach state */ svc->sched_data = s; return 0; It should be ok because we use svc->sched_data only from entry points, the scheduler does not use it internally. > +} > + > +static void ip_vs_mh_done_svc(struct ip_vs_service *svc) > +{ > + struct ip_vs_mh_state *s = svc->sched_data; > + > + /* got to clean up hash buckets here */ > + ip_vs_mh_reset(s); > + > + /* release the table itself */ > + ip_vs_mh_state_free(&s->rcu_head); Above should be: call_rcu(&s->rcu_head, ip_vs_mh_state_free); As result, the RCU callback will be called after RCU grace period giving chance to RCU readers to stop using the svc->sched_data state. > + IP_VS_DBG(6, "MH lookup table (memory=%zdbytes) released\n", > + sizeof(struct ip_vs_mh_lookup) * IP_VS_MH_TAB_SIZE); Regards -- Julian Anastasov <ja@xxxxxx>
