[ovs-dev] [PATCH 40/41] ofproto-dpif-xlate: Put recirc_state, not recirc_id_node, in xlate_in.

[Ben Pfaff]

Thanks, I folded it in.

On Wed, Jan 20, 2016 at 04:41:24PM -0800, Jarno Rajahalme wrote:
> I was a bit too fast, I had to add this incremental to make this patch compile:
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 184eb46..6549191 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -5143,7 +5143,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>      COVERAGE_INC(xlate_actions);
>  
>      if (xin->recirc) {
> -        const struct recirc_state *state = &xin->recirc->state;
> +        const struct recirc_state *state = xin->recirc;
>  
>          xlate_report(&ctx, "Restoring state post-recirculation:");
>  
>   Jarno
> 
> > On Jan 20, 2016, at 4:39 PM, Jarno Rajahalme <jarno at ovn.org> wrote:
> > 
> > Maybe ofproto-dpif-upcall.c could be refactored to not need the recirc_id_node pointer. It is only used for getting the recirc ID and managing recirc_id_node reference counting. If the reference counting could be moved upstream, the pointer could be removed. But the idea was to take a reference only if needed (e.g., when installing a recirculated flow), as taking a reference is extra cost if we are only executing the packet, or deciding not to install a flow for any other reason. However, the typical case is that we install a flow for a miss upcall, so maybe it is not worth optimizing for the exceptional cases?
> > 
> > Acked-by: Jarno Rajahalme <jarno at ovn.org>
> > 
> >> On Jan 18, 2016, at 11:27 PM, Ben Pfaff <blp at ovn.org> wrote:
> >> 
> >> This will make it possible, in an upcoming commit, to construct a
> >> recirc_state locally on the stack to pass to xlate_actions().  It would
> >> also be possible to construct and pass a recirc_id_node on the stack, but
> >> the translation process only uses the recirc_state anyway.  The alternative
> >> here of having upcall_xlate() know that it can recover the recirc_id_node
> >> from the recirc_state isn't great either; it's debatable which is the
> >> better approach.
> >> 
> >> Signed-off-by: Ben Pfaff <blp at ovn.org>
> >> ---
> >> ofproto/ofproto-dpif-rid.h    |  6 ++++++
> >> ofproto/ofproto-dpif-upcall.c |  4 ++--
> >> ofproto/ofproto-dpif-xlate.c  | 11 ++++++++---
> >> ofproto/ofproto-dpif-xlate.h  |  2 +-
> >> 4 files changed, 17 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
> >> index 04ec037..85ec24a 100644
> >> --- a/ofproto/ofproto-dpif-rid.h
> >> +++ b/ofproto/ofproto-dpif-rid.h
> >> @@ -184,6 +184,12 @@ void recirc_free_ofproto(struct ofproto_dpif *, const char *ofproto_name);
> >> 
> >> const struct recirc_id_node *recirc_id_node_find(uint32_t recirc_id);
> >> 
> >> +static inline struct recirc_id_node *
> >> +recirc_id_node_from_state(const struct recirc_state *state)
> >> +{
> >> +    return CONTAINER_OF(state, struct recirc_id_node, state);
> >> +}
> >> +
> >> static inline bool recirc_id_node_try_ref_rcu(const struct recirc_id_node *n_)
> >> {
> >>    struct recirc_id_node *node = CONST_CAST(struct recirc_id_node *, n_);
> >> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> >> index b505206..240bd92 100644
> >> --- a/ofproto/ofproto-dpif-upcall.c
> >> +++ b/ofproto/ofproto-dpif-upcall.c
> >> @@ -1074,8 +1074,8 @@ upcall_xlate(struct udpif *udpif, struct upcall *upcall,
> >>             * upcalls using recirculation ID for which no context can be
> >>             * found).  We may still execute the flow's actions even if we
> >>             * don't install the flow. */
> >> -            upcall->recirc = xin.recirc;
> >> -            upcall->have_recirc_ref = recirc_id_node_try_ref_rcu(xin.recirc);
> >> +            upcall->recirc = recirc_id_node_from_state(xin.recirc);
> >> +            upcall->have_recirc_ref = recirc_id_node_try_ref_rcu(upcall->recirc);
> >>        }
> >>    } else {
> >>        /* For non-miss upcalls, we are either executing actions (one of which
> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> >> index 8ab4b2a..184eb46 100644
> >> --- a/ofproto/ofproto-dpif-xlate.c
> >> +++ b/ofproto/ofproto-dpif-xlate.c
> >> @@ -4828,9 +4828,14 @@ xlate_in_init(struct xlate_in *xin, struct ofproto_dpif *ofproto,
> >>    xin->odp_actions = odp_actions;
> >> 
> >>    /* Do recirc lookup. */
> >> -    xin->recirc = flow->recirc_id
> >> -        ? recirc_id_node_find(flow->recirc_id)
> >> -        : NULL;
> >> +    xin->recirc = NULL;
> >> +    if (flow->recirc_id) {
> >> +        const struct recirc_id_node *node
> >> +            = recirc_id_node_find(flow->recirc_id);
> >> +        if (node) {
> >> +            xin->recirc = &node->state;
> >> +        }
> >> +    }
> >> }
> >> 
> >> void
> >> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
> >> index a135d8f..3b06285 100644
> >> --- a/ofproto/ofproto-dpif-xlate.h
> >> +++ b/ofproto/ofproto-dpif-xlate.h
> >> @@ -142,7 +142,7 @@ struct xlate_in {
> >> 
> >>    /* The recirculation context related to this translation, as returned by
> >>     * xlate_lookup. */
> >> -    const struct recirc_id_node *recirc;
> >> +    const struct recirc_state *recirc;
> >> };
> >> 
> >> void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *,
> >> -- 
> >> 2.1.3
> >> 
> >> _______________________________________________
> >> dev mailing list
> >> dev at openvswitch.org
> >> http://openvswitch.org/mailman/listinfo/dev
> > 
>