From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Ricardo Leitner Subject: Re: [PATCH net-next 1/5] sctp: add the rhashtable apis for sctp global transport hashtable Date: Thu, 7 Jan 2016 15:23:24 -0200 Message-ID: <20160107172324.GA6074@mrl.redhat.com> References: <2c413161eb3a2c74453bd8dc11815af7e237515e.1451490447.git.lucien.xin@gmail.com> <568C0DA3.1090604@gmail.com> <20160106181950.GE6061@mrl.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Xin Long , network dev , linux-sctp@vger.kernel.org, Vlad Yasevich , daniel@iogearbox.net, davem To: Vlad Yasevich Return-path: Received: from mx1.redhat.com ([209.132.183.28]:45263 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751941AbcAGRX3 (ORCPT ); Thu, 7 Jan 2016 12:23:29 -0500 Content-Disposition: inline In-Reply-To: <20160106181950.GE6061@mrl.redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On Wed, Jan 06, 2016 at 04:19:50PM -0200, Marcelo Ricardo Leitner wrote: > On Thu, Jan 07, 2016 at 01:01:11AM +0800, Xin Long wrote: > > On Wed, Jan 6, 2016 at 2:38 AM, Vlad Yasevich wrote: > > > On 12/30/2015 10:50 AM, Xin Long wrote: > > >> tranport hashtbale will replace the association hashtable to do the > > >> lookup for transport, and then get association by t->assoc, rhashtable > > >> apis will be used because of it's resizable, scalable and using rcu. > > >> > > >> lport + rport + paddr will be the base hashkey to locate the chain, > > >> with net to protect one netns from another, then plus the laddr to > > >> compare to get the target. > > >> > > >> this patch will provider the lookup functions: > > >> - sctp_epaddr_lookup_transport > > >> - sctp_addrs_lookup_transport > > >> > > >> hash/unhash functions: > > >> - sctp_hash_transport > > >> - sctp_unhash_transport > > >> > > >> init/destroy functions: > > >> - sctp_transport_hashtable_init > > >> - sctp_transport_hashtable_destroy > > >> > > >> Signed-off-by: Xin Long > > >> Signed-off-by: Marcelo Ricardo Leitner > > >> --- > > >> include/net/sctp/sctp.h | 11 ++++ > > >> include/net/sctp/structs.h | 5 ++ > > >> net/sctp/input.c | 131 +++++++++++++++++++++++++++++++++++++++++++++ > > >> 3 files changed, 147 insertions(+) > > >> > > >> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h > > >> index ce13cf2..7bbdfba 100644 > > >> --- a/include/net/sctp/sctp.h > > >> +++ b/include/net/sctp/sctp.h > > >> @@ -143,6 +143,17 @@ void sctp_icmp_proto_unreachable(struct sock *sk, > > >> struct sctp_transport *t); > > >> void sctp_backlog_migrate(struct sctp_association *assoc, > > >> struct sock *oldsk, struct sock *newsk); > > >> +int sctp_transport_hashtable_init(void); > > >> +void sctp_transport_hashtable_destroy(void); > > >> +void sctp_hash_transport(struct sctp_transport *t); > > >> +void sctp_unhash_transport(struct sctp_transport *t); > > >> +struct sctp_transport *sctp_addrs_lookup_transport( > > >> + struct net *net, > > >> + const union sctp_addr *laddr, > > >> + const union sctp_addr *paddr); > > >> +struct sctp_transport *sctp_epaddr_lookup_transport( > > >> + const struct sctp_endpoint *ep, > > >> + const union sctp_addr *paddr); > > >> > > >> /* > > >> * sctp/proc.c > > >> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h > > >> index eea9bde..4ab87d0 100644 > > >> --- a/include/net/sctp/structs.h > > >> +++ b/include/net/sctp/structs.h > > >> @@ -48,6 +48,7 @@ > > >> #define __sctp_structs_h__ > > >> > > >> #include > > >> +#include > > >> #include /* linux/in.h needs this!! */ > > >> #include /* We get struct sockaddr_in. */ > > >> #include /* We get struct in6_addr */ > > >> @@ -123,6 +124,8 @@ extern struct sctp_globals { > > >> struct sctp_hashbucket *assoc_hashtable; > > >> /* This is the sctp port control hash. */ > > >> struct sctp_bind_hashbucket *port_hashtable; > > >> + /* This is the hash of all transports. */ > > >> + struct rhashtable transport_hashtable; > > >> > > >> /* Sizes of above hashtables. */ > > >> int ep_hashsize; > > >> @@ -147,6 +150,7 @@ extern struct sctp_globals { > > >> #define sctp_assoc_hashtable (sctp_globals.assoc_hashtable) > > >> #define sctp_port_hashsize (sctp_globals.port_hashsize) > > >> #define sctp_port_hashtable (sctp_globals.port_hashtable) > > >> +#define sctp_transport_hashtable (sctp_globals.transport_hashtable) > > >> #define sctp_checksum_disable (sctp_globals.checksum_disable) > > >> > > >> /* SCTP Socket type: UDP or TCP style. */ > > >> @@ -753,6 +757,7 @@ static inline int sctp_packet_empty(struct sctp_packet *packet) > > >> struct sctp_transport { > > >> /* A list of transports. */ > > >> struct list_head transports; > > >> + struct rhash_head node; > > >> > > >> /* Reference counting. */ > > >> atomic_t refcnt; > > >> diff --git a/net/sctp/input.c b/net/sctp/input.c > > >> index b6493b3..bac8278 100644 > > >> --- a/net/sctp/input.c > > >> +++ b/net/sctp/input.c > > >> @@ -782,6 +782,137 @@ hit: > > >> return ep; > > >> } > > >> > > >> +/* rhashtable for transport */ > > >> +struct sctp_hash_cmp_arg { > > >> + const union sctp_addr *laddr; > > >> + const union sctp_addr *paddr; > > >> + const struct net *net; > > >> +}; > > >> + > > >> +static inline int sctp_hash_cmp(struct rhashtable_compare_arg *arg, > > >> + const void *ptr) > > >> +{ > > >> + const struct sctp_hash_cmp_arg *x = arg->key; > > >> + const struct sctp_transport *t = ptr; > > >> + struct sctp_association *asoc = t->asoc; > > >> + const struct net *net = x->net; > > >> + > > >> + if (x->laddr->v4.sin_port != htons(asoc->base.bind_addr.port)) > > >> + return 1; > > >> + if (!sctp_cmp_addr_exact(&t->ipaddr, x->paddr)) > > >> + return 1; > > >> + if (!net_eq(sock_net(asoc->base.sk), net)) > > >> + return 1; > > >> + if (!sctp_bind_addr_match(&asoc->base.bind_addr, > > >> + x->laddr, sctp_sk(asoc->base.sk))) > > >> + return 1; > > >> + > > >> + return 0; > > >> +} > > >> + > > >> +static inline u32 sctp_hash_obj(const void *data, u32 len, u32 seed) > > >> +{ > > >> + const struct sctp_transport *t = data; > > >> + const union sctp_addr *paddr = &t->ipaddr; > > >> + const struct net *net = sock_net(t->asoc->base.sk); > > >> + u16 lport = htons(t->asoc->base.bind_addr.port); > > >> + u32 addr; > > >> + > > >> + if (paddr->sa.sa_family == AF_INET6) > > >> + addr = jhash(&paddr->v6.sin6_addr, 16, seed); > > >> + else > > >> + addr = paddr->v4.sin_addr.s_addr; > > >> + > > >> + return jhash_3words(addr, ((__u32)paddr->v4.sin_port) << 16 | > > >> + (__force __u32)lport, net_hash_mix(net), seed); > > >> +} > > >> + > > >> +static inline u32 sctp_hash_key(const void *data, u32 len, u32 seed) > > >> +{ > > >> + const struct sctp_hash_cmp_arg *x = data; > > >> + const union sctp_addr *paddr = x->paddr; > > >> + const struct net *net = x->net; > > >> + u16 lport = x->laddr->v4.sin_port; > > >> + u32 addr; > > >> + > > >> + if (paddr->sa.sa_family == AF_INET6) > > >> + addr = jhash(&paddr->v6.sin6_addr, 16, seed); > > >> + else > > >> + addr = paddr->v4.sin_addr.s_addr; > > >> + > > >> + return jhash_3words(addr, ((__u32)paddr->v4.sin_port) << 16 | > > >> + (__force __u32)lport, net_hash_mix(net), seed); > > >> +} > > >> + > > >> +static const struct rhashtable_params sctp_hash_params = { > > >> + .head_offset = offsetof(struct sctp_transport, node), > > >> + .hashfn = sctp_hash_key, > > >> + .obj_hashfn = sctp_hash_obj, > > >> + .obj_cmpfn = sctp_hash_cmp, > > >> + .automatic_shrinking = true, > > >> +}; > > >> + > > >> +int sctp_transport_hashtable_init(void) > > >> +{ > > >> + return rhashtable_init(&sctp_transport_hashtable, &sctp_hash_params); > > >> +} > > >> + > > >> +void sctp_transport_hashtable_destroy(void) > > >> +{ > > >> + rhashtable_destroy(&sctp_transport_hashtable); > > >> +} > > >> + > > >> +void sctp_hash_transport(struct sctp_transport *t) > > >> +{ > > >> + struct sctp_sockaddr_entry *addr; > > >> + struct sctp_hash_cmp_arg arg; > > >> + > > >> + addr = list_entry(t->asoc->base.bind_addr.address_list.next, > > >> + struct sctp_sockaddr_entry, list); > > >> + arg.laddr = &addr->a; > > >> + arg.paddr = &t->ipaddr; > > >> + arg.net = sock_net(t->asoc->base.sk); > > >> + > > >> +reinsert: > > >> + if (rhashtable_lookup_insert_key(&sctp_transport_hashtable, &arg, > > >> + &t->node, sctp_hash_params) == -EBUSY) > > >> + goto reinsert; > > >> +} > > >> + > > >> +void sctp_unhash_transport(struct sctp_transport *t) > > >> +{ > > >> + rhashtable_remove_fast(&sctp_transport_hashtable, &t->node, > > >> + sctp_hash_params); > > >> +} > > >> + > > >> +struct sctp_transport *sctp_addrs_lookup_transport( > > >> + struct net *net, > > >> + const union sctp_addr *laddr, > > >> + const union sctp_addr *paddr) > > >> +{ > > >> + struct sctp_hash_cmp_arg arg = { > > >> + .laddr = laddr, > > >> + .paddr = paddr, > > >> + .net = net, > > >> + }; > > >> + > > >> + return rhashtable_lookup_fast(&sctp_transport_hashtable, &arg, > > >> + sctp_hash_params); > > >> +} > > >> + > > >> +struct sctp_transport *sctp_epaddr_lookup_transport( > > >> + const struct sctp_endpoint *ep, > > >> + const union sctp_addr *paddr) > > >> +{ > > >> + struct sctp_sockaddr_entry *addr; > > >> + struct net *net = sock_net(ep->base.sk); > > >> + > > >> + addr = list_entry(ep->base.bind_addr.address_list.next, > > >> + struct sctp_sockaddr_entry, list); > > >> + > > >> + return sctp_addrs_lookup_transport(net, &addr->a, paddr); > > >> +} > > >> + > > > > > > I don't think that this right, mainly because not all endpoint > > > addresses will be copied to association bind_addr list. As a result, > > > you may actually have an association on this endpoint to a given > > > peer, but may not be using the first address from the endpoint.. > > That shouldn't be a problem, as laddr is not considered in the hash > function and later on sctp_hash_cmp() will use sctp_bind_addr_match() as > below, which will traverse asoc->base.bind_addr in this case, meaning it > would still see all transport/asoc possibilities. > > Or did I miss something? I did.. Xin kindly explained the issue to me offline. Yup, this is an issue, we will work on it. Thanks