[ovs-dev] [patch v3 2/5] conntrack: Fix alg expectation cleanup.

Darrell Ball dball at vmware.com
Tue Jan 9 21:04:42 UTC 2018



On 1/9/18, 12:56 PM, "ovs-dev-bounces at openvswitch.org on behalf of Ben Pfaff" <ovs-dev-bounces at openvswitch.org on behalf of blp at ovn.org> wrote:

    On Mon, Jan 08, 2018 at 12:54:26PM -0800, Darrell Ball wrote:
    > Presently, alg expectations are removed by being time expired.
    > This was intended to happen before the control connections and
    > was intended to minimize the extra work involved for tracking and
    > removing the expectations.  This is not the best option since it
    > should be possible to remove expectations when a control connection
    > is removed and a new api is in the works to do this. Also, conceptually
    > an expectation should not exist without a control connection context
    > and it can be argued that this should be a strict requirement.
    > 
    > The approach is changed to remove the expectations when the control
    > connections are removed.  The previous code to expire the expectations
    > is removed at the same time.
    > 
    > Fixes: bd5e81a0e ("Userspace Datapath: Add ALG infra and FTP.")
    > Reported-at: https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2017-2DDecember_341683.html&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=awfUaVLDUeHBvrjln1l6lBzZT23Md8yY6AWrPkdOj-E&s=JZqA8YpWWgHp7BIAD_o_Y28w-JfcpHB_r6lgwa-C4iE&e=
    > 
    > Signed-off-by: Darrell Ball <dlu998 at gmail.com>
    > ---
    > 
    > v2->v3: Use hindex map in lieu of hmap for efficiency: Ben P.
    
    I think that the open-coded iteration in expectation_clean() can be more
    simply rewritten, like this:
    
    diff --git a/lib/conntrack.c b/lib/conntrack.c
    index a14b66c16f56..c5ef42c585cf 100644
    --- a/lib/conntrack.c
    +++ b/lib/conntrack.c
    @@ -2622,50 +2622,19 @@ expectation_ref_create(struct hindex *alg_expectation_refs,
         }
     }
     
    -/* This function must be called with the ct->resources read lock taken. */
    -static struct alg_exp_node *
    -expectation_ref_lookup(const struct hindex *alg_expectation_refs,
    -                       const struct conn_key *master_key,
    -                       uint32_t basis)
    -{
    -    uint32_t alg_exp_conn_key_hash = conn_key_hash(master_key, basis);
    -    struct hindex_node *hindex_node =
    -        hindex_node_with_hash(alg_expectation_refs, alg_exp_conn_key_hash);
    -    if (hindex_node) {
    -        struct alg_exp_node *alg_exp_node =
    -            CONTAINER_OF(hindex_node, struct alg_exp_node, node_ref);
    -        return alg_exp_node;
    -    }
    -
    -    return NULL;
    -}
    -
     static void
     expectation_clean(struct conntrack *ct, const struct conn_key *master_key,
                       uint32_t basis)
     {
         ct_rwlock_wrlock(&ct->resources_lock);
    -    struct alg_exp_node *alg_exp_node =
    -        expectation_ref_lookup(&ct->alg_expectation_refs, master_key, basis);
     
    -    if (alg_exp_node) {
    -        expectation_remove(&ct->alg_expectations, &alg_exp_node->key, basis);
    -        hindex_remove(&ct->alg_expectation_refs, &alg_exp_node->node_ref);
    -        struct hindex_node *next_hindex_node =
    -            hindex_next_node_with_hash(&alg_exp_node->node_ref);
    -        free(alg_exp_node);
    -        struct hindex_node *hindex_node = next_hindex_node;
    -
    -        while (hindex_node) {
    -            next_hindex_node = hindex_next_node_with_hash(hindex_node);
    -            struct alg_exp_node *alg_exp_node =
    -                CONTAINER_OF(hindex_node, struct alg_exp_node, node_ref);
    -            expectation_remove(&ct->alg_expectations, &alg_exp_node->key,
    -                               basis);
    -            hindex_remove(&ct->alg_expectation_refs, hindex_node);
    -            free(alg_exp_node);
    -            hindex_node = next_hindex_node;
    -        }
    +    struct alg_exp_node *node, *next;
    +    HINDEX_FOR_EACH_WITH_HASH_SAFE (node, next, node_ref,
    +                                    conn_key_hash(master_key, basis),
    +                                    &ct->alg_expectation_refs) {
    +        expectation_remove(&ct->alg_expectations, &node->key, basis);
    +        hindex_remove(&ct->alg_expectation_refs, &node->node_ref);
    +        free(node);
         }
         ct_rwlock_unlock(&ct->resources_lock);
     }
    
    With this rewrite, patch 1 adding hindex_next_node_with_hash isn't
    needed.
    
    However, this loop (in either version) doesn't do any kind of comparison
    of a full key; it only matches on the hash value.  That seems wrong.  Is
    the lack of a key comparison intentional and correct?

It is a bug; good catch; thanks; the switchover I did to hindex map was a bit careless.
I’ll fix and resend.

    _______________________________________________
    dev mailing list
    dev at openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=awfUaVLDUeHBvrjln1l6lBzZT23Md8yY6AWrPkdOj-E&s=TXVGC6wTzmqGag8Ssr3bAX7o9LAtjL5G2qEJezyxnCY&e=
    



More information about the dev mailing list