On 3 March 2017 at 14:36, Jarno Rajahalme <[email protected]> wrote: > Thanks for the review Joe! > >> On Mar 3, 2017, at 10:09 AM, Joe Stringer <[email protected]> wrote: >> >> On 28 February 2017 at 17:17, Jarno Rajahalme <[email protected]> wrote: >>> Userspace support for datapath original direction conntrack tuple. >>> >>> Signed-off-by: Jarno Rajahalme <[email protected]> >> >> Thanks for the submission. Some feedback below. >> >> <snip> >> >>> diff --git a/include/openvswitch/meta-flow.h >>> b/include/openvswitch/meta-flow.h >>> index aac9945..94cee20 100644 >>> --- a/include/openvswitch/meta-flow.h >>> +++ b/include/openvswitch/meta-flow.h >>> @@ -740,6 +740,139 @@ enum OVS_PACKED_ENUM mf_field_id { >>> */ >>> MFF_CT_LABEL, >>> >>> + /* "ct_nw_proto". >>> + * >>> + * The "protocol" byte in the IPv4 or IPv6 header for the original >>> + * direction conntrack tuple, or of the master conntrack entry, if the >>> + * current connection is a related connection. >>> + * >>> + * The value is initially zero and populated by the CT action. The >>> value >>> + * remains zero after the CT action only if the packet can not be >>> + * associated with a tracked connection, in which case the >>> prerequisites >> >> "Tracked" in the current API documentation refers to whether the >> packet was submitted to the connection tracker during the current >> pipeline processing, and not connection state. To refer to connections >> which have been committed, we call that "committed". See the >> "Connection Tracking Fields" section of ovs-fields(7) for more >> details. >> > > The intent is to not require the connection to be committed, as the value is > properly populated also for the “new” packets. In the above, I was using the > term “tracked connection” in a more general sense, and did not intend to > refer to the “packet is tracked” ct_state bit. Maybe I should change this to > “valid connection”?
Yes, I think that "valid connection" would be more clear - I don't think there's anything in the docs that might confuse the meaning of that. >>> @@ -3397,8 +3397,9 @@ decode_nx_packet_in2(const struct ofp_header *oh, >>> bool loose, >>> } >>> >>> case NXPINT_METADATA: >>> - error = oxm_decode_match(payload.msg, ofpbuf_msgsize(&payload), >>> - tun_table, &pin->flow_metadata); >>> + error = oxm_decode_match_loose(payload.msg, >>> + ofpbuf_msgsize(&payload), >>> + tun_table, &pin->flow_metadata); >>> break; >>> >>> case NXPINT_USERDATA: >> >> Previously we would immediately reject unknown OXM headers; now we >> don't due to the above, right? Is this intentional? This kind of >> behavioural change would be easier to validate if there was a separate >> patch outlining why, what it affects, etc. >> > > Previously packet metadata fields did not have prerequisites, but the new > orig tuple fields do, for the purposes of accepting OpenFlow rules. Here I > did not want to party the packet again to check the prerequisites, as the > packet_in2 is generated by OVS itself. > > I had added this comment on top of nx_pull_raw(): > > /* Prerequisites will only be checked when 'strict' is 'true'. This allows > * decoding conntrack original direction 5-tuple IP addresses without the > * ethertype being present, when decoding metadata only. */ > > The skipping of unknown OXMs here is not strictly needed, but “came in” with > the use of the existing “_loose” functions. Checking in with Ben on how OVN > used this we figured out that this is actually what we want to do. Generally, > the controller should ignore unknown fields, so that the switch can add new > functionality without requiring synchronous controller upgrades. When > packet_in2 is used for a continuation, the controller can copy the metadata > from the original packet_in2 message, including all the unknown fields, to > the resume message it is sending to the switch. OVN does this already. > > I’ll move this to a separate patch to highlight this change. OK, that sounds good thanks! _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
