[ovs-dev] [PATCH 2/2] ofproto-dpif-upcall: Use UKEY_DELETE for ukey deletion.

[Joe Stringer]

On 7 January 2016 at 13:53, Jarno Rajahalme <jarno at ovn.org> wrote:
> While correct, I would find this easier to follow (as an incremental on top of this patch):
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index d611131..390acbf 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2240,9 +2240,8 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
>          size_t n_ops = 0;
>
>          CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) {
> -            bool flow_exists, seq_mismatch;
> +            bool flow_exists;
>              struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER;
> -            enum reval_result result;
>
>              /* Handler threads could be holding a ukey lock while it installs a
>               * new flow, so don't hang around waiting for access to it. */
> @@ -2250,37 +2249,40 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
>                  continue;
>              }
>              flow_exists = ukey->flow_exists;
> -            seq_mismatch = (ukey->dump_seq != dump_seq
> -                            && ukey->reval_seq != reval_seq);
> -
> -            if (purge || !flow_exists) {
> -                result = UKEY_DELETE;
> -            } else if (!seq_mismatch) {
> -                result = UKEY_KEEP;
> -            } else {
> -                struct dpif_flow_stats stats;
> -                COVERAGE_INC(revalidate_missed_dp_flow);
> -                memset(&stats, 0, sizeof stats);
> -                result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
> -                                         reval_seq, &recircs);
> -            }
> -            if (flow_exists && result != UKEY_KEEP) {
> -                /* Takes ownership of 'recircs'. */
> -                reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
> -                              &odp_actions);
> +            if (flow_exists) {
> +                enum reval_result result;
> +                bool seq_mismatch = (ukey->dump_seq != dump_seq
> +                                     && ukey->reval_seq != reval_seq);
> +
> +                if (purge) {
> +                    result = UKEY_DELETE;
> +                } else if (!seq_mismatch) {
> +                    result = UKEY_KEEP;
> +                } else {
> +                    struct dpif_flow_stats stats;
> +                    COVERAGE_INC(revalidate_missed_dp_flow);
> +                    memset(&stats, 0, sizeof stats);
> +                    result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
> +                                             reval_seq, &recircs);
> +                }
> +                if (result != UKEY_KEEP) {
> +                    /* Clears 'recircs' if filled by revalidate_ukey(). */
> +                    reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
> +                                  &odp_actions);
> +                }
>              }
>              ovs_mutex_unlock(&ukey->mutex);
>
> -            if (n_ops == REVALIDATE_MAX_BATCH) {
> -                push_ukey_ops(udpif, umap, ops, n_ops);
> -                n_ops = 0;
> -            }
> -
>              if (!flow_exists) {
>                  ovs_mutex_lock(&umap->mutex);
>                  ukey_delete(umap, ukey);
>                  ovs_mutex_unlock(&umap->mutex);
>              }
> +
> +            if (n_ops == REVALIDATE_MAX_BATCH) {
> +                push_ukey_ops(udpif, umap, ops, n_ops);
> +                n_ops = 0;
> +            }
>          }
>
>          if (n_ops) {
>
>
> Your call. Either way:
>
> Acked-by: Jarno Rajahalme <jarno at ovn.org>

Thanks, that looks more straightforward. I folded those changes in, tweaked
the commit message and pushed this to master. Given that it's more
cosmetic rather than functional, I haven't opted to backport it at
this stage but let me know if you think otherwise.