OK. Do you want to post a new version of the series that adds your changes and acks?
On Fri, Jun 09, 2017 at 09:23:44PM +0000, Darrell Ball wrote: > I noticed the pre-existing hash function, conn_key_hash() was missing > hash_finish(), I added it locally and retested. > > > > On 6/8/17, 8:34 PM, "[email protected] on behalf of Darrell > Ball" <[email protected] on behalf of [email protected]> wrote: > > Acked-by: Darrell Ball [email protected] > > > > I added an incremental so we can use the new abstraction in the > pre-existing > > hash function – conn_key_hash(). I am not sure if we need a separate > patch ? > > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index 55029c8..d3e514d 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > @@ -1509,20 +1509,30 @@ conn_key_extract(struct conntrack *ct, struct > dp_packet *pkt, ovs_be16 dl_type, > > > > return false; > > } > > + > > +static uint32_t > > +ct_addr_hash_add(uint32_t hash, const struct ct_addr *addr) > > +{ > > + BUILD_ASSERT_DECL(sizeof *addr % 4 == 0); > > + return hash_add_bytes32(hash, (const uint32_t *) addr, sizeof *addr); > > +} > > + > > +static uint32_t > > +ct_endpoint_hash_add(uint32_t hash, const struct ct_endpoint *ep) > > +{ > > + BUILD_ASSERT_DECL(sizeof *ep % 4 == 0); > > + return hash_add_bytes32(hash, (const uint32_t *) ep, sizeof *ep); > > +} > > ^L > > /* Symmetric */ > > static uint32_t > > conn_key_hash(const struct conn_key *key, uint32_t basis) > > { > > uint32_t hsrc, hdst, hash; > > - int i; > > > > hsrc = hdst = basis; > > - > > - for (i = 0; i < sizeof(key->src) / sizeof(uint32_t); i++) { > > - hsrc = hash_add(hsrc, ((uint32_t *) &key->src)[i]); > > - hdst = hash_add(hdst, ((uint32_t *) &key->dst)[i]); > > - } > > + hsrc = ct_endpoint_hash_add(hsrc, &key->src); > > + hdst = ct_endpoint_hash_add(hdst, &key->dst); > > > > /* Even if source and destination are swapped the hash will be the > same. */ > > hash = hsrc ^ hdst; > > @@ -1613,20 +1623,6 @@ nat_ipv6_addr_increment(struct in6_addr > *ipv6_aligned, uint32_t increment) > > } > > > > static uint32_t > > -ct_addr_hash_add(uint32_t hash, const struct ct_addr *addr) > > -{ > > - BUILD_ASSERT_DECL(sizeof *addr % 4 == 0); > > - return hash_add_bytes32(hash, (const uint32_t *) addr, sizeof *addr); > > -} > > - > > -static uint32_t > > -ct_endpoint_hash_add(uint32_t hash, const struct ct_endpoint *ep) > > -{ > > - BUILD_ASSERT_DECL(sizeof *ep % 4 == 0); > > - return hash_add_bytes32(hash, (const uint32_t *) ep, sizeof *ep); > > -} > > - > > -static uint32_t > > nat_range_hash(const struct conn *conn, uint32_t basis) > > { > > uint32_t hash = basis; > > (END) > > > > > > > > > > On 6/7/17, 9:37 AM, "[email protected] on behalf of Ben > Pfaff" <[email protected] on behalf of [email protected]> wrote: > > > > From: Darrell Ball <[email protected]> > > > > Part of the hash input for nat_range_hash() was accidentally > > omitted, so this fixes the problem. Also, add a missing call to > > hash_finish(). > > > > Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT > Support.") > > Signed-off-by: Darrell Ball <[email protected]> > > Signed-off-by: Ben Pfaff <[email protected]> > > --- > > lib/conntrack-private.h | 4 ++++ > > lib/conntrack.c | 47 > ++++++++++++++++++++++++++--------------------- > > 2 files changed, 30 insertions(+), 21 deletions(-) > > > > diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h > > index bfa88f0fe7e2..55084d3ea64e 100644 > > --- a/lib/conntrack-private.h > > +++ b/lib/conntrack-private.h > > @@ -42,6 +42,10 @@ struct ct_endpoint { > > }; > > }; > > > > +/* Verify that there is no padding in struct ct_endpoint, to > facilitate > > + * hashing in ct_endpoint_hash_add(). */ > > +BUILD_ASSERT_DECL(sizeof(struct ct_endpoint) == sizeof(struct > ct_addr) + 4); > > + > > /* Changes to this structure need to be reflected in conn_key_hash() > */ > > struct conn_key { > > struct ct_endpoint src; > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index 44a6bc4e3ccf..55029c82e7a6 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > @@ -1613,36 +1613,41 @@ nat_ipv6_addr_increment(struct in6_addr > *ipv6_aligned, uint32_t increment) > > } > > > > static uint32_t > > -nat_range_hash(const struct conn *conn, uint32_t basis) > > +ct_addr_hash_add(uint32_t hash, const struct ct_addr *addr) > > { > > - uint32_t hash = basis; > > - int i; > > - uint16_t port; > > + BUILD_ASSERT_DECL(sizeof *addr % 4 == 0); > > + return hash_add_bytes32(hash, (const uint32_t *) addr, sizeof > *addr); > > +} > > > > - for (i = 0; > > - i < sizeof(conn->nat_info->min_addr) / sizeof(uint32_t); > > - i++) { > > - hash = hash_add(hash, ((uint32_t *) > &conn->nat_info->min_addr)[i]); > > - hash = hash_add(hash, ((uint32_t *) > &conn->nat_info->max_addr)[i]); > > - } > > +static uint32_t > > +ct_endpoint_hash_add(uint32_t hash, const struct ct_endpoint *ep) > > +{ > > + BUILD_ASSERT_DECL(sizeof *ep % 4 == 0); > > + return hash_add_bytes32(hash, (const uint32_t *) ep, sizeof *ep); > > +} > > > > - memcpy(&port, &conn->nat_info->min_port, sizeof port); > > - hash = hash_add(hash, port); > > +static uint32_t > > +nat_range_hash(const struct conn *conn, uint32_t basis) > > +{ > > + uint32_t hash = basis; > > > > - for (i = 0; i < sizeof(conn->key.src.addr) / sizeof(uint32_t); > i++) { > > - hash = hash_add(hash, ((uint32_t *) &conn->key.src)[i]); > > - hash = hash_add(hash, ((uint32_t *) &conn->key.dst)[i]); > > - } > > + hash = ct_addr_hash_add(hash, &conn->nat_info->min_addr); > > + hash = ct_addr_hash_add(hash, &conn->nat_info->max_addr); > > + hash = hash_add(hash, > > + (conn->nat_info->max_port << 16) > > + | conn->nat_info->min_port); > > > > - memcpy(&port, &conn->key.src.port, sizeof port); > > - hash = hash_add(hash, port); > > - memcpy(&port, &conn->key.dst.port, sizeof port); > > - hash = hash_add(hash, port); > > + hash = ct_endpoint_hash_add(hash, &conn->key.src); > > + hash = ct_endpoint_hash_add(hash, &conn->key.dst); > > > > hash = hash_add(hash, (OVS_FORCE uint32_t) conn->key.dl_type); > > hash = hash_add(hash, conn->key.nw_proto); > > hash = hash_add(hash, conn->key.zone); > > - return hash; > > + > > + /* The purpose of the second parameter is to distinguish hashes > of data of > > + * different length; our data always has the same length so > there is no > > + * value in counting. */ > > + return hash_finish(hash, 0); > > } > > > > static bool > > > > -- > > 2.10.2 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=0-D9j1lxPv8FuvIlzvLsBxXMvnqlIWhS_O_iGeh0O_E&s=DMIKKrfIU2O9SgvyTV0Yvs6CSu3pnRc6u4-ZBL_pRho&e= > > > > > > > _______________________________________________ > dev mailing list > [email protected] > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=0-D9j1lxPv8FuvIlzvLsBxXMvnqlIWhS_O_iGeh0O_E&s=DMIKKrfIU2O9SgvyTV0Yvs6CSu3pnRc6u4-ZBL_pRho&e= > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
