When the datapath flow table is modified while a flow dump operation is
in progress, it is possible for the same flow to be dumped twice. In
such cases, revalidators may perform redundant work, or attempt to
delete the same flow twice.
This was causing intermittent testsuite failures for test #670 -
"ofproto-dpif, active-backup bonding" where a flow (that had not
previously been dumped) was dumped, revalidated and deleted twice.
The logs show errors such as:
"failed to flow_get (No such file or directory) skb_priority(0),..."
"failed to flow_del (No such file or directory) skb_priority(0),..."
This patch adds a 'flow_exists' field to 'struct udpif_key' to track
whether the flow is (in progress) to be deleted. After doing a ukey
lookup, we check whether ukey->mark or ukey->flow indicates that the
flow has already been handled. If it has already been handled, we skip
handling the flow again.
We also defer ukey cleanup for flows that fail revalidation, so that the
ukey will still exist if the same flow is dumped twice. This allows the
above logic to work in this case.
Signed-off-by: Joe Stringer <joestringer@nicira.com>
Acked-by: Alex Wang <alexw@nicira.com>
VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
+COVERAGE_DEFINE(upcall_duplicate_flow);
+
/* A thread that reads upcalls from dpif, forwards each upcall's packet,
* and possibly sets up a kernel flow as a cache. */
struct handler {
/* A thread that reads upcalls from dpif, forwards each upcall's packet,
* and possibly sets up a kernel flow as a cache. */
struct handler {
long long int created; /* Estimation of creation time. */
bool mark; /* Used by mark and sweep GC algorithm. */
long long int created; /* Estimation of creation time. */
bool mark; /* Used by mark and sweep GC algorithm. */
+ bool flow_exists; /* Ensures flows are only deleted once. */
struct odputil_keybuf key_buf; /* Memory for 'key'. */
struct xlate_cache *xcache; /* Cache for xlate entries that
struct odputil_keybuf key_buf; /* Memory for 'key'. */
struct xlate_cache *xcache; /* Cache for xlate entries that
ukey->key_len = key_len;
ukey->mark = false;
ukey->key_len = key_len;
ukey->mark = false;
+ ukey->flow_exists = true;
ukey->created = used ? used : time_msec();
memset(&ukey->stats, 0, sizeof ukey->stats);
ukey->xcache = NULL;
ukey->created = used ? used : time_msec();
memset(&ukey->stats, 0, sizeof ukey->stats);
ukey->xcache = NULL;
+ if (ukey && (ukey->mark || !ukey->flow_exists)) {
+ /* The flow has already been dumped. This can occasionally occur
+ * if the datapath is changed in the middle of a flow dump. Rather
+ * than perform the same work twice, skip the flow this time. */
+ COVERAGE_INC(upcall_duplicate_flow);
+ continue;
+ }
+
if (must_del || (used && used < now - max_idle)) {
struct dump_op *dop = &ops[n_ops++];
if (must_del || (used && used < now - max_idle)) {
struct dump_op *dop = &ops[n_ops++];
+ if (ukey) {
+ ukey->flow_exists = false;
+ }
dump_op_init(dop, udump->key, udump->key_len, ukey, udump);
continue;
}
dump_op_init(dop, udump->key, udump->key_len, ukey, udump);
continue;
}
ukey->mark = true;
if (!revalidate_ukey(udpif, udump, ukey)) {
ukey->mark = true;
if (!revalidate_ukey(udpif, udump, ukey)) {
+ ukey->flow_exists = false;
dpif_flow_del(udpif->dpif, udump->key, udump->key_len, NULL);
dpif_flow_del(udpif->dpif, udump->key, udump->key_len, NULL);
- ukey_delete(revalidator, ukey);
+ /* The ukey will be cleaned up by revalidator_sweep().
+ * This helps to avoid deleting the same flow twice. */
}
list_remove(&udump->list_node);
}
list_remove(&udump->list_node);
HMAP_FOR_EACH_SAFE (ukey, next, hmap_node, &revalidator->ukeys) {
if (!purge && ukey->mark) {
ukey->mark = false;
HMAP_FOR_EACH_SAFE (ukey, next, hmap_node, &revalidator->ukeys) {
if (!purge && ukey->mark) {
ukey->mark = false;
+ } else if (!ukey->flow_exists) {
+ ukey_delete(revalidator, ukey);
} else {
struct dump_op *op = &ops[n_ops++];
} else {
struct dump_op *op = &ops[n_ops++];