<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"><<a href="mailto:wryan@nicira.com" target="_blank">wryan@nicira.com</a>></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, &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->ofproto == ofproto && flow_equal(&miss->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, &md, &upcall->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>
- && !miss->put<br>
&& upcall->dpif_upcall.type == DPIF_UC_MISS) {<br>
struct ofpbuf mask;<br>
bool megaflow;<br>
<br>
- miss->put = true;<br>
-<br></blockquote><div><br></div><div><br></div><div>Here, the removal of 'miss->put', 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's see if there is anything shown up during the tcp_crr test.</div><div><br></div><div><br></div></div></div></div>