Thanks Ryan, it is pretty close. Please see my comments below:
One high level comment is that, we assume new_xcfg is non-null. Could we
add assertion to the
external functions? e.g. xlate_ofport_remove()
>
@@ -324,10 +332,76 @@ static bool dscp_from_skb_priority(const struct xport
> *, uint32_t skb_priority,
>
> static struct xc_entry *xlate_cache_add_entry(struct xlate_cache *xc,
> enum xc_type type);
> +static void xlate_xbridge_init(struct xbridge *);
> +static void xlate_xbundle_init(struct xbundle *);
> +static void xlate_xport_init(struct xport *);
>
Could we add the xlate_cfg as input argument here? Even though we always
insert to the new_xcfg,
I think adding it makes it clear.
> @@ -339,17 +413,6 @@ xlate_ofproto_set(struct ofproto_dpif *ofproto, const
> char *name,
> bool variable_length_userdata,
> size_t max_mpls_depth)
> {
> - struct xbridge *xbridge = xbridge_lookup(ofproto);
> -
> - if (!xbridge) {
> - xbridge = xzalloc(sizeof *xbridge);
> - xbridge->ofproto = ofproto;
> -
> - hmap_insert(&xbridges, &xbridge->hmap_node, hash_pointer(ofproto,
> 0));
> - hmap_init(&xbridge->xports);
> - list_init(&xbridge->xbundles);
> - }
> -
> if (xbridge->ml != ml) {
> mac_learning_unref(xbridge->ml);
> xbridge->ml = mac_learning_ref(ml);
> @@ -380,7 +443,6 @@ xlate_ofproto_set(struct ofproto_dpif *ofproto, const
> char *name,
> xbridge->netflow = netflow_ref(netflow);
> }
>
> - free(xbridge->name);
> xbridge->name = xstrdup(name);
>
>
I think it is better to keep the name copy and free together. Either
inside or outside the xlate_x*_set().
> static void
> +xlate_xbridge_copy(struct xbridge *xbridge)
> +{
> + struct xbundle *xbundle;
> + struct xport *xport;
> + struct xbridge *new_xbridge = xzalloc(sizeof *xbridge);
> + new_xbridge->ofproto = xbridge->ofproto;
> + xlate_xbridge_init(new_xbridge);
> +
> + xlate_xbridge_set(new_xbridge, xbridge->name,
> + xbridge->dpif, xbridge->miss_rule,
> + xbridge->no_packet_in_rule, xbridge->ml,
> xbridge->stp,
> + xbridge->mbridge, xbridge->sflow, xbridge->ipfix,
> + xbridge->netflow, xbridge->frag,
> xbridge->forward_bpdu,
> + xbridge->has_in_band, xbridge->enable_recirc,
> + xbridge->variable_length_userdata,
> + xbridge->max_mpls_depth);
> + LIST_FOR_EACH (xbundle, list_node, &xbridge->xbundles) {
> + xlate_xbundle_copy(new_xbridge, xbundle);
> + }
> +
> + HMAP_FOR_EACH (xport, ofp_node, &xbridge->xports) {
> + if (!xport->xbundle) {
> + xlate_xport_copy(new_xbridge, NULL, xport);
> + }
> + }
> +}
> +
>
maybe a comment here explaining why we need to check "!xport->xbundle"?
> +
> +/* Sets the current xlate configuration to new_xcfg and frees the old
> xlate
> + * configuration in xcfgp.
> + *
> + * This needs to be called after editing the xlate configuration.
> + *
> + * Functions that edit the new xlate configuration are
> + * xlate_<ofport/bundle/ofport>_set and
> xlate_<ofport/bundle/ofport>_remove.
> + *
> + * A sample workflow:
> + *
> + * xlate_init_new_xcfg();
> + * ...
> + * edit_xlate_configuration();
> + * ...
> + * xlate_set_new_xcfg(); */
> +void
> +xlate_set_new_xcfg()
> +{
> + struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
> +
> + ovsrcu_set(&xcfgp, new_xcfg);
> + ovsrcu_postpone(xlate_xcfg_free, xcfg);
> +}
>
Maybe nitpicking, could we move xlate_set_new_xcfg() after
xlate_init_new_xcfg()?
And avoid the
comment duplication?
--
> 1.7.9.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Again, it is really glad to see the xlate_rwlock gone.!
Acked-by: Alex Wang <al...@nicira.com>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
