Thanks. I applied this to master.
On Thu, Jan 21, 2016 at 01:09:29PM -0800, Jarno Rajahalme wrote:
> Sounds like a plan,
>
> Jarno
>
> > On Jan 21, 2016, at 8:42 AM, Ben Pfaff <b...@ovn.org> wrote:
> >
> > 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 <b...@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 <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
