[ovs-dev] [PATCH] revalidator: Selectively perform xlate side-effects.
Joe Stringer
joestringer at nicira.com
Mon Jun 23 06:29:54 UTC 2014
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 <joestringer at 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 <leng at vmware.com>
> Signed-off-by: Joe Stringer <joestringer at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140623/5390b7c9/attachment-0005.html>
More information about the dev
mailing list