Re: [ovs-dev] [patch_v1] flow.c: refactor ct_orig_tuple check in miniflow_extract.

[Joe Stringer]

On 25 May 2017 at 18:53, Darrell Ball  wrote:
>
>
> On 5/22/17, 4:44 PM, "ovs-dev-boun...@openvswitch.org on behalf of Joe 
> Stringer"  wrote:
>
> On 20 May 2017 at 11:09, Darrell Ball  wrote:
> > The checks to populate ct_orig_tuple in miniflow_extract
> > includes recirc_id being non-zero.  This is changed here
> > to populate the ct_orig_tuple fields based only on ct_state
> > per the logic of the design.
> >
> > Signed-off-by: Darrell Ball 
>
> I'm not exactly sure what "per the logic of the design" is supposed to
> mean, and I suspect that someone reading through the git logs would be
> just as lost. As far as I'm aware, any patch to OVS should be "per the
> logic of the design", right?
>
> In general, a change can be consistent with an
> existing design or a change to the existing design itself.
> However, “per the logic of the design” can be assumed by default
> and hence is usually considered superfluous, which I think is what you
> want to convey – sure.
>
> I’ll include the text I used on the related thread, as I agree, the
> motivation is not necessarily obvious, just by linking ct_orig_tuple to
> ct_state, as I did in this commit message.

OK, thanks!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch_v1] flow.c: refactor ct_orig_tuple check in miniflow_extract.

[Greg Rose]

On Fri, 2017-05-26 at 15:09 +, Darrell Ball wrote:
> http://openvswitch.org/support/dist-docs/ovs-ofctl.8.pdf
> 
> 
> “resubmit([port],[table],ct)
> Re-searches this OpenFlow flow table (or the table whose number is specified 
> by table)
> with the in_port field replaced by port (if port is specified) and the packet 
> 5-tuple fields
> swapped with the corresponding conntrack original direction tuple fields (if 
> ct is speci-
> fied, see ct_nw_src above), and executes the actions found, if any, in 
> addition to any
> other actions in this flow entry. The in_port and swapped 5-tuple fields are 
> restored
> immediately after the search, before any actions are executed.
> The ct option requires a valid connection tracking state as a match 
> prerequisite in the flow
> where this action is placed. Examples of valid connection tracking state 
> matches include
> ct_state=+new, ct_state=+est, ct_state=+rel, and ct_state=+trk-inv.”

Thank you!

- Greg



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch_v1] flow.c: refactor ct_orig_tuple check in miniflow_extract.

[Darrell Ball]

http://openvswitch.org/support/dist-docs/ovs-ofctl.8.pdf


“resubmit([port],[table],ct)
Re-searches this OpenFlow flow table (or the table whose number is specified by 
table)
with the in_port field replaced by port (if port is specified) and the packet 
5-tuple fields
swapped with the corresponding conntrack original direction tuple fields (if ct 
is speci-
fied, see ct_nw_src above), and executes the actions found, if any, in addition 
to any
other actions in this flow entry. The in_port and swapped 5-tuple fields are 
restored
immediately after the search, before any actions are executed.
The ct option requires a valid connection tracking state as a match 
prerequisite in the flow
where this action is placed. Examples of valid connection tracking state 
matches include
ct_state=+new, ct_state=+est, ct_state=+rel, and ct_state=+trk-inv.”



On 5/26/17, 8:00 AM, "ovs-dev-boun...@openvswitch.org on behalf of Greg Rose" 
 wrote:

On Fri, 2017-05-26 at 01:53 +, Darrell Ball wrote:
> 
> On 5/22/17, 4:44 PM, "ovs-dev-boun...@openvswitch.org on behalf of Joe 
Stringer"  wrote:
> 
> On 20 May 2017 at 11:09, Darrell Ball  wrote:
> > The checks to populate ct_orig_tuple in miniflow_extract
> > includes recirc_id being non-zero.  This is changed here
> > to populate the ct_orig_tuple fields based only on ct_state
> > per the logic of the design.
> >
> > Signed-off-by: Darrell Ball 
> 
> I'm not exactly sure what "per the logic of the design" is supposed to
> mean, and I suspect that someone reading through the git logs would be
> just as lost. As far as I'm aware, any patch to OVS should be "per the
> logic of the design", right?
> 
> In general, a change can be consistent with an 
> existing design or a change to the existing design itself. 
> However, “per the logic of the design” can be assumed by default
> and hence is usually considered superfluous, which I think is what you
> want to convey – sure.

At the time I thought you meant more like 'per the specification'.  Now
I realize I came off as an idiot.  So is there a specification for what
the ct_orig_tuple fields should be set to?  Is there some rule we can
refer to that states explicitly that ct_orig_tuple should be populated
based only upon ct_state?  Or is it something that should just be clear
by understanding the code?

Sorry if I'm missing something obvious.

- Greg

> 
> I’ll include the text I used on the related thread, as I agree, the
> motivation is not necessarily obvious, just by linking ct_orig_tuple to
> ct_state, as I did in this commit message.
> 
> Do you mean "The ct_orig_tuple fields are designed to be read only if
> the ct_state is valid, so do not populate that field if the packet has
> not gone through the connection tracker"?
> 
> I think that this kind of
> description would provide at least a little extra context to help
> understand the motivation behind the change.
> 
> 
> 
> ___
> dev mailing list
> d...@openvswitch.org
> 
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=nAHjQgAdipmU4BHowXyPhoSGZvhBYs0Gprz21Toem1c=wrTPUsNoG_MEIRSjPdI7ExOiyaQh-f8tqCJLyyWrlo8=
 
> 
> 
> 
> 
> 
> 
> ___
> dev mailing list
> d...@openvswitch.org
> 
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwIGaQ=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=CX6BNdRBoU5RME8jxR8Ym69dSbg__GatHpbkcUq-nU8=hATlW0-8J6fQPfCUJvqQ02qZaTHiCBn6ZjXOtZ26Yqo=
 



___
dev mailing list
d...@openvswitch.org

https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwIGaQ=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=CX6BNdRBoU5RME8jxR8Ym69dSbg__GatHpbkcUq-nU8=hATlW0-8J6fQPfCUJvqQ02qZaTHiCBn6ZjXOtZ26Yqo=
 


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch_v1] flow.c: refactor ct_orig_tuple check in miniflow_extract.

[Greg Rose]

On Fri, 2017-05-26 at 01:53 +, Darrell Ball wrote:
> 
> On 5/22/17, 4:44 PM, "ovs-dev-boun...@openvswitch.org on behalf of Joe 
> Stringer"  wrote:
> 
> On 20 May 2017 at 11:09, Darrell Ball  wrote:
> > The checks to populate ct_orig_tuple in miniflow_extract
> > includes recirc_id being non-zero.  This is changed here
> > to populate the ct_orig_tuple fields based only on ct_state
> > per the logic of the design.
> >
> > Signed-off-by: Darrell Ball 
> 
> I'm not exactly sure what "per the logic of the design" is supposed to
> mean, and I suspect that someone reading through the git logs would be
> just as lost. As far as I'm aware, any patch to OVS should be "per the
> logic of the design", right?
> 
> In general, a change can be consistent with an 
> existing design or a change to the existing design itself. 
> However, “per the logic of the design” can be assumed by default
> and hence is usually considered superfluous, which I think is what you
> want to convey – sure.

At the time I thought you meant more like 'per the specification'.  Now
I realize I came off as an idiot.  So is there a specification for what
the ct_orig_tuple fields should be set to?  Is there some rule we can
refer to that states explicitly that ct_orig_tuple should be populated
based only upon ct_state?  Or is it something that should just be clear
by understanding the code?

Sorry if I'm missing something obvious.

- Greg

> 
> I’ll include the text I used on the related thread, as I agree, the
> motivation is not necessarily obvious, just by linking ct_orig_tuple to
> ct_state, as I did in this commit message.
> 
> Do you mean "The ct_orig_tuple fields are designed to be read only if
> the ct_state is valid, so do not populate that field if the packet has
> not gone through the connection tracker"?
> 
> I think that this kind of
> description would provide at least a little extra context to help
> understand the motivation behind the change.
> 
> 
> 
> ___
> dev mailing list
> d...@openvswitch.org
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=nAHjQgAdipmU4BHowXyPhoSGZvhBYs0Gprz21Toem1c=wrTPUsNoG_MEIRSjPdI7ExOiyaQh-f8tqCJLyyWrlo8=
>  
> 
> 
> 
> 
> 
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch_v1] flow.c: refactor ct_orig_tuple check in miniflow_extract.

[Darrell Ball]

On 5/22/17, 4:44 PM, "ovs-dev-boun...@openvswitch.org on behalf of Joe 
Stringer"  wrote:

On 20 May 2017 at 11:09, Darrell Ball  wrote:
> The checks to populate ct_orig_tuple in miniflow_extract
> includes recirc_id being non-zero.  This is changed here
> to populate the ct_orig_tuple fields based only on ct_state
> per the logic of the design.
>
> Signed-off-by: Darrell Ball 

I'm not exactly sure what "per the logic of the design" is supposed to
mean, and I suspect that someone reading through the git logs would be
just as lost. As far as I'm aware, any patch to OVS should be "per the
logic of the design", right?

In general, a change can be consistent with an 
existing design or a change to the existing design itself. 
However, “per the logic of the design” can be assumed by default
and hence is usually considered superfluous, which I think is what you
want to convey – sure.

I’ll include the text I used on the related thread, as I agree, the
motivation is not necessarily obvious, just by linking ct_orig_tuple to
ct_state, as I did in this commit message.

Do you mean "The ct_orig_tuple fields are designed to be read only if
the ct_state is valid, so do not populate that field if the packet has
not gone through the connection tracker"?

I think that this kind of
description would provide at least a little extra context to help
understand the motivation behind the change.



___
dev mailing list
d...@openvswitch.org

https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=nAHjQgAdipmU4BHowXyPhoSGZvhBYs0Gprz21Toem1c=wrTPUsNoG_MEIRSjPdI7ExOiyaQh-f8tqCJLyyWrlo8=
 






___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch_v1] flow.c: refactor ct_orig_tuple check in miniflow_extract.

[Joe Stringer]

On 20 May 2017 at 11:09, Darrell Ball  wrote:
> The checks to populate ct_orig_tuple in miniflow_extract
> includes recirc_id being non-zero.  This is changed here
> to populate the ct_orig_tuple fields based only on ct_state
> per the logic of the design.
>
> Signed-off-by: Darrell Ball 

I'm not exactly sure what "per the logic of the design" is supposed to
mean, and I suspect that someone reading through the git logs would be
just as lost. As far as I'm aware, any patch to OVS should be "per the
logic of the design", right?

Do you mean "The ct_orig_tuple fields are designed to be read only if
the ct_state is valid, so do not populate that field if the packet has
not gone through the connection tracker"? I think that this kind of
description would provide at least a little extra context to help
understand the motivation behind the change.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch_v1] flow.c: refactor ct_orig_tuple check in miniflow_extract.

[Greg Rose]

On Sat, 2017-05-20 at 11:09 -0700, Darrell Ball wrote:
> The checks to populate ct_orig_tuple in miniflow_extract
> includes recirc_id being non-zero.  This is changed here
> to populate the ct_orig_tuple fields based only on ct_state
> per the logic of the design.
> 
> Signed-off-by: Darrell Ball 
> ---
>  lib/flow.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/flow.c b/lib/flow.c
> index 7f98a46..5ef783a 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -615,12 +615,15 @@ miniflow_extract(struct dp_packet *packet, struct 
> miniflow *dst)
>  }
>  miniflow_push_uint32(mf, dp_hash, md->dp_hash);
>  miniflow_push_uint32(mf, in_port, odp_to_u32(md->in_port.odp_port));
> -if (md->recirc_id || md->ct_state) {
> +if (md->ct_state) {
>  miniflow_push_uint32(mf, recirc_id, md->recirc_id);
>  miniflow_push_uint8(mf, ct_state, md->ct_state);
>  ct_nw_proto_p = miniflow_pointer(mf, ct_nw_proto);
>  miniflow_push_uint8(mf, ct_nw_proto, 0);
>  miniflow_push_uint16(mf, ct_zone, md->ct_zone);
> +} else if (md->recirc_id) {
> +miniflow_push_uint32(mf, recirc_id, md->recirc_id);
> +miniflow_pad_to_64(mf, recirc_id);
>  }
>  
>  if (md->ct_state) {

That looks right to me if the assertion in the commit statement is
correct.

Reviewed-by: Greg Rose 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [patch_v1] flow.c: refactor ct_orig_tuple check in miniflow_extract.

[Darrell Ball]

The checks to populate ct_orig_tuple in miniflow_extract
includes recirc_id being non-zero.  This is changed here
to populate the ct_orig_tuple fields based only on ct_state
per the logic of the design.

Signed-off-by: Darrell Ball 
---
 lib/flow.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/flow.c b/lib/flow.c
index 7f98a46..5ef783a 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -615,12 +615,15 @@ miniflow_extract(struct dp_packet *packet, struct 
miniflow *dst)
 }
 miniflow_push_uint32(mf, dp_hash, md->dp_hash);
 miniflow_push_uint32(mf, in_port, odp_to_u32(md->in_port.odp_port));
-if (md->recirc_id || md->ct_state) {
+if (md->ct_state) {
 miniflow_push_uint32(mf, recirc_id, md->recirc_id);
 miniflow_push_uint8(mf, ct_state, md->ct_state);
 ct_nw_proto_p = miniflow_pointer(mf, ct_nw_proto);
 miniflow_push_uint8(mf, ct_nw_proto, 0);
 miniflow_push_uint16(mf, ct_zone, md->ct_zone);
+} else if (md->recirc_id) {
+miniflow_push_uint32(mf, recirc_id, md->recirc_id);
+miniflow_pad_to_64(mf, recirc_id);
 }
 
 if (md->ct_state) {
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev