<div dir="ltr">Thanks Ryan, this is a great refactoring.<div><br></div><div>Looks good to me,</div><div><br></div><div>Minor issues:</div><div><br></div><div>1.  Could you rebase the patch against master?  Need to fix some new calls, added after you posted the patch.</div>

<div><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, May 7, 2014 at 3:14 PM, Ryan Wilson <span dir="ltr">&lt;<a href="mailto:wryan@nicira.com" target="_blank">wryan@nicira.com</a>&gt;</span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The upcall hander keeps a hash table which hashes flow to a list of<br>
corresponding packets.</blockquote><div><br></div><div><br></div><div>s/hander/handler</div><div> </div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

 </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
@@ -710,62 +687,55 @@ compose_slow_path(struct udpif *udpif, struct xlate_out *xout,<br>
     odp_put_userspace_action(pid, &amp;cookie, sizeof cookie.slow_path, buf);<br>
 }<br>
<br>
-static struct flow_miss *<br>
-flow_miss_find(struct hmap *todo, const struct ofproto_dpif *ofproto,<br>
-               const struct flow *flow, uint32_t hash)<br>
+static void<br>
+upcall_init(struct upcall *upcall, struct flow *flow, struct ofpbuf *packet,<br>
+            struct ofproto_dpif *ofproto, struct dpif_upcall *dupcall,<br>
+            odp_port_t odp_in_port)<br>
 {<br>
-    struct flow_miss *miss;<br>
-<br>
-    HMAP_FOR_EACH_WITH_HASH (miss, hmap_node, hash, todo) {<br>
-        if (miss-&gt;ofproto == ofproto &amp;&amp; flow_equal(&amp;miss-&gt;flow, flow)) {<br>
-            return miss;<br>
-        }<br>
+    struct xlate_in xin;<br>
+    struct pkt_metadata md = pkt_metadata_from_flow(flow);<br></blockquote><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+    flow_extract(packet, &amp;md, &amp;upcall-&gt;flow);<br>
+<br></blockquote><div><br></div><div><br></div><div>Add a newline between local variable declaration and the code.</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


+   <br>
<br>
         /* Do not install a flow into the datapath if:<br>
          *<br>
          *    - The datapath already has too many flows.<br>
          *<br>
-         *    - An earlier iteration of this loop already put the same flow.<br>
-         *<br>
          *    - We received this packet via some flow installed in the kernel<br>
          *      already. */<br>
         if (may_put<br>
-            &amp;&amp; !miss-&gt;put<br>
             &amp;&amp; upcall-&gt;dpif_upcall.type == DPIF_UC_MISS) {<br>
             struct ofpbuf mask;<br>
             bool megaflow;<br>
<br>
-            miss-&gt;put = true;<br>
-<br></blockquote><div><br></div><div><br></div><div>Here, the removal of &#39;miss-&gt;put&#39;, may cause the warning of inserting duplicated flows (when upcalls from same flow</div><div>at handled in same batch).  We think it is okay, to have this warning, since it should be very rare and it is will not</div>

<div>cause duplicated flows in datapath.  Let&#39;s see if there is anything shown up during the tcp_crr test.</div><div><br></div><div><br></div></div></div></div>