Sure, on the way.
On 6/9/17, 2:35 PM, "Ben Pfaff" <[email protected]> wrote:
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
