[ovs-dev,2/6] revalidator: Don't modify ukey from xlate_ukey().

Refactor the newly introduced xlate_ukey() function to just perform the
translation, not modify the ukey.

Signed-off-by: Joe Stringer <joe@ovn.org>
---
 ofproto/ofproto-dpif-upcall.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

Is there any reason not to squash this with the previous patch?

If you want to keep two commits for clarity I would split it like:

commit1) move the block above odp_flow_key_to_flow()
commit2) factor out xlate_ukey() in the second.

2016-09-20 18:47 GMT-07:00 Joe Stringer <joe@ovn.org>:

> Refactor the newly introduced xlate_ukey() function to just perform the
> translation, not modify the ukey.
>
> Signed-off-by: Joe Stringer <joe@ovn.org>
> ---
>  ofproto/ofproto-dpif-upcall.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 67b28d9ccb53..064f043b9eac 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -1825,6 +1825,7 @@ struct reval_context {
>      struct flow_wildcards *wc;
>      struct ofpbuf *odp_actions;
>      struct netflow **netflow;
> +    struct xlate_cache *xcache;
>
>      /* Required output parameters */
>      struct xlate_out xout;
> @@ -1838,10 +1839,8 @@ struct reval_context {
>   * The caller is responsible for uninitializing ctx->xout on success.
>   */
>  static int
> -xlate_ukey(struct udpif *udpif, struct udpif_key *ukey,
> -           const struct dpif_flow_stats *push, struct reval_context *ctx,
> -           bool need_revalidate)
> -    OVS_REQUIRES(ukey->mutex)
> +xlate_ukey(struct udpif *udpif, const struct udpif_key *ukey,
> +           const struct dpif_flow_stats *push, struct reval_context *ctx)
>  {
>      struct ofproto_dpif *ofproto;
>      ofp_port_t ofp_in_port;
> @@ -1859,21 +1858,14 @@ xlate_ukey(struct udpif *udpif, struct udpif_key
> *ukey,
>          return error;
>      }
>
> -    if (need_revalidate) {
> -        xlate_cache_clear(ukey->xcache);
> -    }
> -    if (!ukey->xcache) {
> -        ukey->xcache = xlate_cache_new();
> -    }
> -
>      xlate_in_init(&xin, ofproto, ofproto_dpif_get_tables_
> version(ofproto),
>                    &ctx->flow, ofp_in_port, NULL, push->tcp_flags,
> -                  NULL, need_revalidate ? ctx->wc : NULL,
> ctx->odp_actions);
> +                  NULL, ctx->wc, ctx->odp_actions);
>      if (push->n_packets) {
>          xin.resubmit_stats = push;
>          xin.allow_side_effects = true;
>      }
> -    xin.xcache = ukey->xcache;
> +    xin.xcache = ctx->xcache;
>      xlate_actions(&xin, &ctx->xout);
>
>      return 0;
> @@ -1905,17 +1897,17 @@ revalidate_ukey(struct udpif *udpif, struct
> udpif_key *ukey,
>                  struct recirc_refs *recircs)
>      OVS_REQUIRES(ukey->mutex)
>  {
> +    bool need_revalidate = (ukey->reval_seq != reval_seq);
>      struct xlate_out *xoutp;
>      struct netflow *netflow;
>      struct dpif_flow_stats push;
>      struct flow_wildcards dp_mask, wc;
>      enum reval_result result;
>      long long int last_used;
> -    bool need_revalidate;
>      struct reval_context ctx = {
>          .odp_actions = odp_actions,
>          .netflow = &netflow,
> -        .wc = &wc,
> +        .wc = need_revalidate ? &wc : NULL,
>      };
>
>      result = UKEY_DELETE;
> @@ -1923,7 +1915,6 @@ revalidate_ukey(struct udpif *udpif, struct
> udpif_key *ukey,
>      netflow = NULL;
>
>      ofpbuf_clear(odp_actions);
> -    need_revalidate = (ukey->reval_seq != reval_seq);
>      last_used = ukey->stats.used;
>      push.used = stats->used;
>      push.tcp_flags = stats->tcp_flags;
> @@ -1952,7 +1943,14 @@ revalidate_ukey(struct udpif *udpif, struct
> udpif_key *ukey,
>          goto exit;
>      }
>
> -    if (xlate_ukey(udpif, ukey, &push, &ctx, need_revalidate)) {
> +    if (need_revalidate) {
> +        xlate_cache_clear(ukey->xcache);
> +    }
> +    if (!ukey->xcache) {
> +        ukey->xcache = xlate_cache_new();
> +    }
> +    ctx.xcache = ukey->xcache;
> +    if (xlate_ukey(udpif, ukey, &push, &ctx)) {
>          goto exit;
>      }
>      xoutp = &ctx.xout;
> --
> 2.9.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>

On 28 September 2016 at 14:22, Daniele Di Proietto <diproiettod@ovn.org> wrote:
> Is there any reason not to squash this with the previous patch?

It was an attempt at separating functional changes from cosmetic.

> If you want to keep two commits for clarity I would split it like:
>
> commit1) move the block above odp_flow_key_to_flow()
> commit2) factor out xlate_ukey() in the second.

You're right, and this highlighted a bug where the ukey->xcache would
be allocated (empty) in odp translation / xlate lookup error cases.
I'm going with your suggestion to keep them separate.