On 13 October 2014 17:09, 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> > > For whatever reason, the new struct odputil_uidbuf stuck out at me, so > I spent a few minutes trying to figure out why it was needed. > One use, as the new 'uid' member in struct ukey_op, doesn't appear to > be used for anything. When the delete the member, the build still > succeeds. > > The other use for this structure is in dpif-netlink.c. I find myself > wondering whether it is valuable there either. It seems that in > struct dpif_netlink_flow the main purpose of uid_buf is to make it > possible to handle uids of types or sizes other than ovs_u128, but I > am not sure that that is essential, especially since the dpif > interface itself only supports ovs_u128. I think it might be more > convenient to replace > > const struct nlattr *uid; /* OVS_FLOW_ATTR_UID. */ > size_t uid_len; > ... > struct odputil_uidbuf uid_buf; /* Buffer to hold 'uid'. */ > > by something like: > > ovs_u128 uid; /* OVS_FLOW_ATTR_UID. */ > bool uid_present; /* Is there a UID? */ > > The one case that this wouldn't handle neatly, I think, is the case > where the kernel passes to userspace a UID of the wrong size, but I > think for that we could just mark the uid as not present (and possibly > log something). Actually odp_uid_from_nlattrs() looks pretty close to > that anyhow. > > Acked-by: Ben Pfaff <b...@nicira.com> >
Thanks for the review. I agree that the odputil_uidbuf structure is likely unnecessary - its original use was when these patches exposed more netlink to ofproto-dpif-upcall. I'll take a look at removing it as per your comments. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
