On 1 May 2015 at 13:07, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
>
>> 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 ;-)
If only I had the foresight to do so ;-)
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
