From 26d79bf2686b08a49c881d5bb54501d7584f5111 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Fri, 6 Aug 2010 11:46:24 -0700 Subject: [PATCH] vswitchd: Only re-learn from flows that output to OFPP_NORMAL. Commit e96a4d8035 "bridge: Feed flow stats into learning table." started feeding flow statistics back into the learning table, but it did not distinguish between flows with and flows without an action that outputs to OFPP_NORMAL. Flows without such an action are not put into the learning table initially, because bridge_normal_ofhook_cb() is not called for them, but since that commit they have been put into the learning table when their flows are reassessed. This is inconsistent--flows without OFPP_NORMAL should either be learned from all the time or never, not sometimes. I can see valid arguments both ways, but since it was always my intention not to learn from such flows, this commit disables learning from them. Problem found by code inspection. I don't know of any observed bug that this fixes. --- ofproto/ofproto.c | 2 +- ofproto/ofproto.h | 6 +++--- vswitchd/bridge.c | 19 ++++++++++++------- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 461053ab..d3912c17 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -2062,7 +2062,7 @@ rule_account(struct ofproto *ofproto, struct rule *rule, uint64_t extra_bytes) && total_bytes > rule->accounted_bytes) { ofproto->ofhooks->account_flow_cb( - &rule->cr.flow, rule->odp_actions, rule->n_odp_actions, + &rule->cr.flow, rule->tags, rule->odp_actions, rule->n_odp_actions, total_bytes - rule->accounted_bytes, ofproto->aux); rule->accounted_bytes = total_bytes; } diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h index 507c5656..6bac9626 100644 --- a/ofproto/ofproto.h +++ b/ofproto/ofproto.h @@ -145,9 +145,9 @@ struct ofhooks { bool (*normal_cb)(const flow_t *, const struct ofpbuf *packet, struct odp_actions *, tag_type *, uint16_t *nf_output_iface, void *aux); - void (*account_flow_cb)(const flow_t *, const union odp_action *, - size_t n_actions, unsigned long long int n_bytes, - void *aux); + void (*account_flow_cb)(const flow_t *, tag_type tags, + const union odp_action *, size_t n_actions, + unsigned long long int n_bytes, void *aux); void (*account_checkpoint_cb)(void *aux); }; void ofproto_revalidate(struct ofproto *, tag_type); diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 7174f2c8..e2c7fb68 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -2470,11 +2470,12 @@ bridge_normal_ofhook_cb(const flow_t *flow, const struct ofpbuf *packet, struct bridge *br = br_; COVERAGE_INC(bridge_process_flow); + return process_flow(br, flow, packet, actions, tags, nf_output_iface); } static void -bridge_account_flow_ofhook_cb(const flow_t *flow, +bridge_account_flow_ofhook_cb(const flow_t *flow, tag_type tags, const union odp_action *actions, size_t n_actions, unsigned long long int n_bytes, void *br_) @@ -2482,20 +2483,24 @@ bridge_account_flow_ofhook_cb(const flow_t *flow, struct bridge *br = br_; const union odp_action *a; struct port *in_port; - tag_type tags = 0; + tag_type dummy = 0; int vlan; - /* Feed information from the active flows back into the learning table - * to ensure that table is always in sync with what is actually flowing - * through the datapath. */ - if (is_admissible(br, flow, false, &tags, &vlan, &in_port)) { + /* Feed information from the active flows back into the learning table to + * ensure that table is always in sync with what is actually flowing + * through the datapath. + * + * We test that 'tags' is nonzero to ensure that only flows that include an + * OFPP_NORMAL action are used for learning. This works because + * bridge_normal_ofhook_cb() always sets a nonzero tag value. */ + if (tags && is_admissible(br, flow, false, &dummy, &vlan, &in_port)) { update_learning_table(br, flow, vlan, in_port); } + /* Account for bond slave utilization. */ if (!br->has_bonded_ports) { return; } - for (a = actions; a < &actions[n_actions]; a++) { if (a->type == ODPAT_OUTPUT) { struct port *out_port = port_from_dp_ifidx(br, a->output.port); -- 2.30.2