<div dir="ltr"><div>This looks much tidier, I like it. Particularly given the elimination of 4 goto statements :-). Comments inline.<br></div><div class="gmail_extra"><br></div><div class="gmail_extra"><br><div class="gmail_quote">

On 17 May 2014 11:28, Ben Pfaff <span dir="ltr">&lt;<a href="mailto:blp@nicira.com" target="_blank">blp@nicira.com</a>&gt;</span> wrote:<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">

+ * Notes<br>+ * -----<br>+ *<br>+ * All error reporting is deferred to the call to dpif_flow_dump_destroy().<br>+ *<br>+ * Previous versions of this interface allowed to caller to say whether it<br>+ * actually needed actions or statistics, to allow for optimizations in cases<br>

+ * where they were not needed.  This version always provided actions and<br>+ * statistics because every existing caller requests them.<br>+ */<br></blockquote><div><br></div><div>The main reason the flow_dumper code asked for actions in the current code is that they&#39;re practically free most of the time (dpif_flow_dump_next() only misses actions if they won&#39;t fit in the netlink message, in which case dpif_flow_dump_next() sends a separate call to fetch them). We were thinking about removing actions from the common case though, so more flows could be dumped per batch. I&#39;m not sure if this affects your view on this interface, perhaps we&#39;ll just cross that bridge when we come to it.</div>

<div><br></div><div>+Ethan any thoughts on this?</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">

+        for (f = flows; f &lt; &amp;flows[n_dumped]; f++) {<br>+            long long int used = f-&gt;stats.used;<br>+            uint32_t hash = hash_bytes(f-&gt;key, f-&gt;key_len, udpif-&gt;secret);<br>+            struct udpif_key *ukey = ukey_lookup(udpif, f-&gt;key, f-&gt;key_len,<br>

+                                                 hash);<br>+            bool mark;<br>+<br>+            if (!used &amp;&amp; ukey) {<br>+                bool already_dumped;<br>+<br>+                ovs_mutex_lock(&amp;ukey-&gt;mutex);<br>

+                already_dumped = ukey-&gt;mark || !ukey-&gt;flow_exists;<br>+                used = ukey-&gt;created;<br>+                ovs_mutex_unlock(&amp;ukey-&gt;mutex);<br>+<br>+                if (already_dumped) {<br>

+                    /* The flow has already been dumped. This can occasionally<br>+                     * occur if the datapath is changed in the middle of a flow<br>+                     * dump. Rather than perform the same work twice, skip the<br>

+                     * flow this time. */<br>+                    COVERAGE_INC(upcall_duplicate_flow);<br>+                    continue;<br>                 }<br>             }<br></blockquote><div><br></div><div>I&#39;m not sure whether you would want it in this in a separate patch, but it appears that we only try to detect &quot;already_dumped&quot; for flows that haven&#39;t been used before. Flows which have been used before and are duplicated will execute through to revalidate_ukey(), and potentially revalidate twice (but only if the first revalidation was successful).</div>

<div><br></div><div>I&#39;ve been looking at replacing &#39;ukey-&gt;mark&#39; with a seq which would simplify detecting already-dumped flows, so I could try to address these concerns separately if you like.</div><div><br>

</div></div></div><div class="gmail_extra"><br></div><div class="gmail_extra"><div>With this patch, it is much easier to tell how many flows are being handled in a batch. Out of curiousity, do you know:</div><div>* How many can we fit inside a buffer of NL_DUMP_BUFSIZE (avg case)?</div>

<div>* How does that compare to REVALIDATE_MAX_BATCH?</div><div>* Is it worth counting these for future reference?</div></div></div>