> On May 1, 2015, at 11:11, Joe Stringer <joestrin...@nicira.com> wrote:
>
> 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.
Sounds right, sorry for missing that in my review. I hope you did not plant
this on purpose ;-)
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
