It seems we can do away with udpif->enable_ufid field. udpif has
access to the backer, so it can know
about the datapath capability.
The debug interface just need to control another global 'enable'
variable. ufid can only be use if both conditions
are true. This will also remove the possibility of turning on udpif
when the datapath does not support it.
On Wed, Dec 17, 2014 at 10:34 AM, Joe Stringer <joestrin...@nicira.com> wrote:
> Previously, the dpif layer was responsible for determining datapath
> support for UFIDs, which resulted in all ovs-dpctl utilities
> inserting/deleting flows from the datapath each time they are run.
> Shift this responsibility up to the dpif_backer.
>
> There are two users of this functionality: Revalidators check for UFID
> support to request a terser dump using UFIDs, and dpif-netlink uses this
> to request flow_del operations to only return the UFID/stats. The latter
> case was previously hidden from revalidators, but this change makes them
> aware of it, and reuses the same "udpif->enable_ufid" flag for reducing
> overhead of both flow dump and flow delete.
>
> Signed-off-by: Joe Stringer <joestrin...@nicira.com>
> ---
> lib/dpif-netdev.c | 7 -------
> lib/dpif-netlink.c | 46
> +----------------------------------------
> lib/dpif-provider.h | 4 ----
> lib/dpif.c | 15 +-------------
> lib/dpif.h | 3 ++-
> ofproto/ofproto-dpif-upcall.c | 17 +++++++++------
> ofproto/ofproto-dpif.c | 44 +++++++++++++++++++++++++++++++++++++++
> ofproto/ofproto-dpif.h | 1 +
> 8 files changed, 60 insertions(+), 77 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index a0e508c..890870c 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1584,12 +1584,6 @@ get_dpif_flow_stats(const struct dp_netdev_flow
> *netdev_flow,
> }
> }
>
> -static bool
> -dpif_netdev_check_ufid(struct dpif *dpif OVS_UNUSED)
> -{
> - return true;
> -}
> -
> /* Converts to the dpif_flow format, using 'key_buf' and 'mask_buf' for
> * storing the netlink-formatted key/mask. 'key_buf' may be the same as
> * 'mask_buf'. Actions will be returned without copying, by relying on RCU to
> @@ -3254,7 +3248,6 @@ const struct dpif_class dpif_netdev_class = {
> dpif_netdev_enable_upcall,
> dpif_netdev_disable_upcall,
> dpif_netdev_get_datapath_version,
> - dpif_netdev_check_ufid,
> };
>
> static void
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 63bbddc..bf06cc9 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -138,8 +138,6 @@ static void dpif_netlink_flow_get_stats(const struct
> dpif_netlink_flow *,
> struct dpif_flow_stats *);
> static void dpif_netlink_flow_to_dpif_flow(struct dpif *, struct dpif_flow *,
> const struct dpif_netlink_flow *);
> -static bool dpif_netlink_check_ufid__(struct dpif *dpif);
> -static bool dpif_netlink_check_ufid(struct dpif *dpif);
>
> /* One of the dpif channels between the kernel and userspace. */
> struct dpif_channel {
> @@ -190,11 +188,6 @@ struct dpif_netlink {
> /* Change notification. */
> struct nl_sock *port_notifier; /* vport multicast group subscriber. */
> bool refresh_channels;
> -
> - /* If the datapath supports indexing flows using unique identifiers, then
> - * we can reduce the size of netlink messages by omitting fields like the
> - * flow key during flow operations. */
> - bool ufid_supported;
> };
>
> static void report_loss(struct dpif_netlink *, struct dpif_channel *,
> @@ -311,7 +304,6 @@ open_dpif(const struct dpif_netlink_dp *dp, struct dpif
> **dpifp)
> dp->dp_ifindex, dp->dp_ifindex);
>
> dpif->dp_ifindex = dp->dp_ifindex;
> - dpif->ufid_supported = dpif_netlink_check_ufid__(&dpif->dpif);
> *dpifp = &dpif->dpif;
>
> return 0;
> @@ -1329,8 +1321,7 @@ dpif_netlink_init_flow_del(struct dpif_netlink *dpif,
> struct dpif_netlink_flow *request)
> {
> return dpif_netlink_init_flow_del__(dpif, del->key, del->key_len,
> - del->ufid, dpif->ufid_supported,
> - request);
> + del->ufid, del->terse, request);
> }
>
> struct dpif_netlink_flow_dump {
> @@ -1729,40 +1720,6 @@ dpif_netlink_handler_uninit(struct dpif_handler
> *handler)
> }
> #endif
>
> -/* Checks support for unique flow identifiers. */
> -static bool
> -dpif_netlink_check_ufid__(struct dpif *dpif_)
> -{
> - struct flow flow;
> - struct odputil_keybuf keybuf;
> - struct ofpbuf key;
> - ovs_u128 ufid;
> -
> - memset(&flow, 0, sizeof flow);
> - flow.dl_type = htons(0x1234);
> -
> - ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> - odp_flow_key_from_flow(&key, &flow, NULL, 0, true);
> - dpif_flow_hash(dpif_, ofpbuf_data(&key), ofpbuf_size(&key), &ufid);
> -
> - return dpif_probe_feature(dpif_, "UFID", &key, &ufid);
> -}
> -
> -static bool
> -dpif_netlink_check_ufid(struct dpif *dpif_)
> -{
> - const struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
> -
> - if (dpif->ufid_supported) {
> - VLOG_INFO("%s: Datapath supports userspace flow ids",
> - dpif_name(dpif_));
> - } else {
> - VLOG_INFO("%s: Datapath does not support userspace flow ids",
> - dpif_name(dpif_));
> - }
> - return dpif->ufid_supported;
> -}
> -
> /* Synchronizes 'channels' in 'dpif->handlers' with the set of vports
> * currently in 'dpif' in the kernel, by adding a new set of channels for
> * any kernel vport that lacks one and deleting any channels that have no
> @@ -2319,7 +2276,6 @@ const struct dpif_class dpif_netlink_class = {
> NULL, /* enable_upcall */
> NULL, /* disable_upcall */
> dpif_netlink_get_datapath_version, /* get_datapath_version */
> - dpif_netlink_check_ufid,
> };
>
> static int
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 568c71f..7b4878eb 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -373,10 +373,6 @@ struct dpif_class {
> /* Get datapath version. Caller is responsible for freeing the string
> * returned. */
> char *(*get_datapath_version)(void);
> -
> - /* Returns whether 'dpif' supports unique flow identifiers for flow
> - * operations. */
> - bool (*get_ufid_support)(struct dpif *);
> };
>
> extern const struct dpif_class dpif_netlink_class;
> diff --git a/lib/dpif.c b/lib/dpif.c
> index a2696c6..649ce09 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -905,20 +905,6 @@ dpif_probe_feature(struct dpif *dpif, const char *name,
> return enable_feature;
> }
>
> -/* Tests whether 'dpif' supports userspace flow ids. We can skip serializing
> - * some flow attributes for datapaths that support this feature.
> - *
> - * Returns true if 'dpif' supports UFID for flow operations.
> - * Returns false if 'dpif' does not support UFID. */
> -bool
> -dpif_get_enable_ufid(struct dpif *dpif)
> -{
> - if (dpif->dpif_class->get_ufid_support) {
> - return dpif->dpif_class->get_ufid_support(dpif);
> - }
> - return false;
> -}
> -
> /* A dpif_operate() wrapper for performing a single DPIF_OP_FLOW_GET. */
> int
> dpif_flow_get(struct dpif *dpif,
> @@ -987,6 +973,7 @@ dpif_flow_del(struct dpif *dpif,
> op.u.flow_del.key_len = key_len;
> op.u.flow_del.ufid = ufid;
> op.u.flow_del.stats = stats;
> + op.u.flow_del.terse = false;
>
> opp = &op;
> dpif_operate(dpif, &opp, 1);
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 834b75f..c5bdf91 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -517,7 +517,6 @@ enum dpif_flow_put_flags {
>
> bool dpif_probe_feature(struct dpif *, const char *name,
> const struct ofpbuf *key, const ovs_u128 *ufid);
> -bool dpif_get_enable_ufid(struct dpif *);
> void dpif_flow_hash(const struct dpif *, const void *key, size_t key_len,
> ovs_u128 *hash);
> int dpif_flow_flush(struct dpif *);
> @@ -656,6 +655,8 @@ struct dpif_flow_del {
> const struct nlattr *key; /* Flow to delete. */
> size_t key_len; /* Length of 'key' in bytes. */
> const ovs_u128 *ufid; /* Unique identifier of flow to delete.
> */
> + bool terse; /* OK to skip sending/receiving full flow
> + * info? */
>
> /* Output. */
> struct dpif_flow_stats *stats; /* Optional flow statistics. */
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index f0cd4cc..6feaa75 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -430,6 +430,7 @@ udpif_start_threads(struct udpif *udpif, size_t
> n_handlers,
> {
> if (udpif && n_handlers && n_revalidators) {
> size_t i;
> + bool enable_ufid;
>
> udpif->n_handlers = n_handlers;
> udpif->n_revalidators = n_revalidators;
> @@ -444,7 +445,8 @@ udpif_start_threads(struct udpif *udpif, size_t
> n_handlers,
> "handler", udpif_upcall_handler, handler);
> }
>
> - atomic_init(&udpif->enable_ufid, dpif_get_enable_ufid(udpif->dpif));
> + enable_ufid = ofproto_dpif_get_enable_ufid(udpif->backer);
> + atomic_init(&udpif->enable_ufid, enable_ufid);
> dpif_enable_upcall(udpif->dpif);
>
> ovs_barrier_init(&udpif->reval_barrier, udpif->n_revalidators);
> @@ -1673,7 +1675,8 @@ exit:
> }
>
> static void
> -delete_op_init__(struct ukey_op *op, const struct dpif_flow *flow)
> +delete_op_init__(struct udpif *udpif, struct ukey_op *op,
> + const struct dpif_flow *flow)
> {
> op->ukey = NULL;
> op->dop.type = DPIF_OP_FLOW_DEL;
> @@ -1681,10 +1684,11 @@ delete_op_init__(struct ukey_op *op, const struct
> dpif_flow *flow)
> op->dop.u.flow_del.key_len = flow->key_len;
> op->dop.u.flow_del.ufid = flow->ufid_present ? &flow->ufid : NULL;
> op->dop.u.flow_del.stats = &op->stats;
> + atomic_read_relaxed(&udpif->enable_ufid, &op->dop.u.flow_del.terse);
> }
>
> static void
> -delete_op_init(struct ukey_op *op, struct udpif_key *ukey)
> +delete_op_init(struct udpif *udpif, struct ukey_op *op, struct udpif_key
> *ukey)
> {
> op->ukey = ukey;
> op->dop.type = DPIF_OP_FLOW_DEL;
> @@ -1692,6 +1696,7 @@ delete_op_init(struct ukey_op *op, struct udpif_key
> *ukey)
> op->dop.u.flow_del.key_len = ukey->key_len;
> op->dop.u.flow_del.ufid = ukey->ufid_present ? &ukey->ufid : NULL;
> op->dop.u.flow_del.stats = &op->stats;
> + atomic_read_relaxed(&udpif->enable_ufid, &op->dop.u.flow_del.terse);
> }
>
> static void
> @@ -1858,7 +1863,7 @@ revalidate(struct revalidator *revalidator)
> } else {
> log_unexpected_flow(f, error);
> if (error != ENOENT) {
> - delete_op_init__(&ops[n_ops++], f);
> + delete_op_init__(udpif, &ops[n_ops++], f);
> }
> }
> continue;
> @@ -1889,7 +1894,7 @@ revalidate(struct revalidator *revalidator)
> ukey->flow_exists = keep;
>
> if (!keep) {
> - delete_op_init(&ops[n_ops++], ukey);
> + delete_op_init(udpif, &ops[n_ops++], ukey);
> }
> ovs_mutex_unlock(&ukey->mutex);
> }
> @@ -1958,7 +1963,7 @@ revalidator_sweep__(struct revalidator *revalidator,
> bool purge)
> ukey)))) {
> struct ukey_op *op = &ops[n_ops++];
>
> - delete_op_init(op, ukey);
> + delete_op_init(udpif, op, ukey);
> if (n_ops == REVALIDATE_MAX_BATCH) {
> push_ukey_ops(udpif, umap, ops, n_ops);
> n_ops = 0;
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 2165bda..81c2c6d 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -271,6 +271,9 @@ struct dpif_backer {
> struct recirc_id_pool *rid_pool; /* Recirculation ID pool. */
> bool enable_recirc; /* True if the datapath supports recirculation */
>
> + /* True if the datapath supports unique flow identifiers */
> + bool enable_ufid;
> +
> /* True if the datapath supports variable-length
> * OVS_USERSPACE_ATTR_USERDATA in OVS_ACTION_ATTR_USERSPACE actions.
> * False if the datapath supports only 8-byte (or shorter) userdata. */
> @@ -373,6 +376,12 @@ ofproto_dpif_get_enable_recirc(const struct ofproto_dpif
> *ofproto)
> return ofproto->backer->enable_recirc;
> }
>
> +bool
> +ofproto_dpif_get_enable_ufid(struct dpif_backer *backer)
> +{
> + return backer->enable_ufid;
> +}
> +
> static struct ofport_dpif *get_ofp_port(const struct ofproto_dpif *ofproto,
> ofp_port_t ofp_port);
> static void ofproto_trace(struct ofproto_dpif *, struct flow *,
> @@ -866,6 +875,7 @@ struct odp_garbage {
> static bool check_variable_length_userdata(struct dpif_backer *backer);
> static size_t check_max_mpls_depth(struct dpif_backer *backer);
> static bool check_recirc(struct dpif_backer *backer);
> +static bool check_ufid(struct dpif_backer *backer);
> static bool check_masked_set_action(struct dpif_backer *backer);
>
> static int
> @@ -963,6 +973,7 @@ open_dpif_backer(const char *type, struct dpif_backer
> **backerp)
> backer->enable_recirc = check_recirc(backer);
> backer->max_mpls_depth = check_max_mpls_depth(backer);
> backer->masked_set_action = check_masked_set_action(backer);
> + backer->enable_ufid = check_ufid(backer);
> backer->rid_pool = recirc_id_pool_create();
>
> backer->enable_tnl_push_pop = dpif_supports_tnl_push_pop(backer->dpif);
> @@ -1031,6 +1042,39 @@ check_recirc(struct dpif_backer *backer)
> return enable_recirc;
> }
>
> +/* Tests whether 'dpif' supports userspace flow ids. We can skip serializing
> + * some flow attributes for datapaths that support this feature.
> + *
> + * Returns true if 'dpif' supports UFID for flow operations.
> + * Returns false if 'dpif' does not support UFID. */
> +static bool
> +check_ufid(struct dpif_backer *backer)
> +{
> + struct flow flow;
> + struct odputil_keybuf keybuf;
> + struct ofpbuf key;
> + ovs_u128 ufid;
> + bool enable_ufid;
> +
> + memset(&flow, 0, sizeof flow);
> + flow.dl_type = htons(0x1234);
> +
> + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> + odp_flow_key_from_flow(&key, &flow, NULL, 0, true);
> + dpif_flow_hash(backer->dpif, ofpbuf_data(&key), ofpbuf_size(&key),
> &ufid);
> +
> + enable_ufid = dpif_probe_feature(backer->dpif, "UFID", &key, &ufid);
> +
> + if (enable_ufid) {
> + VLOG_INFO("%s: Datapath supports userspace flow ids",
> + dpif_name(backer->dpif));
> + } else {
> + VLOG_INFO("%s: Datapath does not support userspace flow ids",
> + dpif_name(backer->dpif));
> + }
> + return enable_ufid;
> +}
> +
> /* Tests whether 'backer''s datapath supports variable-length
> * OVS_USERSPACE_ATTR_USERDATA in OVS_ACTION_ATTR_USERSPACE actions. We need
> * to disable some features on older datapaths that don't support this
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index c03e606..1170c33 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -75,6 +75,7 @@ BUILD_ASSERT_DECL(N_TABLES >= 2 && N_TABLES <= 255);
>
> size_t ofproto_dpif_get_max_mpls_depth(const struct ofproto_dpif *);
> bool ofproto_dpif_get_enable_recirc(const struct ofproto_dpif *);
> +bool ofproto_dpif_get_enable_ufid(struct dpif_backer *backer);
>
> struct rule_dpif *rule_dpif_lookup(struct ofproto_dpif *, struct flow *,
> struct flow_wildcards *, bool take_ref,
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
