| Message ID | 20160831180605.20969-4-joe@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
With small nits below, Acked-by: Jarno Rajahalme <jarno@ovn.org> > On Aug 31, 2016, at 11:06 AM, Joe Stringer <joe@ovn.org> wrote: > > Ukeys already have a well-defined lifetime that starts from being > created, inserted into the umaps, having the corresponding flow > installed, then the flow deleted, the ukey removed from the umap, > rcu-deferral of its deletion, and finally freedom. > > However, until now it's all been represented behind a simple boolean > "flow_exists" with a bunch of implicit logic sprinkled around the > accessors. This patch attempts to make the ukey lifetime a bit clearer > by outlining the correct transitions and asserting that their lifetime > proceeds as expected. > > This should improve the readability of the current code, and also make > the following patch easier to reason about. > > Signed-off-by: Joe Stringer <joe@ovn.org> > --- > v2: First post. > --- > ofproto/ofproto-dpif-upcall.c | 140 ++++++++++++++++++++++++++++-------------- > 1 file changed, 94 insertions(+), 46 deletions(-) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index c4034f57f33e..ce5a392caf78 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -236,6 +236,17 @@ struct upcall { > uint64_t odp_actions_stub[1024 / 8]; /* Stub for odp_actions. */ > }; > > +/* Ukeys must transition through these states using transition_ukey(). */ > +enum ukey_state { > + UKEY_CREATED = 0, > + UKEY_VISIBLE, /* Ukey is in umap, datapath flow install is queued. */ > + UKEY_OPERATIONAL, /* Ukey is in umap, datapath flow is installed. */ > + UKEY_EVICTING, /* Ukey is in umap, datapath flow delete is queued. */ > + UKEY_EVICTED, /* Ukey is in umap, datapath flow is deleted. */ > + UKEY_DELETED, /* Ukey removed from umap, ukey free is deferred. */ > +}; > +#define N_UKEY_STATES (UKEY_DELETED + 1) > + > /* 'udpif_key's are responsible for tracking the little bit of state udpif > * needs to do flow expiration which can't be pulled directly from the > * datapath. They may be created by any handler or revalidator thread at any > @@ -266,8 +277,8 @@ struct udpif_key { > long long int created OVS_GUARDED; /* Estimate of creation time. */ > uint64_t dump_seq OVS_GUARDED; /* Tracks udpif->dump_seq. */ > uint64_t reval_seq OVS_GUARDED; /* Tracks udpif->reval_seq. */ > - bool flow_exists OVS_GUARDED; /* Ensures flows are only deleted > - once. */ > + enum ukey_state state OVS_GUARDED; /* Tracks ukey lifetime. */ > + > /* Datapath flow actions as nlattrs. Protected by RCU. Read with > * ukey_get_actions(), and write with ukey_set_actions(). */ > OVSRCU_TYPE(struct ofpbuf *) actions; > @@ -334,9 +345,9 @@ static int ukey_create_from_dpif_flow(const struct udpif *, > struct udpif_key **); > static void ukey_get_actions(struct udpif_key *, const struct nlattr **actions, > size_t *size); > -static bool ukey_install_start(struct udpif *, struct udpif_key *ukey); > -static bool ukey_install_finish(struct udpif_key *ukey, int error); > +static bool ukey_install__(struct udpif *, struct udpif_key *ukey); You need OVS_TRY_LOCK(true, ukey->mutex) here as, and in general all declarations should have the same thread safety annotations as the definitions themselves. > static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey); > +static void transition_ukey(struct udpif_key *, enum ukey_state dat); You need OVS_REQUIRES(ukey->mutex) here for thread safety analysis to be done. > static struct udpif_key *ukey_lookup(struct udpif *udpif, > const ovs_u128 *ufid, > const unsigned pmd_id); > @@ -349,6 +360,8 @@ static enum upcall_type classify_upcall(enum dpif_upcall_type type, > > static void put_op_init(struct ukey_op *op, struct udpif_key *ukey, > enum dpif_flow_put_flags flags); > +static void delete_op_init(struct udpif *udpif, struct ukey_op *op, > + struct udpif_key *ukey); > > static int upcall_receive(struct upcall *, const struct dpif_backer *, > const struct dp_packet *packet, enum dpif_upcall_type, > @@ -1337,7 +1350,7 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls, > if (should_install_flow(udpif, upcall)) { > struct udpif_key *ukey = upcall->ukey; > > - if (ukey_install_start(udpif, ukey)) { > + if (ukey_install(udpif, ukey)) { > upcall->ukey_persists = true; > put_op_init(&ops[n_ops++], ukey, DPIF_FP_CREATE); > } > @@ -1359,20 +1372,23 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls, > } > } > > - /* Execute batch. > - * > - * We install ukeys before installing the flows, locking them for exclusive > - * access by this thread for the period of installation. This ensures that > - * other threads won't attempt to delete the flows as we are creating them. > - */ > + /* Execute batch. */ > n_opsp = 0; > for (i = 0; i < n_ops; i++) { > opsp[n_opsp++] = &ops[i].dop; > } > dpif_operate(udpif->dpif, opsp, n_opsp); > for (i = 0; i < n_ops; i++) { > - if (ops[i].ukey) { > - ukey_install_finish(ops[i].ukey, ops[i].dop.error); > + struct udpif_key *ukey = ops[i].ukey; > + > + if (ukey) { > + if (ops[i].dop.error) { > + transition_ukey(ukey, UKEY_EVICTED); > + } else { > + ovs_mutex_lock(&ukey->mutex); > + transition_ukey(ukey, UKEY_OPERATIONAL); > + ovs_mutex_unlock(&ukey->mutex); > + } Upper transition_ukey() requires the mutex as well. > } > } > } > @@ -1445,7 +1461,7 @@ ukey_create__(const struct nlattr *key, size_t key_len, > ovs_mutex_init(&ukey->mutex); > ukey->dump_seq = dump_seq; > ukey->reval_seq = reval_seq; > - ukey->flow_exists = false; > + ukey->state = UKEY_CREATED; > ukey->created = time_msec(); > memset(&ukey->stats, 0, sizeof ukey->stats); > ukey->stats.used = used; > @@ -1557,7 +1573,7 @@ ukey_create_from_dpif_flow(const struct udpif *udpif, > * On success, returns true, installs the ukey and returns it in a locked > * state. Otherwise, returns false. */ > static bool > -ukey_install_start(struct udpif *udpif, struct udpif_key *new_ukey) > +ukey_install__(struct udpif *udpif, struct udpif_key *new_ukey) > OVS_TRY_LOCK(true, new_ukey->mutex) > { > struct umap *umap; > @@ -1591,6 +1607,7 @@ ukey_install_start(struct udpif *udpif, struct udpif_key *new_ukey) > } else { > ovs_mutex_lock(&new_ukey->mutex); > cmap_insert(&umap->cmap, &new_ukey->cmap_node, new_ukey->hash); > + transition_ukey(new_ukey, UKEY_VISIBLE); > locked = true; > } > ovs_mutex_unlock(&umap->mutex); > @@ -1599,37 +1616,57 @@ ukey_install_start(struct udpif *udpif, struct udpif_key *new_ukey) > } > > static void > -ukey_install_finish__(struct udpif_key *ukey) OVS_REQUIRES(ukey->mutex) > +transition_ukey(struct udpif_key *ukey, enum ukey_state dst) > + OVS_REQUIRES(ukey->mutex) > { > - ukey->flow_exists = true; > -} > + ovs_assert(dst >= ukey->state); > + if (ukey->state == dst) { > + return; > + } > + > + /* Valid state transitions: > + * UKEY_CREATED -> UKEY_VISIBLE > + * Ukey is now visible in the umap. > + * UKEY_VISIBLE -> UKEY_OPERATIONAL > + * A handler has installed the flow, and the flow is in the datapath. > + * UKEY_VISIBLE -> UKEY_EVICTING > + * A handler installs the flow, then revalidator sweeps the ukey before > + * the flow is dumped. Most likely the flow was installed; start trying > + * to delete it. > + * UKEY_VISIBLE -> UKEY_EVICTED > + * A handler attempts to install the flow, but the datapath rejects it. > + * Consider that the datapath has already destroyed it. > + * UKEY_OPERATIONAL -> UKEY_EVICTING > + * A revalidator decides to evict the datapath flow. > + * UKEY_EVICTING -> UKEY_EVICTED > + * A revalidator has evicted the datapath flow. > + * UKEY_EVICTED -> UKEY_DELETED > + * A revalidator has removed the ukey from the umap and is deleting it. > + */ > + if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE && > + dst < UKEY_DELETED)) { > + ukey->state = dst; > + } else { > + struct ds ds = DS_EMPTY_INITIALIZER; > > -static bool > -ukey_install_finish(struct udpif_key *ukey, int error) > - OVS_RELEASES(ukey->mutex) > -{ > - if (!error) { > - ukey_install_finish__(ukey); > + odp_format_ufid(&ukey->ufid, &ds); > + VLOG_WARN("Invalid state transition for ukey %s: %d -> %d", > + ds_cstr(&ds), ukey->state, dst); > + ds_destroy(&ds); > } > - ovs_mutex_unlock(&ukey->mutex); > - > - return !error; > } > > static bool > ukey_install(struct udpif *udpif, struct udpif_key *ukey) > { > - /* The usual way to keep 'ukey->flow_exists' in sync with the datapath is > - * to call ukey_install_start(), install the corresponding datapath flow, > - * then call ukey_install_finish(). The netdev interface using upcall_cb() > - * doesn't provide a function to separately finish the flow installation, > - * so we perform the operations together here. > - * > - * This is fine currently, as revalidator threads will only delete this > - * ukey during revalidator_sweep() and only if the dump_seq is mismatched. > - * It is unlikely for a revalidator thread to advance dump_seq and reach > - * the next GC phase between ukey creation and flow installation. */ > - return ukey_install_start(udpif, ukey) && ukey_install_finish(ukey, 0); > + bool installed; > + > + installed = ukey_install__(udpif, ukey); > + if (installed) { > + ovs_mutex_unlock(&ukey->mutex); > + } > + > + return installed; > } > > /* Searches for a ukey in 'udpif->ukeys' that matches 'flow' and attempts to > @@ -1665,9 +1702,8 @@ ukey_acquire(struct udpif *udpif, const struct dpif_flow *flow, > if (retval) { > goto done; > } > - install = ukey_install_start(udpif, ukey); > + install = ukey_install__(udpif, ukey); > if (install) { > - ukey_install_finish__(ukey); > retval = 0; > } else { > ukey_delete__(ukey); > @@ -1705,8 +1741,11 @@ static void > ukey_delete(struct umap *umap, struct udpif_key *ukey) > OVS_REQUIRES(umap->mutex) > { > + ovs_mutex_lock(&ukey->mutex); > cmap_remove(&umap->cmap, &ukey->cmap_node, ukey->hash); > ovsrcu_postpone(ukey_delete__, ukey); > + transition_ukey(ukey, UKEY_DELETED); > + ovs_mutex_unlock(&ukey->mutex); > } > > static bool > @@ -1969,6 +2008,7 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops, size_t n_ops) > > if (op->ukey) { > ovs_mutex_lock(&op->ukey->mutex); > + transition_ukey(op->ukey, UKEY_EVICTED); > push->used = MAX(stats->used, op->ukey->stats.used); > push->tcp_flags = stats->tcp_flags | op->ukey->stats.tcp_flags; > push->n_packets = stats->n_packets - op->ukey->stats.n_packets; > @@ -2058,9 +2098,11 @@ static void > reval_op_init(struct ukey_op *op, enum reval_result result, > struct udpif *udpif, struct udpif_key *ukey, > struct recirc_refs *recircs, struct ofpbuf *odp_actions) > + OVS_REQUIRES(ukey->mutex) > { > if (result == UKEY_DELETE) { > delete_op_init(udpif, op, ukey); > + transition_ukey(ukey, UKEY_EVICTING); > } else if (result == UKEY_MODIFY) { > /* Store the new recircs. */ > recirc_refs_swap(&ukey->recircs, recircs); > @@ -2159,6 +2201,9 @@ revalidate(struct revalidator *revalidator) > continue; > } > > + /* The flow is now confirmed to be in the datapath. */ > + transition_ukey(ukey, UKEY_OPERATIONAL); > + > if (!used) { > used = ukey->created; > } > @@ -2169,7 +2214,6 @@ revalidate(struct revalidator *revalidator) > reval_seq, &recircs); > } > ukey->dump_seq = dump_seq; > - ukey->flow_exists = result != UKEY_DELETE; > > if (result != UKEY_KEEP) { > /* Takes ownership of 'recircs'. */ > @@ -2223,15 +2267,16 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) > size_t n_ops = 0; > > CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) { > - bool flow_exists; > + enum ukey_state ukey_state; > > /* Handler threads could be holding a ukey lock while it installs a > * new flow, so don't hang around waiting for access to it. */ > if (ovs_mutex_trylock(&ukey->mutex)) { > continue; > } > - flow_exists = ukey->flow_exists; > - if (flow_exists) { > + ukey_state = ukey->state; > + if (ukey_state == UKEY_OPERATIONAL > + || (ukey_state == UKEY_VISIBLE && purge)) { > struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER; > bool seq_mismatch = (ukey->dump_seq != dump_seq > && ukey->reval_seq != reval_seq); > @@ -2256,7 +2301,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) > } > ovs_mutex_unlock(&ukey->mutex); > > - if (!flow_exists) { > + if (ukey_state == UKEY_EVICTED) { > /* The common flow deletion case involves deletion of the flow > * during the dump phase and ukey deletion here. */ > ovs_mutex_lock(&umap->mutex); > @@ -2296,6 +2341,7 @@ revalidator_purge(struct revalidator *revalidator) > /* In reaction to dpif purge, purges all 'ukey's with same 'pmd_id'. */ > static void > dp_purge_cb(void *aux, unsigned pmd_id) > + OVS_NO_THREAD_SAFETY_ANALYSIS > { > struct udpif *udpif = aux; > size_t i; > @@ -2308,8 +2354,10 @@ dp_purge_cb(void *aux, unsigned pmd_id) > size_t n_ops = 0; > > CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) { > - if (ukey->pmd_id == pmd_id) { > + if (ukey->pmd_id == pmd_id) { > delete_op_init(udpif, &ops[n_ops++], ukey); > + transition_ukey(ukey, UKEY_EVICTING); > + > if (n_ops == REVALIDATE_MAX_BATCH) { > push_ukey_ops(udpif, umap, ops, n_ops); > n_ops = 0; > -- > 2.9.3 >
On 31 August 2016 at 13:17, Jarno Rajahalme <jarno@ovn.org> wrote: > With small nits below, > > Acked-by: Jarno Rajahalme <jarno@ovn.org> Thanks, I also noticed a couple of VLOGs missing their ratelimiters. >> @@ -334,9 +345,9 @@ static int ukey_create_from_dpif_flow(const struct udpif *, >> struct udpif_key **); >> static void ukey_get_actions(struct udpif_key *, const struct nlattr **actions, >> size_t *size); >> -static bool ukey_install_start(struct udpif *, struct udpif_key *ukey); >> -static bool ukey_install_finish(struct udpif_key *ukey, int error); >> +static bool ukey_install__(struct udpif *, struct udpif_key *ukey); > > You need OVS_TRY_LOCK(true, ukey->mutex) here as, and in general all declarations should have the same thread safety annotations as the definitions themselves. OK. I wasn't sure whether this was necessary. >> static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey); >> +static void transition_ukey(struct udpif_key *, enum ukey_state dat); > > You need OVS_REQUIRES(ukey->mutex) here for thread safety analysis to be done. Ack, will update. >> @@ -1359,20 +1372,23 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls, >> } >> } >> >> - /* Execute batch. >> - * >> - * We install ukeys before installing the flows, locking them for exclusive >> - * access by this thread for the period of installation. This ensures that >> - * other threads won't attempt to delete the flows as we are creating them. >> - */ >> + /* Execute batch. */ >> n_opsp = 0; >> for (i = 0; i < n_ops; i++) { >> opsp[n_opsp++] = &ops[i].dop; >> } >> dpif_operate(udpif->dpif, opsp, n_opsp); >> for (i = 0; i < n_ops; i++) { >> - if (ops[i].ukey) { >> - ukey_install_finish(ops[i].ukey, ops[i].dop.error); >> + struct udpif_key *ukey = ops[i].ukey; >> + >> + if (ukey) { >> + if (ops[i].dop.error) { >> + transition_ukey(ukey, UKEY_EVICTED); >> + } else { >> + ovs_mutex_lock(&ukey->mutex); >> + transition_ukey(ukey, UKEY_OPERATIONAL); >> + ovs_mutex_unlock(&ukey->mutex); >> + } > > Upper transition_ukey() requires the mutex as well. True. I was thinking that the upper was still in UKEY_CREATED state, but it is actually UKEY_VISIBLE which means someone else could look at it. I'll update this.
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index c4034f57f33e..ce5a392caf78 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -236,6 +236,17 @@ struct upcall { uint64_t odp_actions_stub[1024 / 8]; /* Stub for odp_actions. */ }; +/* Ukeys must transition through these states using transition_ukey(). */ +enum ukey_state { + UKEY_CREATED = 0, + UKEY_VISIBLE, /* Ukey is in umap, datapath flow install is queued. */ + UKEY_OPERATIONAL, /* Ukey is in umap, datapath flow is installed. */ + UKEY_EVICTING, /* Ukey is in umap, datapath flow delete is queued. */ + UKEY_EVICTED, /* Ukey is in umap, datapath flow is deleted. */ + UKEY_DELETED, /* Ukey removed from umap, ukey free is deferred. */ +}; +#define N_UKEY_STATES (UKEY_DELETED + 1) + /* 'udpif_key's are responsible for tracking the little bit of state udpif * needs to do flow expiration which can't be pulled directly from the * datapath. They may be created by any handler or revalidator thread at any @@ -266,8 +277,8 @@ struct udpif_key { long long int created OVS_GUARDED; /* Estimate of creation time. */ uint64_t dump_seq OVS_GUARDED; /* Tracks udpif->dump_seq. */ uint64_t reval_seq OVS_GUARDED; /* Tracks udpif->reval_seq. */ - bool flow_exists OVS_GUARDED; /* Ensures flows are only deleted - once. */ + enum ukey_state state OVS_GUARDED; /* Tracks ukey lifetime. */ + /* Datapath flow actions as nlattrs. Protected by RCU. Read with * ukey_get_actions(), and write with ukey_set_actions(). */ OVSRCU_TYPE(struct ofpbuf *) actions; @@ -334,9 +345,9 @@ static int ukey_create_from_dpif_flow(const struct udpif *, struct udpif_key **); static void ukey_get_actions(struct udpif_key *, const struct nlattr **actions, size_t *size); -static bool ukey_install_start(struct udpif *, struct udpif_key *ukey); -static bool ukey_install_finish(struct udpif_key *ukey, int error); +static bool ukey_install__(struct udpif *, struct udpif_key *ukey); static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey); +static void transition_ukey(struct udpif_key *, enum ukey_state dst); static struct udpif_key *ukey_lookup(struct udpif *udpif, const ovs_u128 *ufid, const unsigned pmd_id); @@ -349,6 +360,8 @@ static enum upcall_type classify_upcall(enum dpif_upcall_type type, static void put_op_init(struct ukey_op *op, struct udpif_key *ukey, enum dpif_flow_put_flags flags); +static void delete_op_init(struct udpif *udpif, struct ukey_op *op, + struct udpif_key *ukey); static int upcall_receive(struct upcall *, const struct dpif_backer *, const struct dp_packet *packet, enum dpif_upcall_type, @@ -1337,7 +1350,7 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls, if (should_install_flow(udpif, upcall)) { struct udpif_key *ukey = upcall->ukey; - if (ukey_install_start(udpif, ukey)) { + if (ukey_install(udpif, ukey)) { upcall->ukey_persists = true; put_op_init(&ops[n_ops++], ukey, DPIF_FP_CREATE); } @@ -1359,20 +1372,23 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls, } } - /* Execute batch. - * - * We install ukeys before installing the flows, locking them for exclusive - * access by this thread for the period of installation. This ensures that - * other threads won't attempt to delete the flows as we are creating them. - */ + /* Execute batch. */ n_opsp = 0; for (i = 0; i < n_ops; i++) { opsp[n_opsp++] = &ops[i].dop; } dpif_operate(udpif->dpif, opsp, n_opsp); for (i = 0; i < n_ops; i++) { - if (ops[i].ukey) { - ukey_install_finish(ops[i].ukey, ops[i].dop.error); + struct udpif_key *ukey = ops[i].ukey; + + if (ukey) { + if (ops[i].dop.error) { + transition_ukey(ukey, UKEY_EVICTED); + } else { + ovs_mutex_lock(&ukey->mutex); + transition_ukey(ukey, UKEY_OPERATIONAL); + ovs_mutex_unlock(&ukey->mutex); + } } } } @@ -1445,7 +1461,7 @@ ukey_create__(const struct nlattr *key, size_t key_len, ovs_mutex_init(&ukey->mutex); ukey->dump_seq = dump_seq; ukey->reval_seq = reval_seq; - ukey->flow_exists = false; + ukey->state = UKEY_CREATED; ukey->created = time_msec(); memset(&ukey->stats, 0, sizeof ukey->stats); ukey->stats.used = used; @@ -1557,7 +1573,7 @@ ukey_create_from_dpif_flow(const struct udpif *udpif, * On success, returns true, installs the ukey and returns it in a locked * state. Otherwise, returns false. */ static bool -ukey_install_start(struct udpif *udpif, struct udpif_key *new_ukey) +ukey_install__(struct udpif *udpif, struct udpif_key *new_ukey) OVS_TRY_LOCK(true, new_ukey->mutex) { struct umap *umap; @@ -1591,6 +1607,7 @@ ukey_install_start(struct udpif *udpif, struct udpif_key *new_ukey) } else { ovs_mutex_lock(&new_ukey->mutex); cmap_insert(&umap->cmap, &new_ukey->cmap_node, new_ukey->hash); + transition_ukey(new_ukey, UKEY_VISIBLE); locked = true; } ovs_mutex_unlock(&umap->mutex); @@ -1599,37 +1616,57 @@ ukey_install_start(struct udpif *udpif, struct udpif_key *new_ukey) } static void -ukey_install_finish__(struct udpif_key *ukey) OVS_REQUIRES(ukey->mutex) +transition_ukey(struct udpif_key *ukey, enum ukey_state dst) + OVS_REQUIRES(ukey->mutex) { - ukey->flow_exists = true; -} + ovs_assert(dst >= ukey->state); + if (ukey->state == dst) { + return; + } + + /* Valid state transitions: + * UKEY_CREATED -> UKEY_VISIBLE + * Ukey is now visible in the umap. + * UKEY_VISIBLE -> UKEY_OPERATIONAL + * A handler has installed the flow, and the flow is in the datapath. + * UKEY_VISIBLE -> UKEY_EVICTING + * A handler installs the flow, then revalidator sweeps the ukey before + * the flow is dumped. Most likely the flow was installed; start trying + * to delete it. + * UKEY_VISIBLE -> UKEY_EVICTED + * A handler attempts to install the flow, but the datapath rejects it. + * Consider that the datapath has already destroyed it. + * UKEY_OPERATIONAL -> UKEY_EVICTING + * A revalidator decides to evict the datapath flow. + * UKEY_EVICTING -> UKEY_EVICTED + * A revalidator has evicted the datapath flow. + * UKEY_EVICTED -> UKEY_DELETED + * A revalidator has removed the ukey from the umap and is deleting it. + */ + if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE && + dst < UKEY_DELETED)) { + ukey->state = dst; + } else { + struct ds ds = DS_EMPTY_INITIALIZER; -static bool -ukey_install_finish(struct udpif_key *ukey, int error) - OVS_RELEASES(ukey->mutex) -{ - if (!error) { - ukey_install_finish__(ukey); + odp_format_ufid(&ukey->ufid, &ds); + VLOG_WARN("Invalid state transition for ukey %s: %d -> %d", + ds_cstr(&ds), ukey->state, dst); + ds_destroy(&ds); } - ovs_mutex_unlock(&ukey->mutex); - - return !error; } static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey) { - /* The usual way to keep 'ukey->flow_exists' in sync with the datapath is - * to call ukey_install_start(), install the corresponding datapath flow, - * then call ukey_install_finish(). The netdev interface using upcall_cb() - * doesn't provide a function to separately finish the flow installation, - * so we perform the operations together here. - * - * This is fine currently, as revalidator threads will only delete this - * ukey during revalidator_sweep() and only if the dump_seq is mismatched. - * It is unlikely for a revalidator thread to advance dump_seq and reach - * the next GC phase between ukey creation and flow installation. */ - return ukey_install_start(udpif, ukey) && ukey_install_finish(ukey, 0); + bool installed; + + installed = ukey_install__(udpif, ukey); + if (installed) { + ovs_mutex_unlock(&ukey->mutex); + } + + return installed; } /* Searches for a ukey in 'udpif->ukeys' that matches 'flow' and attempts to @@ -1665,9 +1702,8 @@ ukey_acquire(struct udpif *udpif, const struct dpif_flow *flow, if (retval) { goto done; } - install = ukey_install_start(udpif, ukey); + install = ukey_install__(udpif, ukey); if (install) { - ukey_install_finish__(ukey); retval = 0; } else { ukey_delete__(ukey); @@ -1705,8 +1741,11 @@ static void ukey_delete(struct umap *umap, struct udpif_key *ukey) OVS_REQUIRES(umap->mutex) { + ovs_mutex_lock(&ukey->mutex); cmap_remove(&umap->cmap, &ukey->cmap_node, ukey->hash); ovsrcu_postpone(ukey_delete__, ukey); + transition_ukey(ukey, UKEY_DELETED); + ovs_mutex_unlock(&ukey->mutex); } static bool @@ -1969,6 +2008,7 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops, size_t n_ops) if (op->ukey) { ovs_mutex_lock(&op->ukey->mutex); + transition_ukey(op->ukey, UKEY_EVICTED); push->used = MAX(stats->used, op->ukey->stats.used); push->tcp_flags = stats->tcp_flags | op->ukey->stats.tcp_flags; push->n_packets = stats->n_packets - op->ukey->stats.n_packets; @@ -2058,9 +2098,11 @@ static void reval_op_init(struct ukey_op *op, enum reval_result result, struct udpif *udpif, struct udpif_key *ukey, struct recirc_refs *recircs, struct ofpbuf *odp_actions) + OVS_REQUIRES(ukey->mutex) { if (result == UKEY_DELETE) { delete_op_init(udpif, op, ukey); + transition_ukey(ukey, UKEY_EVICTING); } else if (result == UKEY_MODIFY) { /* Store the new recircs. */ recirc_refs_swap(&ukey->recircs, recircs); @@ -2159,6 +2201,9 @@ revalidate(struct revalidator *revalidator) continue; } + /* The flow is now confirmed to be in the datapath. */ + transition_ukey(ukey, UKEY_OPERATIONAL); + if (!used) { used = ukey->created; } @@ -2169,7 +2214,6 @@ revalidate(struct revalidator *revalidator) reval_seq, &recircs); } ukey->dump_seq = dump_seq; - ukey->flow_exists = result != UKEY_DELETE; if (result != UKEY_KEEP) { /* Takes ownership of 'recircs'. */ @@ -2223,15 +2267,16 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) size_t n_ops = 0; CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) { - bool flow_exists; + enum ukey_state ukey_state; /* Handler threads could be holding a ukey lock while it installs a * new flow, so don't hang around waiting for access to it. */ if (ovs_mutex_trylock(&ukey->mutex)) { continue; } - flow_exists = ukey->flow_exists; - if (flow_exists) { + ukey_state = ukey->state; + if (ukey_state == UKEY_OPERATIONAL + || (ukey_state == UKEY_VISIBLE && purge)) { struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER; bool seq_mismatch = (ukey->dump_seq != dump_seq && ukey->reval_seq != reval_seq); @@ -2256,7 +2301,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) } ovs_mutex_unlock(&ukey->mutex); - if (!flow_exists) { + if (ukey_state == UKEY_EVICTED) { /* The common flow deletion case involves deletion of the flow * during the dump phase and ukey deletion here. */ ovs_mutex_lock(&umap->mutex); @@ -2296,6 +2341,7 @@ revalidator_purge(struct revalidator *revalidator) /* In reaction to dpif purge, purges all 'ukey's with same 'pmd_id'. */ static void dp_purge_cb(void *aux, unsigned pmd_id) + OVS_NO_THREAD_SAFETY_ANALYSIS { struct udpif *udpif = aux; size_t i; @@ -2308,8 +2354,10 @@ dp_purge_cb(void *aux, unsigned pmd_id) size_t n_ops = 0; CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) { - if (ukey->pmd_id == pmd_id) { + if (ukey->pmd_id == pmd_id) { delete_op_init(udpif, &ops[n_ops++], ukey); + transition_ukey(ukey, UKEY_EVICTING); + if (n_ops == REVALIDATE_MAX_BATCH) { push_ukey_ops(udpif, umap, ops, n_ops); n_ops = 0;
Ukeys already have a well-defined lifetime that starts from being created, inserted into the umaps, having the corresponding flow installed, then the flow deleted, the ukey removed from the umap, rcu-deferral of its deletion, and finally freedom. However, until now it's all been represented behind a simple boolean "flow_exists" with a bunch of implicit logic sprinkled around the accessors. This patch attempts to make the ukey lifetime a bit clearer by outlining the correct transitions and asserting that their lifetime proceeds as expected. This should improve the readability of the current code, and also make the following patch easier to reason about. Signed-off-by: Joe Stringer <joe@ovn.org> --- v2: First post. --- ofproto/ofproto-dpif-upcall.c | 140 ++++++++++++++++++++++++++++-------------- 1 file changed, 94 insertions(+), 46 deletions(-)