[ovs-dev] [PATCHv7 10/11] dpif: Index flows using unique identifiers.

[Joe Stringer]

On 13 October 2014 17:09, Ben Pfaff <blp at nicira.com> wrote:

> On Tue, Oct 07, 2014 at 12:23:37AM +1300, Joe Stringer wrote:
> > This patch modifies the dpif interface to allow flows to be manipulated
> > using a 128-bit identifier. This allows revalidator threads to perform
> > datapath operations faster, as they do not need to serialise the entire
> > flow key for operations like flow_get and flow_delete. In conjunction
> > with a future patch to simplify the dump interface, this provides a
> > significant performance benefit for revalidation.
> >
> > When handlers assemble flow_put operations, they specify a unique
> > identifier (UID) for each flow as it is passed down to the datapath to
> > be stored with the flow. The UID is currently provided to handlers
> > by the dpif during upcall processing.
> >
> > When revalidators assemble flow_get or flow_del operations, they specify
> > the UID for the flow along with the key, and a boolean for whether to
> > send the request using only a UID or to send the request using the UID
> > and flow key. The former is preferred for newer datapaths that support
> > UID, while the latter is used for backwards compatibility.
> >
> > Signed-off-by: Joe Stringer <joestringer at nicira.com>
>
> For whatever reason, the new struct odputil_uidbuf stuck out at me, so
> I spent a few minutes trying to figure out why it was needed.
> One use, as the new 'uid' member in struct ukey_op, doesn't appear to
> be used for anything.  When the delete the member, the build still
> succeeds.
>
> The other use for this structure is in dpif-netlink.c.  I find myself
> wondering whether it is valuable there either.  It seems that in
> struct dpif_netlink_flow the main purpose of uid_buf is to make it
> possible to handle uids of types or sizes other than ovs_u128, but I
> am not sure that that is essential, especially since the dpif
> interface itself only supports ovs_u128.  I think it might be more
> convenient to replace
>
>     const struct nlattr *uid;           /* OVS_FLOW_ATTR_UID. */
>     size_t uid_len;
> ...
>     struct odputil_uidbuf uid_buf;      /* Buffer to hold 'uid'. */
>
> by something like:
>
>     ovs_u128 uid;                       /* OVS_FLOW_ATTR_UID. */
>     bool uid_present;                   /* Is there a UID? */
>
> The one case that this wouldn't handle neatly, I think, is the case
> where the kernel passes to userspace a UID of the wrong size, but I
> think for that we could just mark the uid as not present (and possibly
> log something).  Actually odp_uid_from_nlattrs() looks pretty close to
> that anyhow.
>
> Acked-by: Ben Pfaff <blp at nicira.com>
>

Thanks for the review. I agree that the odputil_uidbuf structure is likely
unnecessary - its original use was when these patches exposed more netlink
to ofproto-dpif-upcall. I'll take a look at removing it as per your
comments.