Re: [ovs-dev] [PATCH] tests: Only run test on kernel datapath

2019-10-04 Thread Darrell Ball
on. > >>>> > >>>> Also change the name of the test to make it more informativeand > >>>> less redundant and add comments with a short explanation. > >>>> > >>>> Fixes: d7fd61a ("tests: Add check for correct l3l4 conntrack f

Re: [ovs-dev] [PATCH v2] tests: Add check for correct l3l4 conntrack frag reassembly

2019-10-04 Thread Darrell Ball
On Fri, Oct 4, 2019 at 7:05 AM Gregory Rose wrote: > > On 10/3/2019 6:56 PM, Darrell Ball wrote: > > Thanks for the patch > > > > This approach will not work for the userspace datapath > > > > Few issues off the top of my head: > > > > 1/

Re: [ovs-dev] [PATCH v2] tests: Add check for correct l3l4 conntrack frag reassembly

2019-10-03 Thread Darrell Ball
On Thu, Oct 3, 2019 at 6:56 PM Darrell Ball wrote: > Thanks for the patch > > This approach will not work for the userspace datapath > > Few issues off the top of my head: > > 1/ packet-out frees the packet (which is a fragment in this case) after > completion >

Re: [ovs-dev] [PATCH v2] tests: Add check for correct l3l4 conntrack frag reassembly

2019-10-03 Thread Darrell Ball
Thanks for the patch This approach will not work for the userspace datapath Few issues off the top of my head: 1/ packet-out frees the packet (which is a fragment in this case) after completion hence multiple packet-outs need to be part of a single Openflow bundle command as in other similar

Re: [ovs-dev] [PATCH] datapath: Fix conntrack cache with timeout

2019-09-29 Thread Darrell Ball
Thanks for the patch Looks good and matches the upstream version, including the rcu deference fixup. Thanks for remembering to add the requested test incremental, post fix. Darrell On Fri, Sep 27, 2019 at 2:14 PM Yi-Hung Wei wrote: > This patch is from the following upstream net-next commit

Re: [ovs-dev] [PATCH branch-2.6] conntrack: Fix ICMPv4 error data L4 length check.

2019-09-28 Thread Darrell Ball
Thanks for doing this Vishal ! Except for minor patch formatting issues (inline), this is fine and also tests fine. On Fri, Sep 27, 2019 at 10:54 PM Vishal Deep Ajmera < vishal.deep.ajm...@ericsson.com> wrote: > From: Darrell Ball > > The ICMPv4 error data L4 length check was

Re: [ovs-dev] [patch v3] conntrack: Add option to disable TCP sequence checking.

2019-09-25 Thread Darrell Ball
On Wed, Sep 25, 2019 at 1:46 PM Ben Pfaff wrote: > On September 25, 2019 1:42:36 PM PDT, Darrell Ball > wrote: >> >> Thank you >> >> Pls see inline >> >> On Wed, Sep 25, 2019 at 10:26 AM Ben Pfaff wrote: >> >>> On Tue, Sep 24, 201

[ovs-dev] [patch v4] conntrack: Add option to disable TCP sequence checking.

2019-09-25 Thread Darrell Ball
. Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html Signed-off-by: Darrell Ball --- v4: Splice tcp sequence number check in tcp_conn_update() out as a function for clarity (Ben). Fix up some dpctl man page comments (Ben). s/ckk/chk/ for 'tcp_seq_chk

Re: [ovs-dev] [patch v3] conntrack: Add option to disable TCP sequence checking.

2019-09-25 Thread Darrell Ball
Thank you Pls see inline On Wed, Sep 25, 2019 at 10:26 AM Ben Pfaff wrote: > On Tue, Sep 24, 2019 at 03:47:35PM -0700, Darrell Ball wrote: > > This may be needed in some special cases, such as to support some > hardware > > offload implementations. Note that disabling T

Re: [ovs-dev] [patch v6] conntrack: Optimize recirculations.

2019-09-25 Thread Darrell Ball
On Wed, Sep 25, 2019 at 10:51 AM Ben Pfaff wrote: > On Mon, Aug 26, 2019 at 09:05:44AM -0700, Darrell Ball wrote: > > Cache the 'conn' context and use it when it is valid. The cached 'conn' > > context will get reset if it is not expected to be valid; the cost to do > &g

[ovs-dev] [patch v3] conntrack: Add option to disable TCP sequence checking.

2019-09-24 Thread Darrell Ball
. Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html Signed-off-by: Darrell Ball --- v3: Make manpage comments more verbose. Expand commit message comments. v2: Per particular requirement, support 'no-tcp-seq-chk' rather than 'liberal' mode. NEWS

Re: [ovs-dev] [patch v2] conntrack: Add option to disable TCP sequence checking.

2019-09-24 Thread Darrell Ball
700, Darrell Ball wrote: > > On Wed, Jun 12, 2019 at 10:58 AM Ben Pfaff wrote: > > > > > On Wed, Jun 12, 2019 at 10:31:17AM -0700, Darrell Ball wrote: > > > > On Wed, Jun 12, 2019 at 10:09 AM Ben Pfaff wrote: > > > > > > > > > On Wed, Jun 12, 2

Re: [ovs-dev] [patch v1] conntrack: Fix 'check_orig_tuple()' Valgrind false positive.

2019-09-24 Thread Darrell Ball
Thanks Ben Would you mind applying to 2.12 as well. Darrell On Tue, Sep 24, 2019 at 2:34 PM Ben Pfaff wrote: > On Mon, Sep 23, 2019 at 04:44:33PM -0700, Darrell Ball wrote: > > Valgrind reported that 'pkt->md.ct_orig_tuple.ipv4.ipv4_proto' is > > uninitialized in

[ovs-dev] [patch v1] conntrack: Fix 'check_orig_tuple()' Valgrind false positive.

2019-09-23 Thread Darrell Ball
4733c527da ("conntrack: Validate accessing of conntrack data in pkt_metadata.") CC: Yifeng Sun Signed-off-by: Darrell Ball --- lib/conntrack.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/conntrack.c b/lib/conntrack.c index fd71e6c..b56ef06 100644 --- a/l

[ovs-dev] [patch v2] conntrack: Fix 'reverse_nat_packet()' variable datatype.

2019-08-30 Thread Darrell Ball
und by inspection. Fixes: edd1bef468c0 ("dpdk: Add more ICMP Related NAT support.") Signed-off-by: Darrell Ball --- v2: Elaborate added comments. lib/conntrack.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/conntrack.c b/lib/conntrack.c index e5266e5..6452d82 100

[ovs-dev] [patch v1] conntrack: Fix 'reverse_nat_packet()' variable datatype.

2019-08-29 Thread Darrell Ball
und by inspection. Fixes: edd1bef468c0 ("dpdk: Add more ICMP Related NAT support.") Signed-off-by: Darrell Ball --- lib/conntrack.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/conntrack.c b/lib/conntrack.c index e5266e5..79d2711 100644 --- a/lib/conntrack.c +++ b/li

Re: [ovs-dev] [patch v2] conntrack: Fix ICMPv4 error data L4 length check.

2019-08-29 Thread Darrell Ball
On Thu, Aug 29, 2019 at 7:37 AM Ben Pfaff wrote: > On Tue, Aug 27, 2019 at 04:59:02PM -0700, Darrell Ball wrote: > > The ICMPv4 error data L4 length check was found to be too strict for TCP, > > expecting a minimum of 20 rather than 8 bytes. This worked by > > hapens

Re: [ovs-dev] [PATCH v3] userspace: Enable non-bridge port as tunnel endpoint.

2019-08-29 Thread Darrell Ball
much code. There is more work associated with handling the difference b/w datapath types (kernel vs userspace) plus other things. Using the bridge internal interface for the vtep, as is already supported, has some advantages including that it is already there. > > On Wed, Aug 28, 2019 at 12:46

Re: [ovs-dev] [PATCH v3] userspace: Enable non-bridge port as tunnel endpoint.

2019-08-28 Thread Darrell Ball
Thanks for the patch How about writing a system test ? Darrell On Wed, Aug 28, 2019 at 10:50 AM Yifeng Sun wrote: > For userspace datapath, currently only the bridge itself, the LOCAL port, > can be the tunnel endpoint to encap/decap tunnel packets. This patch > enables non-bridge port as

Re: [ovs-dev] [patch v1] conntrack: Fix ICMPV4 error data L4 length check.

2019-08-28 Thread Darrell Ball
On Wed, Aug 28, 2019 at 1:43 AM Vishal Deep Ajmera < vishal.deep.ajm...@ericsson.com> wrote: > That is interesting > > i just tried applying on top of tree and I see that the git applies some > changes (2 lines) > > in extract_l4_icmp6() rather the intended extract_l4_icmp() as in the > patch I

[ovs-dev] [patch v2] conntrack: Fix ICMPv4 error data L4 length check.

2019-08-27 Thread Darrell Ball
") CC: Daniele Di Proietto Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/361949.html Reported-by: Vishal Deep Ajmera Signed-off-by: Vishal Deep Ajmera Co-authored-by: Vishal Deep Ajmera Signed-off-by: Darrell Ball --- v2: Rebase to fix git applying to wron

Re: [ovs-dev] [patch v1] conntrack: Fix ICMPV4 error data L4 length check.

2019-08-27 Thread Darrell Ball
On Tue, Aug 27, 2019 at 2:02 AM Vishal Deep Ajmera < vishal.deep.ajm...@ericsson.com> wrote: > Hi Darrell, > > Thanks for the patch. When I applied the patch to latest master, > I see that we take care of length check (< 8) only for ICMPv6 and > not for ICMPv4. That is interesting i just tried

Re: [ovs-dev] [PATCH] conntrack: Correct length check for tcp packet inside ICMP data.

2019-08-26 Thread Darrell Ball
On Fri, Aug 23, 2019 at 9:09 AM Darrell Ball wrote: > Thanks for the patch > > Goes back to release 2.6/day one :-). > > I'll provide more feedback after today. > I sent an alternative patch here https://mail.openvswitch.org/pipermail/ovs-dev/2019-August/362013.htm

Re: [ovs-dev] [patch v1] conntrack: Fix ICMPV4 error data L4 length check.

2019-08-26 Thread Darrell Ball
Resent this patch, as it had a bad e-mail address Darrell On Mon, Aug 26, 2019 at 9:06 AM Darrell Ball wrote: > The ICMP error data L4 length check was found to be too strict for TCP, > expecting a minimum of 20 rather than 8 bytes. This worked by > hapenstance for other inner

[ovs-dev] [patch v1] conntrack: Fix ICMPV4 error data L4 length check.

2019-08-26 Thread Darrell Ball
s-dev/2019-August/361949.html Reported-by: Vishal Deep Ajmera Signed-off-by: Vishal Deep Ajmera Co-authored-by: Vishal Deep Ajmera Signed-off-by: Darrell Ball --- lib/conntrack.c | 38 ++ lib/packets.h | 3 +++ 2 files changed, 25 insertions(+), 16 deletion

[ovs-dev] [patch v1] conntrack: Fix ICMPV4 error data L4 length check.

2019-08-26 Thread Darrell Ball
s-dev/2019-August/361949.html Reported-by: Vishal Deep Ajmera Signed-off-by: Vishal Deep Ajmera Co-authored-by: Vishal Deep Ajmera Signed-off-by: Darrell Ball --- lib/conntrack.c | 38 ++ lib/packets.h | 3 +++ 2 files changed, 25 insertions(+), 16 deletion

[ovs-dev] [patch v2] conntrack: Add option to disable TCP sequence checking.

2019-08-26 Thread Darrell Ball
. Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html Signed-off-by: Darrell Ball --- v2: Per particular requirement, support 'no-tcp-seq-chk' rather than 'liberal' mode. Make manpage comments more verbose. Expand commit message comments. lib/conntrack

[ovs-dev] [patch v6] conntrack: Optimize recirculations.

2019-08-26 Thread Darrell Ball
. A negative test is added to check the resetting of the cached 'conn'. Signed-off-by: Darrell Ball --- v6: a/ Added 'conn' reset for mpls push case to force 'invalid', for consistency reasons. b/ Add missed lock around 'conn->mark' and 'conn->label' access in 'process_on

Re: [ovs-dev] [PATCH] conntrack: Correct length check for tcp packet inside ICMP data.

2019-08-23 Thread Darrell Ball
Thanks for the patch Goes back to release 2.6/day one :-). I'll provide more feedback after today. On Fri, Aug 23, 2019 at 6:20 AM Vishal Deep Ajmera < vishal.deep.ajm...@ericsson.com> wrote: > An ICMP packet with type destination or host not reachable also carries > 28 bytes of ICMP data

Re: [ovs-dev] [PATCH] conntrack: check the result of extract_l3_ipv4/6

2019-08-21 Thread Darrell Ball
On Wed, Aug 21, 2019 at 3:13 PM Ben Pfaff wrote: > On Mon, Aug 19, 2019 at 08:35:11AM -0700, Darrell Ball wrote: > > Thanks for the patch > > > > On Sun, Aug 18, 2019 at 11:01 PM Li RongQing > wrote: > > > > > the result of extract_l3_ipv4/6 should be c

Re: [ovs-dev] [PATCH v2 9/9] system-traffic: Add zone-based conntrack timeout policy test

2019-08-20 Thread Darrell Ball
On Mon, Aug 12, 2019 at 5:22 PM Yi-Hung Wei wrote: > On Sun, Aug 11, 2019 at 12:30 PM Darrell Ball wrote: > > > > I did some further testing and ran into another issue; in this case, > one, I did not expect. > > > > I added an additional sending of packet

Re: [ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

2019-08-20 Thread Darrell Ball
On Tue, Aug 20, 2019 at 12:30 PM Yi-Hung Wei wrote: > On Tue, Aug 20, 2019 at 12:46 AM Darrell Ball wrote: > > After fixing a bug in my proposed incremental and adding tracking of an > already removed sub timeout policy: > > Pls double check. > > Thanks for the pro

Re: [ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

2019-08-20 Thread Darrell Ball
On Mon, Aug 19, 2019 at 7:41 PM Darrell Ball wrote: > > > On Mon, Aug 19, 2019 at 12:42 PM Darrell Ball wrote: > >> >> >> On Mon, Aug 19, 2019 at 10:52 AM Yi-Hung Wei >> wrote: >> >>> On Fri, Aug 16, 2019 at 5:07 PM Darrell Ball wrote: >

Re: [ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

2019-08-19 Thread Darrell Ball
On Mon, Aug 19, 2019 at 12:42 PM Darrell Ball wrote: > > > On Mon, Aug 19, 2019 at 10:52 AM Yi-Hung Wei wrote: > >> On Fri, Aug 16, 2019 at 5:07 PM Darrell Ball wrote: >> > >> > Thanks for the patch >> > >> > Pls let me know if this incr

Re: [ovs-dev] [PATCH v4 6/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

2019-08-19 Thread Darrell Ball
On Mon, Aug 19, 2019 at 11:12 AM Yi-Hung Wei wrote: > On Fri, Aug 16, 2019 at 5:10 PM Darrell Ball wrote: > > > > Thanks for the patch > > > > Pls let me know if the following incremental works for you. > > > > diff --git a/ofproto/ofproto-dpif.c b/o

Re: [ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

2019-08-19 Thread Darrell Ball
On Mon, Aug 19, 2019 at 10:52 AM Yi-Hung Wei wrote: > On Fri, Aug 16, 2019 at 5:07 PM Darrell Ball wrote: > > > > Thanks for the patch > > > > Pls let me know if this incremental works for you. > > Main change is logging fix for timeout policy deletion. > &g

Re: [ovs-dev] [PATCH] conntrack: check the result of extract_l3_ipv4/6

2019-08-19 Thread Darrell Ball
Thanks for the patch On Sun, Aug 18, 2019 at 11:01 PM Li RongQing wrote: > the result of extract_l3_ipv4/6 should be checked in reverse_nat_packet > when it is false, meaning this packet is wrong, should not do handle it > continually > > Signed-off-by: Li RongQing > --- > lib/conntrack.c |

Re: [ovs-dev] [PATCH v4 6/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

2019-08-16 Thread Darrell Ball
Thanks for the patch Pls let me know if the following incremental works for you. diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 244155a..cb8b51e 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -168,6 +168,12 @@ struct ct_timeout_policy {

Re: [ovs-dev] [PATCH v4 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

2019-08-16 Thread Darrell Ball
Thanks for the patch Pls let me know if this incremental works for you. Main change is logging fix for timeout policy deletion. Darrell diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 1d4ee60..00d957b 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -2822,11 +2822,10 @@

Re: [ovs-dev] [PATCH v4 0/9] Support zone-based conntrack timeout policy

2019-08-15 Thread Darrell Ball
Thanks for the patches I did some quick adhoc testing with 4.4.0-119 and 5.0.0-23. Some results worth noting: 1/ Failing to delete timeout policies with non-zero refcounts results in WARN logging > 2019-08-16T00:21:32.240Z|00153|dpif_netlink|WARN|failed to delete timeout policy ovs_tp_0_udp4

Re: [ovs-dev] [PATCH v3 2/9] ovs-vsctl: Add conntrack zone commands.

2019-08-15 Thread Darrell Ball
On Thu, Aug 15, 2019 at 10:53 AM William Tu wrote: > Thanks for the review. > > On Mon, Aug 12, 2019 at 09:54:42PM -0700, Darrell Ball wrote: > > Thanks for the patch > > > > Thanks for the fixups; mostly minor comments inline. > > > > On Mon, Aug 1

Re: [ovs-dev] [PATCH v3 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

2019-08-14 Thread Darrell Ball
Thanks for the patch mostly minor comments On Mon, Aug 12, 2019 at 5:54 PM Yi-Hung Wei wrote: > This patch first defines the dpif interface for a datapath to support > adding, deleting, getting and dumping conntrack timeout policy. > The timeout policy is identified by a 4 bytes unsigned

Re: [ovs-dev] [PATCH v3 6/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

2019-08-14 Thread Darrell Ball
On Wed, Aug 14, 2019 at 1:28 PM Yi-Hung Wei wrote: > On Tue, Aug 13, 2019 at 7:46 PM Darrell Ball wrote: > > > > Thanks for the patch > > > > Some high level comments: > > > > 1/ The ct_tp_kill_list code is still in common code > > I think we

Re: [ovs-dev] [PATCH v3 1/9] ovs-vswitchd: Add Datapath, CT_Zone, and CT_Zone_Policy tables.

2019-08-14 Thread Darrell Ball
On Wed, Aug 14, 2019 at 9:47 AM Yi-Hung Wei wrote: > On Mon, Aug 12, 2019 at 7:46 PM Darrell Ball wrote: > >> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema > >> index f7c6eb8983cd..c0a2242ad345 100644 > >> --- a/vswitchd/vswitch

Re: [ovs-dev] [PATCH v3 9/9] ofproto-dpif-xlate: Translate timeout policy in ct action

2019-08-13 Thread Darrell Ball
On Tue, Aug 13, 2019 at 8:03 PM Darrell Ball wrote: > Thanks for the patch > > few more comments > > On Mon, Aug 12, 2019 at 5:57 PM Yi-Hung Wei wrote: > >> This patch derives the timeout policy based on ct zone from the >> internal data structure that we maintai

Re: [ovs-dev] [PATCH v3 9/9] ofproto-dpif-xlate: Translate timeout policy in ct action

2019-08-13 Thread Darrell Ball
Thanks for the patch few more comments On Mon, Aug 12, 2019 at 5:57 PM Yi-Hung Wei wrote: > This patch derives the timeout policy based on ct zone from the > internal data structure that we maintain on dpif layer. > > It also adds a system traffic test to verify the zone-based conntrack >

Re: [ovs-dev] [PATCH v3 6/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

2019-08-13 Thread Darrell Ball
Thanks for the patch Some high level comments: 1/ The ct_tp_kill_list code is still in common code I think we discussed moving that to the dpif backer code ct_timeout_policy_unref() is adding to this deferred kill list which is not needed for userspace datapath. 2/

Re: [ovs-dev] [PATCH v3 9/9] ofproto-dpif-xlate: Translate timeout policy in ct action

2019-08-13 Thread Darrell Ball
On Tue, Aug 13, 2019 at 2:33 PM Yi-Hung Wei wrote: > On Tue, Aug 13, 2019 at 11:43 AM Darrell Ball wrote: > > Sure, circling back to this part > > > > yep, it is the Linux In-tree kernel module rather than OVS tree module > > > > dball@ubuntu:~/ovs$ modinfo

Re: [ovs-dev] [PATCH v3 9/9] ofproto-dpif-xlate: Translate timeout policy in ct action

2019-08-13 Thread Darrell Ball
On Tue, Aug 13, 2019 at 11:01 AM Yi-Hung Wei wrote: > On Mon, Aug 12, 2019 at 7:35 PM Darrell Ball wrote: > > > > Thanks for the patch > > > > Not a full review; I just did a quick run of the test using a more > recent kernel version > > > > dball@u

Re: [ovs-dev] [PATCH v3 2/9] ovs-vsctl: Add conntrack zone commands.

2019-08-12 Thread Darrell Ball
Thanks for the patch Thanks for the fixups; mostly minor comments inline. On Mon, Aug 12, 2019 at 5:53 PM Yi-Hung Wei wrote: > From: William Tu > > The patch adds commands creating/deleting/listing conntrack zone > timeout policies: > $ ovs-vsctl {add,del,list}-zone-tp dp zone=zone_id ... >

Re: [ovs-dev] [PATCH v3 1/9] ovs-vswitchd: Add Datapath, CT_Zone, and CT_Zone_Policy tables.

2019-08-12 Thread Darrell Ball
Thanks for the patch On Mon, Aug 12, 2019 at 5:52 PM Yi-Hung Wei wrote: > From: Justin Pettit > > Signed-off-by: Justin Pettit > Signed-off-by: Yi-Hung Wei > Co-authored-by: Yi-Hung Wei > --- > vswitchd/vswitch.ovsschema | 51 - > vswitchd/vswitch.xml | 275 >

Re: [ovs-dev] [PATCH v3 9/9] ofproto-dpif-xlate: Translate timeout policy in ct action

2019-08-12 Thread Darrell Ball
Thanks for the patch Not a full review; I just did a quick run of the test using a more recent kernel version dball@ubuntu:~/ovs$ uname -r 5.0.0-23-generic dball@ubuntu:~/ovs$ lsb_release -a No LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 18.04.3 LTS Release: 18.04

Re: [ovs-dev] [PATCH v3 0/9] Support zone-based conntrack timeout policy

2019-08-12 Thread Darrell Ball
Thanks for the patches On Mon, Aug 12, 2019 at 5:51 PM Yi-Hung Wei wrote: > This patch series enables zone-based conntrack timeout policy support in > OVS. > Timeout policy is a set of timeout attributes that can be associated with a > connection when it is committed. Then, the connection

Re: [ovs-dev] [PATCH v2 9/9] system-traffic: Add zone-based conntrack timeout policy test

2019-08-12 Thread Darrell Ball
On Mon, Aug 12, 2019 at 5:15 PM Yi-Hung Wei wrote: > On Sun, Aug 11, 2019 at 12:30 PM Darrell Ball wrote: > > > > I did some further testing and ran into another issue; in this case, > one, I did not expect. > > > > I added an additional sending of packet

Re: [ovs-dev] [PATCH v2 9/9] system-traffic: Add zone-based conntrack timeout policy test

2019-08-12 Thread Darrell Ball
On Mon, Aug 12, 2019 at 5:22 PM Yi-Hung Wei wrote: > On Sun, Aug 11, 2019 at 12:30 PM Darrell Ball wrote: > > > > I did some further testing and ran into another issue; in this case, > one, I did not expect. > > > > I added an additional sending of packet

Re: [ovs-dev] [PATCH v2 9/9] system-traffic: Add zone-based conntrack timeout policy test

2019-08-11 Thread Darrell Ball
c=10.1.1.2,dst=10.1.1.1,sport=,dport=),zone=5 On Tue, Aug 6, 2019 at 12:16 PM Darrell Ball wrote: > > > On Tue, Aug 6, 2019 at 11:07 AM Yi-Hung Wei wrote: > >> On Tue, Aug 6, 2019 at 10:21 AM Darrell Ball wrote: >> > >> > >> > I did

Re: [ovs-dev] [PATCH v2 8/9] ofproto-dpif-xlate: Translate timeout policy in ct action

2019-08-11 Thread Darrell Ball
On Fri, Aug 9, 2019 at 1:10 PM Justin Pettit wrote: > > > On Aug 1, 2019, at 3:07 PM, Yi-Hung Wei wrote: > > > > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > > index 79a2314500cf..57b32ccb610f 100644 > > --- a/lib/dpif-provider.h > > +++ b/lib/dpif-provider.h > > @@ -536,6 +536,11

Re: [ovs-dev] [PATCH v2 5/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

2019-08-11 Thread Darrell Ball
On Fri, Aug 9, 2019 at 1:23 PM Justin Pettit wrote: > > > On Aug 7, 2019, at 11:40 AM, Darrell Ball wrote: > > > > There are 3 behaviors with the patchset that are datapath specific > > > > 1/ Unwildcarding of commit flows with timeout policies > > A

Re: [ovs-dev] [PATCH v2 2/9] ovs-vsctl: Add conntrack zone commands.

2019-08-07 Thread Darrell Ball
On Wed, Aug 7, 2019 at 1:37 PM William Tu wrote: > Thanks for the review. > > On Mon, Aug 05, 2019 at 04:12:02PM -0700, Darrell Ball wrote: > > Thanks for the patch > > > > I noticed '--may-exist' and '--if-exists' are supported now for > > add--zone-tp/d

Re: [ovs-dev] [PATCH v2 5/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

2019-08-07 Thread Darrell Ball
On Wed, Aug 7, 2019 at 11:51 AM Darrell Ball wrote: > > > On Wed, Aug 7, 2019 at 11:40 AM Darrell Ball wrote: > >> >> >> On Tue, Aug 6, 2019 at 9:57 PM Justin Pettit wrote: >> >>> >>> > On Aug 5, 2019, at 8:07 PM, Darrell Ball wrot

Re: [ovs-dev] [PATCH v2 5/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

2019-08-07 Thread Darrell Ball
On Wed, Aug 7, 2019 at 11:40 AM Darrell Ball wrote: > > > On Tue, Aug 6, 2019 at 9:57 PM Justin Pettit wrote: > >> >> > On Aug 5, 2019, at 8:07 PM, Darrell Ball wrote: >> > >> > On Thu, Aug 1, 2019 at 3:10 PM Yi-Hung Wei >> wrote: >> &

Re: [ovs-dev] [PATCH v2 5/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

2019-08-07 Thread Darrell Ball
On Tue, Aug 6, 2019 at 9:57 PM Justin Pettit wrote: > > > On Aug 5, 2019, at 8:07 PM, Darrell Ball wrote: > > > > On Thu, Aug 1, 2019 at 3:10 PM Yi-Hung Wei wrote: > > > >> +struct ct_timeout_policy { > >> +struct uuid uuid; > >&g

Re: [ovs-dev] [PATCH v2 9/9] system-traffic: Add zone-based conntrack timeout policy test

2019-08-06 Thread Darrell Ball
On Tue, Aug 6, 2019 at 11:07 AM Yi-Hung Wei wrote: > On Tue, Aug 6, 2019 at 10:21 AM Darrell Ball wrote: > > > > > > I did some more testing and found a similar problem as in V1. > > > > This test can be run successfully once and then fails after

Re: [ovs-dev] [PATCH v2 8/9] ofproto-dpif-xlate: Translate timeout policy in ct action

2019-08-06 Thread Darrell Ball
On Mon, Aug 5, 2019 at 8:51 PM Darrell Ball wrote: > Thanks for the patch > > The main comment I had from the V1 patch was adding the check > > +if (ofc->flags & NX_CT_F_COMMIT) { > > in compose_conntrack_action() > > I see that was done. > > Aft

Re: [ovs-dev] [PATCH v2 9/9] system-traffic: Add zone-based conntrack timeout policy test

2019-08-06 Thread Darrell Ball
On Mon, Aug 5, 2019 at 9:03 PM Darrell Ball wrote: > Thanks for the patch > > I see the test is much improved now from V1 and passes - thanks > > Ideally, tests should be associated with some code for context > It could be folded into patch 8 > I did some more testin

Re: [ovs-dev] [PATCH v2 9/9] system-traffic: Add zone-based conntrack timeout policy test

2019-08-05 Thread Darrell Ball
Thanks for the patch I see the test is much improved now from V1 and passes - thanks Ideally, tests should be associated with some code for context It could be folded into patch 8 On Thu, Aug 1, 2019 at 3:12 PM Yi-Hung Wei wrote: > This patch adds a system traffic test to verify the

Re: [ovs-dev] [PATCH v2 8/9] ofproto-dpif-xlate: Translate timeout policy in ct action

2019-08-05 Thread Darrell Ball
Thanks for the patch The main comment I had from the V1 patch was adding the check +if (ofc->flags & NX_CT_F_COMMIT) { in compose_conntrack_action() I see that was done. After a quick scan, I had one minor comment inline. On Thu, Aug 1, 2019 at 3:12 PM Yi-Hung Wei wrote: > This patch

Re: [ovs-dev] [PATCH v2 5/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables

2019-08-05 Thread Darrell Ball
Thanks for the patch comments inline On Thu, Aug 1, 2019 at 3:10 PM Yi-Hung Wei wrote: > This patch consumes the CT_Zone and CT_Timeout_Policy tables, maintains > the zone-based timeout policy in the vswitchd. Whenever there is a > database change, vswitchd will read the datapath, CT_Zone, and

Re: [ovs-dev] [PATCH v2 4/9] ct-dpif, dpif-netlink: Add conntrack timeout policy support

2019-08-05 Thread Darrell Ball
Thanks for the patch I am going to avoid commenting on style or code conciseness in the interests of time On Thu, Aug 1, 2019 at 3:10 PM Yi-Hung Wei wrote: > This patch first defines the dpif interface for a datapath to support > adding, deleting, getting and dumping conntrack timeout policy.

Re: [ovs-dev] [PATCH v2 1/9] ovs-vswitchd: Add Datapath, CT_Zone, and CT_Zone_Policy tables.

2019-08-05 Thread Darrell Ball
One comment fix: s/ "min": 0, "max": "65535"}},/ "min": 0, "max": "65536"}},/ On Mon, Aug 5, 2019 at 4:09 PM Darrell Ball wrote: > Thanks for the patch > > I avoided duplicate comments from what Justin suggested >

Re: [ovs-dev] [PATCH v2 2/9] ovs-vsctl: Add conntrack zone commands.

2019-08-05 Thread Darrell Ball
Thanks for the patch I noticed '--may-exist' and '--if-exists' are supported now for add--zone-tp/del-zone-tp - thanks The check for duplicate timeout policies now correctly checks all key and values - thanks Some more comments inline I am trying to avoid duplicate comment from Justin, so I just

Re: [ovs-dev] [PATCH v2 1/9] ovs-vswitchd: Add Datapath, CT_Zone, and CT_Zone_Policy tables.

2019-08-05 Thread Darrell Ball
Thanks for the patch I avoided duplicate comments from what Justin suggested comments inline On Thu, Aug 1, 2019 at 3:08 PM Yi-Hung Wei wrote: > From: Justin Pettit > > From: Justin Pettit > > Signed-off-by: Justin Pettit > --- > vswitchd/vswitch.ovsschema | 43 +++- >

Re: [ovs-dev] [PATCH 00/12] Support zone-based conntrack timeout policy

2019-07-31 Thread Darrell Ball
On Wed, Jul 31, 2019 at 1:25 AM Ilya Maximets wrote: > On 29.07.2019 21:53, Yi-Hung Wei wrote: > > Hi Ilya, > > > > Thanks for your comment. > > > > On Mon, Jul 29, 2019 at 2:22 AM Ilya Maximets > wrote: > >> > >> Hi everyone, > >> > >> My 2 cents for the feature design: > >> > >> From the

Re: [ovs-dev] [PATCH] OVN: fix DNAT/SNAT system-ovn unit tests

2019-07-30 Thread Darrell Ball
Thanks Lorenzo Looks good I did not retest this patch, but it is the same code you sent before that I had tested On Mon, Jul 29, 2019 at 4:41 AM Lorenzo Bianconi < lorenzo.bianc...@redhat.com> wrote: > Fix conntrack checks in the following tests in tests/system-ovn.at: > - ovn -- DNAT and SNAT

Re: [ovs-dev] [PATCH] test: do not require python2 for CHECK_CONNTRACK macro

2019-07-30 Thread Darrell Ball
On Tue, Jul 30, 2019 at 10:41 AM Mark Michelson wrote: > Why do these macros require python at all? > alg test tools > > On 7/29/19 7:48 AM, Lorenzo Bianconi wrote: > > Do not strictly require python2 for CHECK_CONNTRACK macro definitions in > > system-{kmod,userspace}-macros.at > > > >

Re: [ovs-dev] [PATCH] test: do not require python2 for CHECK_CONNTRACK macro

2019-07-30 Thread Darrell Ball
Thanks Lorenzo Looks good On Mon, Jul 29, 2019 at 4:48 AM Lorenzo Bianconi < lorenzo.bianc...@redhat.com> wrote: > Do not strictly require python2 for CHECK_CONNTRACK macro definitions in > system-{kmod,userspace}-macros.at > > Signed-off-by: Lorenzo Bianconi > --- >

Re: [ovs-dev] [PATCH 05/12] ct-dpif: Add conntrack timeout policy support in dpif layer

2019-07-29 Thread Darrell Ball
On Mon, Jul 29, 2019 at 3:51 PM Yi-Hung Wei wrote: > On Mon, Jul 29, 2019 at 1:12 PM Darrell Ball wrote: > >> > "is_default" - can you explain this one ? > >> > >> This flag is used to configure the default timeout policy in the > >> datapath

Re: [ovs-dev] [PATCH 03/12] ovs-vsctl: Add datapath and CT zone commands.

2019-07-29 Thread Darrell Ball
added one more comment. On Fri, Jul 26, 2019 at 4:10 PM Darrell Ball wrote: > added one more comment for now > > > On Fri, Jul 26, 2019 at 11:13 AM Darrell Ball wrote: > >> Thanks for the patch >> >> Not a full review; just some initial testing >> >&g

Re: [ovs-dev] [PATCH 00/12] Support zone-based conntrack timeout policy

2019-07-29 Thread Darrell Ball
On Mon, Jul 29, 2019 at 11:53 AM Yi-Hung Wei wrote: > Hi Ilya, > > Thanks for your comment. > > On Mon, Jul 29, 2019 at 2:22 AM Ilya Maximets > wrote: > > > > Hi everyone, > > > > My 2 cents for the feature design: > > > > From the user's perspective: > > > > * 'add-dp'/'del-dp' commands looks

Re: [ovs-dev] [PATCH 05/12] ct-dpif: Add conntrack timeout policy support in dpif layer

2019-07-29 Thread Darrell Ball
On Mon, Jul 29, 2019 at 12:37 PM Yi-Hung Wei wrote: > On Fri, Jul 26, 2019 at 11:41 AM Darrell Ball wrote: > > > > Thanks for the patch > > > > I found this patch hard to review since it does not contain > implementations > > The same comment applies to

Re: [ovs-dev] [PATCH 05/12] ct-dpif: Add conntrack timeout policy support in dpif layer

2019-07-29 Thread Darrell Ball
On Fri, Jul 26, 2019 at 11:41 AM Darrell Ball wrote: > Thanks for the patch > > I found this patch hard to review since it does not contain implementations > The same comment applies to Patch 6 > I think Patches 5-7 can be combined into one patch, which will make review > easi

Re: [ovs-dev] [PATCH 08/12] datapath-config: Consume datapath, CT_Zone, and CT_Timeout_Policy tables

2019-07-28 Thread Darrell Ball
One more comment Not a full review; just focusing on the more important parts for now. On Fri, Jul 26, 2019 at 5:44 PM Darrell Ball wrote: > Thanks for the patch; not a full review > > On Thu, Jul 25, 2019 at 4:29 PM Yi-Hung Wei wrote: > >> This patch reads the

Re: [ovs-dev] [PATCH 12/12] system-traffic: Add zone-based conntrack timeout policy test

2019-07-27 Thread Darrell Ball
One more comment inline regarding to the cleanup part On Fri, Jul 26, 2019 at 9:27 AM Darrell Ball wrote: > Thanks for the patch > > Comments inline > > On Thu, Jul 25, 2019 at 4:31 PM Yi-Hung Wei wrote: > >> This patch adds a system traffic test to verify the zone-ba

Re: [ovs-dev] [PATCH 11/12] ofproto-dpif-xlate: Translate timeout policy in ct action

2019-07-27 Thread Darrell Ball
One more comment inline On Thu, Jul 25, 2019 at 10:02 PM Darrell Ball wrote: > Thanks for the patch > > Few comments inline > > On Thu, Jul 25, 2019 at 4:30 PM Yi-Hung Wei wrote: > >> This patch derives the timeout policy based on ct zone from the >> inte

Re: [ovs-dev] [PATCH 08/12] datapath-config: Consume datapath, CT_Zone, and CT_Timeout_Policy tables

2019-07-26 Thread Darrell Ball
Thanks for the patch; not a full review On Thu, Jul 25, 2019 at 4:29 PM Yi-Hung Wei wrote: > This patch reads the datapath, CT_Zone, and CT_Timeout_Policy tables > from ovsdb, stores the information in a per datapath internal datapath > structure, and pushes down the conntrack timeout policy

Re: [ovs-dev] [PATCH 03/12] ovs-vsctl: Add datapath and CT zone commands.

2019-07-26 Thread Darrell Ball
added one more comment for now On Fri, Jul 26, 2019 at 11:13 AM Darrell Ball wrote: > Thanks for the patch > > Not a full review; just some initial testing > > > 1/ AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 > icmp_reply_blah=3])]) > > The

Re: [ovs-dev] [PATCH 05/12] ct-dpif: Add conntrack timeout policy support in dpif layer

2019-07-26 Thread Darrell Ball
Thanks for the patch I found this patch hard to review since it does not contain implementations The same comment applies to Patch 6 I think Patches 5-7 can be combined into one patch, which will make review easier. some other comments inline On Thu, Jul 25, 2019 at 4:27 PM Yi-Hung Wei wrote:

Re: [ovs-dev] [PATCH 03/12] ovs-vsctl: Add datapath and CT zone commands.

2019-07-26 Thread Darrell Ball
Thanks for the patch Not a full review; just some initial testing 1/ AT_CHECK([RUN_OVS_VSCTL([add-zone-tp netdev zone=2 icmp_first=2 icmp_reply_blah=3])]) The above syntax is NOT flagged as an error 2/ AT_CHECK([RUN_OVS_VSCTL([--may-exist add-zone-tp netdev zone=2 icmp_first=2

Re: [ovs-dev] [PATCH 12/12] system-traffic: Add zone-based conntrack timeout policy test

2019-07-26 Thread Darrell Ball
Thanks for the patch Comments inline On Thu, Jul 25, 2019 at 4:31 PM Yi-Hung Wei wrote: > This patch adds a system traffic test to verify the zone-based conntrack > timeout feature. The test uses ovs-vsctl commands to configure > the customized ICMP and UDP timeout on zone 5 to a shorter

Re: [ovs-dev] [PATCH 11/12] ofproto-dpif-xlate: Translate timeout policy in ct action

2019-07-25 Thread Darrell Ball
Thanks for the patch Few comments inline On Thu, Jul 25, 2019 at 4:30 PM Yi-Hung Wei wrote: > This patch derives the timeout policy based on ct zone from the > internal data structure that reads the configuration from ovsdb. > > Signed-off-by: Yi-Hung Wei > --- > lib/ct-dpif.c

Re: [ovs-dev] [PATCH v3] OVN: run local logical flows first in S_ROUTER_OUT_SNAT table

2019-07-20 Thread Darrell Ball
Thanks for the quick turnaround couple comments inline On Sat, Jul 20, 2019 at 4:07 AM Lorenzo Bianconi < lorenzo.bianc...@redhat.com> wrote: > > The following tests consistently fail for kernel and userspace datapaths > > > > 136: ovn -- DNAT and SNAT on distributed router - N/S FAILED ( > >

Re: [ovs-dev] [PATCH v3] OVN: run local logical flows first in S_ROUTER_OUT_SNAT table

2019-07-19 Thread Darrell Ball
The following tests consistently fail for kernel and userspace datapaths 136: ovn -- DNAT and SNAT on distributed router - N/S FAILED ( system-ovn.at:1337) 137: ovn -- DNAT and SNAT on distributed router - E/W FAILED ( system-ovn.at:1510) after this commit commit

Re: [ovs-dev] [patch v5] conntrack: Optimize recirculations.

2019-07-09 Thread Darrell Ball
Thanks for the review ! On Mon, Jul 8, 2019 at 2:58 PM Ben Pfaff wrote: > On Sun, Jun 09, 2019 at 07:32:03AM -0700, Darrell Ball wrote: > > Cache the 'conn' context and use it when it is valid. The cached 'conn' > > context will get reset if it is not expected to be valid;

Re: [ovs-dev] [PATCH] Documentation: Clarify connection tracking tutorial

2019-06-14 Thread Darrell Ball
Hi Greg few comments inline. On Fri, Jun 14, 2019 at 1:44 PM Greg Rose wrote: > The current documentation states that "all packets entering OVS for > the first time are "untracked"". However there is a minor exception > to this in the case where a packet (re)enters the same datapath and > the

Re: [ovs-dev] [PATCH 0/5 v2] datapath: Support 5.0.x kernel version

2019-06-13 Thread Darrell Ball
On Wed, Jun 12, 2019 at 3:38 PM Yifeng Sun wrote: > This series of patches enabled OVS to run on kernel 5.0.x. > > Tests shows that this series of patches passed check-kmod tests or > didn't introduce new failed check-kmod tests for the following > kernel versions: > 3.10.0-957.12.1.el7.x86_64

Re: [ovs-dev] [patch v2] conntrack: Add option to disable TCP sequence checking.

2019-06-12 Thread Darrell Ball
On Wed, Jun 12, 2019 at 10:58 AM Ben Pfaff wrote: > On Wed, Jun 12, 2019 at 10:31:17AM -0700, Darrell Ball wrote: > > On Wed, Jun 12, 2019 at 10:09 AM Ben Pfaff wrote: > > > > > On Wed, Jun 12, 2019 at 08:46:06AM -0700, Darrell Ball wrote: > > > > On Mo

Re: [ovs-dev] [patch v2] conntrack: Add option to disable TCP sequence checking.

2019-06-12 Thread Darrell Ball
On Wed, Jun 12, 2019 at 10:09 AM Ben Pfaff wrote: > On Wed, Jun 12, 2019 at 08:46:06AM -0700, Darrell Ball wrote: > > On Mon, Jun 10, 2019 at 9:51 AM Ben Pfaff wrote: > > > > > On Sun, Jun 09, 2019 at 07:35:09AM -0700, Darrell Ball wrote: > > > > This

Re: [ovs-dev] [patch v2] conntrack: Add option to disable TCP sequence checking.

2019-06-12 Thread Darrell Ball
On Mon, Jun 10, 2019 at 9:51 AM Ben Pfaff wrote: > On Sun, Jun 09, 2019 at 07:35:09AM -0700, Darrell Ball wrote: > > This may be needed in some special cases, such as to support some > > hardware offload implementations. > > > > Reported-at: > https://mail.openvswi

[ovs-dev] [patch v2] conntrack: Add option to disable TCP sequence checking.

2019-06-09 Thread Darrell Ball
This may be needed in some special cases, such as to support some hardware offload implementations. Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359188.html Signed-off-by: Darrell Ball --- v2: Per particular requirement, support 'no-tcp-seq-chk' rather than 'liberal

[ovs-dev] [patch v5] conntrack: Optimize recirculations.

2019-06-09 Thread Darrell Ball
Cache the 'conn' context and use it when it is valid. The cached 'conn' context will get reset if it is not expected to be valid; a negative test is added to check this. Signed-off-by: Darrell Ball --- v5: Check for alg ctl on recirculation to handle potential corner case. Remove

  1   2   3   4   5   6   7   8   9   10   >