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

Ben Pfaff blp at ovn.org
Thu Jan 21 16:42:18 UTC 2016


I think I'd be more comfortable letting you figure that out, if that's
OK, and just stick to the compromise we have here for the moment.

On Wed, Jan 20, 2016 at 05:46:31PM -0800, Jarno Rajahalme wrote:
> I did not look at the details yet, but basically the reference would be released if gotten initially and not taken for the flow install.
> 
>   Jarno
> 
> > On Jan 20, 2016, at 4:59 PM, Ben Pfaff <blp at ovn.org> wrote:
> > 
> > I think I like the idea of only needing a recirc_state in that code, but
> > I don't think I fully understand yet.  I guess it's pretty easy to make
> > xlate_in_init() take the reference and pass back the recirc_state
> > instead of the recirc_id_node, but where would the eventual release of
> > the reference happen?
> > 
> > On Wed, Jan 20, 2016 at 04:39:12PM -0800, Jarno Rajahalme 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
> >> 
> 



More information about the dev mailing list