[ovs-dev] [PATCH] conntrack: compute hash value once when calling conn_key_lookup

[Darrell Ball]

Thanks for looking

On Fri, Jan 18, 2019 at 1:47 AM Li RongQing <lirongqing at baidu.com> wrote:

> it is expensive to compute hash value, and When call conn_lookup,
> hash value is computed twice, in fact, we can cache it and pass
> it to conn_lookup
>
> Signed-off-by: Zhang Yu <zhangyu31 at baidu.com>
> Signed-off-by: Li RongQing <lirongqing at baidu.com>
> ---
>  lib/conntrack.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 11a1e05bd..f01c2ceac 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -743,12 +743,13 @@ un_nat_packet(struct dp_packet *pkt, const struct
> conn *conn,
>   * and a hash would have already been needed. Hence, this function
>   * is just intended for code clarity. */
>  static struct conn *
> -conn_lookup(struct conntrack *ct, const struct conn_key *key, long long
> now)
> +conn_lookup(struct conntrack *ct, const struct conn_key *key, long long
> now,
>

The function comment says it all:
/* Typical usage of this helper is in non per-packet code;
 * this is because the bucket lock needs to be held for lookup
 * and a hash would have already been needed. Hence, this function
 * is just intended for code clarity. */


> +            uint32_t hash)
>  {
>      struct conn_lookup_ctx ctx;
>      ctx.conn = NULL;
>      ctx.key = *key;
> -    ctx.hash = conn_key_hash(key, ct->hash_basis);
> +    ctx.hash = hash;
>      unsigned bucket = hash_to_bucket(ctx.hash);
>      conn_key_lookup(&ct->buckets[bucket], &ctx, now);
>      return ctx.conn;
> @@ -758,9 +759,10 @@ static void
>  conn_seq_skew_set(struct conntrack *ct, const struct conn_key *key,
>                    long long now, int seq_skew, bool seq_skew_dir)
>  {
> -    unsigned bucket = hash_to_bucket(conn_key_hash(key, ct->hash_basis));
> +    uint32_t hash = conn_key_hash(key, ct->hash_basis);
> +    unsigned bucket = hash_to_bucket(hash);
>      ct_lock_lock(&ct->buckets[bucket].lock);
> -    struct conn *conn = conn_lookup(ct, key, now);
> +    struct conn *conn = conn_lookup(ct, key, now, hash);
>      if (conn && seq_skew) {
>          conn->seq_skew = seq_skew;
>          conn->seq_skew_dir = seq_skew_dir;
> @@ -777,12 +779,13 @@ nat_clean(struct conntrack *ct, struct conn *conn,
>      nat_conn_keys_remove(&ct->nat_conn_keys, &conn->rev_key,
> ct->hash_basis);
>      ct_rwlock_unlock(&ct->resources_lock);
>      ct_lock_unlock(&ctb->lock);
> -    unsigned bucket_rev_conn =
> -        hash_to_bucket(conn_key_hash(&conn->rev_key, ct->hash_basis));
> +
> +    uint32_t hash = conn_key_hash(&conn->rev_key, ct->hash_basis);
> +    unsigned bucket_rev_conn = hash_to_bucket(hash);
>      ct_lock_lock(&ct->buckets[bucket_rev_conn].lock);
>      ct_rwlock_wrlock(&ct->resources_lock);
>      long long now = time_msec();
> -    struct conn *rev_conn = conn_lookup(ct, &conn->rev_key, now);
> +    struct conn *rev_conn = conn_lookup(ct, &conn->rev_key, now, hash);
>      struct nat_conn_key_node *nat_conn_key_node =
>          nat_conn_keys_lookup(&ct->nat_conn_keys, &conn->rev_key,
>                               ct->hash_basis);
> @@ -1016,7 +1019,7 @@ create_un_nat_conn(struct conntrack *ct, struct conn
> *conn_for_un_nat_copy,
>      uint32_t un_nat_hash = conn_key_hash(&nc->key, ct->hash_basis);
>      unsigned un_nat_conn_bucket = hash_to_bucket(un_nat_hash);
>      ct_lock_lock(&ct->buckets[un_nat_conn_bucket].lock);
> -    struct conn *rev_conn = conn_lookup(ct, &nc->key, now);
> +    struct conn *rev_conn = conn_lookup(ct, &nc->key, now, un_nat_hash);
>
>      if (alg_un_nat) {
>          if (!rev_conn) {
> --
> 2.16.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>