diff mbox

[ovs-dev,2/2] conntrack: Hash entire NAT data structure in nat_range_hash().

Message ID 89DBA58A-EDDC-4E49-AD35-302FD6B6AB73@vmware.com
State Not Applicable
Headers show

Commit Message

Darrell Ball June 9, 2017, 3:34 a.m. UTC
Acked-by: Darrell Ball dlu998@gmail.com


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 ?

(END)




On 6/7/17, 9:37 AM, "ovs-dev-bounces@openvswitch.org on behalf of Ben Pfaff" <ovs-dev-bounces@openvswitch.org on behalf of blp@ovn.org> wrote:

    From: Darrell Ball <dlu998@gmail.com>

    
    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 <dlu998@gmail.com>

    Signed-off-by: Ben Pfaff <blp@ovn.org>

    ---
     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
    dev@openvswitch.org
    https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Comments

Darrell Ball June 9, 2017, 9:23 p.m. UTC | #1
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, "ovs-dev-bounces@openvswitch.org on behalf of Darrell Ball" <ovs-dev-bounces@openvswitch.org on behalf of dball@vmware.com> wrote:

    Acked-by: Darrell Ball dlu998@gmail.com

    
    
    
    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, "ovs-dev-bounces@openvswitch.org on behalf of Ben Pfaff" <ovs-dev-bounces@openvswitch.org on behalf of blp@ovn.org> wrote:
    
    
    
        From: Darrell Ball <dlu998@gmail.com>

    
        
    
        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 <dlu998@gmail.com>

    
        Signed-off-by: Ben Pfaff <blp@ovn.org>

    
        ---
    
         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
    
        dev@openvswitch.org
    
        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
    dev@openvswitch.org
    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=
Ben Pfaff June 9, 2017, 9:35 p.m. UTC | #2
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, "ovs-dev-bounces@openvswitch.org on behalf of Darrell Ball" <ovs-dev-bounces@openvswitch.org on behalf of dball@vmware.com> wrote:
> 
>     Acked-by: Darrell Ball dlu998@gmail.com
>     
>     
>     
>     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, "ovs-dev-bounces@openvswitch.org on behalf of Ben Pfaff" <ovs-dev-bounces@openvswitch.org on behalf of blp@ovn.org> wrote:
>     
>     
>     
>         From: Darrell Ball <dlu998@gmail.com>
>     
>         
>     
>         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 <dlu998@gmail.com>
>     
>         Signed-off-by: Ben Pfaff <blp@ovn.org>
>     
>         ---
>     
>          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
>     
>         dev@openvswitch.org
>     
>         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
>     dev@openvswitch.org
>     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= 
>     
>
Darrell Ball June 9, 2017, 9:36 p.m. UTC | #3
Sure, on the way.

On 6/9/17, 2:35 PM, "Ben Pfaff" <blp@ovn.org> 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, "ovs-dev-bounces@openvswitch.org on behalf of Darrell Ball" <ovs-dev-bounces@openvswitch.org on behalf of dball@vmware.com> wrote:

    > 

    >     Acked-by: Darrell Ball dlu998@gmail.com

    >     

    >     

    >     

    >     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, "ovs-dev-bounces@openvswitch.org on behalf of Ben Pfaff" <ovs-dev-bounces@openvswitch.org on behalf of blp@ovn.org> wrote:

    >     

    >     

    >     

    >         From: Darrell Ball <dlu998@gmail.com>

    >     

    >         

    >     

    >         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 <dlu998@gmail.com>

    >     

    >         Signed-off-by: Ben Pfaff <blp@ovn.org>

    >     

    >         ---

    >     

    >          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

    >     

    >         dev@openvswitch.org

    >     

    >         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

    >     dev@openvswitch.org

    >     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= 

    >     

    >
diff mbox

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;