On 14 October 2014 09:49, Ben Pfaff <b...@nicira.com> wrote: > On Tue, Oct 07, 2014 at 12:23:37AM +1300, Joe Stringer wrote: > > This patch modifies the dpif interface to allow flows to be manipulated > > using a 128-bit identifier. This allows revalidator threads to perform > > datapath operations faster, as they do not need to serialise the entire > > flow key for operations like flow_get and flow_delete. In conjunction > > with a future patch to simplify the dump interface, this provides a > > significant performance benefit for revalidation. > > > > When handlers assemble flow_put operations, they specify a unique > > identifier (UID) for each flow as it is passed down to the datapath to > > be stored with the flow. The UID is currently provided to handlers > > by the dpif during upcall processing. > > > > When revalidators assemble flow_get or flow_del operations, they specify > > the UID for the flow along with the key, and a boolean for whether to > > send the request using only a UID or to send the request using the UID > > and flow key. The former is preferred for newer datapaths that support > > UID, while the latter is used for backwards compatibility. > > > > Signed-off-by: Joe Stringer <joestrin...@nicira.com> > > I looked over the updates to the patches before this one and I'm happy > with them. If there's one that I'm a bit concerned about, it's > "upcall: Create ukeys in handler threads." I don't have anything to > point at in that one, I just feel like I don't fully understand the > implications of the changes. I wonder at your confidence level; if > it's high, then I'm happy with that one, and if it is not then I will > take longer to study it. >
The scope of that patch has changed a bit over time, and it's become less clean than I'd originally hoped due to particular corner cases. I would appreciate a fresh review on it. > > In lib/dpctl.c, I think that calling dpif_flow_hash() will work OK for > dpif-netdev, because it knows the secret hash basis. But standalone > ovs-dpctl will not know the secret, so it will use a bad UID. Should > we do something about that? > I think that the most robust way to handle this is to skip generating the UID from the standalone ovs-dpctl and only send the flow key down to the datapath. This would mean that the datapath needs to be able to index flows by flow key [so far, this has been the case for dpif-netlink but not dpif-netdev.] In addition to this, from the datapath perspective we don't currently specify exactly what should happen when both UID and flow-key are specified, but the UID is invalid - eg, the situation you point out above. There are two possibilites for handling this: * Try the UID, then if that fails, try the flow-key and return the result of the flow-key operation. * Try the UID, then if that fails, return the failure. It's not obvious to me which way makes more sense. Receiving an invalid UID is an unusual circumstance, so I think the latter would draw more attention to any situation where userspace mistakenly sends invalid UIDs with flows (or UIDs that don't match the flow that's intended to be deleted). _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
