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