[ovs-dev] [PATCH v3 2/2] ofproto-dpif-xlate: Implement RCU locking in ofproto-dpif-xlate.

Alex Wang alexw at nicira.com
Fri May 16 21:17:20 UTC 2014


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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



Again, it is really glad to see the xlate_rwlock gone.!


Acked-by: Alex Wang <alexw at nicira.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140516/eab7cfb8/attachment-0005.html>


More information about the dev mailing list