[ovs-dev] [threaded-learning 25/25] ofproto-dpif: Move "learn" actions into individual threads.
Ben Pfaff
blp at nicira.com
Wed Sep 11 05:27:25 UTC 2013
Before OVS introduced threading, any given ``learn'' action took effect in
the flow table before the next incoming flow was set up. This meant that
if a packet came in, used ``learn'' to set up a flow to handle replies, and
then a reply came in, the reply would always hit the flow set up by the
``learn'' action. Until now, with the threading implementation, though,
the effects of ``learn'' actions happen asynchronously via a queue, which
means that the reply can arrive before the flow to handle it is set up.
This introduced an unacceptable regression in important use cases.
This commit fixes the problem by switching back to executing learn actions
before forwarding the packet that triggered it.
I imagine that there is considerable opportunity for optimization here.
Bug #19147.
Bug #19244.
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
ofproto/ofproto-dpif-xlate.c | 12 ++++++------
ofproto/ofproto-dpif.c | 38 +++++---------------------------------
ofproto/ofproto-provider.h | 4 ++--
ofproto/ofproto.c | 3 ++-
4 files changed, 15 insertions(+), 42 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 611861c..1ac164b 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2097,7 +2097,8 @@ static void
xlate_learn_action(struct xlate_ctx *ctx,
const struct ofpact_learn *learn)
{
- struct ofputil_flow_mod *fm;
+ uint64_t ofpacts_stub[1024 / 8];
+ struct ofputil_flow_mod fm;
struct ofpbuf ofpacts;
ctx->xout->has_learn = true;
@@ -2108,11 +2109,10 @@ xlate_learn_action(struct xlate_ctx *ctx,
return;
}
- fm = xmalloc(sizeof *fm);
- ofpbuf_init(&ofpacts, 0);
- learn_execute(learn, &ctx->xin->flow, fm, &ofpacts);
-
- ofproto_dpif_flow_mod(ctx->xbridge->ofproto, fm);
+ ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
+ learn_execute(learn, &ctx->xin->flow, &fm, &ofpacts);
+ ofproto_dpif_flow_mod(ctx->xbridge->ofproto, &fm);
+ ofpbuf_uninit(&ofpacts);
}
static void
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c8c861d..2521c05 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -508,7 +508,6 @@ struct ofproto_dpif {
uint64_t n_missed;
/* Work queues. */
- struct guarded_list flow_mods; /* Contains "struct flow_mod"s. */
struct guarded_list pins; /* Contains "struct ofputil_packet_in"s. */
};
@@ -551,16 +550,13 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
/* Initial mappings of port to bridge mappings. */
static struct shash init_ofp_ports = SHASH_INITIALIZER(&init_ofp_ports);
-/* Executes and takes ownership of 'fm'. */
+/* Executes 'fm'. The caller retains ownership of 'fm' and everything in
+ * it. */
void
ofproto_dpif_flow_mod(struct ofproto_dpif *ofproto,
struct ofputil_flow_mod *fm)
{
- if (!guarded_list_append(&ofproto->flow_mods, &fm->list_node, 1024)) {
- COVERAGE_INC(flow_mod_overflow);
- free(fm->ofpacts);
- free(fm);
- }
+ ofproto_flow_mod(&ofproto->up, fm);
}
/* Appends 'pin' to the queue of "packet ins" to be sent to the controller.
@@ -1268,7 +1264,6 @@ construct(struct ofproto *ofproto_)
classifier_init(&ofproto->facets);
ofproto->consistency_rl = LLONG_MIN;
- guarded_list_init(&ofproto->flow_mods);
guarded_list_init(&ofproto->pins);
ofproto_dpif_unixctl_init();
@@ -1393,11 +1388,10 @@ destruct(struct ofproto *ofproto_)
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
struct rule_dpif *rule, *next_rule;
struct ofputil_packet_in *pin, *next_pin;
- struct ofputil_flow_mod *fm, *next_fm;
struct facet *facet, *next_facet;
- struct list flow_mods, pins;
struct cls_cursor cursor;
struct oftable *table;
+ struct list pins;
ovs_rwlock_rdlock(&ofproto->facets.rwlock);
cls_cursor_init(&cursor, &ofproto->facets, NULL);
@@ -1426,14 +1420,6 @@ destruct(struct ofproto *ofproto_)
}
}
- guarded_list_steal_all(&ofproto->flow_mods, &flow_mods);
- LIST_FOR_EACH_SAFE (fm, next_fm, list_node, &flow_mods) {
- list_remove(&fm->list_node);
- free(fm->ofpacts);
- free(fm);
- }
- guarded_list_destroy(&ofproto->flow_mods);
-
guarded_list_steal_all(&ofproto->pins, &pins);
LIST_FOR_EACH_SAFE (pin, next_pin, list_node, &pins) {
list_remove(&pin->list_node);
@@ -1468,9 +1454,8 @@ run_fast(struct ofproto *ofproto_)
{
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
struct ofputil_packet_in *pin, *next_pin;
- struct ofputil_flow_mod *fm, *next_fm;
- struct list flow_mods, pins;
struct ofport_dpif *ofport;
+ struct list pins;
/* Do not perform any periodic activity required by 'ofproto' while
* waiting for flow restore to complete. */
@@ -1478,19 +1463,6 @@ run_fast(struct ofproto *ofproto_)
return 0;
}
- guarded_list_steal_all(&ofproto->flow_mods, &flow_mods);
- LIST_FOR_EACH_SAFE (fm, next_fm, list_node, &flow_mods) {
- int error = ofproto_flow_mod(&ofproto->up, fm);
- if (error && !VLOG_DROP_WARN(&rl)) {
- VLOG_WARN("learning action failed to modify flow table (%s)",
- ofperr_get_name(error));
- }
-
- list_remove(&fm->list_node);
- free(fm->ofpacts);
- free(fm);
- }
-
guarded_list_steal_all(&ofproto->pins, &pins);
LIST_FOR_EACH_SAFE (pin, next_pin, list_node, &pins) {
connmgr_send_packet_in(ofproto->up.connmgr, pin);
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 90fee04..4712dc4 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -368,8 +368,8 @@ struct rule {
struct eviction_group *eviction_group OVS_GUARDED_BY(ofproto_mutex);
struct heap_node evg_node OVS_GUARDED_BY(ofproto_mutex);
- /* OpenFlow actions. See additional thread-safe notes on struct
- * rule_actions. */
+ /* OpenFlow actions. See struct rule_actions for more thread-safety
+ * notes. */
struct rule_actions *actions OVS_GUARDED;
/* In owning meter's 'rules' list. An empty list if there is no meter. */
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 969ad37..f74d14d 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1824,7 +1824,8 @@ ofproto_add_flow(struct ofproto *ofproto, const struct match *match,
* OFPERR_* OpenFlow error code on failure, or OFPROTO_POSTPONE if the
* operation cannot be initiated now but may be retried later.
*
- * This is a helper function for in-band control and fail-open. */
+ * This is a helper function for in-band control and fail-open and the "learn"
+ * action. */
int
ofproto_flow_mod(struct ofproto *ofproto, struct ofputil_flow_mod *fm)
OVS_EXCLUDED(ofproto_mutex)
--
1.7.10.4
More information about the dev
mailing list