On Aug 7, 2012, at 1:50 PM, Ben Pfaff wrote:
> OVS has all kinds of odd fields, e.g. registers, and it doesn't make sense
> to try to match on all of them. This commit changes learning-switch to
> only try to match on the fields defined by OpenFlow 1.0. That's still not
> minimal, but it's more reasonable.
>
> This commit should not have an immediately visible effect since
> ovs-controller always sends OF1.0 format flows to the switch, and OF1.0
> format flows don't have these extra fields. But in the future when we
> add support for new protocols and flow formats to ovs-controller, it
> will make a difference.
>
> Signed-off-by: Ben Pfaff <[email protected]>
> ---
> lib/learning-switch.c | 37 ++++++++++++++++++---------------
> lib/ofp-util.c | 54 ++++++++++++++++++++++++++++++++----------------
> lib/ofp-util.h | 1 +
> 3 files changed, 57 insertions(+), 35 deletions(-)
>
> diff --git a/lib/learning-switch.c b/lib/learning-switch.c
> index f52d3c9..e4287c4 100644
> --- a/lib/learning-switch.c
> +++ b/lib/learning-switch.c
> @@ -112,6 +112,7 @@ struct lswitch *
> lswitch_create(struct rconn *rconn, const struct lswitch_config *cfg)
> {
> struct lswitch *sw;
> + uint32_t ofpfw;
>
> sw = xzalloc(sizeof *sw);
> sw->rconn = rconn;
> @@ -123,24 +124,26 @@ lswitch_create(struct rconn *rconn, const struct
> lswitch_config *cfg)
> : NULL);
> sw->action_normal = cfg->mode == LSW_NORMAL;
>
> - flow_wildcards_init_exact(&sw->wc);
> - if (cfg->wildcards) {
> - uint32_t ofpfw;
> -
> - if (cfg->wildcards == UINT32_MAX) {
> - /* Try to wildcard as many fields as possible, but we cannot
> - * wildcard all fields. We need in_port to detect moves. We
> need
> - * Ethernet source and dest and VLAN VID to do L2 learning. */
> - ofpfw = (OFPFW10_DL_TYPE | OFPFW10_DL_VLAN_PCP
> - | OFPFW10_NW_SRC_ALL | OFPFW10_NW_DST_ALL
> - | OFPFW10_NW_TOS | OFPFW10_NW_PROTO
> - | OFPFW10_TP_SRC | OFPFW10_TP_DST);
> - } else {
> - ofpfw = cfg->wildcards;
> - }
> + switch (cfg->wildcards) {
> + case 0:
> + ofpfw = 0;
> + break;
>
> - ofputil_wildcard_from_ofpfw10(ofpfw, &sw->wc);
> + case UINT32_MAX:
> + /* Try to wildcard as many fields as possible, but we cannot
> + * wildcard all fields. We need in_port to detect moves. We need
> + * Ethernet source and dest and VLAN VID to do L2 learning. */
> + ofpfw = (OFPFW10_DL_TYPE | OFPFW10_DL_VLAN_PCP
> + | OFPFW10_NW_SRC_ALL | OFPFW10_NW_DST_ALL
> + | OFPFW10_NW_TOS | OFPFW10_NW_PROTO
> + | OFPFW10_TP_SRC | OFPFW10_TP_DST);
> + break;
> +
> + default:
> + ofpfw = cfg->wildcards;
> + break;
> }
> + ofputil_wildcard_from_ofpfw10(ofpfw, &sw->wc);
>
This is nice, I like the look of this better as well.
Everything looks good to me.
Thanks,
Kyle
> sw->default_queue = cfg->default_queue;
> hmap_init(&sw->queue_numbers);
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 1fe30b4..030c812 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -3319,23 +3319,8 @@ ofputil_put_action(enum ofputil_action_code code,
> struct ofpbuf *buf)
> }
> #include "ofp-util.def"
>
> -/* "Normalizes" the wildcards in 'rule'. That means:
> - *
> - * 1. If the type of level N is known, then only the valid fields for that
> - * level may be specified. For example, ARP does not have a TOS field,
> - * so nw_tos must be wildcarded if 'rule' specifies an ARP flow.
> - * Similarly, IPv4 does not have any IPv6 addresses, so ipv6_src and
> - * ipv6_dst (and other fields) must be wildcarded if 'rule' specifies
> an
> - * IPv4 flow.
> - *
> - * 2. If the type of level N is not known (or not understood by Open
> - * vSwitch), then no fields at all for that level may be specified.
> For
> - * example, Open vSwitch does not understand SCTP, an L4 protocol, so
> the
> - * L4 fields tp_src and tp_dst must be wildcarded if 'rule' specifies
> an
> - * SCTP flow.
> - */
> -void
> -ofputil_normalize_rule(struct cls_rule *rule)
> +static void
> +ofputil_normalize_rule__(struct cls_rule *rule, bool may_log)
> {
> enum {
> MAY_NW_ADDR = 1 << 0, /* nw_src, nw_dst */
> @@ -3409,7 +3394,7 @@ ofputil_normalize_rule(struct cls_rule *rule)
>
> /* Log any changes. */
> if (!flow_wildcards_equal(&wc, &rule->wc)) {
> - bool log = !VLOG_DROP_INFO(&bad_ofmsg_rl);
> + bool log = may_log && !VLOG_DROP_INFO(&bad_ofmsg_rl);
> char *pre = log ? cls_rule_to_string(rule) : NULL;
>
> rule->wc = wc;
> @@ -3426,6 +3411,39 @@ ofputil_normalize_rule(struct cls_rule *rule)
> }
> }
>
> +/* "Normalizes" the wildcards in 'rule'. That means:
> + *
> + * 1. If the type of level N is known, then only the valid fields for that
> + * level may be specified. For example, ARP does not have a TOS field,
> + * so nw_tos must be wildcarded if 'rule' specifies an ARP flow.
> + * Similarly, IPv4 does not have any IPv6 addresses, so ipv6_src and
> + * ipv6_dst (and other fields) must be wildcarded if 'rule' specifies
> an
> + * IPv4 flow.
> + *
> + * 2. If the type of level N is not known (or not understood by Open
> + * vSwitch), then no fields at all for that level may be specified.
> For
> + * example, Open vSwitch does not understand SCTP, an L4 protocol, so
> the
> + * L4 fields tp_src and tp_dst must be wildcarded if 'rule' specifies
> an
> + * SCTP flow.
> + *
> + * If this function changes 'rule', it logs a rate-limited informational
> + * message. */
> +void
> +ofputil_normalize_rule(struct cls_rule *rule)
> +{
> + ofputil_normalize_rule__(rule, true);
> +}
> +
> +/* Same as ofputil_normalize_rule() without the logging. Thus, this function
> + * is suitable for a program's internal use, whereas ofputil_normalize_rule()
> + * sense for use on flows received from elsewhere (so that a bug in the
> program
> + * that sent them can be reported and corrected). */
> +void
> +ofputil_normalize_rule_quiet(struct cls_rule *rule)
> +{
> + ofputil_normalize_rule__(rule, false);
> +}
> +
> /* Parses a key or a key-value pair from '*stringp'.
> *
> * On success: Stores the key into '*keyp'. Stores the value, if present,
> into
> diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> index 64f6c39..7b30e87 100644
> --- a/lib/ofp-util.h
> +++ b/lib/ofp-util.h
> @@ -114,6 +114,7 @@ void ofputil_cls_rule_from_ofp10_match(const struct
> ofp10_match *,
> unsigned int priority,
> struct cls_rule *);
> void ofputil_normalize_rule(struct cls_rule *);
> +void ofputil_normalize_rule_quiet(struct cls_rule *);
> void ofputil_cls_rule_to_ofp10_match(const struct cls_rule *,
> struct ofp10_match *);
>
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
