Sounds good.
Ethan
On Tue, Aug 7, 2012 at 1:31 PM, Ben Pfaff <b...@nicira.com> wrote:
> I think you might be right about which way is a little prettier, but
> all this code gets deleted a few commits later so I don't think I'm
> going to tweak the style.
>
> Thanks,
>
> Ben.
>
> On Mon, Jul 30, 2012 at 03:12:37PM -0700, Ethan Jackson wrote:
>> In flow_wildcards_combine(), does it make sense to move the new line
>> above the eth_addr_bitands() so it's with the similar code? Totally
>> aesthetic, so it doesn't matter much. Same in flow_wildcards_hash().
>>
>> Looks like a big win to me, thanks.
>> Ethan
>>
>> On Fri, Jul 20, 2012 at 4:25 PM, Ben Pfaff <b...@nicira.com> wrote:
>> > Signed-off-by: Ben Pfaff <b...@nicira.com>
>> > ---
>> > lib/classifier.c | 11 +++++------
>> > lib/flow.c | 24 ++++++++++++------------
>> > lib/flow.h | 10 ++++------
>> > lib/meta-flow.c | 24 ++++++++++++++++--------
>> > lib/nx-match.c | 4 ++--
>> > lib/ofp-util.c | 16 ++++++++--------
>> > tests/test-classifier.c | 7 +++++--
>> > 7 files changed, 52 insertions(+), 44 deletions(-)
>> >
>> > diff --git a/lib/classifier.c b/lib/classifier.c
>> > index 33245ae..446ca4e 100644
>> > --- a/lib/classifier.c
>> > +++ b/lib/classifier.c
>> > @@ -362,7 +362,7 @@ cls_rule_set_nw_dst_masked(struct cls_rule *rule,
>> > ovs_be32 ip, ovs_be32 mask)
>> > void
>> > cls_rule_set_nw_dscp(struct cls_rule *rule, uint8_t nw_dscp)
>> > {
>> > - rule->wc.wildcards &= ~FWW_NW_DSCP;
>> > + rule->wc.nw_tos_mask |= IP_DSCP_MASK;
>> > rule->flow.nw_tos &= ~IP_DSCP_MASK;
>> > rule->flow.nw_tos |= nw_dscp & IP_DSCP_MASK;
>> > }
>> > @@ -370,7 +370,7 @@ cls_rule_set_nw_dscp(struct cls_rule *rule, uint8_t
>> > nw_dscp)
>> > void
>> > cls_rule_set_nw_ecn(struct cls_rule *rule, uint8_t nw_ecn)
>> > {
>> > - rule->wc.wildcards &= ~FWW_NW_ECN;
>> > + rule->wc.nw_tos_mask |= IP_ECN_MASK;
>> > rule->flow.nw_tos &= ~IP_ECN_MASK;
>> > rule->flow.nw_tos |= nw_ecn & IP_ECN_MASK;
>> > }
>> > @@ -723,10 +723,10 @@ cls_rule_format(const struct cls_rule *rule, struct
>> > ds *s)
>> > format_eth_masked(s, "arp_sha", f->arp_sha, wc->arp_sha_mask);
>> > format_eth_masked(s, "arp_tha", f->arp_tha, wc->arp_tha_mask);
>> > }
>> > - if (!(w & FWW_NW_DSCP)) {
>> > + if (wc->nw_tos_mask & IP_DSCP_MASK) {
>> > ds_put_format(s, "nw_tos=%"PRIu8",", f->nw_tos & IP_DSCP_MASK);
>> > }
>> > - if (!(w & FWW_NW_ECN)) {
>> > + if (wc->nw_tos_mask & IP_ECN_MASK) {
>> > ds_put_format(s, "nw_ecn=%"PRIu8",", f->nw_tos & IP_ECN_MASK);
>> > }
>> > if (!(w & FWW_NW_TTL)) {
>> > @@ -1289,8 +1289,7 @@ flow_equal_except(const struct flow *a, const struct
>> > flow *b,
>> > wildcards->dl_dst_mask)
>> > && (wc & FWW_NW_PROTO || a->nw_proto == b->nw_proto)
>> > && (wc & FWW_NW_TTL || a->nw_ttl == b->nw_ttl)
>> > - && (wc & FWW_NW_DSCP || !((a->nw_tos ^ b->nw_tos) &
>> > IP_DSCP_MASK))
>> > - && (wc & FWW_NW_ECN || !((a->nw_tos ^ b->nw_tos) &
>> > IP_ECN_MASK))
>> > + && !((a->nw_tos ^ b->nw_tos) & wildcards->nw_tos_mask)
>> > && !((a->nw_frag ^ b->nw_frag) & wildcards->nw_frag_mask)
>> > && eth_addr_equal_except(a->arp_sha, b->arp_sha,
>> > wildcards->arp_sha_mask)
>> > diff --git a/lib/flow.c b/lib/flow.c
>> > index 222983a..c0d5a47 100644
>> > --- a/lib/flow.c
>> > +++ b/lib/flow.c
>> > @@ -468,12 +468,7 @@ flow_zero_wildcards(struct flow *flow, const struct
>> > flow_wildcards *wildcards)
>> > flow->nw_proto = 0;
>> > }
>> > flow->ipv6_label &= wildcards->ipv6_label_mask;
>> > - if (wc & FWW_NW_DSCP) {
>> > - flow->nw_tos &= ~IP_DSCP_MASK;
>> > - }
>> > - if (wc & FWW_NW_ECN) {
>> > - flow->nw_tos &= ~IP_ECN_MASK;
>> > - }
>> > + flow->nw_tos &= wildcards->nw_tos_mask;
>> > if (wc & FWW_NW_TTL) {
>> > flow->nw_ttl = 0;
>> > }
>> > @@ -607,7 +602,7 @@ flow_wildcards_init_catchall(struct flow_wildcards *wc)
>> > memset(wc->dl_dst_mask, 0, ETH_ADDR_LEN);
>> > memset(wc->arp_sha_mask, 0, ETH_ADDR_LEN);
>> > memset(wc->arp_tha_mask, 0, ETH_ADDR_LEN);
>> > - memset(wc->zeros, 0, sizeof wc->zeros);
>> > + wc->nw_tos_mask = 0;
>> > }
>> >
>> > /* Initializes 'wc' as an exact-match set of wildcards; that is, 'wc'
>> > does not
>> > @@ -635,7 +630,7 @@ flow_wildcards_init_exact(struct flow_wildcards *wc)
>> > memset(wc->dl_dst_mask, 0xff, ETH_ADDR_LEN);
>> > memset(wc->arp_sha_mask, 0xff, ETH_ADDR_LEN);
>> > memset(wc->arp_tha_mask, 0xff, ETH_ADDR_LEN);
>> > - memset(wc->zeros, 0, sizeof wc->zeros);
>> > + wc->nw_tos_mask = UINT8_MAX;
>> > }
>> >
>> > /* Returns true if 'wc' is exact-match, false if 'wc' wildcards any bits
>> > or
>> > @@ -663,7 +658,8 @@ flow_wildcards_is_exact(const struct flow_wildcards
>> > *wc)
>> > || !ipv6_mask_is_exact(&wc->ipv6_dst_mask)
>> > || wc->ipv6_label_mask != htonl(UINT32_MAX)
>> > || !ipv6_mask_is_exact(&wc->nd_target_mask)
>> > - || wc->nw_frag_mask != UINT8_MAX) {
>> > + || wc->nw_frag_mask != UINT8_MAX
>> > + || wc->nw_tos_mask != UINT8_MAX) {
>> > return false;
>> > }
>> >
>> > @@ -701,7 +697,8 @@ flow_wildcards_is_catchall(const struct flow_wildcards
>> > *wc)
>> > || !ipv6_mask_is_any(&wc->ipv6_dst_mask)
>> > || wc->ipv6_label_mask != htonl(0)
>> > || !ipv6_mask_is_any(&wc->nd_target_mask)
>> > - || wc->nw_frag_mask != 0) {
>> > + || wc->nw_frag_mask != 0
>> > + || wc->nw_tos_mask != 0) {
>> > return false;
>> > }
>> >
>> > @@ -749,6 +746,7 @@ flow_wildcards_combine(struct flow_wildcards *dst,
>> > eth_addr_bitand(src1->dl_dst_mask, src2->dl_dst_mask,
>> > dst->dl_dst_mask);
>> > eth_addr_bitand(src1->arp_sha_mask, src2->arp_sha_mask,
>> > dst->arp_sha_mask);
>> > eth_addr_bitand(src1->arp_tha_mask, src2->arp_tha_mask,
>> > dst->arp_tha_mask);
>> > + dst->nw_tos_mask = src1->nw_tos_mask & src2->nw_tos_mask;
>> > }
>> >
>> > /* Returns a hash of the wildcards in 'wc'. */
>> > @@ -788,7 +786,8 @@ flow_wildcards_equal(const struct flow_wildcards *a,
>> > || !eth_addr_equals(a->dl_src_mask, b->dl_src_mask)
>> > || !eth_addr_equals(a->dl_dst_mask, b->dl_dst_mask)
>> > || !eth_addr_equals(a->arp_sha_mask, b->arp_sha_mask)
>> > - || !eth_addr_equals(a->arp_tha_mask, b->arp_tha_mask)) {
>> > + || !eth_addr_equals(a->arp_tha_mask, b->arp_tha_mask)
>> > + || a->nw_tos_mask != b->nw_tos_mask) {
>> > return false;
>> > }
>> >
>> > @@ -863,7 +862,8 @@ flow_wildcards_has_extra(const struct flow_wildcards
>> > *a,
>> > || (a->metadata_mask & b->metadata_mask) != b->metadata_mask
>> > || (a->tp_src_mask & b->tp_src_mask) != b->tp_src_mask
>> > || (a->tp_dst_mask & b->tp_dst_mask) != b->tp_dst_mask
>> > - || (a->nw_frag_mask & b->nw_frag_mask) != b->nw_frag_mask);
>> > + || (a->nw_frag_mask & b->nw_frag_mask) != b->nw_frag_mask
>> > + || (a->nw_tos_mask & b->nw_tos_mask) != b->nw_tos_mask);
>> > }
>> >
>> > /* Sets the wildcard mask for register 'idx' in 'wc' to 'mask'.
>> > diff --git a/lib/flow.h b/lib/flow.h
>> > index c007758..cc4c98d 100644
>> > --- a/lib/flow.h
>> > +++ b/lib/flow.h
>> > @@ -151,13 +151,11 @@ typedef unsigned int OVS_BITWISE flow_wildcards_t;
>> > #define FWW_IN_PORT ((OVS_FORCE flow_wildcards_t) (1 << 0))
>> > #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))
>> > +#define FWW_NW_TTL ((OVS_FORCE flow_wildcards_t) (1 << 3))
>> > +#define FWW_ALL ((OVS_FORCE flow_wildcards_t) (((1 << 4)) - 1))
>> >
>> > /* Remember to update FLOW_WC_SEQ when adding or removing FWW_*. */
>> > -BUILD_ASSERT_DECL(FWW_ALL == ((1 << 6) - 1) && FLOW_WC_SEQ == 14);
>> > +BUILD_ASSERT_DECL(FWW_ALL == ((1 << 4) - 1) && FLOW_WC_SEQ == 14);
>> >
>> > /* Information on wildcards for a flow, as a supplement to "struct flow".
>> > *
>> > @@ -183,7 +181,7 @@ struct flow_wildcards {
>> > uint8_t dl_dst_mask[6]; /* 1-bit in each significant dl_dst bit.
>> > */
>> > uint8_t arp_sha_mask[6]; /* 1-bit in each significant dl_dst bit.
>> > */
>> > uint8_t arp_tha_mask[6]; /* 1-bit in each significant dl_dst bit.
>> > */
>> > - uint8_t zeros[1]; /* Padding field set to zero. */
>> > + uint8_t nw_tos_mask; /* 1-bit in each significant nw_tos bit.
>> > */
>> > };
>> >
>> > /* Remember to update FLOW_WC_SEQ when updating struct flow_wildcards. */
>> > diff --git a/lib/meta-flow.c b/lib/meta-flow.c
>> > index f1e34a3..49d4bed 100644
>> > --- a/lib/meta-flow.c
>> > +++ b/lib/meta-flow.c
>> > @@ -242,7 +242,7 @@ static const struct mf_field mf_fields[MFF_N_IDS] = {
>> > }, {
>> > MFF_IP_DSCP, "nw_tos", NULL,
>> > MF_FIELD_SIZES(u8),
>> > - MFM_NONE, FWW_NW_DSCP,
>> > + MFM_NONE, 0,
>> > MFS_DECIMAL,
>> > MFP_IP_ANY,
>> > true,
>> > @@ -251,7 +251,7 @@ static const struct mf_field mf_fields[MFF_N_IDS] = {
>> > }, {
>> > MFF_IP_ECN, "nw_ecn", NULL,
>> > 1, 2,
>> > - MFM_NONE, FWW_NW_ECN,
>> > + MFM_NONE, 0,
>> > MFS_DECIMAL,
>> > MFP_IP_ANY,
>> > true,
>> > @@ -558,8 +558,6 @@ mf_is_all_wild(const struct mf_field *mf, const struct
>> > flow_wildcards *wc)
>> > case MFF_IN_PORT:
>> > case MFF_ETH_TYPE:
>> > case MFF_IP_PROTO:
>> > - case MFF_IP_DSCP:
>> > - case MFF_IP_ECN:
>> > case MFF_IP_TTL:
>> > case MFF_ARP_OP:
>> > assert(mf->fww_bit != 0);
>> > @@ -606,6 +604,11 @@ mf_is_all_wild(const struct mf_field *mf, const
>> > struct flow_wildcards *wc)
>> > case MFF_IPV6_LABEL:
>> > return !wc->ipv6_label_mask;
>> >
>> > + case MFF_IP_DSCP:
>> > + return !(wc->nw_tos_mask & IP_DSCP_MASK);
>> > + case MFF_IP_ECN:
>> > + return !(wc->nw_tos_mask & IP_ECN_MASK);
>> > +
>> > case MFF_ND_TARGET:
>> > return ipv6_mask_is_any(&wc->nd_target_mask);
>> >
>> > @@ -648,8 +651,6 @@ mf_get_mask(const struct mf_field *mf, const struct
>> > flow_wildcards *wc,
>> > case MFF_IN_PORT:
>> > case MFF_ETH_TYPE:
>> > case MFF_IP_PROTO:
>> > - case MFF_IP_DSCP:
>> > - case MFF_IP_ECN:
>> > case MFF_IP_TTL:
>> > case MFF_ARP_OP:
>> > assert(mf->fww_bit != 0);
>> > @@ -702,6 +703,13 @@ mf_get_mask(const struct mf_field *mf, const struct
>> > flow_wildcards *wc,
>> > mask->be32 = wc->ipv6_label_mask;
>> > break;
>> >
>> > + case MFF_IP_DSCP:
>> > + mask->u8 = wc->nw_tos_mask & IP_DSCP_MASK;
>> > + break;
>> > + case MFF_IP_ECN:
>> > + mask->u8 = wc->nw_tos_mask & IP_ECN_MASK;
>> > + break;
>> > +
>> > case MFF_ND_TARGET:
>> > mask->ipv6 = wc->nd_target_mask;
>> > break;
>> > @@ -1413,12 +1421,12 @@ mf_set_wild(const struct mf_field *mf, struct
>> > cls_rule *rule)
>> > break;
>> >
>> > case MFF_IP_DSCP:
>> > - rule->wc.wildcards |= FWW_NW_DSCP;
>> > + rule->wc.nw_tos_mask &= ~IP_DSCP_MASK;
>> > rule->flow.nw_tos &= ~IP_DSCP_MASK;
>> > break;
>> >
>> > case MFF_IP_ECN:
>> > - rule->wc.wildcards |= FWW_NW_ECN;
>> > + rule->wc.nw_tos_mask &= ~IP_ECN_MASK;
>> > rule->flow.nw_tos &= ~IP_ECN_MASK;
>> > break;
>> >
>> > diff --git a/lib/nx-match.c b/lib/nx-match.c
>> > index 0f67692..d9ebe1e 100644
>> > --- a/lib/nx-match.c
>> > +++ b/lib/nx-match.c
>> > @@ -433,12 +433,12 @@ nxm_put_ip(struct ofpbuf *b, const struct cls_rule
>> > *cr,
>> >
>> > nxm_put_frag(b, cr);
>> >
>> > - if (!(wc & FWW_NW_DSCP)) {
>> > + if (cr->wc.nw_tos_mask & IP_DSCP_MASK) {
>> > nxm_put_8(b, oxm ? OXM_OF_IP_DSCP : NXM_OF_IP_TOS,
>> > flow->nw_tos & IP_DSCP_MASK);
>> > }
>> >
>> > - if (!(wc & FWW_NW_ECN)) {
>> > + if (cr->wc.nw_tos_mask & IP_ECN_MASK) {
>> > nxm_put_8(b, oxm ? OXM_OF_IP_ECN : NXM_NX_IP_ECN,
>> > flow->nw_tos & IP_ECN_MASK);
>> > }
>> > diff --git a/lib/ofp-util.c b/lib/ofp-util.c
>> > index ab47778..d7ec9da 100644
>> > --- a/lib/ofp-util.c
>> > +++ b/lib/ofp-util.c
>> > @@ -89,7 +89,7 @@ ofputil_wildcard_from_ofpfw10(uint32_t ofpfw, struct
>> > flow_wildcards *wc)
>> > flow_wildcards_init_catchall(wc);
>> >
>> > /* Start with wildcard fields that aren't defined by ofp10_match. */
>> > - wc->wildcards = FWW_NW_ECN | FWW_NW_TTL;
>> > + wc->wildcards = FWW_NW_TTL;
>> >
>> > if (ofpfw & OFPFW10_IN_PORT) {
>> > wc->wildcards |= FWW_IN_PORT;
>> > @@ -100,8 +100,9 @@ ofputil_wildcard_from_ofpfw10(uint32_t ofpfw, struct
>> > flow_wildcards *wc)
>> > if (ofpfw & OFPFW10_NW_PROTO) {
>> > wc->wildcards |= FWW_NW_PROTO;
>> > }
>> > - if (ofpfw & OFPFW10_NW_TOS) {
>> > - wc->wildcards |= FWW_NW_DSCP;
>> > +
>> > + if (!(ofpfw & OFPFW10_NW_TOS)) {
>> > + wc->nw_tos_mask |= IP_DSCP_MASK;
>> > }
>> >
>> > wc->nw_src_mask = ofputil_wcbits_to_netmask(ofpfw >>
>> > OFPFW10_NW_SRC_SHIFT);
>> > @@ -203,7 +204,7 @@ ofputil_cls_rule_to_ofp10_match(const struct cls_rule
>> > *rule,
>> > << OFPFW10_NW_SRC_SHIFT);
>> > ofpfw |= (ofputil_netmask_to_wcbits(wc->nw_dst_mask)
>> > << OFPFW10_NW_DST_SHIFT);
>> > - if (wc->wildcards & FWW_NW_DSCP) {
>> > + if (!(wc->nw_tos_mask & IP_DSCP_MASK)) {
>> > ofpfw |= OFPFW10_NW_TOS;
>> > }
>> > if (!wc->tp_src_mask) {
>> > @@ -473,7 +474,7 @@ ofputil_cls_rule_to_ofp11_match(const struct cls_rule
>> > *rule,
>> > match->dl_type = ofputil_dl_type_to_openflow(rule->flow.dl_type);
>> > }
>> >
>> > - if (rule->wc.wildcards & FWW_NW_DSCP) {
>> > + if (!(rule->wc.nw_tos_mask & IP_DSCP_MASK)) {
>> > wc |= OFPFW11_NW_TOS;
>> > } else {
>> > match->nw_tos = rule->flow.nw_tos & IP_DSCP_MASK;
>> > @@ -1506,7 +1507,7 @@ ofputil_usable_protocols(const struct cls_rule *rule)
>> > }
>> >
>> > /* Only NXM supports matching IP ECN bits. */
>> > - if (!(wc->wildcards & FWW_NW_ECN)) {
>> > + if (wc->nw_tos_mask & IP_ECN_MASK) {
>> > return OFPUTIL_P_NXM_ANY;
>> > }
>> >
>> > @@ -4098,8 +4099,7 @@ ofputil_normalize_rule(struct cls_rule *rule)
>> > wc.wildcards |= FWW_NW_PROTO;
>> > }
>> > if (!(may_match & MAY_IPVx)) {
>> > - wc.wildcards |= FWW_NW_DSCP;
>> > - wc.wildcards |= FWW_NW_ECN;
>> > + wc.nw_tos_mask = 0;
>> > wc.wildcards |= FWW_NW_TTL;
>> > }
>> > if (!(may_match & MAY_ARP_SHA)) {
>> > diff --git a/tests/test-classifier.c b/tests/test-classifier.c
>> > index e7cf734..a978afb 100644
>> > --- a/tests/test-classifier.c
>> > +++ b/tests/test-classifier.c
>> > @@ -56,7 +56,7 @@
>> > CLS_FIELD(0, dl_src, DL_SRC) \
>> > CLS_FIELD(0, dl_dst, DL_DST) \
>> > CLS_FIELD(FWW_NW_PROTO, nw_proto, NW_PROTO) \
>> > - CLS_FIELD(FWW_NW_DSCP, nw_tos, NW_DSCP)
>> > + CLS_FIELD(0, nw_tos, NW_DSCP)
>> >
>> > /* Field indexes.
>> > *
>> > @@ -218,7 +218,8 @@ match(const struct cls_rule *wild, const struct flow
>> > *fixed)
>> > eq = !((fixed->metadata ^ wild->flow.metadata)
>> > & wild->wc.metadata_mask);
>> > } else if (f_idx == CLS_F_IDX_NW_DSCP) {
>> > - eq = !((fixed->nw_tos ^ wild->flow.nw_tos) & IP_DSCP_MASK);
>> > + eq = !((fixed->nw_tos ^ wild->flow.nw_tos) &
>> > + (wild->wc.nw_tos_mask & IP_DSCP_MASK));
>> > } else {
>> > NOT_REACHED();
>> > }
>> > @@ -499,6 +500,8 @@ make_rule(int wc_fields, unsigned int priority, int
>> > value_pat)
>> > rule->cls_rule.wc.tun_id_mask = htonll(UINT64_MAX);
>> > } else if (f_idx == CLS_F_IDX_METADATA) {
>> > rule->cls_rule.wc.metadata_mask = htonll(UINT64_MAX);
>> > + } else if (f_idx == CLS_F_IDX_NW_DSCP) {
>> > + rule->cls_rule.wc.nw_tos_mask |= IP_DSCP_MASK;
>> > } else {
>> > NOT_REACHED();
>> > }
>> > --
>> > 1.7.2.5
>> >
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
