[ovs-dev] [RFC 1/3] revalidator: Don't delete missed dp flows immediately.

Alex Wang alexw at nicira.com
Wed Jul 2 18:22:08 UTC 2014


Hey Joe,

Awesome work for the two solutions.

Few comments:
1. For this patch, could we make it for branch-2.3 only?

On master, I'm still seeing dp flow deleted only with a single ping~!!!
(without flow table modification & hping3)
So, there must be something wrong else.

2. After brief discussion with Keith, we found that we could actually tell
if there is a miss-dump via checking the 'last_seen'.  And we could simply
force a full revalidation for each miss-dumped ukeys.

So here is my proposed addition:

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index df929d6..ba9ac71 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1184,7 +1184,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key
*ukey,
     }

     may_learn = push.n_packets > 0;
-    if (ukey->xcache && !udpif->need_revalidate) {
+    if (ukey->xcache && ukey->last_seen == 1 && !udpif->need_revalidate) {
         xlate_push_stats(ukey->xcache, may_learn, &push);
         ok = true;
         goto exit;
@@ -1462,13 +1462,10 @@ revalidator_sweep__(struct revalidator
*revalidator, bool purge)
     /* During garbage collection, this revalidator completely owns its
ukeys
      * map, and therefore doesn't need to do any locking. */
     HMAP_FOR_EACH_SAFE (ukey, next, hmap_node, revalidator->ukeys) {
-        bool missed_reval;
-
-        missed_reval = ukey->last_seen && need_revalidate;
         ukey->last_seen++;
         if (!ukey->flow_exists) {
             ukey_delete(revalidator, ukey);
-        } else if (purge || missed_reval || ukey->last_seen ==
MAX_UKEY_AGE) {
+        } else if (purge || ukey->last_seen == MAX_UKEY_AGE) {
             struct dump_op *op = &ops[n_ops++];

             /* If we have previously seen a flow in the datapath, but
didn't


3. With your patch + the addition above

I confirmed that the issue has been fixed on my local setup. (only for
branch-2.3)

There is more issue with master.

Thanks,
Alex Wang,




On Wed, Jul 2, 2014 at 1:14 AM, Joe Stringer <joestringer at nicira.com> wrote:

> There is no guarantee that performing a flow_dump will return a complete
> version of the datapath's flow table. Occasionally, a flow may be
> completely missed even if it is present in the datapath (particularly if
> the datapath decides to rehash its flow tables). Previously we would
> delete such "missed flows" immediately. This patch gives those flows a
> chance to be dumped, which should improve jitter/ latency for flows in
> this corner case.
>
> Signed-off-by: Joe Stringer <joestringer at nicira.com>
> ---
>  ofproto/ofproto-dpif-upcall.c |   33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 62ea48a..df929d6 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -185,7 +185,8 @@ struct udpif_key {
>      struct ovs_mutex mutex;                   /* Guards the following. */
>      struct dpif_flow_stats stats OVS_GUARDED; /* Last known stats.*/
>      long long int created OVS_GUARDED;        /* Estimate of creation
> time. */
> -    uint64_t dump_seq OVS_GUARDED;            /* Tracks udpif->dump_seq.
> */
> +    int last_seen OVS_GUARDED;                /* For garbage-collecting
> flows
> +                                                 not seen for a while. */
>      bool flow_exists OVS_GUARDED;             /* Ensures flows are only
> deleted
>                                                   once. */
>
> @@ -195,6 +196,10 @@ struct udpif_key {
>      struct odputil_keybuf key_buf;            /* Memory for 'key'. */
>  };
>
> +/* Maximum number of dumps that the datapath can fail to dump a flow
> before
> + * revalidators clean up the ukey and delete the flow. */
> +#define MAX_UKEY_AGE 3
> +
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>  static struct list all_udpifs = LIST_INITIALIZER(&all_udpifs);
>
> @@ -1041,7 +1046,7 @@ ukey_create(const struct nlattr *key, size_t
> key_len, long long int used)
>      ukey->key_len = key_len;
>
>      ovs_mutex_lock(&ukey->mutex);
> -    ukey->dump_seq = 0;
> +    ukey->last_seen = -1;
>      ukey->flow_exists = true;
>      ukey->created = used ? used : time_msec();
>      memset(&ukey->stats, 0, sizeof ukey->stats);
> @@ -1355,10 +1360,8 @@ revalidate(struct revalidator *revalidator)
>  {
>      struct udpif *udpif = revalidator->udpif;
>      struct dpif_flow_dump_thread *dump_thread;
> -    uint64_t dump_seq;
>      unsigned int flow_limit;
>
> -    dump_seq = seq_read(udpif->dump_seq);
>      atomic_read(&udpif->flow_limit, &flow_limit);
>      dump_thread = dpif_flow_dump_thread_create(udpif->dump);
>      for (;;) {
> @@ -1410,7 +1413,7 @@ revalidate(struct revalidator *revalidator)
>                  continue;
>              }
>
> -            already_dumped = ukey->dump_seq == dump_seq;
> +            already_dumped = ukey->last_seen == 0;
>              if (already_dumped) {
>                  /* The flow has already been dumped and handled by another
>                   * revalidator during this flow dump operation. Skip it.
> */
> @@ -1427,10 +1430,11 @@ revalidate(struct revalidator *revalidator)
>              } else {
>                  keep = revalidate_ukey(udpif, ukey, f);
>              }
> -            ukey->dump_seq = dump_seq;
>              ukey->flow_exists = keep;
>
> -            if (!keep) {
> +            if (keep) {
> +                ukey->last_seen = 0;
> +            } else {
>                  dump_op_init(&ops[n_ops++], f->key, f->key_len, ukey);
>              }
>              ovs_mutex_unlock(&ukey->mutex);
> @@ -1450,21 +1454,26 @@ revalidator_sweep__(struct revalidator
> *revalidator, bool purge)
>      struct dump_op ops[REVALIDATE_MAX_BATCH];
>      struct udpif_key *ukey, *next;
>      size_t n_ops;
> -    uint64_t dump_seq;
> +    bool need_revalidate;
>
>      n_ops = 0;
> -    dump_seq = seq_read(revalidator->udpif->dump_seq);
> +    need_revalidate = revalidator->udpif->need_revalidate;
>
>      /* During garbage collection, this revalidator completely owns its
> ukeys
>       * map, and therefore doesn't need to do any locking. */
>      HMAP_FOR_EACH_SAFE (ukey, next, hmap_node, revalidator->ukeys) {
> +        bool missed_reval;
> +
> +        missed_reval = ukey->last_seen && need_revalidate;
> +        ukey->last_seen++;
>          if (!ukey->flow_exists) {
>              ukey_delete(revalidator, ukey);
> -        } else if (purge || ukey->dump_seq != dump_seq) {
> +        } else if (purge || missed_reval || ukey->last_seen ==
> MAX_UKEY_AGE) {
>              struct dump_op *op = &ops[n_ops++];
>
> -            /* If we have previously seen a flow in the datapath, but it
> -             * hasn't been seen in the current dump, delete it. */
> +            /* If we have previously seen a flow in the datapath, but
> didn't
> +             * see it during the most recent dump, delete it. This allows
> us
> +             * to clean up the ukey and keep the statistics consistent. */
>              dump_op_init(op, ukey->key, ukey->key_len, ukey);
>              if (n_ops == REVALIDATE_MAX_BATCH) {
>                  push_dump_ops(revalidator, ops, n_ops);
> --
> 1.7.10.4
>
>



More information about the dev mailing list