From 18b063cdbf3e59c1922ffd34fd03f2dc5b0c6db6 Mon Sep 17 00:00:00 2001 From: Joe Stringer Date: Tue, 11 Feb 2014 13:55:33 -0800 Subject: [PATCH] upcall: Defer ukey deletion until after pushing stats. It is possible for a datapath to dump the same flow twice, for instance if the flow is the last in a batch of flows to be dumped, then a new flow is inserted into the same bucket before the flow dumper fetches another batch. In this case, datapath flow stats may be duplicated: The revalidator records the stats from the first flow, using the ukey to get the stats delta. The ukey is deleted, then the revalidator reads the second (duplicate) flow and cannot lookup the ukey for the delta. As such, it will push the stats as-is. This patch reduces the likelihood of such stats duplications by deferring ukey deletion until after stats are pushed for deleted flows. Signed-off-by: Joe Stringer Signed-off-by: Ben Pfaff --- ofproto/ofproto-dpif-upcall.c | 39 +++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 16 deletions(-) diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index fbc9ed899..7ff55a0b8 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1375,12 +1375,14 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps) { struct udpif *udpif = revalidator->udpif; - struct { - struct dpif_flow_stats ukey_stats; /* Stats stored in the ukey. */ - struct dpif_flow_stats stats; /* Stats for 'op'. */ - struct dpif_op op; /* Flow del operation. */ - } ops[REVALIDATE_MAX_BATCH]; + struct dump_op { + struct udpif_key *ukey; + struct udpif_flow_dump *udump; + struct dpif_flow_stats stats; /* Stats for 'op'. */ + struct dpif_op op; /* Flow del operation. */ + }; + struct dump_op ops[REVALIDATE_MAX_BATCH]; struct dpif_op *opsp[REVALIDATE_MAX_BATCH]; struct udpif_flow_dump *udump, *next_udump; size_t n_ops, i, n_flows; @@ -1413,21 +1415,15 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps) } if (must_del || (used && used < now - max_idle)) { - struct dpif_flow_stats *ukey_stats = &ops[n_ops].ukey_stats; - struct dpif_op *op = &ops[n_ops].op; + struct dump_op *dop = &ops[n_ops++]; + struct dpif_op *op = &dop->op; + dop->ukey = ukey; + dop->udump = udump; op->type = DPIF_OP_FLOW_DEL; op->u.flow_del.key = udump->key; op->u.flow_del.key_len = udump->key_len; op->u.flow_del.stats = &ops[n_ops].stats; - n_ops++; - - if (ukey) { - *ukey_stats = ukey->stats; - ukey_delete(revalidator, ukey); - } else { - memset(ukey_stats, 0, sizeof *ukey_stats); - } continue; } @@ -1466,7 +1462,7 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps) for (i = 0; i < n_ops; i++) { struct dpif_flow_stats push, *stats, *ukey_stats; - ukey_stats = &ops[i].ukey_stats; + ukey_stats = &ops[i].ukey->stats; stats = ops[i].op.u.flow_del.stats; push.used = MAX(stats->used, ukey_stats->used); push.tcp_flags = stats->tcp_flags | ukey_stats->tcp_flags; @@ -1499,6 +1495,17 @@ revalidate_udumps(struct revalidator *revalidator, struct list *udumps) } } + for (i = 0; i < n_ops; i++) { + struct udpif_key *ukey = ops[i].ukey; + + /* Look up the ukey to prevent double-free if the datapath dumps the + * same flow twice. */ + ukey = ukey_lookup(revalidator, ops[i].udump); + if (ukey) { + ukey_delete(revalidator, ukey); + } + } + LIST_FOR_EACH_SAFE (udump, next_udump, list_node, udumps) { list_remove(&udump->list_node); free(udump); -- 2.20.1