Hello, Thank you for your comments. On Sat, Mar 24, 2018 at 01:26:04PM +0200, Julian Anastasov wrote: > > 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. > ok. I will remove indentions in patch 1 and 3. > > 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. > Yes, these printing format was from SH scheduler.. I will fix these properly. > > +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. > OK. Above case will be handled. > > +} > > + > > +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. > Ok, I missed adding to schedule rcu callback for freeing the state. It will be fixed. Thanks. > > + IP_VS_DBG(6, "MH lookup table (memory=%zdbytes) released\n", > > + sizeof(struct ip_vs_mh_lookup) * IP_VS_MH_TAB_SIZE); > Regards -- Inju Song NAVER Corporation -- To unsubscribe from this list: send the line "unsubscribe lvs-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
