On 11 September 2015 at 15:50, Jarno Rajahalme <[email protected]> wrote:
> [I tried to reproduce the lost email message from the patch (from github).]
>
> With the comments below,
>
> Acked-by: Jarno Rajahalme <[email protected]>
Appreciate the extra effort.
>> This patch adds a new 32-bit metadata field to the connection tracking
>> interface. When a mark is specified as part of the ct action and the
>> connection is committed, the value is saved with the current connection.
> The mark would be re-written by later conntrack actions specifying the mark
> on the same connection. However, I don’t know if that is useful. Should we
> restrict the mark writing only to the case when the connection is committed,
> like you write above?
I think the above is ambiguously worded. Perhaps more accurate is
"When a mark is specified as part of the ct action and the connection
*has been* commited, ..."
The acceptable variations of this action could be tightened a bit, I
agree. One option is to limit it so far to "nested actions can only be
specified if the commit flag is set". This is the simplest option that
covers the use cases we have so far.
>> @@ -656,11 +657,15 @@ struct ovs_action_push_tnl {
>> * enum ovs_ct_attr - Attributes for %OVS_ACTION_ATTR_CT action.
>> * @OVS_CT_ATTR_FLAGS: u32 connection tracking flags.
>> * @OVS_CT_ATTR_ZONE: u16 connection tracking zone.
>> + * @OVS_CT_ATTR_MARK: u32 value followed by u32 mask. For each bit set in
>> the
>> + * mask, the corresponding bit in the value is copied to the connection
>> + * tracking mark field in the connection.
> So the mask is not optional in the datapath ct action? Not saying it should
> be, just asking.
Your observation is correct.
>> @@ -3607,12 +3607,12 @@ dp_execute_cb(void *aux_, struct dp_packet
>> **packets, int cnt,
>> VLOG_WARN("Cannot execute conntrack action in userspace.");
>> break;
>>
>> + case OVS_ACTION_ATTR_SET:
>> + case OVS_ACTION_ATTR_SET_MASKED:
>> case OVS_ACTION_ATTR_PUSH_VLAN:
>> case OVS_ACTION_ATTR_POP_VLAN:
>> case OVS_ACTION_ATTR_PUSH_MPLS:
>> case OVS_ACTION_ATTR_POP_MPLS:
>> - case OVS_ACTION_ATTR_SET:
>> - case OVS_ACTION_ATTR_SET_MASKED:
>> case OVS_ACTION_ATTR_SAMPLE:
>> case OVS_ACTION_ATTR_HASH:
>> case OVS_ACTION_ATTR_UNSPEC:
> Seems like a spurious change, but does not matter really.
I noticed this afterwards looking over it on github, previously there
was a change here but it is no longer necessary. I can drop this
fragment.
>> diff --git a/lib/flow.h b/lib/flow.h
>> index 2d6d3ee..b5edaa0 100644
>> --- a/lib/flow.h
>> +++ b/lib/flow.h
>> @@ -103,10 +103,12 @@ struct flow {
>> union flow_in_port in_port; /* Input port.*/
>> uint32_t recirc_id; /* Must be exact match. */
>> uint32_t conj_id; /* Conjunction ID. */
>> + uint32_t ct_mark; /* Connection mark. With L3 to avoid L4
>> match.*/
> Comment needs an update, as this is the “metadata” stage.
>> uint16_t ct_zone; /* Connection Zone. */
>> uint8_t ct_state; /* Connection state. */
>
> I would still put at least ct_state and maybe ct_zone right after recirc_id
> for the benefit of cases where those are used, but where mark remains zero.
OK, sure. So far I kept all the CT pieces together, but I have no
strong inclination on this.
>> @@ -748,6 +748,23 @@ enum OVS_PACKED_ENUM mf_field_id {
>> */
>> MFF_CT_ZONE,
>>
>> + /* "ct_mark".
>> + *
>> + * Connection tracking mark. The mark is carried with the
>> + * connection tracking state. On Linux this corresponds to the
>> + * nf_conn's "mark" member but the exact implementation is
>> + * platform-dependent.
>> + *
>> + * Type: be32.
>> + * Maskable: bitwise.
>> + * Formatting: hexadecimal.
>> + * Prerequisites: none.
>> + * Access: read/write.
> But we prevent writing outside of conntrack action. Maybe mention that in the
> comment above.
Agree, this would be more clear. Same applies for label.
>> @@ -2971,6 +2972,7 @@ compose_output_action__(struct xlate_ctx *ctx,
>> ofp_port_t ofp_port,
>> flow_pkt_mark = flow->pkt_mark;
>> flow_ct_state = flow->ct_state;
>> flow_ct_zone = flow->ct_zone;
>> + flow_ct_mark = flow->ct_mark;
>
> Are the ct_* fields actually changed here, i.e., do they need to be preserved?
I guess not. I thought it might be necessary for preservation across
bridges, but the peer logic was already executed.
>> @@ -4152,6 +4155,28 @@ recirc_unroll_actions(const struct ofpact *ofpacts,
>> size_t ofpacts_len,
>> }
>>
>> static void
>> +put_ct_mark(const struct flow *flow, struct flow *base_flow,
>> + struct ofpbuf *odp_actions, struct flow_wildcards *wc)
>> +{
>> + uint32_t base;
>> + struct {
>> + uint32_t key;
>> + uint32_t mask;
>> + } odp_attr;
>> +
>> + base = base_flow->ct_mark;
>> + odp_attr.key = flow->ct_mark;
>> + odp_attr.mask = wc->masks.ct_mark;
>> +
>> + if (odp_attr.mask && odp_attr.key != base) {
>
> What if we set the mark in two consecutive conntrack actions, where the
> second one is the same value as in the base flow initially (i.e., 1). Since
> the mark in the second conntrack is the same as in base, it would not be
> “put”, so the second conntrack action (possibly in a different zone) would
> not include the mark at all?
Another test candidate :-) Agree, this is wrong. Seems like the
approach of reusing do_xlate_actions() for these nested attributes is
faulty: None of this should be reflected in the key yet, only when the
recirculate comes back with a valid ct_state. Rather, we should just
read the action and serialize the equivalent OVS_CT_ATTR_MARK at this
point. I'll rework this. There's some other related issues like
actions=ct(commit,exec(set_field(1->ct_mark))) actually reflects the
set_field in the flow_key, even though the ct interface says that ct_*
fields are only populated when the table is specified.
>> @@ -1364,6 +1364,10 @@ by using the \fBct\fR action.
>> .IP \fBct_zone=\fIvalue
>> Matches connection zone \fIvalue\fR exactly.
>> .
>> +.IP \fBct_mark=\fIvalue\fR[\fB/\fImask\fR]
>> +Matches connection mark \fIvalue\fR either exactly or with optional
>> +\fImask\fR.
>> +.
>> .PP
>> Defining IPv6 flows (those with \fBdl_type\fR equal to 0x86dd) requires
>> support for NXM. The following shorthand notations are available for
>> @@ -1622,6 +1626,10 @@ is not provided, then the default is to use zone
>> zero. The \fBzone\fR may be
>> specified either as an immediate 16-bit \fIvalue\fR, or may be provided
>> from an
>> NXM field \fIsrc\fR. The \fIstart\fR and \fIend\fR pair are inclusive, and
>> must
>> specify a 16-bit range within the field.
>> +.IP \fBmark=\fR\fImark\fR[/\fImask\fR]
>> +Store a 32-bit metadata value (masked by \fImask\fR) with the connection.
>> +This will populate the \fBct_mark\fR flow field if the \fBtable\fR is
>> +specified in the \fBct\fR action.
>
> table?
That's what the action is called right now. Perhaps something like
"return_table" would be more explicit, to give more of the impression
that the ct() action with a return_table specified means that it is
outputted to the connection tracker, then the connection tracker will
return the packet into the OpenFlow pipeline at this table.
I had earlier feedback it could be called "goto_table", but as I
understand it the goto_table instruction in OVS does not fork the
pipeline. Similarly something like "Resubmit_table" is misleading,
because it doesn't act like a function call. Slightly more accurate
might be "recirculate_table", because the action causes re-injection
of the packet into the pipeline, but recirculation by itself doesn't
imply the pipeline fork that occurs.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
