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

[Pravin Shelar]

On Tue, Sep 30, 2014 at 10:09 PM, Joe Stringer <joestringer at nicira.com> wrote:
> On 1 October 2014 11:59, Joe Stringer <joestringer at nicira.com> wrote:
>>
>>
>>
>> On 1 October 2014 11:55, Pravin Shelar <pshelar at nicira.com> wrote:
>>>
>>> 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.
>>
>>
>> OK, this sounds quite reasonable.
>
>
> Somehow it slipped my mind earlier, if the OVS_UID_F_SKIP_KEY flag is set,
> and a flow doesn't have a UID, then does flow dump skip the flow entirely,
> or return the key for it? I'm leaning towards the latter. Already the
> OVS_UID_F_SKIP_KEY semantics are a request that might be ignored (eg, newer
> userspace with older datapath), and it seems better to include all of the
> flows to catch corner cases like this.

ok, but this would be an error. So we can also add error msg.