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 <[email protected]>
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 <[email protected]>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
