[ovs-dev] [RFCv4 09/10] xlate: Clear conntrack fields when traversing peers.

[Joe Stringer]

On 1 May 2015 at 13:07, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>
>> On May 1, 2015, at 11:11, Joe Stringer <joestringer at nicira.com> wrote:
>>
>> Thanks for the quick response,
>>
>> On 30 April 2015 at 14:18, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>>>> @@ -748,6 +748,11 @@ struct ofpact_unroll_xlate {
>>>>    /* Metadata in xlate context, visible to controller via PACKET_INs. */
>>>>    uint8_t  rule_table_id;       /* 0xFF if none. */
>>>>    ovs_be64 rule_cookie;         /* OVS_BE64_MAX if none. */
>>>> +
>>>> +    /* Whether conntrack was executed prior to recirculation. If so, related
>>>> +     * fields may be made available post-recirculation, until peer traversal
>>>> +     * or subsequent conntrack execution. */
>>>> +    bool conntracked;
>>>> };
>>>
>>> Having ‘conntracked’ in the of ofpact_unroll_xlate would make sense if we both saved and restored the ‘conntracked’ value across some actions (other than output to patch ports). Unrolling ‘conntracked’ after a resubmit that did the conntrack action would reset the bit to 0 for the rest of the pipeline, which is not what we want. In short, we do not need ‘conntracked’ here.
>>
>> Right, I think I was trying to handle a case where there is a
>> conntrack action, resubmit, then other actions causing recirculation.
>> However, I think this is
>> a) Not possible to trigger currently
>> b) Not a sane case to support - conntrack fields should only be
>> exposed to the OpenFlow pipeline via conntrack(recirc), not other
>> subsequent recirc actions.
>> c) Perhaps not handled correctly by this approach anyway.
>>
>> I'll drop this piece.
>>
>>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>>> index 5561410..9a9b4a6 100644
>>>> --- a/ofproto/ofproto-dpif-xlate.c
>>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>>> @@ -283,6 +283,11 @@ struct xlate_ctx {
>>>>     * the MPLS label stack that was originally present. */
>>>>    bool was_mpls;
>>>>
>>>> +    /* True if conntrack has been performed on this packet during processing
>>>> +     * on the current bridge. This is used to determine whether conntrack
>>>> +     * state from the datapath should be honored after recirculation. */
>>>> +    bool conntracked;
>>>> +
>>>
>>> This should be initialized to false when an xlate_ctx is created.
>>
>> Yeah, I missed this somehow.
>>
>>>> @@ -4113,6 +4137,9 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc)
>>>>    put_connhelper(odp_actions, ofc);
>>>>    nl_msg_end_nested(odp_actions, ct_offset);
>>>>
>>>> +    /* Use conn_* fields from datapath during recirculation upcall. */
>>>> +    ctx->conntracked = true;
>>>> +
>>>>    if (ofc->flags & NX_CT_F_RECIRC) {
>>>>        compose_recirculate_action__(ctx, NULL, 0, 0, NULL);
>>>>    }
>>
>> Actually I think this only needs to occur if NX_CT_F_RECIRC is set. We
>> shouldn't randomly get conntrack fields exposed from other recirculate
>> actions, only the conntrack action with recirc flag should expose
>> those fields and make them matchable.
>
> Sounds right, sorry for missing that in my review. I hope you did not plant this on purpose ;-)

If only I had the foresight to do so ;-)