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 <joestrin...@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 <joestrin...@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
>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
