[ovs-dev] [xc 4/4] ofproto-dpif: Track relevant fields for wildcarding and add xout cache.

Justin Pettit jpettit at nicira.com
Thu Jun 6 07:37:19 UTC 2013

On Jun 5, 2013, at 10:43 PM, Ben Pfaff <blp at nicira.com> wrote:

> Justin, I know that you're eager for a full review of this patch, but
> it's just not going to happen tonight.  I have some initial feedback
> here, and I'll finish it up tomorrow morning.

No problem.  I appreciate you continuing to look at it so late tonight.

I agree with your comments and your interpretation of the structures and their behavior.  Sorry the comments were not as thorough as I'd have liked, but they got dropped in the rush to get the code out (with the plan to add them in the next revision).  I'll address that in v2.

> Have you tried doing "make check-valgrind" on at least some tests to
> check for memory leaks (and bugs)?  It's a little time consuming
> (takes several minutes, maybe half an hour? with TESTSUITEFLAGS=-j8)
> but it can be useful especially for large changes.

Yes, I had been running it on the new xout tests.  I expanded it to all the ofproto-dpif tests, and it found a problem with the trace command.  I've appended a patch that fixes the issue.  (And it was even a problem I specifically pointed out in a new comment!)

Let me know how the rest of the patch goes, and I'll turn around a new version quickly.

Thanks again!



diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index d126ea0..b367c9b 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5055,6 +5055,8 @@ facet_check_consistency(struct facet *facet)
     /* Check the datapath actions for consistency. */
     xlate_in_init(&xin, ofproto, &facet->flow, &facet->initial_vals, rule,
                   0, NULL);
+    flow_wildcards_init_catchall(&xout.wc);
     xlate_actions(&xin, &xout);
     ok = ofpbuf_equal(&facet->xc->xout.odp_actions, &xout.odp_actions)
@@ -5753,6 +5755,7 @@ rule_dpif_execute(struct rule_dpif *rule, const struct flo
     xlate_in_init(&xin, ofproto, flow, &initial_vals, rule, stats.tcp_flags,
     xin.resubmit_stats = &stats;
+    flow_wildcards_init_catchall(&xout.wc);
     xlate_actions(&xin, &xout);
     execute_odp_actions(ofproto, flow, xout.odp_actions.data,
@@ -5812,6 +5815,8 @@ send_packet(const struct ofport_dpif *ofport, struct ofpbu
     xin.ofpacts_len = sizeof output;
     xin.ofpacts = &output.ofpact;
     xin.resubmit_stats = &stats;
+    flow_wildcards_init_catchall(&xout.wc);
     xlate_actions(&xin, &xout);
     error = dpif_execute(ofproto->backer->dpif,
@@ -7262,6 +7267,7 @@ xlate_actions_for_side_effects(struct xlate_in *xin)
     struct xlate_out xout;
+    flow_wildcards_init_catchall(&xout.wc);
     xlate_actions(xin, &xout);
@@ -8162,6 +8168,7 @@ packet_out(struct ofproto *ofproto_, struct ofpbuf *packet
     xin.ofpacts_len = ofpacts_len;
     xin.ofpacts = ofpacts;
+    flow_wildcards_init_catchall(&xout.wc);
     xlate_actions(&xin, &xout);
     dpif_execute(ofproto->backer->dpif, key.data, key.size,
                  xout.odp_actions.data, xout.odp_actions.size, packet);
@@ -8549,6 +8556,8 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct f
         trace.xin.resubmit_hook = trace_resubmit;
         trace.xin.report_hook = trace_report;
+        flow_wildcards_init_catchall(&trace.xout.wc);
         xlate_actions(&trace.xin, &trace.xout);
         ds_put_char(ds, '\n');

More information about the dev mailing list