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
