Looks like a win to me, thanks.
Ethan
On Fri, Jul 20, 2012 at 4:25 PM, Ben Pfaff <b...@nicira.com> wrote:
> It might have been a useful optimization at one point to have FWW_*
> correspond in OFPFW10_* where possible, but it doesn't seem worthwhile for
> only 3 corresponding values. It also makes the code somewhat more
> confusing.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> lib/flow.h | 12 +++++-------
> lib/ofp-util.c | 48 +++++++++++++++++++++---------------------------
> 2 files changed, 26 insertions(+), 34 deletions(-)
>
> diff --git a/lib/flow.h b/lib/flow.h
> index 0cec1c9..c007758 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -148,14 +148,12 @@ flow_hash(const struct flow *flow, uint32_t basis)
>
> typedef unsigned int OVS_BITWISE flow_wildcards_t;
>
> -/* Same values and meanings as corresponding OFPFW10_* bits. */
> #define FWW_IN_PORT ((OVS_FORCE flow_wildcards_t) (1 << 0))
> -#define FWW_DL_TYPE ((OVS_FORCE flow_wildcards_t) (1 << 4))
> -#define FWW_NW_PROTO ((OVS_FORCE flow_wildcards_t) (1 << 5))
> -/* No corresponding OFPFW10_* bits. */
> -#define FWW_NW_DSCP ((OVS_FORCE flow_wildcards_t) (1 << 1))
> -#define FWW_NW_ECN ((OVS_FORCE flow_wildcards_t) (1 << 2))
> -#define FWW_NW_TTL ((OVS_FORCE flow_wildcards_t) (1 << 3))
> +#define FWW_DL_TYPE ((OVS_FORCE flow_wildcards_t) (1 << 1))
> +#define FWW_NW_PROTO ((OVS_FORCE flow_wildcards_t) (1 << 2))
> +#define FWW_NW_DSCP ((OVS_FORCE flow_wildcards_t) (1 << 3))
> +#define FWW_NW_ECN ((OVS_FORCE flow_wildcards_t) (1 << 4))
> +#define FWW_NW_TTL ((OVS_FORCE flow_wildcards_t) (1 << 5))
> #define FWW_ALL ((OVS_FORCE flow_wildcards_t) (((1 << 6)) - 1))
>
> /* Remember to update FLOW_WC_SEQ when adding or removing FWW_*. */
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 4d5dfbb..ab47778 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -76,27 +76,6 @@ ofputil_netmask_to_wcbits(ovs_be32 netmask)
> return 32 - ip_count_cidr_bits(netmask);
> }
>
> -/* A list of the FWW_* and OFPFW10_ bits that have the same value, meaning,
> and
> - * name. */
> -#define WC_INVARIANT_LIST \
> - WC_INVARIANT_BIT(IN_PORT) \
> - WC_INVARIANT_BIT(DL_TYPE) \
> - WC_INVARIANT_BIT(NW_PROTO)
> -
> -/* Verify that all of the invariant bits (as defined on WC_INVARIANT_LIST)
> - * actually have the same names and values. */
> -#define WC_INVARIANT_BIT(NAME) BUILD_ASSERT_DECL(FWW_##NAME ==
> OFPFW10_##NAME);
> - WC_INVARIANT_LIST
> -#undef WC_INVARIANT_BIT
> -
> -/* WC_INVARIANTS is the invariant bits (as defined on WC_INVARIANT_LIST) all
> - * OR'd together. */
> -static const flow_wildcards_t WC_INVARIANTS = 0
> -#define WC_INVARIANT_BIT(NAME) | FWW_##NAME
> - WC_INVARIANT_LIST
> -#undef WC_INVARIANT_BIT
> -;
> -
> /* Converts the OpenFlow 1.0 wildcards in 'ofpfw' (OFPFW10_*) into a
> * flow_wildcards in 'wc' for use in struct cls_rule. It is the caller's
> * responsibility to handle the special case where the flow match's dl_vlan
> is
> @@ -108,14 +87,20 @@ ofputil_wildcard_from_ofpfw10(uint32_t ofpfw, struct
> flow_wildcards *wc)
>
> /* Initialize most of rule->wc. */
> flow_wildcards_init_catchall(wc);
> - wc->wildcards = (OVS_FORCE flow_wildcards_t) ofpfw & WC_INVARIANTS;
>
> - /* Wildcard fields that aren't defined by ofp10_match. */
> - wc->wildcards |= FWW_NW_ECN | FWW_NW_TTL;
> + /* Start with wildcard fields that aren't defined by ofp10_match. */
> + wc->wildcards = FWW_NW_ECN | FWW_NW_TTL;
>
> + if (ofpfw & OFPFW10_IN_PORT) {
> + wc->wildcards |= FWW_IN_PORT;
> + }
> + if (ofpfw & OFPFW10_DL_TYPE) {
> + wc->wildcards |= FWW_DL_TYPE;
> + }
> + if (ofpfw & OFPFW10_NW_PROTO) {
> + wc->wildcards |= FWW_NW_PROTO;
> + }
> if (ofpfw & OFPFW10_NW_TOS) {
> - /* OpenFlow 1.0 defines a TOS wildcard, but it's much later in
> - * the enum than we can use. */
> wc->wildcards |= FWW_NW_DSCP;
> }
>
> @@ -204,7 +189,16 @@ ofputil_cls_rule_to_ofp10_match(const struct cls_rule
> *rule,
> uint32_t ofpfw;
>
> /* Figure out most OpenFlow wildcards. */
> - ofpfw = (OVS_FORCE uint32_t) (wc->wildcards & WC_INVARIANTS);
> + ofpfw = 0;
> + if (wc->wildcards & FWW_IN_PORT) {
> + ofpfw |= OFPFW10_IN_PORT;
> + }
> + if (wc->wildcards & FWW_DL_TYPE) {
> + ofpfw |= OFPFW10_DL_TYPE;
> + }
> + if (wc->wildcards & FWW_NW_PROTO) {
> + ofpfw |= OFPFW10_NW_PROTO;
> + }
> ofpfw |= (ofputil_netmask_to_wcbits(wc->nw_src_mask)
> << OFPFW10_NW_SRC_SHIFT);
> ofpfw |= (ofputil_netmask_to_wcbits(wc->nw_dst_mask)
> --
> 1.7.2.5
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
