Thanks for the quick response,
On 30 April 2015 at 14:18, Jarno Rajahalme <jrajaha...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
