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 <ja...@ovn.org>
>
> > On Jan 18, 2016, at 11:27 PM, Ben Pfaff <b...@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 <b...@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@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
