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

[Jarno Rajahalme]

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
>>