[ovs-dev] [PATCHv9 09/12] datapath: Add support for unique flow identifiers.

[Pravin Shelar]

On Fri, Oct 31, 2014 at 4:55 PM, Joe Stringer <joestringer at nicira.com> wrote:
> If a datapath is created with the flag OVS_DP_F_INDEX_BY_UFID, then an
> additional table_instance is added to the flow_table, which is indexed
> by unique identifiers ("UFID"). Userspace implementations can specify a
> UFID 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_UFID and a UFID is not
> specified at flow setup time, then that operation will fail. If
> OVS_UFID_F_* flags are specified for an operation, then they will modify
> what is returned through the operation. For instance, OVS_UFID_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>
> ---
> v9: No change.
> v8: Rename UID -> UFID "unique flow identifier".
>     Fix null dereference when adding flow without uid or mask.
>     If UFID and not match are specified, and lookup fails, return ENOENT.
>     Rebase.
> v7: Remove OVS_DP_F_INDEX_BY_UID.
>     Rework UID serialisation for variable-length UID.
>     Log error if uid not specified and OVS_UID_F_SKIP_KEY is set.
>     Rebase against "probe" logging changes.
> 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.
> ---

Patch looks good. I have few comments:
- Can you make union of unmasked_key and (fid, ufid_hash), since they
are mutually exclusive.
- There is no need to periodically rehash ufid table since the lookup
are not triggered by incoming packets.
- ovs_flow_tbl_destroy() can iterate flow table and delete both hash
entries. that way there is no need to iterate both tables.
- I suggested kernel_only flags, but we have to keep unmasked_key for
compat code. So I think we can remove the kernel only key generation
and build ufid hash-table only if ufid is given by userspace.