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

[Joe Stringer]

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.