On Tue, Mar 03, 2015 at 03:00:34PM -0800, Jarno Rajahalme wrote: > Thanks for your detailed comments, and sorry for only compiling with > gcc before posting.
No problem. > > On Mar 3, 2015, at 11:22 AM, Ben Pfaff <[email protected]> wrote: > > > > (snip) > > > ofproto-dpif-rid.c > > ------------------ > > > > This comment in recirc_run() gave me pause. 250 ms "should be" enough? > > What happens if someone odd happens; I hope that nothing > > e.g. dereferences a wild pointer and explodes? > > > > /* Delete the expired. These have been lingering for at least 250 > > ms, > > * which should be enough for any ongoing recirculations to be > > * finished. */ > > > > The worst that can happen is that a recirculated packet cannot be > associated with a recirculation context (and needs to be > dropped). This is no worse than dropping a packet on the initial > input, which, while undesirable, is not catastrophic. > > Lingering is useful in cases where we are only executing actions > without setting up a datapath flow, such as packet-outs, but also > with slow-pathed flows, or cases where datapath flow set-up > fails. It also helps testing with ???trace -generate??? followed by > dummy-receive of the recirculated packet. In these cases the > recirculation context would be likely released before the > recirculated packet comes back from the datapath. If the datapath > manages to hold on to the recirculated packet longer than 250ms IMO > it is OK to drop it. That's all good information. It wasn't clear to me from the comments. Would you mind adding some of the information from the two paragraphs above to the code? > > This line in recirc_metadata_hash() makes me hope we're careful enough > > about zeroing holes (if any) in ofpacts: > > > > hash = hash_words64((const uint64_t *)ofpacts, > > OFPACT_ALIGN(ofpacts_len) / OFPACT_ALIGNTO, > > hash); > > > > We are already using bitwise comparison (ofpbuf_equal()) of rule???s > actions in revalidation, so I have assumed holes are already taken > care of. The worst that can happen if not is that a new recirc id > will be allocated when an old one could have been reused - no > catastrophic failures. Recirculated packets get a lookup on the ID, > which is not affected by any hash differences due to potential holes > in actions. OK, that's a good point, great. > > I'm not sure why recirc_free_ofproto() logs errors. Can't there still > > be recirc_ids if there were bonds or if the ofproto didn't go idle and > > free all its recirc ids before exit? Or is the higher-level code > > supposed to clean up before it kills the recirc id engine? > > I adopted this from the existing recirculation code. I think the > intent has been to make sure clean up has been done so that there > are no stale ofproto pointers after an ofproto is destroyed. OK, makes sense then, thank you. (Maybe I should have seen that this was not changed, but this commit was such a big rewrite that I reviewed it as "fresh" code instead as a diff.) _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
