This no longer applies. I plan to send a fresh version.
On 20 May 2014 09:12, Joe Stringer <joestrin...@nicira.com> wrote:
> Rather than setting and resetting the 'mark' field in the ukey, this
> patch introduces a seq to track whether a flow has been seen during the
> most recent dump. This tidies the code and simplifies the logic for
> detecting when flows are duplicated from the datapath.
>
> Signed-off-by: Joe Stringer <joestrin...@nicira.com>
> ---
> ofproto/ofproto-dpif-upcall.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 522aa8c..6adc729 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -167,8 +167,7 @@ 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. */
> - bool mark OVS_GUARDED; /* For mark and sweep
> garbage
> - collection. */
> + uint64_t dump_seq OVS_GUARDED; /* Tracks udpif->dump_seq.
> */
> bool flow_exists OVS_GUARDED; /* Ensures flows are only
> deleted
> once. */
>
> @@ -1131,7 +1130,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->mark = false;
> + ukey->dump_seq = 0;
> ukey->flow_exists = true;
> ukey->created = used ? used : time_msec();
> memset(&ukey->stats, 0, sizeof ukey->stats);
> @@ -1441,6 +1440,7 @@ revalidate(struct revalidator *revalidator)
> const struct nlattr *key, *mask, *actions;
> size_t key_len, mask_len, actions_len;
> const struct dpif_flow_stats *stats;
> + uint64_t dump_seq;
> long long int now;
> unsigned int flow_limit;
> size_t n_ops;
> @@ -1448,13 +1448,14 @@ revalidate(struct revalidator *revalidator)
>
> n_ops = 0;
> now = time_msec();
> + dump_seq = seq_read(udpif->dump_seq);
> atomic_read(&udpif->flow_limit, &flow_limit);
>
> dpif_flow_dump_state_init(udpif->dpif, &state);
> while (dpif_flow_dump_next(&udpif->dump, state, &key, &key_len, &mask,
> &mask_len, &actions, &actions_len,
> &stats)) {
> struct udpif_key *ukey;
> - bool mark, may_destroy;
> + bool keep, may_destroy;
> long long int used, max_idle;
> uint32_t hash;
> size_t n_flows;
> @@ -1466,7 +1467,7 @@ revalidate(struct revalidator *revalidator)
> if (!used && ukey) {
> ovs_mutex_lock(&ukey->mutex);
>
> - if (ukey->mark || !ukey->flow_exists) {
> + if (ukey->dump_seq == dump_seq) {
> /* The flow has already been dumped. This can occasionally
> * occur if the datapath is changed in the middle of a
> flow
> * dump. Rather than perform the same work twice, skip the
> @@ -1480,6 +1481,7 @@ revalidate(struct revalidator *revalidator)
> ovs_mutex_unlock(&ukey->mutex);
> }
>
> + /* Delete flows more aggressively if over the limit. */
> n_flows = udpif_get_n_flows(udpif);
> max_idle = ofproto_max_idle;
> if (n_flows > flow_limit) {
> @@ -1487,7 +1489,7 @@ revalidate(struct revalidator *revalidator)
> }
>
> if ((used && used < now - max_idle) || n_flows > flow_limit * 2) {
> - mark = false;
> + keep = false;
> } else {
> if (!ukey) {
> ukey = ukey_create(key, key_len, used);
> @@ -1500,17 +1502,18 @@ revalidate(struct revalidator *revalidator)
> }
> }
>
> - mark = revalidate_ukey(udpif, ukey, mask, mask_len, actions,
> + keep = revalidate_ukey(udpif, ukey, mask, mask_len, actions,
> actions_len, stats);
> }
>
> if (ukey) {
> ovs_mutex_lock(&ukey->mutex);
> - ukey->mark = ukey->flow_exists = mark;
> + ukey->dump_seq = dump_seq;
> + ukey->flow_exists = keep;
> ovs_mutex_unlock(&ukey->mutex);
> }
>
> - if (!mark) {
> + if (!keep) {
> dump_op_init(&ops[n_ops++], key, key_len, ukey);
> }
>
> @@ -1548,22 +1551,21 @@ 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;
>
> n_ops = 0;
> + dump_seq = seq_read(revalidator->udpif->dump_seq);
>
> /* 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) {
> - if (!purge && ukey->mark) {
> - ukey->mark = false;
> - } else if (!ukey->flow_exists) {
> + if (!ukey->flow_exists) {
> ukey_delete(revalidator, ukey);
> - } else {
> + } else if (purge || ukey->dump_seq != dump_seq) {
> struct dump_op *op = &ops[n_ops++];
>
> - /* 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. */
> + /* If we have previously seen a flow in the datapath, but it
> + * hasn't been revalidated with the current seq, delete it.
> */
> 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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
