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

[Joe Stringer]

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.