On second thought, I noticed that this doesn't fix odd behaviour in the netflow expiration test.
I sent out a patch to revert the original patch: http://openvswitch.org/pipermail/dev/2014-June/041868.html On 23 June 2014 18:29, Joe Stringer <joestrin...@nicira.com> wrote: > Alex, could you review this series? > > I think that the correct solution is to perform "retroactive > side-effects", that is, perform side effects as though they had happened at > the time that the flow was hit. Anything that may hide these effects (like > mac table flush) would need to be taken into account. However, such > information is unavailable, and would be non-trivial to collect and > maintain. So I propose that this solution as a slightly better effort than > what we had previously. > > > On 16 June 2014 17:09, Joe Stringer <joestrin...@nicira.com> wrote: > >> Commit a48c85b2d6 (revalidator: Use xcache when revalidation is >> required.) introduced a bug where xlate_actions effects from the old >> cache would be performed. This could cause stale mac-learning entries to >> re-appear, and in some cases, learnt flows to be re-learnt even after >> the parent flow is deleted. >> >> This patch splits the attribution of stats from the execution of side >> effects for the flow, always attributing the stats to the rules that >> created them, but selectively executing side-effects based on the >> validity of the flow. >> >> This handles three cases in slightly different ways: >> * If revalidation is unnecessary, attribute stats and perform >> side-effects using the existing xcache. >> * If revalidation is necessary, and the flow is valid, then attribute >> stats using the existing xcache, then perform full revalidation. >> Perform side-effects based on the newly built xcache. >> * If revalidation is necessary, and the flow is invalid, then attribute >> stats using the old xcache, and do not perform side-effects. >> >> VMware-BZ: #1268574 >> >> Reported-by: Len Gao <l...@vmware.com> >> Signed-off-by: Joe Stringer <joestrin...@nicira.com> >> --- >> ofproto/ofproto-dpif-upcall.c | 10 ++++-- >> ofproto/ofproto-dpif-xlate.c | 73 >> +++++++++++++++++++++++++++++++---------- >> ofproto/ofproto-dpif-xlate.h | 3 +- >> 3 files changed, 65 insertions(+), 21 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c >> index b38f226..281955f 100644 >> --- a/ofproto/ofproto-dpif-upcall.c >> +++ b/ofproto/ofproto-dpif-upcall.c >> @@ -1148,12 +1148,14 @@ revalidate_ukey(struct udpif *udpif, struct >> udpif_key *ukey, >> >> may_learn = push.n_packets > 0; >> if (ukey->xcache) { >> - xlate_push_stats(ukey->xcache, may_learn, &push); >> + /* Defer side-effects if the xcache might be out of date. */ >> + bool execute_side_effects = may_learn && !udpif->need_revalidate; >> + >> + xlate_push_stats(ukey->xcache, execute_side_effects, &push); >> if (udpif->need_revalidate) { >> xlate_cache_clear(ukey->xcache); >> push.n_packets = 0; >> push.n_bytes = 0; >> - may_learn = false; >> } else { >> ok = true; >> goto exit; >> @@ -1173,7 +1175,6 @@ revalidate_ukey(struct udpif *udpif, struct >> udpif_key *ukey, >> xlate_in_init(&xin, ofproto, &flow, NULL, push.tcp_flags, NULL); >> xin.resubmit_stats = push.n_packets ? &push : NULL; >> xin.xcache = ukey->xcache; >> - xin.may_learn = may_learn; >> xin.skip_wildcards = !udpif->need_revalidate; >> xlate_actions(&xin, &xout); >> xoutp = &xout; >> @@ -1214,6 +1215,9 @@ revalidate_ukey(struct udpif *udpif, struct >> udpif_key *ukey, >> } >> } >> ok = true; >> + if (may_learn) { >> + xlate_push_effects(ukey->xcache, &push); >> + } >> >> exit: >> if (netflow) { >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index 71eaad1..ca1b62e 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -338,6 +338,8 @@ static bool dscp_from_skb_priority(const struct xport >> *, uint32_t skb_priority, >> >> static struct xc_entry *xlate_cache_add_entry(struct xlate_cache *xc, >> enum xc_type type); >> +static void xlate_push_stats__(struct xlate_cache *, >> + const struct dpif_flow_stats *); >> static void xlate_xbridge_init(struct xlate_cfg *, struct xbridge *); >> static void xlate_xbundle_init(struct xlate_cfg *, struct xbundle *); >> static void xlate_xport_init(struct xlate_cfg *, struct xport *); >> @@ -3886,10 +3888,46 @@ xlate_cache_normal(struct ofproto_dpif *ofproto, >> struct flow *flow, int vlan) >> update_learning_table(xbridge, flow, &wc, vlan, xbundle); >> } >> >> -/* Push stats and perform side effects of flow translation. */ >> +/* Perform side effects of flow translation. */ >> void >> -xlate_push_stats(struct xlate_cache *xcache, bool may_learn, >> - const struct dpif_flow_stats *stats) >> +xlate_push_effects(struct xlate_cache *xcache, >> + const struct dpif_flow_stats *stats) >> +{ >> + struct xc_entry *entry; >> + struct ofpbuf entries = xcache->entries; >> + >> + XC_ENTRY_FOR_EACH (entry, entries, xcache) { >> + switch (entry->type) { >> + case XC_RULE: >> + case XC_BOND: >> + case XC_NETDEV: >> + case XC_NETFLOW: >> + case XC_MIRROR: >> + case XC_GROUP: >> + /* Handled by xlate_push_stats__(). */ >> + break; >> + >> + case XC_LEARN: >> + ofproto_dpif_flow_mod(entry->u.learn.ofproto, entry-> >> u.learn.fm); >> + break; >> + case XC_NORMAL: >> + xlate_cache_normal(entry->u.normal.ofproto, >> entry->u.normal.flow, >> + entry->u.normal.vlan); >> + break; >> + case XC_FIN_TIMEOUT: >> + xlate_fin_timeout__(entry->u.fin.rule, stats->tcp_flags, >> + entry->u.fin.idle, entry->u.fin.hard); >> + break; >> + default: >> + OVS_NOT_REACHED(); >> + } >> + } >> +} >> + >> +/* Push stats using the cache provided. */ >> +static void >> +xlate_push_stats__(struct xlate_cache *xcache, >> + const struct dpif_flow_stats *stats) >> { >> struct xc_entry *entry; >> struct ofpbuf entries = xcache->entries; >> @@ -3915,30 +3953,31 @@ xlate_push_stats(struct xlate_cache *xcache, bool >> may_learn, >> entry->u.mirror.mirrors, >> stats->n_packets, stats->n_bytes); >> break; >> - case XC_LEARN: >> - if (may_learn) { >> - ofproto_dpif_flow_mod(entry->u.learn.ofproto, >> - entry->u.learn.fm); >> - } >> - break; >> - case XC_NORMAL: >> - xlate_cache_normal(entry->u.normal.ofproto, >> entry->u.normal.flow, >> - entry->u.normal.vlan); >> - break; >> - case XC_FIN_TIMEOUT: >> - xlate_fin_timeout__(entry->u.fin.rule, stats->tcp_flags, >> - entry->u.fin.idle, entry->u.fin.hard); >> - break; >> case XC_GROUP: >> group_dpif_credit_stats(entry->u.group.group, >> entry->u.group.bucket, >> stats); >> break; >> + case XC_LEARN: >> + case XC_NORMAL: >> + case XC_FIN_TIMEOUT: >> + /* Handled by xlate_push_effects(). */ >> + break; >> default: >> OVS_NOT_REACHED(); >> } >> } >> } >> >> +void >> +xlate_push_stats(struct xlate_cache *xcache, bool execute_side_effects, >> + const struct dpif_flow_stats *stats) >> +{ >> + xlate_push_stats__(xcache, stats); >> + if (execute_side_effects) { >> + xlate_push_effects(xcache, stats); >> + } >> +} >> + >> static void >> xlate_dev_unref(struct xc_entry *entry) >> { >> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h >> index 6065db3..5d40203 100644 >> --- a/ofproto/ofproto-dpif-xlate.h >> +++ b/ofproto/ofproto-dpif-xlate.h >> @@ -182,8 +182,9 @@ void xlate_out_copy(struct xlate_out *dst, const >> struct xlate_out *src); >> int xlate_send_packet(const struct ofport_dpif *, struct ofpbuf *); >> >> struct xlate_cache *xlate_cache_new(void); >> -void xlate_push_stats(struct xlate_cache *, bool may_learn, >> +void xlate_push_stats(struct xlate_cache *, bool execute_side_effects, >> const struct dpif_flow_stats *); >> +void xlate_push_effects(struct xlate_cache *, const struct >> dpif_flow_stats *); >> void xlate_cache_clear(struct xlate_cache *); >> void xlate_cache_delete(struct xlate_cache *); >> >> -- >> 1.7.10.4 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev >> > >
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
