On 1 October 2014 06:56, Pravin Shelar <pshe...@nicira.com> wrote: > On Mon, Sep 29, 2014 at 3:59 PM, Joe Stringer <joestrin...@nicira.com> > wrote: > > > > > > On 30 September 2014 10:10, Ben Pfaff <b...@nicira.com> wrote: > >> > >> On Fri, Sep 26, 2014 at 09:28:15PM +1200, Joe Stringer wrote: > >> > If a datapath is created with the flag OVS_DP_F_INDEX_BY_UID, then an > >> > additional table_instance is added to the flow_table, which is indexed > >> > by unique identifiers ("UID"). Userspace implementations can specify a > >> > UID of up to 128 bits along with a flow operation as shorthand for the > >> > key. This allows revalidation performance improvements of up to 50%. > >> > > >> > If a datapath is created using OVS_DP_F_INDEX_BY_UID and a UID is not > >> > specified at flow setup time, then that operation will fail. If > >> > OVS_UID_F_* flags are specified for an operation, then they will > modify > >> > what is returned through the operation. For instance, > OVS_UID_F_SKIP_KEY > >> > allows the datapath to skip returning the key (eg, during dump to > reduce > >> > memory copy). > >> > > >> > Signed-off-by: Joe Stringer <joestrin...@nicira.com> > >> > --- > >> > v6: Fix documentation for supporting UIDs between 32-128 bits. > >> > Minor style fixes. > >> > Rebase. > >> > v5: No change. > >> > v4: Fix memory leaks. > >> > Log when triggering the older userspace issue above. > >> > v3: Initial post. > >> > >> This review is from an ABI standpoint only; it's not a review of the > >> kernel code itself. > >> > >> OVS_UID_ATTR_ID is marked as 32-128 bits long. For the "userdata" > >> attribute of the userspace action, we originally had it fixed at 64 > >> bits, then later we decided that it was more flexible to allow it to > >> be any size. Is there an advantage to fixing it within this range? > > > > > > I'm not sure there's any advantage, that's just the way it's written > right > > now. Perhaps with a bit of tweaking, we could get rid of MAX_UID_BUFSIZE > and > > have no restrictions on the size of this. > > > >> > >> > >> I'm a little surprised that OVS_DP_F_INDEX_BY_UID is necessary. In > >> the past we've only added flags for features that otherwise required a > >> backward-incompatible change to the datapath interface. Is adding a > >> UID such a change? > > > > > > Pravin had some preferences on this during the original drafting, but I > > can't find a direct requirement for this. The alternative means that > flows > > might not be present in both of the hastables (indexed by UID vs. exact > > flow_key), although they would always need to be in the exact flow_key > > table. Might be worth bouncing this off Pravin to see if I'm on the mark > > with how I've used it here. > > I looked into the patch and I think we can get rid of > OVS_DP_F_INDEX_BY_UID. > On flow insert we can use flow-id provided by userspace, if it is not > passed we can generate in kernel and use it to insert it in the UID > hash table. sw_flow can have a flag set for kernel generated flow-uid, > this can be used along with OVS_UID_F_SKIP_KEY in flow dump operation > to return key to userspace or not. On flow dump can always iterate UID > hash table where flow iteration should be relatively stable. >
OK, so in this case generally when userspace doesn't support UIDs, the datapath will generate them mainly to keep the flow-id and flow-key hashtables in sync. (Currently, we just disable the flow-id hashtable). Currently, OVS_UID_F_SKIP_KEY is a request flag that means "omit the flow key". Are you suggesting that it should only omit flow keys for flows which have a userspace-specified flow-id? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev