On Tue, Jun 30, 2015 at 4:52 PM, Joe Stringer <joestrin...@nicira.com> wrote:
> Both the ofproto layer and the odp-util layer have recently added
> notions about fields supported by the datapath. This commit merges the
> two into the same structure.
while I see the benfits of combining the structure, I am not sure it
makes conceptual sense.
It seems we need a structure to store per dpif datpath features and
this strcture should be defined in ofproto-dpif.h.
odp-util also needs some of the same information, but not all of them,
right? Then why not define odp_support
as a seperate sturcture that contains only information needed for
serialization, and add a a function to fill
an odp_support struct from dpif_backer_support when necessary?
>
> Signed-off-by: Joe Stringer <joestrin...@nicira.com>
> ---
> v2: Rebase against master.
> ---
> lib/dpif-netdev.c | 14 +++++++++++---
> lib/odp-util.c | 4 ++--
> lib/odp-util.h | 35 ++++++++++++++++++++++++++++-------
> lib/tnl-ports.c | 4 ++--
> ofproto/bond.c | 2 +-
> ofproto/ofproto-dpif-upcall.c | 7 ++-----
> ofproto/ofproto-dpif-xlate.c | 8 ++++----
> ofproto/ofproto-dpif-xlate.h | 2 +-
> ofproto/ofproto-dpif.c | 22 +++++++++-------------
> ofproto/ofproto-dpif.h | 30 ++----------------------------
> tests/test-odp.c | 4 +++-
> 11 files changed, 65 insertions(+), 67 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index dd6bb1f..653e8e5 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -87,6 +87,15 @@ static struct shash dp_netdevs
> OVS_GUARDED_BY(dp_netdev_mutex)
>
> static struct vlog_rate_limit upcall_rl = VLOG_RATE_LIMIT_INIT(600, 600);
>
> +static struct odp_support dp_netdev_support = {
> + .variable_length_userdata = true,
> + .max_mpls_depth = SIZE_MAX,
> + .masked_set_action = true,
> + .recirc = true,
> + .tnl_push_pop = true,
> + .ufid = true
> +};
> +
> /* Stores a miniflow with inline values */
>
> struct netdev_flow_key {
> @@ -1825,8 +1834,7 @@ dp_netdev_flow_to_dpif_flow(const struct dp_netdev_flow
> *netdev_flow,
> struct odp_flow_key_parms odp_parms = {
> .flow = &netdev_flow->flow,
> .mask = &wc.masks,
> - .recirc = true,
> - .max_mpls_depth = SIZE_MAX,
> + .support = dp_netdev_support,
> };
>
> miniflow_expand(&netdev_flow->cr.mask->mf, &wc.masks);
> @@ -3031,7 +3039,7 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd,
> struct dp_packet *packet_,
> .flow = flow,
> .mask = &wc->masks,
> .odp_in_port = flow->in_port.odp_port,
> - .recirc = true,
> + .support = dp_netdev_support,
> };
>
> ofpbuf_init(&key, 0);
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index c70ee0f..ee763a9 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -3466,7 +3466,7 @@ odp_flow_key_from_flow__(const struct
> odp_flow_key_parms *parms,
>
> nl_msg_put_u32(buf, OVS_KEY_ATTR_SKB_MARK, data->pkt_mark);
>
> - if (parms->recirc) {
> + if (parms->support.recirc) {
> nl_msg_put_u32(buf, OVS_KEY_ATTR_RECIRC_ID, data->recirc_id);
> nl_msg_put_u32(buf, OVS_KEY_ATTR_DP_HASH, data->dp_hash);
> }
> @@ -3541,7 +3541,7 @@ odp_flow_key_from_flow__(const struct
> odp_flow_key_parms *parms,
>
> n = flow_count_mpls_labels(flow, NULL);
> if (export_mask) {
> - n = MIN(n, parms->max_mpls_depth);
> + n = MIN(n, parms->support.max_mpls_depth);
> }
> mpls_key = nl_msg_put_unspec_uninit(buf, OVS_KEY_ATTR_MPLS,
> n * sizeof *mpls_key);
> diff --git a/lib/odp-util.h b/lib/odp-util.h
> index 763e3f9..1a75e1a 100644
> --- a/lib/odp-util.h
> +++ b/lib/odp-util.h
> @@ -158,6 +158,31 @@ int odp_flow_from_string(const char *s,
> const struct simap *port_names,
> struct ofpbuf *, struct ofpbuf *);
>
> +/* Stores the various features which the datapath supports. */
> +struct odp_support {
> + /* 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. */
> + bool variable_length_userdata;
> +
> + /* Maximum number of MPLS label stack entries that the datapath supports
> + * in a match */
> + size_t max_mpls_depth;
> +
> + /* True if the datapath supports masked data in OVS_ACTION_ATTR_SET
> + * actions. */
> + bool masked_set_action;
> +
> + /* True if the datapath supports recirculation. */
> + bool recirc;
> +
> + /* True if the datapath supports tnl_push and pop actions. */
> + bool tnl_push_pop;
> +
> + /* True if the datapath supports OVS_FLOW_ATTR_UFID. */
> + bool ufid;
> +};
> +
> struct odp_flow_key_parms {
> /* The flow and mask to be serialized. In the case of masks, 'flow'
> * is used as a template to determine how to interpret 'mask'. For
> @@ -173,13 +198,9 @@ struct odp_flow_key_parms {
> * port. */
> odp_port_t odp_in_port;
>
> - /* Indicates support for recirculation fields. If this is true, then
> - * these fields will always be serialised. */
> - bool recirc;
> -
> - /* Only used for mask translation: */
> -
> - size_t max_mpls_depth;
> + /* Indicates support for various fields. If the datapath supports a
> field,
> + * then it will always be serialised. */
> + struct odp_support support;
>
> /* The netlink formatted version of the flow. It is used in cases where
> * the mask cannot be constructed from the OVS internal representation
> diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
> index 79c9631..35dd0a5 100644
> --- a/lib/tnl-ports.c
> +++ b/lib/tnl-ports.c
> @@ -167,7 +167,7 @@ tnl_port_show(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
>
> /* Key. */
> odp_parms.odp_in_port = flow.in_port.odp_port;
> - odp_parms.recirc = true;
> + odp_parms.support.recirc = true;
> ofpbuf_use_stack(&buf, &keybuf, sizeof keybuf);
> odp_flow_key_from_flow(&odp_parms, &buf);
> key = buf.data;
> @@ -175,7 +175,7 @@ tnl_port_show(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
>
> /* mask*/
> odp_parms.odp_in_port = wc.masks.in_port.odp_port;
> - odp_parms.recirc = false;
> + odp_parms.support.recirc = false;
> ofpbuf_use_stack(&buf, &maskbuf, sizeof maskbuf);
> odp_flow_key_from_mask(&odp_parms, &buf);
> mask = buf.data;
> diff --git a/ofproto/bond.c b/ofproto/bond.c
> index 2e3ad29..b53c8bb 100644
> --- a/ofproto/bond.c
> +++ b/ofproto/bond.c
> @@ -1137,7 +1137,7 @@ bond_rebalance(struct bond *bond)
> }
> bond->next_rebalance = time_msec() + bond->rebalance_interval;
>
> - use_recirc = ofproto_dpif_get_enable_recirc(bond->ofproto) &&
> + use_recirc = ofproto_dpif_get_support(bond->ofproto)->recirc &&
> bond_may_recirc(bond, NULL, NULL);
>
> if (use_recirc) {
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 85f53e8..5657040 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1362,12 +1362,13 @@ ukey_create_from_upcall(struct upcall *upcall)
> {
> struct odputil_keybuf keystub, maskstub;
> struct ofpbuf keybuf, maskbuf;
> - bool recirc, megaflow;
> + bool megaflow;
> struct odp_flow_key_parms odp_parms = {
> .flow = upcall->flow,
> .mask = &upcall->xout.wc.masks,
> };
>
> + odp_parms.support = *ofproto_dpif_get_support(upcall->ofproto);
> if (upcall->key_len) {
> ofpbuf_use_const(&keybuf, upcall->key, upcall->key_len);
> } else {
> @@ -1375,17 +1376,13 @@ ukey_create_from_upcall(struct upcall *upcall)
> * upcall, so convert the upcall's flow here. */
> ofpbuf_use_stack(&keybuf, &keystub, sizeof keystub);
> odp_parms.odp_in_port = upcall->flow->in_port.odp_port;
> - odp_parms.recirc = true;
> odp_flow_key_from_flow(&odp_parms, &keybuf);
> }
>
> atomic_read_relaxed(&enable_megaflows, &megaflow);
> - recirc = ofproto_dpif_get_enable_recirc(upcall->ofproto);
> ofpbuf_use_stack(&maskbuf, &maskstub, sizeof maskstub);
> if (megaflow) {
> odp_parms.odp_in_port = ODPP_NONE;
> - odp_parms.max_mpls_depth =
> ofproto_dpif_get_max_mpls_depth(upcall->ofproto);
> - odp_parms.recirc = recirc;
> odp_parms.key_buf = &keybuf;
>
> odp_flow_key_from_mask(&odp_parms, &maskbuf);
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index ca6b357..41d2359 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -97,7 +97,7 @@ struct xbridge {
> bool forward_bpdu; /* Bridge forwards STP BPDUs? */
>
> /* Datapath feature support. */
> - struct dpif_backer_support support;
> + struct odp_support support;
> };
>
> struct xbundle {
> @@ -483,7 +483,7 @@ static void xlate_xbridge_set(struct xbridge *, struct
> dpif *,
> const struct dpif_ipfix *,
> const struct netflow *,
> bool forward_bpdu, bool has_in_band,
> - const struct dpif_backer_support *);
> + const struct odp_support *);
> static void xlate_xbundle_set(struct xbundle *xbundle,
> enum port_vlan_mode vlan_mode, int vlan,
> unsigned long *trunks, bool use_priority_tags,
> @@ -555,7 +555,7 @@ xlate_xbridge_set(struct xbridge *xbridge,
> const struct dpif_ipfix *ipfix,
> const struct netflow *netflow,
> bool forward_bpdu, bool has_in_band,
> - const struct dpif_backer_support *support)
> + const struct odp_support *support)
> {
> if (xbridge->ml != ml) {
> mac_learning_unref(xbridge->ml);
> @@ -837,7 +837,7 @@ xlate_ofproto_set(struct ofproto_dpif *ofproto, const
> char *name,
> const struct dpif_ipfix *ipfix,
> const struct netflow *netflow,
> bool forward_bpdu, bool has_in_band,
> - const struct dpif_backer_support *support)
> + const struct odp_support *support)
> {
> struct xbridge *xbridge;
>
> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
> index 8586e40..2518ec7 100644
> --- a/ofproto/ofproto-dpif-xlate.h
> +++ b/ofproto/ofproto-dpif-xlate.h
> @@ -212,7 +212,7 @@ void xlate_ofproto_set(struct ofproto_dpif *, const char
> *name, struct dpif *,
> const struct mbridge *, const struct dpif_sflow *,
> const struct dpif_ipfix *, const struct netflow *,
> bool forward_bpdu, bool has_in_band,
> - const struct dpif_backer_support *support);
> + const struct odp_support *support);
> void xlate_remove_ofproto(struct ofproto_dpif *);
>
> void xlate_bundle_set(struct ofproto_dpif *, struct ofbundle *,
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 13d2f21..61d0e29 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -284,7 +284,7 @@ struct dpif_backer {
> char *dp_version_string;
>
> /* Datapath feature support. */
> - struct dpif_backer_support support;
> + struct odp_support support;
> struct atomic_count tnl_count;
> };
>
> @@ -359,22 +359,16 @@ ofproto_dpif_cast(const struct ofproto *ofproto)
> return CONTAINER_OF(ofproto, struct ofproto_dpif, up);
> }
>
> -size_t
> -ofproto_dpif_get_max_mpls_depth(const struct ofproto_dpif *ofproto)
> -{
> - return ofproto->backer->support.max_mpls_depth;
> -}
> -
> bool
> -ofproto_dpif_get_enable_recirc(const struct ofproto_dpif *ofproto)
> +ofproto_dpif_get_enable_ufid(const struct dpif_backer *backer)
> {
> - return ofproto->backer->support.recirc;
> + return backer->support.ufid;
> }
>
> -bool
> -ofproto_dpif_get_enable_ufid(struct dpif_backer *backer)
> +struct odp_support *
> +ofproto_dpif_get_support(const struct ofproto_dpif *ofproto)
> {
> - return backer->support.ufid;
> + return &ofproto->backer->support;
> }
>
> static void ofproto_trace(struct ofproto_dpif *, struct flow *,
> @@ -1005,7 +999,9 @@ check_recirc(struct dpif_backer *backer)
> bool enable_recirc;
> struct odp_flow_key_parms odp_parms = {
> .flow = &flow,
> - .recirc = true,
> + .support = {
> + .recirc = true,
> + },
> };
>
> memset(&flow, 0, sizeof flow);
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index bb6df5e..c3dbf7f 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -73,34 +73,8 @@ BUILD_ASSERT_DECL(N_TABLES >= 2 && N_TABLES <= 255);
> * Ofproto-dpif-xlate is responsible for translating OpenFlow actions into
> * datapath actions. */
>
> -/* Stores the various features which the corresponding backer supports. */
> -struct dpif_backer_support {
> - /* 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. */
> - bool variable_length_userdata;
> -
> - /* Maximum number of MPLS label stack entries that the datapath supports
> - * in a match */
> - size_t max_mpls_depth;
> -
> - /* True if the datapath supports masked data in OVS_ACTION_ATTR_SET
> - * actions. */
> - bool masked_set_action;
> -
> - /* True if the datapath supports recirculation. */
> - bool recirc;
> -
> - /* True if the datapath supports tnl_push and pop actions. */
> - bool tnl_push_pop;
> -
> - /* True if the datapath supports OVS_FLOW_ATTR_UFID. */
> - bool ufid;
> -};
> -
> -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);
> +bool ofproto_dpif_get_enable_ufid(const struct dpif_backer *backer);
> +struct odp_support *ofproto_dpif_get_support(const struct ofproto_dpif *);
>
> cls_version_t ofproto_dpif_get_tables_version(struct ofproto_dpif *);
>
> diff --git a/tests/test-odp.c b/tests/test-odp.c
> index a1f4cde..3e7939e 100644
> --- a/tests/test-odp.c
> +++ b/tests/test-odp.c
> @@ -56,7 +56,9 @@ parse_keys(bool wc_keys)
> if (!wc_keys) {
> struct odp_flow_key_parms odp_parms = {
> .flow = &flow,
> - .recirc = true,
> + .support = {
> + .recirc = true,
> + },
> };
>
> /* Convert odp_key to flow. */
> --
> 2.1.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
