[ovs-dev] [PATCH v5 6/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

Yi-Hung Wei yihung.wei at gmail.com
Mon Sep 16 20:36:53 UTC 2019


On Fri, Sep 13, 2019 at 5:33 PM Justin Pettit <jpettit at ovn.org> wrote:
>
>
> > On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei <yihung.wei at gmail.com> wrote:
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 751535249e21..4b4c4d722645 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> >
> > +static void
> > +ct_timeout_policy_destroy(struct ct_timeout_policy *ct_tp,
> > +                          struct id_pool *tp_ids)
> > +{
> > +    id_pool_free_id(tp_ids, ct_tp->tp_id);
> > +    simap_destroy(&ct_tp->tp);
> > +    ovsrcu_postpone(free, ct_tp);
> > +}
>
> I assume we're delaying the freeing of 'ct_tp' in case there are people that still have references to it.  Should id_pool_free_id() and simap_destroy() be called from ovsrcu_postpone()?  (I didn't propose a change for this.)

Yes, simap_destroy() should be called from ovsrcu_postpone(). I will
fix that in the next version with the following diff.

For id_pool_free_id(), I think it is fine to keep that in
ct_timeout_policy_destroy().  Currently, we free the timeout policy id
from the pool only when all of the sub timeout policies are deleted in
the datapath.  Therefore, it should be fine to reuse the id.

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index fc970074ced2..910f415dfea0 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5222,12 +5222,18 @@ ct_timeout_policy_alloc(struct simap *tp,
struct id_pool *tp_ids)
 }

 static void
+ct_timeout_policy_destroy__(struct ct_timeout_policy *ct_tp)
+{
+    simap_destroy(&ct_tp->tp);
+    free(ct_tp);
+}
+
+static void
 ct_timeout_policy_destroy(struct ct_timeout_policy *ct_tp,
                           struct id_pool *tp_ids)
 {
     id_pool_free_id(tp_ids, ct_tp->tp_id);
-    simap_destroy(&ct_tp->tp);
-    ovsrcu_postpone(free, ct_tp);
+    ovsrcu_postpone(ct_timeout_policy_destroy__, ct_tp);
 }


> > +static void
> > +ct_set_zone_timeout_policy(const char *datapath_type, uint16_t zone,
> > +                           struct simap *timeout_policy)
> > +{
> > +    struct dpif_backer *backer = shash_find_data(&all_dpif_backers,
> > +                                                 datapath_type);
> > +    if (!backer) {
> > +        return;
> > +    }
> > +
> > +    struct ct_timeout_policy *ct_tp = ct_timeout_policy_lookup(&backer->ct_tps,
> > +                                                               timeout_policy);
> > +    if (!ct_tp) {
> > +        ct_tp = ct_timeout_policy_alloc(timeout_policy, backer->tp_ids);
> > +        if (ct_tp) {
> > +            hmap_insert(&backer->ct_tps, &ct_tp->node, simap_hash(&ct_tp->tp));
> > +            ct_add_timeout_policy_to_dpif(backer->dpif, ct_tp);
> > +        } else {
> > +            VLOG_ERR_RL(&rl, "failed to allocate timeout policy");
>
> The call to ct_timeout_policy_alloc() will already log this error message if there was a problem.
Sounds good.


> > +            return;
> > +        }
> > +    }
> > +
> > +    struct ct_zone *ct_zone = ct_zone_lookup(&backer->ct_zones, zone);
> > +    if (ct_zone) {
> > +        if (ct_zone->ct_tp != ct_tp) {
> > +            /* Add the new zone timeout pollicy. */
> > +            struct ct_zone *new_ct_zone = ct_zone_alloc(zone);
> > +            new_ct_zone->ct_tp = ct_tp;
> > +            ct_tp->ref_count++;
> > +            cmap_replace(&backer->ct_zones, &ct_zone->node, &new_ct_zone->node,
> > +                         hash_int(zone, 0));
> > +
> > +            /* Deletes the old zone timeout policy. */
> > +            ct_timeout_policy_unref(backer, ct_zone->ct_tp);
> > +            ct_zone_destroy(ct_zone);
>
> Is there a reason that a new ct_zone needs to be allocated anew?  Couldn't you just do something like the following?

Thanks, I think it is unnecessary to allocate a new ct_zone. The new
approach looks good to me.

> > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> > index 6cc454371dc7..d53c8e2f80a4 100644
> > --- a/ofproto/ofproto-provider.h
> > +++ b/ofproto/ofproto-provider.h
> > @@ -58,6 +58,7 @@
> > #include "tun-metadata.h"
> > #include "versions.h"
> > #include "vl-mff-map.h"
> > +#include "vswitch-idl.h"
>
> I don't think this #include is necessary with these changes.

Oops, I forget to take this line out in this version.  Thanks for
pointing this out.


> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index d921c4ef8d5f..c43f2ac34e67 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -153,9 +153,36 @@ struct aa_mapping {
> >     char *br_name;
> > };
> >
> > +/* Internal representation of conntrack zone configuration table in OVSDB. */
> > +struct ct_zone {
> > +    uint16_t zone;
>
> I think 'zone_id' is a clearer name for what this is.  I'd also suggest using that through bridge.c.

Yes, it is much clearer to replace 'zone' with 'zone_id' and the other
suggested naming changes in the following. Thanks for the diff.


> > +/* All datapath configuartions, indexed by type. */
> > +static struct hmap all_datapaths = HMAP_INITIALIZER(&all_datapaths);
>
> I don't think it's necessary to use an hmap to store the datapaths, since the usual case will be a single datapath and at most two.  The hmap code special-cases a one-node hmap, so there won't be too much overhead usually, but there is some unnecessary complication and hashing on every lookup.  I'll send out a patch later to switch it to a linked list, and we can see how it compares.

Agreed. It makes more sense to use linked list than hamp.

> > +/* Replace 'old_tp' by 'new_tp' (destroyed 'new_tp'). Returns true if 'old_tp'
> > + * and 'new_tp' contains different data, false if they are the same. */
> > +static bool
> > +update_timeout_policy(struct simap *old_tp, struct simap *new_tp)
> > +{
> > +    bool changed = !simap_equal(old_tp, new_tp);
> > +    simap_swap(old_tp, new_tp);
> > +    simap_destroy(new_tp);
> > +    return changed;
> > +}
>
> It seems like unnecessary work to always call simap_swap().  What about only doing it they're changed?
Sounds good.


> > +static void
> > +reconfigure_datapath_cfgs(const struct ovsrec_open_vswitch *cfg)
>
> I think it would be a bit more consistent to call this datapath_reconfigure() to match bridge_reconfigure().
>
> I agree with Darrell's follow-on patch to this function not to depend "dp_cfg" between database runs.
>
> I would recommend calling datapath_destroy() on all the datapaths on bridge_exit().  It's not strictly necessary, but it makes it easier to track down memory leaks if things are cleaned up as much as possible.

Make sense. Thanks.


> Thanks for the patch!  Let me know what you think about the questions I posed above and the incremental below.

The incremental looks good to me. Thanks for review and the diff!

-Yi-Hung


More information about the dev mailing list