[ovs-dev,branch-2.3,2/2] ofproto: Allow in-place modifications of datapath flows.

On 3 February 2016 at 12:33, Jarno Rajahalme <jarno@ovn.org> wrote:
> From: Ethan Jackson <ethan@nicira.com>
>
> There are certain use cases (such as bond rebalancing) where a
> datapath flow's actions may change, while it's wildcard pattern
> remains the same.  Before this patch, revalidators would note the
> change, delete the flow, and wait for the handlers to install an
> updated version.  This is inefficient, as many packets could get
> punted to userspace before the new flow is finally installed.
>
> To improve the situation, this patch implements in place modification
> of datapath flows.  If the revalidators detect the only change to a
> given ukey is its actions, instead of deleting it, it does a put with
> the MODIFY flag set.
>
> This patch is a backport of commit 43b2f13 to branch-2.3.
>
> Signed-off-by: Ethan J. Jackson <ethan@nicira.com>
> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>

This also squashes in commit c56eba3b7ab0 ("ofproto-dpif-upcall: Don't
delete modified ukeys.").

<snip>

> @@ -1543,62 +1611,68 @@ revalidate(struct revalidator *revalidator)
>      }
>
>      dpif_flow_dump_state_uninit(udpif->dpif, state);
> -}
> -
> -/* Called with exclusive access to 'revalidator' and 'ukey'. */
> -static bool
> -handle_missed_revalidation(struct revalidator *revalidator,
> -                           struct udpif_key *ukey)
> -    OVS_NO_THREAD_SAFETY_ANALYSIS
> -{
> -    struct udpif *udpif = revalidator->udpif;
> -    struct nlattr *mask, *actions;
> -    size_t mask_len, actions_len;
> -    struct dpif_flow_stats stats;
> -    struct ofpbuf *buf;
> -    bool keep = false;
> -
> -    COVERAGE_INC(revalidate_missed_dp_flow);
> -
> -    if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf,
> -                       &mask, &mask_len, &actions, &actions_len, &stats)) {
> -        keep = revalidate_ukey(udpif, ukey, mask, mask_len, actions,
> -                               actions_len, &stats);
> -        ofpbuf_delete(buf);
> -    }
> -
> -    return keep;
> +    ofpbuf_uninit(&odp_actions);
>  }
>
>  static void
>  revalidator_sweep__(struct revalidator *revalidator, bool purge)
>      OVS_NO_THREAD_SAFETY_ANALYSIS
>  {
> +    uint64_t odp_actions_stub[1024 / 8];
> +    struct ofpbuf odp_actions;
>      struct dump_op ops[REVALIDATE_MAX_BATCH];
>      struct udpif_key *ukey, *next;
>      size_t n_ops;
>
>      n_ops = 0;
>
> +    ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub);
> +
>      /* 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 (ukey->flow_exists) {
>              bool missed_flow = !ukey->mark;
> +            enum reval_result result;
> +            struct nlattr *mask;
> +            size_t mask_len;
> +            struct ofpbuf *buf = NULL;
>
>              ukey->mark = false;
> -            if (purge
> -                || (missed_flow
> -                    && revalidator->udpif->need_revalidate
> -                    && !handle_missed_revalidation(revalidator, ukey))) {
> -                struct dump_op *op = &ops[n_ops++];
> -
> -                dump_op_init(op, ukey->key, ukey->key_len, ukey);
> -                if (n_ops == REVALIDATE_MAX_BATCH) {
> -                    push_dump_ops(revalidator, ops, n_ops);
> -                    n_ops = 0;
> +            result = purge ? UKEY_DELETE : UKEY_KEEP;
> +
> +            if (missed_flow && revalidator->udpif->need_revalidate) {
> +                struct udpif *udpif = revalidator->udpif;
> +                struct nlattr *actions;
> +                size_t actions_len;
> +                struct dpif_flow_stats stats;
> +
> +                COVERAGE_INC(revalidate_missed_dp_flow);
> +
> +                if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf,
> +                                   &mask, &mask_len, &actions, &actions_len,
> +                                   &stats)) {
> +                    result = revalidate_ukey(udpif, ukey, mask, mask_len,
> +                                             actions, actions_len, &stats,
> +                                             &odp_actions);

handle_missed_revalidation() would previously return the equivalent of
UKEY_DELETE if the dpif_flow_get() fails. This would lead on to
attempting flow deletion, which I'd expect is most likely also is
going to fail, but it would also result in the ukey getting deleted
(which we want). I suspect that with the current version above, if one
runs 'ovs-dpctl del-flow...' then the corresponding ukeys will never
be deleted.

Simplest (same as current) behaviour is probably this incremental:

                                              &odp_actions);

Separately from this patch, I noticed a couple of other things:

- I couldn't compile branch-2.3 without at least this patch for
lib/type-props.h:
0f395fd366413208aac7072ef81b5aeda6a9e307
(which depends on c002791a30818c2458599f993d1711e03566e7cc)
- I wonder if commit e83c93573b10 ("ofproto-dpif-upcall: Do not
attribute stats when flow_del returns error.") is also a candidate for
backport, but that's more of a statistics issue rather than
reordering/correctness.

[ovs-dev,branch-2.3,2/2] ofproto: Allow in-place modifications of datapath flows.

Commit Message

Comments

Patch