[ovs-dev] [PATCHv6 11/14] datapath: Add support for unique flow identifiers.

[Pravin Shelar]

On Tue, Sep 30, 2014 at 3:15 PM, Joe Stringer <joestringer at nicira.com> wrote:
>
>
> On 1 October 2014 06:56, Pravin Shelar <pshelar at nicira.com> wrote:
>>
>> On Mon, Sep 29, 2014 at 3:59 PM, Joe Stringer <joestringer at nicira.com>
>> wrote:
>> >
>> >
>> > On 30 September 2014 10:10, Ben Pfaff <blp at 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 <joestringer at 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?
>
I used wrong wording, What I mean is keep OVS_UID_F_SKIP_KEY semantic
as it is and do not return kernel generated uids to userspace. Since I
do not see any use of kernel uids to userspace.