Re: [ovs-dev] [PATCH v2 1/2] ofproto-ipfix: use per-domain template timeouts

[Mike Pattrick]

On Tue, Jan 24, 2023 at 2:21 PM Adrián Moreno  wrote:
>
> From: Adrian Moreno 
>
> IPFIX templates have to be sent for each Observation Domain ID.
> Currently, a timer is kept at each dpif_ipfix_exporter to send them.
> This works fine for per-bridge sampling where there is only one
> Observation Domain ID per exporter. However, this is does not work for
> per-flow sampling where more than one Observation Domain IDs can be
> specified by the controller. In this case, ovs-vswitchd will only send
> template information for one (arbitrary) DomainID.
>
> Fix per-flow sampling by using an hmap to keep a timer for each
> Observation Domain ID.
>
> Signed-off-by: Adrian Moreno 
>
> ---
> - v2:
>   - Fixed a potential race condition in unit test.
> - v1:
>   - Added unit test.
>   - Drop domain_id from "struct dpif_ipfix_domain" since the hashmap
>   already uses it as index.
>   - Added OVS_REQUES(mutex) to dpif_ipfix_exporter_find_domain.
> ---
>  ofproto/ofproto-dpif-ipfix.c | 127 ---
>  tests/system-traffic.at  |  52 ++
>  2 files changed, 156 insertions(+), 23 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index 742eed399..fa71fda0f 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -124,11 +124,18 @@ struct dpif_ipfix_port {
>  uint32_t ifindex;
>  };
>
> +struct dpif_ipfix_domain {
> +struct hmap_node hmap_node; /* In struct dpif_ipfix_exporter's domains. 
> */
> +time_t last_template_set_time;
> +};
> +
>  struct dpif_ipfix_exporter {
>  uint32_t exporter_id; /* Exporting Process identifier */
>  struct collectors *collectors;
>  uint32_t seq_number;
> -time_t last_template_set_time;
> +struct hmap domains; /* Contains struct dpif_ipfix_domain indexed by
> +observation domain id. */
> +time_t last_stats_sent_time;
>  struct hmap cache_flow_key_map;  /* ipfix_flow_cache_entry. */
>  struct ovs_list cache_flow_start_timestamp_list;  /* 
> ipfix_flow_cache_entry. */
>  uint32_t cache_active_timeout;  /* In seconds. */
> @@ -617,6 +624,9 @@ static void get_export_time_now(uint64_t *, uint32_t *);
>
>  static void dpif_ipfix_cache_expire_now(struct dpif_ipfix_exporter *, bool);
>
> +static void dpif_ipfix_exporter_del_domain(struct dpif_ipfix_exporter * ,
> +   struct dpif_ipfix_domain * );
> +
>  static bool
>  ofproto_ipfix_bridge_exporter_options_equal(
>  const struct ofproto_ipfix_bridge_exporter_options *a,
> @@ -697,13 +707,14 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter 
> *exporter)
>  exporter->exporter_id = ++exporter_total_count;
>  exporter->collectors = NULL;
>  exporter->seq_number = 1;
> -exporter->last_template_set_time = 0;
> +exporter->last_stats_sent_time = 0;
>  hmap_init(>cache_flow_key_map);
>  ovs_list_init(>cache_flow_start_timestamp_list);
>  exporter->cache_active_timeout = 0;
>  exporter->cache_max_flows = 0;
>  exporter->virtual_obs_id = NULL;
>  exporter->virtual_obs_len = 0;
> +hmap_init(>domains);
>
>  memset(>ipfix_global_stats, 0,
> sizeof(struct dpif_ipfix_global_stats));
> @@ -711,6 +722,7 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter 
> *exporter)
>
>  static void
>  dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter *exporter)
> +OVS_REQUIRES(mutex)
>  {
>  /* Flush the cache with flow end reason "forced end." */
>  dpif_ipfix_cache_expire_now(exporter, true);
> @@ -719,22 +731,29 @@ dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter 
> *exporter)
>  exporter->exporter_id = 0;
>  exporter->collectors = NULL;
>  exporter->seq_number = 1;
> -exporter->last_template_set_time = 0;
> +exporter->last_stats_sent_time = 0;
>  exporter->cache_active_timeout = 0;
>  exporter->cache_max_flows = 0;
>  free(exporter->virtual_obs_id);
>  exporter->virtual_obs_id = NULL;
>  exporter->virtual_obs_len = 0;
>
> +struct dpif_ipfix_domain *dom;
> +HMAP_FOR_EACH_SAFE (dom, hmap_node, >domains) {
> +dpif_ipfix_exporter_del_domain(exporter, dom);
> +}
> +
>  memset(>ipfix_global_stats, 0,
> sizeof(struct dpif_ipfix_global_stats));
>  }
>
>  static void
>  dpif_ipfix_exporter_destroy(struct dpif_ipfix_exporter *exporter)
> +OVS_REQUIRES(mutex)
>  {
>  dpif_ipfix_exporter_clear(exporter);
>  hmap_destroy(>cache_flow_key_map);
> +hmap_destroy(>domains);
>  }
>
>  static bool
> @@ -742,7 +761,7 @@ dpif_ipfix_exporter_set_options(struct 
> dpif_ipfix_exporter *exporter,
>  const struct sset *targets,
>  const uint32_t cache_active_timeout,
>  const uint32_t cache_max_flows,
> -const char *virtual_obs_id)
> +const 

Re: [ovs-dev] [RFC] mcast-snopping: do not learn from well defined multicast IPv6 groups

[Lorenzo Bianconi]

> On 1/23/23 16:36, Lorenzo Bianconi wrote:
> >> On Mon, 2023-01-23 at 10:01 +0100, Lorenzo Bianconi wrote:
> >>> Avoid learning Link-Local reserved multicast addresses if advertised in
> >>> a MLD reports since this interferes with Slaac IPv6 address resolution
> >>> implemented in OVN.
> >>>
> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2154930
> >>> Tested-by: Eduardo Olivares 
> >>> Signed-off-by: Lorenzo Bianconi 
> >>> ---
> >>>  lib/mcast-snooping.c | 69 
> >>>  lib/packets.c    | 11 +++
> >>>  lib/packets.h    | 13 +
> >>>  3 files changed, 74 insertions(+), 19 deletions(-)
> >>>
> >>> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
> >>> index 029ca2855..a9d2e7900 100644
> >>> --- a/lib/mcast-snooping.c
> >>> +++ b/lib/mcast-snooping.c
> >>> @@ -38,6 +38,8 @@
> >>>  COVERAGE_DEFINE(mcast_snooping_learned);
> >>>  COVERAGE_DEFINE(mcast_snooping_expired);
> >>>  
> >>> +VLOG_DEFINE_THIS_MODULE(mcast_snooping);
> >>> +
> >>>  static struct mcast_port_bundle *
> >>>  mcast_snooping_port_lookup(struct ovs_list *list, void *port);
> >>>  static struct mcast_mrouter_bundle *
> >>> @@ -489,6 +491,28 @@ mcast_snooping_add_report(struct mcast_snooping *ms,
> >>>  return count;
> >>>  }
> >>>  
> >>> +static bool
> >>> +mcast_snooping_should_learn_mld_group(struct in6_addr *addr)
> >>> +{
> >>> +    /* we should learn multicast group from the following IPv6 multicast
> >>
> >> Is this supposed to be "should *not* learn multicast group"?
> > 
> > yes, sorry :)
> 
> Hi, Lorenzo,

Hi Dumitru,

> 
> Even in this form, I'm not sure this approach is correct.  I thought
> some more about it and (even if these are reserved groups) an MLD
> snooping switch should probably learn them and only flood packets to
> hosts that registered.
> 
> Moreover, with your patch, if we don't learn the groups we should at
> least make sure all traffic towards the group IPs is flooded and we're
> not actually doing that.
> 
> But this brings me to the original OVN problem.  I think the real issue
> is that in the bug report the user is expecting the logical router port
> to reply with "router advertisement" for a "router solicitation".

correct

> 
> The router solicitation packet is sent to ff02::2 but the OVN logical
> router port never sends an MLD report to join that group.  That means
> that the OVN logical switch with MLD snoop enabled doesn't learn that it
> should forward traffic with dest IP ff02::2 towards the router port too.
> 
> I think the best way to address this for OVN is to just skip generating
> logical flows in OVN for groups that correspond to IPv6 reserved
> multicast addresses.  We currently only skip ff02::1 [0].
> 
> We probably need to change that to include
> all-routers/all-site-router/solicited-node too.
> 
> What do you think?

sure, I am fine with this approach. I will work on a v2.

Regards,
Lorenzo

> 
> Regards,
> Dumitru
> 
> [0] https://github.com/ovn-org/ovn/blob/main/northd/northd.c#L9022
> 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 1/2] ofproto-ipfix: use per-domain template timeouts

[0-day Robot]

Bleep bloop.  Greetings Adrián Moreno, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Comment with 'xxx' marker
#271 FILE: ofproto/ofproto-dpif-ipfix.c:2925:
/* XXX: Make frequency of the (Options) Template and Exporter Process

Lines checked: 379, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 1/2] ofproto-ipfix: use per-domain template timeouts

[Adrián Moreno]

From: Adrian Moreno 

IPFIX templates have to be sent for each Observation Domain ID.
Currently, a timer is kept at each dpif_ipfix_exporter to send them.
This works fine for per-bridge sampling where there is only one
Observation Domain ID per exporter. However, this is does not work for
per-flow sampling where more than one Observation Domain IDs can be
specified by the controller. In this case, ovs-vswitchd will only send
template information for one (arbitrary) DomainID.

Fix per-flow sampling by using an hmap to keep a timer for each
Observation Domain ID.

Signed-off-by: Adrian Moreno 

---
- v2:
  - Fixed a potential race condition in unit test.
- v1:
  - Added unit test.
  - Drop domain_id from "struct dpif_ipfix_domain" since the hashmap
  already uses it as index.
  - Added OVS_REQUES(mutex) to dpif_ipfix_exporter_find_domain.
---
 ofproto/ofproto-dpif-ipfix.c | 127 ---
 tests/system-traffic.at  |  52 ++
 2 files changed, 156 insertions(+), 23 deletions(-)

diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 742eed399..fa71fda0f 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -124,11 +124,18 @@ struct dpif_ipfix_port {
 uint32_t ifindex;
 };
 
+struct dpif_ipfix_domain {
+struct hmap_node hmap_node; /* In struct dpif_ipfix_exporter's domains. */
+time_t last_template_set_time;
+};
+
 struct dpif_ipfix_exporter {
 uint32_t exporter_id; /* Exporting Process identifier */
 struct collectors *collectors;
 uint32_t seq_number;
-time_t last_template_set_time;
+struct hmap domains; /* Contains struct dpif_ipfix_domain indexed by
+observation domain id. */
+time_t last_stats_sent_time;
 struct hmap cache_flow_key_map;  /* ipfix_flow_cache_entry. */
 struct ovs_list cache_flow_start_timestamp_list;  /* 
ipfix_flow_cache_entry. */
 uint32_t cache_active_timeout;  /* In seconds. */
@@ -617,6 +624,9 @@ static void get_export_time_now(uint64_t *, uint32_t *);
 
 static void dpif_ipfix_cache_expire_now(struct dpif_ipfix_exporter *, bool);
 
+static void dpif_ipfix_exporter_del_domain(struct dpif_ipfix_exporter * ,
+   struct dpif_ipfix_domain * );
+
 static bool
 ofproto_ipfix_bridge_exporter_options_equal(
 const struct ofproto_ipfix_bridge_exporter_options *a,
@@ -697,13 +707,14 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter 
*exporter)
 exporter->exporter_id = ++exporter_total_count;
 exporter->collectors = NULL;
 exporter->seq_number = 1;
-exporter->last_template_set_time = 0;
+exporter->last_stats_sent_time = 0;
 hmap_init(>cache_flow_key_map);
 ovs_list_init(>cache_flow_start_timestamp_list);
 exporter->cache_active_timeout = 0;
 exporter->cache_max_flows = 0;
 exporter->virtual_obs_id = NULL;
 exporter->virtual_obs_len = 0;
+hmap_init(>domains);
 
 memset(>ipfix_global_stats, 0,
sizeof(struct dpif_ipfix_global_stats));
@@ -711,6 +722,7 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter 
*exporter)
 
 static void
 dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter *exporter)
+OVS_REQUIRES(mutex)
 {
 /* Flush the cache with flow end reason "forced end." */
 dpif_ipfix_cache_expire_now(exporter, true);
@@ -719,22 +731,29 @@ dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter 
*exporter)
 exporter->exporter_id = 0;
 exporter->collectors = NULL;
 exporter->seq_number = 1;
-exporter->last_template_set_time = 0;
+exporter->last_stats_sent_time = 0;
 exporter->cache_active_timeout = 0;
 exporter->cache_max_flows = 0;
 free(exporter->virtual_obs_id);
 exporter->virtual_obs_id = NULL;
 exporter->virtual_obs_len = 0;
 
+struct dpif_ipfix_domain *dom;
+HMAP_FOR_EACH_SAFE (dom, hmap_node, >domains) {
+dpif_ipfix_exporter_del_domain(exporter, dom);
+}
+
 memset(>ipfix_global_stats, 0,
sizeof(struct dpif_ipfix_global_stats));
 }
 
 static void
 dpif_ipfix_exporter_destroy(struct dpif_ipfix_exporter *exporter)
+OVS_REQUIRES(mutex)
 {
 dpif_ipfix_exporter_clear(exporter);
 hmap_destroy(>cache_flow_key_map);
+hmap_destroy(>domains);
 }
 
 static bool
@@ -742,7 +761,7 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter 
*exporter,
 const struct sset *targets,
 const uint32_t cache_active_timeout,
 const uint32_t cache_max_flows,
-const char *virtual_obs_id)
+const char *virtual_obs_id) OVS_REQUIRES(mutex)
 {
 size_t virtual_obs_len;
 collectors_destroy(exporter->collectors);
@@ -769,6 +788,37 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter 
*exporter,
 return true;
 }
 
+static struct dpif_ipfix_domain *

[ovs-dev] [PATCH v2 2/2] ipfix: make template and stats interval configurable

[Adrián Moreno]

From: Adrian Moreno 

Add options to the IPFIX table configure the interval to send statistics
and template information.

Signed-off-by: Adrian Moreno 

---
- v2:
  - Fixed a potential race condition in unit test.
- v1:
  - Added unit test.
---
 NEWS |  2 ++
 ofproto/ofproto-dpif-ipfix.c | 38 
 ofproto/ofproto.h|  9 +++
 tests/system-traffic.at  | 49 
 vswitchd/bridge.c| 17 +
 vswitchd/vswitch.ovsschema   | 12 -
 vswitchd/vswitch.xml | 20 +++
 7 files changed, 136 insertions(+), 11 deletions(-)

diff --git a/NEWS b/NEWS
index 4f9291bf1..6f0f9e3a0 100644
--- a/NEWS
+++ b/NEWS
@@ -33,6 +33,8 @@ Post-v3.0.0
  * Add new experimental PMD load based sleeping feature. PMD threads can
request to sleep up to a user configured 'pmd-maxsleep' value under
low load conditions.
+   - IPFIX template and statistics intervals can be configured through two new
+   options in the IPFIX table: 'template_interval' and 'stats_interval'.
 
 
 v3.0.0 - 15 Aug 2022
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index fa71fda0f..fdb48b5fd 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -140,6 +140,8 @@ struct dpif_ipfix_exporter {
 struct ovs_list cache_flow_start_timestamp_list;  /* 
ipfix_flow_cache_entry. */
 uint32_t cache_active_timeout;  /* In seconds. */
 uint32_t cache_max_flows;
+uint32_t stats_interval;
+uint32_t template_interval;
 char *virtual_obs_id;
 uint8_t virtual_obs_len;
 
@@ -174,11 +176,6 @@ struct dpif_ipfix {
 
 #define IPFIX_VERSION 0x000a
 
-/* When using UDP, IPFIX Template Records must be re-sent regularly.
- * The standard default interval is 10 minutes (600 seconds).
- * Cf. IETF RFC 5101 Section 10.3.6. */
-#define IPFIX_TEMPLATE_INTERVAL 600
-
 /* Cf. IETF RFC 5101 Section 3.1. */
 OVS_PACKED(
 struct ipfix_header {
@@ -637,6 +634,8 @@ ofproto_ipfix_bridge_exporter_options_equal(
 && a->sampling_rate == b->sampling_rate
 && a->cache_active_timeout == b->cache_active_timeout
 && a->cache_max_flows == b->cache_max_flows
+&& a->stats_interval == b->stats_interval
+&& a->template_interval == b->template_interval
 && a->enable_tunnel_sampling == b->enable_tunnel_sampling
 && a->enable_input_sampling == b->enable_input_sampling
 && a->enable_output_sampling == b->enable_output_sampling
@@ -674,6 +673,8 @@ ofproto_ipfix_flow_exporter_options_equal(
 return (a->collector_set_id == b->collector_set_id
 && a->cache_active_timeout == b->cache_active_timeout
 && a->cache_max_flows == b->cache_max_flows
+&& a->stats_interval == b->stats_interval
+&& a->template_interval == b->template_interval
 && a->enable_tunnel_sampling == b->enable_tunnel_sampling
 && sset_equals(>targets, >targets)
 && nullable_string_is_equal(a->virtual_obs_id, b->virtual_obs_id));
@@ -712,6 +713,9 @@ dpif_ipfix_exporter_init(struct dpif_ipfix_exporter 
*exporter)
 ovs_list_init(>cache_flow_start_timestamp_list);
 exporter->cache_active_timeout = 0;
 exporter->cache_max_flows = 0;
+exporter->stats_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->template_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->last_stats_sent_time = 0;
 exporter->virtual_obs_id = NULL;
 exporter->virtual_obs_len = 0;
 hmap_init(>domains);
@@ -734,6 +738,9 @@ dpif_ipfix_exporter_clear(struct dpif_ipfix_exporter 
*exporter)
 exporter->last_stats_sent_time = 0;
 exporter->cache_active_timeout = 0;
 exporter->cache_max_flows = 0;
+exporter->stats_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->template_interval = OFPROTO_IPFIX_DEFAULT_TEMPLATE_INTERVAL;
+exporter->last_stats_sent_time = 0;
 free(exporter->virtual_obs_id);
 exporter->virtual_obs_id = NULL;
 exporter->virtual_obs_len = 0;
@@ -761,6 +768,8 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter 
*exporter,
 const struct sset *targets,
 const uint32_t cache_active_timeout,
 const uint32_t cache_max_flows,
+const uint32_t stats_interval,
+const uint32_t template_interval,
 const char *virtual_obs_id) OVS_REQUIRES(mutex)
 {
 size_t virtual_obs_len;
@@ -775,6 +784,8 @@ dpif_ipfix_exporter_set_options(struct dpif_ipfix_exporter 
*exporter,
 }
 exporter->cache_active_timeout = cache_active_timeout;
 exporter->cache_max_flows = cache_max_flows;
+exporter->stats_interval = stats_interval;
+exporter->template_interval = template_interval;

Re: [ovs-dev] [RFC] mcast-snopping: do not learn from well defined multicast IPv6 groups

[Dumitru Ceara]

On 1/23/23 16:36, Lorenzo Bianconi wrote:
>> On Mon, 2023-01-23 at 10:01 +0100, Lorenzo Bianconi wrote:
>>> Avoid learning Link-Local reserved multicast addresses if advertised in
>>> a MLD reports since this interferes with Slaac IPv6 address resolution
>>> implemented in OVN.
>>>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2154930
>>> Tested-by: Eduardo Olivares 
>>> Signed-off-by: Lorenzo Bianconi 
>>> ---
>>>  lib/mcast-snooping.c | 69 
>>>  lib/packets.c    | 11 +++
>>>  lib/packets.h    | 13 +
>>>  3 files changed, 74 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/lib/mcast-snooping.c b/lib/mcast-snooping.c
>>> index 029ca2855..a9d2e7900 100644
>>> --- a/lib/mcast-snooping.c
>>> +++ b/lib/mcast-snooping.c
>>> @@ -38,6 +38,8 @@
>>>  COVERAGE_DEFINE(mcast_snooping_learned);
>>>  COVERAGE_DEFINE(mcast_snooping_expired);
>>>  
>>> +VLOG_DEFINE_THIS_MODULE(mcast_snooping);
>>> +
>>>  static struct mcast_port_bundle *
>>>  mcast_snooping_port_lookup(struct ovs_list *list, void *port);
>>>  static struct mcast_mrouter_bundle *
>>> @@ -489,6 +491,28 @@ mcast_snooping_add_report(struct mcast_snooping *ms,
>>>  return count;
>>>  }
>>>  
>>> +static bool
>>> +mcast_snooping_should_learn_mld_group(struct in6_addr *addr)
>>> +{
>>> +    /* we should learn multicast group from the following IPv6 multicast
>>
>> Is this supposed to be "should *not* learn multicast group"?
> 
> yes, sorry :)

Hi, Lorenzo,

Even in this form, I'm not sure this approach is correct.  I thought
some more about it and (even if these are reserved groups) an MLD
snooping switch should probably learn them and only flood packets to
hosts that registered.

Moreover, with your patch, if we don't learn the groups we should at
least make sure all traffic towards the group IPs is flooded and we're
not actually doing that.

But this brings me to the original OVN problem.  I think the real issue
is that in the bug report the user is expecting the logical router port
to reply with "router advertisement" for a "router solicitation".

The router solicitation packet is sent to ff02::2 but the OVN logical
router port never sends an MLD report to join that group.  That means
that the OVN logical switch with MLD snoop enabled doesn't learn that it
should forward traffic with dest IP ff02::2 towards the router port too.

I think the best way to address this for OVN is to just skip generating
logical flows in OVN for groups that correspond to IPv6 reserved
multicast addresses.  We currently only skip ff02::1 [0].

We probably need to change that to include
all-routers/all-site-router/solicited-node too.

What do you think?

Regards,
Dumitru

[0] https://github.com/ovn-org/ovn/blob/main/northd/northd.c#L9022


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

Re: [ovs-dev] [PATCH] system-traffic.at: Skip the 'ICMP6 Related' test if nc is missing.

[David Marchand]

On Mon, Jan 23, 2023 at 3:50 PM Simon Horman  wrote:
>
> On Mon, Jan 23, 2023 at 03:29:50PM +0100, David Marchand wrote:
> > On Mon, Jan 23, 2023 at 3:05 PM Ilya Maximets  wrote:
> > >
> > > Test fails is 'nc' is not available, it should be skipped instead.
> > >
> >
> > Probably not important, but:
> > Fixes: b020a416e24c ("System Tests: Enhance NAT tests.")
> > > Signed-off-by: Ilya Maximets 
> >
> > Reviewed-by: David Marchand 
>
> FWIIW,
>
> Reviewed-by: Simon Horman 
>
> > Some notes:
> > - in system-offloads-traffic.at, there is a similar issue,
> > 5660b89a309d ("dpif-netlink: Offloading meter to tc police action")
> > added calls to nc without checking nc availability,
> > - in system-traffic.at, for "conntrack - ICMP related to original
> > direction", there is no dependency to nc, so it is wrongly skipped if
> > nc is missing,
>
> Agreed on both counts.
> Would you like to post fixes?
> Else I'm happy to do so.

Sorry, I only noticed your reply now.
Please send the fixes, I'll review them.


-- 
David Marchand

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

Re: [ovs-dev] [PATCH] docs: Add HyperThreading notes for auto-lb usage.

[Kevin Traynor]

On 08/01/2023 03:55, Cheng Li wrote:

In my test, if one logical core is pinned to PMD thread while the
other logical(of the same physical core) is not. The PMD
performance is affected the by the not-pinned logical core load.
This maks it difficult to estimate the loads during a dry-run.

Signed-off-by: Cheng Li 
---
  Documentation/topics/dpdk/pmd.rst | 4 
  1 file changed, 4 insertions(+)

diff --git a/Documentation/topics/dpdk/pmd.rst 
b/Documentation/topics/dpdk/pmd.rst
index 9006fd4..b220199 100644
--- a/Documentation/topics/dpdk/pmd.rst
+++ b/Documentation/topics/dpdk/pmd.rst
@@ -312,6 +312,10 @@ If not set, the default variance improvement threshold is 
25%.
  when all PMD threads are running on cores from a single NUMA node. In this
  case cross-NUMA datapaths will not change after reassignment.
  
+For the same reason, please ensure that the pmd threads are pinned to SMT

+siblings if HyperThreading is enabled. Otherwise, PMDs within a NUMA may
+not have the same performance.
+
  The minimum time between 2 consecutive PMD auto load balancing iterations can
  also be configured by::
  


I don't think it's a hard requirement as siblings should not impact as 
much as cross-numa might but it's probably good advice in general.


Acked-by: Kevin Traynor 

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

[ovs-dev] [PATCH ovn v1 2/6] northd: fix unsampled drops and unit test

[Adrián Moreno]

From: Adrian Moreno 

Some drops were left unsampled and a change in IFS made the test fail to
detect them. This patch fixes it.

Fixes: a42c808f30b4 ("northd: add drop sampling")
Acked-by: Mark Michelson 
Signed-off-by: Adrian Moreno 
---
 northd/northd.c | 17 ++---
 tests/ovn.at|  7 +--
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 0944a7b56..820889141 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -5642,7 +5642,7 @@ build_lswitch_port_sec_op(struct ovn_port *op, struct 
hmap *lflows,
 ds_put_format(match, "outport == %s", op->json_key);
 ovn_lflow_add_with_lport_and_hint(
 lflows, op->od, S_SWITCH_IN_L2_UNKNOWN, 50, ds_cstr(match),
-"drop;", op->key, >nbsp->header_);
+debug_drop_action(), op->key, >nbsp->header_);
 return;
 }
 
@@ -5736,7 +5736,7 @@ build_lswitch_output_port_sec_od(struct ovn_datapath *od,
   REGBIT_PORT_SEC_DROP" = check_out_port_sec(); next;");
 
 ovn_lflow_add(lflows, od, S_SWITCH_OUT_APPLY_PORT_SEC, 50,
-  REGBIT_PORT_SEC_DROP" == 1", "drop;");
+  REGBIT_PORT_SEC_DROP" == 1", debug_drop_action());
 ovn_lflow_add(lflows, od, S_SWITCH_OUT_APPLY_PORT_SEC, 0,
   "1", "output;");
 
@@ -6683,7 +6683,8 @@ build_acls(struct ovn_datapath *od, const struct 
chassis_features *features,
struct hmap *lflows, const struct hmap *port_groups,
const struct shash *meter_groups)
 {
-const char *default_acl_action = default_acl_drop ? "drop;" : "next;";
+const char *default_acl_action = default_acl_drop ? debug_drop_action() :
+"next;";
 bool has_stateful = od->has_stateful_acl || od->has_lb_vip;
 const char *ct_blocked_match = features->ct_no_masked_label
? "ct_mark.blocked"
@@ -6752,7 +6753,7 @@ build_acls(struct ovn_datapath *od, const struct 
chassis_features *features,
   REGBIT_CONNTRACK_COMMIT" = 1; next;");
 
 default_acl_action = default_acl_drop
- ? "drop;"
+ ? debug_drop_action()
  : REGBIT_CONNTRACK_COMMIT" = 1; next;";
 ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 1, "ip && !ct.est",
   default_acl_action);
@@ -9085,7 +9086,8 @@ build_lswitch_ip_unicast_lookup(struct ovn_port *op,
  * or IPv6 addresses (or both). */
 struct eth_addr mac;
 bool lsp_enabled = lsp_is_enabled(op->nbsp);
-char *action = lsp_enabled ? "outport = %s; output;" : "drop;";
+const char *action = lsp_enabled ? "outport = %s; output;" :
+   debug_drop_action();
 if (ovs_scan(op->nbsp->addresses[i],
 ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(mac))) {
 ds_clear(match);
@@ -12912,12 +12914,13 @@ build_gateway_redirect_flows_for_lrouter(
   nat_entry_is_v6(nat) ? "6" : "4",
   nat->nb->external_ip);
 ovn_lflow_add(lflows, od, S_ROUTER_IN_GW_REDIRECT, 70,
-  ds_cstr(_ext), "drop;");
+  ds_cstr(_ext), debug_drop_action());
 add_def_flow = false;
 }
 } else if (nat->nb->exempted_ext_ips) {
 ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_GW_REDIRECT,
-75, ds_cstr(_ext), "drop;",
+75, ds_cstr(_ext),
+debug_drop_action(),
 stage_hint);
 }
 ds_destroy(_ext);
diff --git a/tests/ovn.at b/tests/ovn.at
index e9b8bc677..6bd1efb38 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -33910,9 +33910,12 @@ check_sample_drops() {
 AT_CAPTURE_FILE([oflows_sample])
 
 # Check that every drop has now contains a "sample" action.
-for flow in "$drop_matches"; do
-AT_CHECK([grep -q "$flow actions=.*sample.*" oflows_sample], [0], 
[ignore], [ignore], [echo "Flow $flow has a drop and did not get sampled"])
+save_IFS=$IFS
+IFS=$'\n'
+for flow in $drop_matches; do
+AT_CHECK([grep "${flow}actions=.*sample.*" oflows_sample], [0], 
[ignore], [ignore], [echo "Flow $flow has a drop and did not get sampled"])
 done
+IFS=$save_IFS
 }
 
 check_drops() {
-- 
2.39.1

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

[ovs-dev] [PATCH ovn v1 1/6] controller: fix recompute pflows if sampling changes

[Adrián Moreno]

From: Adrian Moreno 

The engine handler needs to return "false" in order to trigger a full
recompute of the physical flows.

Fixes: a42c808f30b4 ("northd: add drop sampling")
Signed-off-by: Adrian Moreno 
---
 controller/ovn-controller.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 265740cab..f20972c76 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -3839,10 +3839,11 @@ pflow_output_sb_sb_global_handler(struct engine_node 
*node, void *data)
 
 if (pfo->debug.collector_set_id != collector_set_id ||
 pfo->debug.obs_domain_id != obs_domain_id) {
-engine_set_node_state(node, EN_UPDATED);
 pfo->debug.collector_set_id = collector_set_id;
 pfo->debug.obs_domain_id = obs_domain_id;
+return false;
 }
+engine_set_node_state(node, EN_UPDATED);
 return true;
 }
 
-- 
2.39.1

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

[ovs-dev] [PATCH ovn v1 5/6] controller: only sample flow if Collector Set exists

[Adrián Moreno]

From: Adrian Moreno 

Adding a OFPACT_SAMPLE action to a flow is useless (and even detrimental
in terms of performance) if a Flow_Sample_Collector_Set row does not
exist with the correspondent id. The sample (i.e: upcall) would take
place but ovs-vswitchd would not have a target IPFIX collector to send
the sample to.

In order to be able to control what Chassis perform sampling without
incurring in any performance cost if not needed, this patch makes the
controller only encode the OFPACT_SAMPLE if there's a valid
Flow_Sample_Collector_Set configured on that chassis.

In order to do that, a new input is added to the en_lflow_output engine
node that is associated with the OVSDB table. But since the information
has to be available in the lib/actions layer, an intermediate helper
struct is added (in lib/ovn-util.{h,c}) to help store and query the
configured collector set ids instead of using the idl directly.

Signed-off-by: Adrian Moreno 
---
 controller/lflow.c  |  1 +
 controller/lflow.h  |  8 --
 controller/ovn-controller.c | 56 -
 include/ovn/actions.h   |  4 +++
 lib/actions.c   |  9 +-
 lib/ovn-util.c  | 51 +
 lib/ovn-util.h  | 26 ++---
 tests/ovn-performance.at| 24 
 tests/ovn.at|  8 ++
 tests/test-ovn.c|  9 ++
 10 files changed, 187 insertions(+), 9 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index 4b1cfe318..08ce0386f 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -911,6 +911,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow 
*lflow,
 .is_switch = ldp->is_switch,
 .group_table = l_ctx_out->group_table,
 .meter_table = l_ctx_out->meter_table,
+.collector_ids = l_ctx_in->collector_ids,
 .lflow_uuid = lflow->header_.uuid,
 .dp_key = ldp->datapath->tunnel_key,
 
diff --git a/controller/lflow.h b/controller/lflow.h
index 9e8f9afd3..9bb61c039 100644
--- a/controller/lflow.h
+++ b/controller/lflow.h
@@ -43,11 +43,12 @@
 #include "openvswitch/uuid.h"
 #include "openvswitch/list.h"
 
-struct ovn_extend_table;
-struct ovsdb_idl_index;
-struct ovn_desired_flow_table;
 struct hmap;
 struct hmap_node;
+struct ovn_desired_flow_table;
+struct ovn_extend_table;
+struct ovsdb_idl_index;
+struct ovsrec_flow_sample_collector_set_table;
 struct sbrec_chassis;
 struct sbrec_load_balancer;
 struct sbrec_logical_flow_table;
@@ -114,6 +115,7 @@ struct lflow_ctx_in {
 const struct hmap *dhcpv6_opts;
 const struct controller_event_options *controller_event_opts;
 const struct smap *template_vars;
+const struct flow_collector_ids *collector_ids;
 bool lb_hairpin_use_ct_mark;
 };
 
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index f20972c76..088389a54 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1047,6 +1047,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
 ovsdb_idl_add_column(ovs_idl, _ssl_col_private_key);
 ovsdb_idl_add_table(ovs_idl, _table_datapath);
 ovsdb_idl_add_column(ovs_idl, _datapath_col_capabilities);
+ovsdb_idl_add_table(ovs_idl, _table_flow_sample_collector_set);
+
 chassis_register_ovs_idl(ovs_idl);
 encaps_register_ovs_idl(ovs_idl);
 binding_register_ovs_idl(ovs_idl);
@@ -1063,6 +1065,10 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
 ovsdb_idl_track_add_column(ovs_idl, _port_col_name);
 ovsdb_idl_track_add_column(ovs_idl, _port_col_interfaces);
 ovsdb_idl_track_add_column(ovs_idl, _port_col_external_ids);
+ovsdb_idl_track_add_column(ovs_idl,
+   _flow_sample_collector_set_col_bridge);
+ovsdb_idl_track_add_column(ovs_idl,
+   _flow_sample_collector_set_col_id);
 mirror_register_ovs_idl(ovs_idl);
 }
 
@@ -1102,7 +1108,8 @@ enum sb_engine_node {
 OVS_NODE(bridge, "bridge") \
 OVS_NODE(port, "port") \
 OVS_NODE(interface, "interface") \
-OVS_NODE(qos, "qos")
+OVS_NODE(qos, "qos") \
+OVS_NODE(flow_sample_collector_set, "flow_sample_collector_set")
 
 enum ovs_engine_node {
 #define OVS_NODE(NAME, NAME_STR) OVS_##NAME,
@@ -2873,6 +2880,9 @@ struct ed_type_lflow_output {
 
 /* Fixed controller_event supported options. */
 struct controller_event_options controller_event_opts;
+
+/* Configured Flow Sample Collector Sets. */
+struct flow_collector_ids collector_ids;
 };
 
 static void
@@ -3011,6 +3021,7 @@ init_lflow_ctx(struct engine_node *node,
 l_ctx_in->dhcpv6_opts = _opts->v6_opts;
 l_ctx_in->controller_event_opts = >controller_event_opts;
 l_ctx_in->template_vars = _vars->local_templates;
+l_ctx_in->collector_ids = >collector_ids;
 
 l_ctx_out->flow_table = >flow_table;
 l_ctx_out->group_table = >group_table;
@@ -3040,6 +3051,7 @@ en_lflow_output_init(struct 

[ovs-dev] [PATCH ovn v1 3/6] controller: add missing drop to loopback check table

[Adrián Moreno]

From: Adrian Moreno 

The drop was implicit (using empty actions). Make it explicit and
sampled.

Fixes: a42c808f30b4 ("northd: add drop sampling")
Acked-by: Mark Michelson 
Signed-off-by: Adrian Moreno 
---
 controller/physical.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/controller/physical.c b/controller/physical.c
index 4dcf44e01..cb2cddb9f 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -1498,6 +1498,7 @@ consider_port_binding(struct ovsdb_idl_index 
*sbrec_port_binding_by_name,
 if (!strcmp(binding->type, "localnet")) {
 /* do not forward traffic from localport to localnet port */
 ofpbuf_clear(ofpacts_p);
+put_drop(debug, OFTABLE_CHECK_LOOPBACK, ofpacts_p);
 match_outport_dp_and_port_keys(, dp_key, port_key);
 match_set_reg_masked(, MFF_LOG_FLAGS - MFF_REG0,
  MLF_LOCALPORT, MLF_LOCALPORT);
-- 
2.39.1

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

[ovs-dev] [PATCH ovn v1 0/6] drop sampling: Fixes and optimizations

[Adrián Moreno]

While testing, I discovered some problems with drop sampling (first 4
patches).

Also, this series introduces an optimization. In order to avoid adding
sample actions on Chassis that do not have a Flow_Sample_Collector_Set
configured (which would generate a useless upcall), make the controller
monitor this table in OVS and recompute flows when it's changed.

The engine logic is pretty simple since this table is assumed to change
very rarely.

--
v1:
- Fixed commit message in patch 4.

Adrian Moreno (6):
  controller: fix recompute pflows if sampling changes
  northd: fix unsampled drops and unit test
  controller: add missing drop to loopback check table
  controller: set sampling port to OFP_NONE for drops
  controller: only sample flow if Collector Set exists
  controller: only sample pflow if Collector Set exists

 controller/lflow.c  |   1 +
 controller/lflow.h  |   8 +-
 controller/ovn-controller.c | 161 +---
 controller/physical.c   |   2 +
 include/ovn/actions.h   |   4 +
 lib/actions.c   |   9 +-
 lib/ovn-util.c  |  51 
 lib/ovn-util.h  |  26 +-
 northd/northd.c |  17 ++--
 tests/ovn-performance.at|  24 ++
 tests/ovn.at|  15 +++-
 tests/test-ovn.c|   9 ++
 12 files changed, 280 insertions(+), 47 deletions(-)

-- 
2.39.1

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

[ovs-dev] [PATCH ovn v1 6/6] controller: only sample pflow if Collector Set exists

[Adrián Moreno]

From: Adrian Moreno 

Make physical (pflow) engine node also depend on
Flow_Sample_Collector_Set table and only enable flow sampling if the
right collector set exists.

Signed-off-by: Adrian Moreno 
---
 controller/ovn-controller.c | 104 ++--
 1 file changed, 75 insertions(+), 29 deletions(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 088389a54..7c640a9d7 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -3187,6 +3187,64 @@ lflow_output_flow_sample_collector_set_handler(struct 
engine_node *node,
 return true;
 }
 
+static void
+pflow_output_get_debug(struct engine_node *node, struct physical_debug *debug)
+{
+const struct ovsrec_open_vswitch_table *ovs_table =
+EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node));
+const struct ovsrec_bridge_table *bridge_table =
+EN_OVSDB_GET(engine_get_input("OVS_bridge", node));
+const struct sbrec_sb_global_table *sb_global_table =
+EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
+const struct sbrec_sb_global *sb_global =
+sbrec_sb_global_table_first(sb_global_table);
+
+if (!debug) {
+return;
+}
+debug->collector_set_id = 0;
+debug->obs_domain_id = 0;
+
+const struct ovsrec_open_vswitch *ovs_cfg =
+ovsrec_open_vswitch_table_first(ovs_table);
+if (!ovs_cfg) {
+return;
+}
+
+const struct ovsrec_bridge *br_int;
+br_int = get_bridge(bridge_table, br_int_name(ovs_cfg));
+if (!br_int) {
+return;
+}
+
+const uint32_t debug_collector_set =
+smap_get_uint(_global->options, "debug_drop_collector_set", 0);
+if (!debug_collector_set) {
+return;
+}
+
+struct ovsdb_idl_index *ovsrec_flow_sample_collector_set_by_id =
+engine_ovsdb_node_get_index(
+engine_get_input("OVS_flow_sample_collector_set", node), "id");
+
+struct ovsrec_flow_sample_collector_set *s =
+ovsrec_flow_sample_collector_set_index_init_row(
+ovsrec_flow_sample_collector_set_by_id);
+
+ovsrec_flow_sample_collector_set_index_set_id(s, debug_collector_set);
+ovsrec_flow_sample_collector_set_index_set_bridge(s, br_int);
+if (!ovsrec_flow_sample_collector_set_index_find(
+ovsrec_flow_sample_collector_set_by_id, s)) {
+ovsrec_flow_sample_collector_set_index_destroy_row(s);
+return;
+}
+ovsrec_flow_sample_collector_set_index_destroy_row(s);
+
+debug->collector_set_id = debug_collector_set;
+debug->obs_domain_id = smap_get_uint(_global->options,
+ "debug_drop_domain_id", 0);
+}
+
 static bool
 lflow_output_sb_mac_binding_handler(struct engine_node *node, void *data)
 {
@@ -3637,11 +3695,6 @@ static void init_physical_ctx(struct engine_node *node,
 chassis = chassis_lookup_by_name(sbrec_chassis_by_name, chassis_id);
 }
 
-const struct sbrec_sb_global_table *sb_global_table =
-EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
-const struct sbrec_sb_global *sb_global =
-sbrec_sb_global_table_first(sb_global_table);
-
 ovs_assert(br_int && chassis);
 
 struct ed_type_ct_zones *ct_zones_data =
@@ -3663,13 +3716,8 @@ static void init_physical_ctx(struct engine_node *node,
 p_ctx->local_bindings = _data->lbinding_data.bindings;
 p_ctx->patch_ofports = _vif_data->patch_ofports;
 p_ctx->chassis_tunnels = _vif_data->chassis_tunnels;
-p_ctx->debug.collector_set_id = smap_get_uint(_global->options,
-  "debug_drop_collector_set",
-  0);
 
-p_ctx->debug.obs_domain_id = smap_get_uint(_global->options,
-   "debug_drop_domain_id",
-   0);
+pflow_output_get_debug(node, _ctx->debug);
 }
 
 static void *
@@ -3873,26 +3921,16 @@ pflow_output_activated_ports_handler(struct engine_node 
*node, void *data)
 }
 
 static bool
-pflow_output_sb_sb_global_handler(struct engine_node *node, void *data)
+pflow_output_debug_handler(struct engine_node *node, void *data)
 {
-const struct sbrec_sb_global_table *sb_global_table =
-EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
-const struct sbrec_sb_global *sb_global =
-sbrec_sb_global_table_first(sb_global_table);
-
 struct ed_type_pflow_output *pfo = data;
+struct physical_debug debug;
+
+pflow_output_get_debug(node, );
 
-uint32_t collector_set_id = smap_get_uint(_global->options,
-  "debug_drop_collector_set",
-  0);
-uint32_t obs_domain_id = smap_get_uint(_global->options,
-   "debug_drop_domain_id",
-   0);
-
-if 

[ovs-dev] [PATCH ovn v1 4/6] controller: set sampling port to OFP_NONE for drops

[Adrián Moreno]

From: Adrian Moreno 

The default zero value would likely match an existing openflow port and
end up generating a sample with wrong output interface information.
Since in this case we're sampling in the middle of the pipeline, the
correct value for sampling port is OFP_NONE.

Fixes: a42c808f30b4 ("northd: add drop sampling")
Signed-off-by: Adrian Moreno 
---
 controller/physical.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/controller/physical.c b/controller/physical.c
index cb2cddb9f..e718268ea 100644
--- a/controller/physical.c
+++ b/controller/physical.c
@@ -843,6 +843,7 @@ put_drop(const struct physical_debug *debug, uint8_t 
table_id,
 os->collector_set_id = debug->collector_set_id;
 os->obs_domain_id = (debug->obs_domain_id << 24);
 os->obs_point_id = table_id;
+os->sampling_port = OFPP_NONE;
 }
 }
 
-- 
2.39.1

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

Re: [ovs-dev] [OVN RFC 0/7] OVN IC bugfixes & proposals/questions

[Vladislav Odintsov]

Thanks Ilya for the quick and useful response!
We’ll dig into monitor/db_change_aware logic.

Regards,
Vladislav Odintsov

> On 24 Jan 2023, at 17:00, Ilya Maximets  wrote:
> 
> On 1/24/23 14:12, Vladislav Odintsov wrote:
>> Hi Ilya,
>> 
>> could you please take a look on this?
>> Maybe you can advice any direction how to investigate this issue?
>> 
>> Thanks in advance.
>> 
>> Regards,
>> Vladislav Odintsov
>> 
>>> On 24 Nov 2022, at 21:10, Anton Vazhnetsov  wrote:
>>> 
>>> Hi, Terry!
>>> 
>>> In continuation to our yesterday’s conversation [0], we were able to 
>>> reproduce the issue with KeyError. We found that the problem is not 
>>> connected with ovsdb-server load but it appears when the ovsdb-server 
>>> schema is converted online (it even doesn’t matter whether the real ovs 
>>> schema is changed) while the active connection persists.
>>> Please use next commands to reproduce it:
>>> 
>>> # in terminal 1
>>> 
>>> ovsdb-tool create ./ovs.db /usr/share/ovn/ovn-nb.ovsschema
>>> ovsdb-server --remote punix://$(pwd)/ovs.sock  
>>> $(pwd)/ovs.db -vconsole:dbg
>>> 
>>> 
>>> # in terminal 2. run python shell
>>> python3
>>> # setup connection
>>> import ovsdbapp.schema.ovn_northbound.impl_idl as nb_idl
>>> from ovsdbapp.backend.ovs_idl import connection
>>> 
>>> remote = "unix:///"
>>> 
>>> def get_idl():
>>>"""Connection getter."""
>>> 
>>>idl = connection.OvsdbIdl.from_server(remote, "OVN_Northbound",
>>>  leader_only=False)
>>>return nb_idl.OvnNbApiIdlImpl(connection.Connection(idl, 100))
>>> 
>>> idl = get_idl()
>>> 
>>> 
>>> # in terminal 1
>>> ovsdb-client convert unix:$(pwd)/ovs.sock /usr/share/ovn/ovn-nb.ovsschema
>>> 
>>> # in terminal 2 python shell:
>>> idl.ls_add("test").execute()
>>> 
>>> 
>>> We get following traceback:
>>> 
>>> Traceback (most recent call last):
>>>  File 
>>> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/connection.py", 
>>> line 131, in run
>>>txn.results.put(txn.do_commit())
>>>  File 
>>> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/transaction.py", 
>>> line 143, in do_commit
>>>self.post_commit(txn)
>>>  File 
>>> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/transaction.py", 
>>> line 73, in post_commit
>>>command.post_commit(txn)
>>>  File 
>>> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/command.py", 
>>> line 94, in post_commit
>>>row = self.api.tables[self.table_name].rows[real_uuid]
>>>  File "/usr/lib64/python3.6/collections/__init__.py", line 991, in 
>>> __getitem__
>>>raise KeyError(key)
>>> KeyError: UUID('0256afa4-6dd0-4c2c-b6a2-686a360ab331') 
>>> 
>>> In ovsdb-server debug logs we see that update2 or update3 messages are not 
>>> sent from server in response to client’s transaction, just reply with 
>>> result UUID:
>>> 2022-11-24T17:42:36Z|00306|poll_loop|DBG|wakeup due to [POLLIN] on fd 18 
>>> (///root/ovsdb-problem/ovs.sock<->) at lib/stream-fd.c:157
>>> 2022-11-24T17:42:36Z|00307|jsonrpc|DBG|unix#5: received request, 
>>> method="transact", 
>>> params=["OVN_Northbound",{"uuid-name":"row03ef28d6_93f1_43bc_b07a_eae58d4bd1c5","table":"Logical_Switch","op":"insert","row":{"name”:"test"}}],
>>>  id=5
>>> 2022-11-24T17:42:36Z|00308|jsonrpc|DBG|unix#5: send reply, 
>>> result=[{"uuid":["uuid","4eb7c407-beec-46ca-b816-19f942e57721"]}], id=5
>>> 
>>> We checked same with ovn-nbctl running in daemon mode and found that the 
>>> problem is not reproduced (ovsdb-server after database conversion sends out 
>>> update3 message to ovn-nbctl daemon process in response to transaction, for 
>>> example ovs-appctl -t  run ls-add test-ls):
> 
> If the update3 is not sent, it means that the client doesn't monitor
> this table.  You need to look at monitor requests sent and replied
> to figure out the difference.
> 
> Since the database conversion is involved, server will close all
> monitors on schema update and notify all clients that are db_change_aware.
> Clients should re-send monitor requests at this point.  If clients
> are not db_change_aware, server will just disconnect them, so they
> can re-connect and see the new schema and send new monitor requests.
> 
> On a quick glance I don't see python idl handling 'monitor_canceled'
> notification.  That is most likely a root cause.  Python IDL claims
> to be db_change_aware, but it doesn't seem to be.
> 
> Best regards, Ilya Maximets.
> 
>>> 2022-11-24T17:54:51Z|00623|jsonrpc|DBG|unix#7: received request, 
>>> method="transact", 
>>> params=["OVN_Northbound",{"uuid-name":"rowcdb152ce_a9af_4761_b965_708ad300fcb7","table":"Logical_Switch","op":"insert","row":{"name":"test-ls"}},{"comment":"ovn-nbctl:
>>>  run ls-add test-ls","op":"comment"}], id=5
>>> 2022-11-24T17:54:51Z|00624|jsonrpc|DBG|unix#7: send notification, 
>>> method="update3", 
>>> 

Re: [ovs-dev] [PATCH ovn] Update release-process.rst to have the 2024 calendar.

[Mark Michelson]

Thanks for the review, Dumitru. I added my sign-off and pushed the 
change to main.


On 1/19/23 04:41, Dumitru Ceara wrote:

On 1/16/23 14:33, Mark Michelson wrote:

---
  Documentation/internals/release-process.rst | 20 ++--
  1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/internals/release-process.rst 
b/Documentation/internals/release-process.rst
index 13a22fa60..ac278bd4a 100644
--- a/Documentation/internals/release-process.rst
+++ b/Documentation/internals/release-process.rst
@@ -134,34 +134,34 @@ approximate:
  Release Calendar
  
  
-The 2022 timetable is shown below. Note that these dates are not set in stone.

+The 2023 timetable is shown below. Note that these dates are not set in stone.
  If extenuating circumstances arise, a release may be delayed from its target
  date.
  
  +-+-+-+-+

  | Release | Soft Freeze | Branch Creation | Release |
  +-+-+-+-+
-| 22.03.0 | Feb 4   | Feb 18  | Mar 4   |
+| 23.03.0 | Feb 3   | Feb 17  | Mar 3   |
  +-+-+-+-+
-| 22.06.0 | May 6   | May 20  | Jun 3   |
+| 23.06.0 | May 5   | May 19  | Jun 2   |
  +-+-+-+-+
-| 22.09.0 | Aug 5   | Aug 19  | Sep 2   |
+| 23.09.0 | Aug 4   | Aug 18  | Sep 1   |
  +-+-+-+-+
-| 22.12.0 | Nov 4   | Nov 18  | Dec 2   |
+| 23.12.0 | Nov 3   | Nov 17  | Dec 1   |
  +-+-+-+-+
  
-Below is the 2023 timetable

+Below is the 2024 timetable
  
  +-+-+-+-+

  | Release | Soft Freeze | Branch Creation | Release |
  +-+-+-+-+
-| 23.03.0 | Feb 3   | Feb 17  | Mar 3   |
+| 24.03.0 | Feb 2   | Feb 16  | Mar 1   |
  +-+-+-+-+
-| 23.06.0 | May 5   | May 19  | Jun 2   |
+| 24.06.0 | May 10  | May 24  | Jun 7   |
  +-+-+-+-+
-| 23.09.0 | Aug 4   | Aug 18  | Sep 1   |
+| 24.09.0 | Aug 9   | Aug 23  | Sep 6   |
  +-+-+-+-+
-| 23.12.0 | Nov 3   | Nov 17  | Dec 1   |
+| 24.12.0 | Nov 8   | Nov 22  | Dec 6   |
  +-+-+-+-+
  
  Contact


Hi Mark,

Acked-by: Dumitru Ceara 

As 0-day Robot pointed out, it looks like you need to sign off as well.

Regards,
Dumitru




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

Re: [ovs-dev] [OVN RFC 0/7] OVN IC bugfixes & proposals/questions

[Ilya Maximets]

On 1/24/23 14:12, Vladislav Odintsov wrote:
> Hi Ilya,
> 
> could you please take a look on this?
> Maybe you can advice any direction how to investigate this issue?
> 
> Thanks in advance.
> 
> Regards,
> Vladislav Odintsov
> 
>> On 24 Nov 2022, at 21:10, Anton Vazhnetsov  wrote:
>>
>> Hi, Terry!
>>
>> In continuation to our yesterday’s conversation [0], we were able to 
>> reproduce the issue with KeyError. We found that the problem is not 
>> connected with ovsdb-server load but it appears when the ovsdb-server schema 
>> is converted online (it even doesn’t matter whether the real ovs schema is 
>> changed) while the active connection persists.
>> Please use next commands to reproduce it:
>>
>> # in terminal 1
>>
>> ovsdb-tool create ./ovs.db /usr/share/ovn/ovn-nb.ovsschema
>> ovsdb-server --remote punix://$(pwd)/ovs.sock  
>> $(pwd)/ovs.db -vconsole:dbg
>>
>>
>> # in terminal 2. run python shell
>> python3
>> # setup connection
>> import ovsdbapp.schema.ovn_northbound.impl_idl as nb_idl
>> from ovsdbapp.backend.ovs_idl import connection
>>
>> remote = "unix:///"
>>
>> def get_idl():
>>    """Connection getter."""
>>
>>    idl = connection.OvsdbIdl.from_server(remote, "OVN_Northbound",
>>  leader_only=False)
>>    return nb_idl.OvnNbApiIdlImpl(connection.Connection(idl, 100))
>>
>> idl = get_idl()
>>
>>
>> # in terminal 1
>> ovsdb-client convert unix:$(pwd)/ovs.sock /usr/share/ovn/ovn-nb.ovsschema
>>
>> # in terminal 2 python shell:
>> idl.ls_add("test").execute()
>>
>>
>> We get following traceback:
>>
>> Traceback (most recent call last):
>>  File 
>> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/connection.py", 
>> line 131, in run
>>    txn.results.put(txn.do_commit())
>>  File 
>> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/transaction.py", 
>> line 143, in do_commit
>>    self.post_commit(txn)
>>  File 
>> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/transaction.py", 
>> line 73, in post_commit
>>    command.post_commit(txn)
>>  File 
>> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/command.py", line 
>> 94, in post_commit
>>    row = self.api.tables[self.table_name].rows[real_uuid]
>>  File "/usr/lib64/python3.6/collections/__init__.py", line 991, in 
>> __getitem__
>>    raise KeyError(key)
>> KeyError: UUID('0256afa4-6dd0-4c2c-b6a2-686a360ab331') 
>>
>> In ovsdb-server debug logs we see that update2 or update3 messages are not 
>> sent from server in response to client’s transaction, just reply with result 
>> UUID:
>> 2022-11-24T17:42:36Z|00306|poll_loop|DBG|wakeup due to [POLLIN] on fd 18 
>> (///root/ovsdb-problem/ovs.sock<->) at lib/stream-fd.c:157
>> 2022-11-24T17:42:36Z|00307|jsonrpc|DBG|unix#5: received request, 
>> method="transact", 
>> params=["OVN_Northbound",{"uuid-name":"row03ef28d6_93f1_43bc_b07a_eae58d4bd1c5","table":"Logical_Switch","op":"insert","row":{"name”:"test"}}],
>>  id=5
>> 2022-11-24T17:42:36Z|00308|jsonrpc|DBG|unix#5: send reply, 
>> result=[{"uuid":["uuid","4eb7c407-beec-46ca-b816-19f942e57721"]}], id=5
>>
>> We checked same with ovn-nbctl running in daemon mode and found that the 
>> problem is not reproduced (ovsdb-server after database conversion sends out 
>> update3 message to ovn-nbctl daemon process in response to transaction, for 
>> example ovs-appctl -t  run ls-add test-ls):

If the update3 is not sent, it means that the client doesn't monitor
this table.  You need to look at monitor requests sent and replied
to figure out the difference.

Since the database conversion is involved, server will close all
monitors on schema update and notify all clients that are db_change_aware.
Clients should re-send monitor requests at this point.  If clients
are not db_change_aware, server will just disconnect them, so they
can re-connect and see the new schema and send new monitor requests.

On a quick glance I don't see python idl handling 'monitor_canceled'
notification.  That is most likely a root cause.  Python IDL claims
to be db_change_aware, but it doesn't seem to be.

Best regards, Ilya Maximets.

>> 2022-11-24T17:54:51Z|00623|jsonrpc|DBG|unix#7: received request, 
>> method="transact", 
>> params=["OVN_Northbound",{"uuid-name":"rowcdb152ce_a9af_4761_b965_708ad300fcb7","table":"Logical_Switch","op":"insert","row":{"name":"test-ls"}},{"comment":"ovn-nbctl:
>>  run ls-add test-ls","op":"comment"}], id=5
>> 2022-11-24T17:54:51Z|00624|jsonrpc|DBG|unix#7: send notification, 
>> method="update3", 
>> params=[["monid","OVN_Northbound"],"----",{"Logical_Switch":{"0b147f2c-248d-496a-b718-a5328d3c2995":{"insert":{"name":"test-ls"]
>> 2022-11-24T17:54:51Z|00625|jsonrpc|DBG|unix#7: send reply, 
>> result=[{"uuid":["uuid","0b147f2c-248d-496a-b718-a5328d3c2995"]},{}], id=5
>>
>> So it seems that the problem is in python-ovs, not in ovsdb-server.
>> We tested with ovsdb-server running version 2.17.3 and python-ovs 2.13.5 and 
>> also python-ovs 

Re: [ovs-dev] [OVN RFC 0/7] OVN IC bugfixes & proposals/questions

[Vladislav Odintsov]

Hi Ilya,

could you please take a look on this?
Maybe you can advice any direction how to investigate this issue?

Thanks in advance.

Regards,
Vladislav Odintsov

> On 24 Nov 2022, at 21:10, Anton Vazhnetsov  wrote:
> 
> Hi, Terry!
> 
> In continuation to our yesterday’s conversation [0], we were able to 
> reproduce the issue with KeyError. We found that the problem is not connected 
> with ovsdb-server load but it appears when the ovsdb-server schema is 
> converted online (it even doesn’t matter whether the real ovs schema is 
> changed) while the active connection persists. 
> Please use next commands to reproduce it:
> 
> # in terminal 1
> 
> ovsdb-tool create ./ovs.db /usr/share/ovn/ovn-nb.ovsschema
> ovsdb-server --remote punix://$(pwd)/ovs.sock $(pwd)/ovs.db -vconsole:dbg
> 
> 
> # in terminal 2. run python shell
> python3
> # setup connection
> import ovsdbapp.schema.ovn_northbound.impl_idl as nb_idl
> from ovsdbapp.backend.ovs_idl import connection
> 
> remote = "unix:///"
> 
> def get_idl():
>"""Connection getter."""
> 
>idl = connection.OvsdbIdl.from_server(remote, "OVN_Northbound",
>  leader_only=False)
>return nb_idl.OvnNbApiIdlImpl(connection.Connection(idl, 100))
> 
> idl = get_idl()
> 
> 
> # in terminal 1
> ovsdb-client convert unix:$(pwd)/ovs.sock /usr/share/ovn/ovn-nb.ovsschema
> 
> # in terminal 2 python shell:
> idl.ls_add("test").execute()
> 
> 
> We get following traceback:
> 
> Traceback (most recent call last):
>  File 
> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/connection.py", 
> line 131, in run
>txn.results.put(txn.do_commit())
>  File 
> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/transaction.py", 
> line 143, in do_commit
>self.post_commit(txn)
>  File 
> "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/transaction.py", 
> line 73, in post_commit
>command.post_commit(txn)
>  File "/usr/lib/python3.6/site-packages/ovsdbapp/backend/ovs_idl/command.py", 
> line 94, in post_commit
>row = self.api.tables[self.table_name].rows[real_uuid]
>  File "/usr/lib64/python3.6/collections/__init__.py", line 991, in __getitem__
>raise KeyError(key)
> KeyError: UUID('0256afa4-6dd0-4c2c-b6a2-686a360ab331') 
> 
> In ovsdb-server debug logs we see that update2 or update3 messages are not 
> sent from server in response to client’s transaction, just reply with result 
> UUID:
> 2022-11-24T17:42:36Z|00306|poll_loop|DBG|wakeup due to [POLLIN] on fd 18 
> (///root/ovsdb-problem/ovs.sock<->) at lib/stream-fd.c:157
> 2022-11-24T17:42:36Z|00307|jsonrpc|DBG|unix#5: received request, 
> method="transact", 
> params=["OVN_Northbound",{"uuid-name":"row03ef28d6_93f1_43bc_b07a_eae58d4bd1c5","table":"Logical_Switch","op":"insert","row":{"name”:"test"}}],
>  id=5
> 2022-11-24T17:42:36Z|00308|jsonrpc|DBG|unix#5: send reply, 
> result=[{"uuid":["uuid","4eb7c407-beec-46ca-b816-19f942e57721"]}], id=5
> 
> We checked same with ovn-nbctl running in daemon mode and found that the 
> problem is not reproduced (ovsdb-server after database conversion sends out 
> update3 message to ovn-nbctl daemon process in response to transaction, for 
> example ovs-appctl -t  run ls-add test-ls):
> 2022-11-24T17:54:51Z|00623|jsonrpc|DBG|unix#7: received request, 
> method="transact", 
> params=["OVN_Northbound",{"uuid-name":"rowcdb152ce_a9af_4761_b965_708ad300fcb7","table":"Logical_Switch","op":"insert","row":{"name":"test-ls"}},{"comment":"ovn-nbctl:
>  run ls-add test-ls","op":"comment"}], id=5
> 2022-11-24T17:54:51Z|00624|jsonrpc|DBG|unix#7: send notification, 
> method="update3", 
> params=[["monid","OVN_Northbound"],"----",{"Logical_Switch":{"0b147f2c-248d-496a-b718-a5328d3c2995":{"insert":{"name":"test-ls"]
> 2022-11-24T17:54:51Z|00625|jsonrpc|DBG|unix#7: send reply, 
> result=[{"uuid":["uuid","0b147f2c-248d-496a-b718-a5328d3c2995"]},{}], id=5
> 
> So it seems that the problem is in python-ovs, not in ovsdb-server.
> We tested with ovsdb-server running version 2.17.3 and python-ovs 2.13.5 and 
> also python-ovs 2.17.3, the behaviour is the same.
> 
> Do you have any ideas what can be a reason for such behaviour?
> 
> 0: 
> https://review.opendev.org/c/openstack/ovsdbapp/+/865454/comments/674c57e6_3849591b
> 
> Regards, Anton.

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

[ovs-dev] [PATCH v8 15/15] tests: Comment currently failing TC system-traffic tests.

[Eelco Chaudron]

The goal was to run 200 successful tc tests in a row. To do this the
following was run:

  for i in {1..200}; do make check-offloads || break; \
echo "ALL_200_OK: $i"; done;

Unfortunately, a bunch of test cases showed occasional failures.
For now, they are excluded from the test cases and need further
investigation. They are:

  802.1ad - vlan_limit
  conntrack - DNAT load balancing
  conntrack - DNAT load balancing with NC
  conntrack - ICMP related
  conntrack - ICMP related to original direction
  conntrack - ICMP related with NAT
  conntrack - IPv4 fragmentation with fragments specified
  conntrack - multiple namespaces, internal ports
  conntrack - zones from other field
  conntrack - zones from other field, more tests
  datapath - basic truncate action
  datapath - multiple mpls label pop
  datapath - truncate and output to gre tunnel
  datapath - truncate and output to gre tunnel by simulated packets

Some other test cases also fail due to what looks like problems
in the tc kernel conntrack implementation. For details see the
details in the system-offloads.at exclusion list definition.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |   43 +--
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 18e542aea..edffdd73b 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -60,20 +60,51 @@ m4_define([CHECK_CONNTRACK_TIMEOUT],
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
+# TC does not support moving ports to a different namespace than vswitchd's
+# namespace, so we need to disable this test.
 conntrack - multiple namespaces, internal ports
+
+# When moving through different zones, it can take up to ~8 seconds before
+# the conntrack state gets updated causing these tests to fail.
 conntrack - ct metadata, multiple zones
-conntrack - ICMP related
-conntrack - ICMP related to original direction
+conntrack - multiple zones, local
+conntrack - multi-stage pipeline, local
+
+# The kernel's tcf_ct_act() function does not seem to take care of any (QinQ)
+# VLAN headers causing commits to fail. However, if this is solved, we have to
+# make sure conntrack does not break the VLAN boundary, i.e., putting together
+# two packets with different CVLAN+SVLAN values.
 conntrack - IPv4 fragmentation + cvlan
-conntrack - IPv4 fragmentation with fragments specified
 conntrack - IPv6 fragmentation + cvlan
+
+# Fragmentation handling in ct zone 9 does not seem to work correctly.
+# When moving this test over to the default zone all works fine.
 conntrack - Fragmentation over vxlan
 conntrack - IPv6 Fragmentation over vxlan
-conntrack - multiple zones, local
-conntrack - multi-stage pipeline, local
+
+# Occasionaly we fail on the 'execute ct(commit) failed (Invalid argument) on
+# packet...' log message being present
+conntrack - zones from other field
+conntrack - zones from other field, more tests
+conntrack - multiple namespaces, internal ports
+conntrack - IPv4 fragmentation with fragments specified
+
+# Occasionaly we fail on the 'failed to flow_get/flow_del (No such file or 
directory)
+# ufid:..' log message being present.
+datapath - multiple mpls label pop
+datapath - basic truncate action
+conntrack - ICMP related
+conntrack - ICMP related to original direction
 conntrack - ICMP related with NAT
 conntrack - DNAT load balancing
-conntrack - DNAT load balancing with NC"
+conntrack - DNAT load balancing with NC
+802.1ad - vlan_limit
+
+# Occasionalt we fail with extreme high byte counters, i.e.
+# n_bytes=18446744073705804134
+datapath - truncate and output to gre tunnel by simulated packets
+datapath - truncate and output to gre tunnel
+"
 echo "$ovs_test_skip_list" | sed "s// /g"])
 
 m4_include([tests/system-traffic.at])

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

[ovs-dev] [PATCH v8 14/15] tests: Fix reading of OpenFlow byte counters in GRE test cases.

[Eelco Chaudron]

With some datapaths, read TC, it takes a bit longer to update the
OpenFlow statistics. Rather than adding an additional delay, try
to read the counters multiple times until we get the desired value.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |2 --
 tests/system-traffic.at  |   15 ++-
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index f4adac7b3..18e542aea 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -60,8 +60,6 @@ m4_define([CHECK_CONNTRACK_TIMEOUT],
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
-datapath - truncate and output to gre tunnel by simulated packets
-datapath - truncate and output to gre tunnel
 conntrack - multiple namespaces, internal ports
 conntrack - ct metadata, multiple zones
 conntrack - ICMP related
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index d162674c9..60cda6a4a 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1708,9 +1708,8 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=2" | 
sed -n 's/.*\(n\_bytes=[
 n_bytes=242
 ])
 dnl After truncation = outer ETH(14) + outer IP(20) + GRE(4) + 100 = 138B
-AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
-n_bytes=138
-])
+OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed 
-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p' | grep "n_bytes=138"],
+   [ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed 
-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'])
 
 dnl check tunnel pop path, from at_ns0 to at_ns1
 dnl This 200-byte packet is simulated on behalf of ns_gre0
@@ -1745,9 +1744,8 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=2" | 
sed -n 's/.*\(n\_bytes=[
 n_bytes=242
 ])
 dnl After truncation = outer ETH(14) + outer IP(20) + GRE(4) + 100 = 138B
-AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
-n_bytes=138
-])
+OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed 
-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p' | grep "n_bytes=138"],
+   [ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed 
-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'])
 
 dnl check tunnel pop path, from at_ns0 to at_ns1
 dnl This 200-byte packet is simulated on behalf of ns_gre0
@@ -1755,9 +1753,8 @@ ovs-ofctl -O OpenFlow13 packet-out br-underlay "in_port=1 
packet=02908ca8a149faa
 
 dnl After truncation = 100 byte at loopback device p2(4)
 OVS_REVALIDATOR_PURGE()
-AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip], [0], [dnl
- n_packets=1, n_bytes=100, priority=1,ip,in_port=4 actions=drop
-])
+OVS_WAIT_UNTIL([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip | 
grep "n_packets=1, n_bytes=100, priority=1,ip,in_port=4 actions=drop"],
+   [ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip])
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP

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

[ovs-dev] [PATCH v8 13/15] netdev-offload-tc: If the flow has not been used, report it as such.

[Eelco Chaudron]

If a tc flow was installed but has not yet been used, report it as such.

In addition, add a delay to the "IGMP - flood under normal action" test
case to make it work with many repetitions. This delay is also present
in other ICMP/IGMP tests.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 lib/tc.c |   14 +-
 tests/system-offloads.at |3 +--
 tests/system-traffic.at  |2 ++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/lib/tc.c b/lib/tc.c
index 447ab376e..c8f16874b 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -1366,7 +1366,19 @@ get_user_hz(void)
 static void
 nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower)
 {
-uint64_t lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
+uint64_t lastused;
+
+/* On creation both tm->install and tm->lastuse are set to jiffies
+ * by the kernel. So if both values are the same, the flow has not been
+ * used yet.
+ *
+ * Note that tm->firstuse can not be used due to some kernel bug, i.e.,
+ * hardware offloaded flows do not update tm->firstuse. */
+if (tm->lastuse == tm->install) {
+lastused = 0;
+} else {
+lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
+}
 
 if (flower->lastused < lastused) {
 flower->lastused = lastused;
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 57be710a7..f4adac7b3 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -75,8 +75,7 @@ conntrack - multiple zones, local
 conntrack - multi-stage pipeline, local
 conntrack - ICMP related with NAT
 conntrack - DNAT load balancing
-conntrack - DNAT load balancing with NC
-IGMP - flood under normal action"
+conntrack - DNAT load balancing with NC"
 echo "$ovs_test_skip_list" | sed "s// /g"])
 
 m4_include([tests/system-traffic.at])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index b140a1e26..d162674c9 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -7153,6 +7153,8 @@ f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 
49 45 65 eb 4a e0 dnl
 00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl
 00 00 00 00 > /dev/null])
 
+sleep 1
+
 AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -e .*ipv4 | sort | dnl
   strip_stats | strip_used | strip_recirc | dnl
   sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'],

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

[ovs-dev] [PATCH v8 12/15] odp-util: Make odp_flow_key_from_flow__ nlattr order the same as the kernel.

[Eelco Chaudron]

Make the order of the Netlink attributes for odp_flow_key_from_flow__()
the same as the kernel will return them.

This will make sure the attributes displayed in the dpctl/dump-flows
output appear in the same order for all datapath.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 lib/odp-util.c|   21 +-
 tests/dpif-netdev.at  |   28 +++---
 tests/mcast-snooping.at   |4 +-
 tests/nsh.at  |   10 ++---
 tests/odp.at  |   84 -
 tests/ofproto-dpif.at |   30 +++
 tests/packet-type-aware.at|   22 +--
 tests/pmd.at  |2 -
 tests/system-offloads.at  |1 
 tests/tunnel-push-pop-ipv6.at |2 -
 tests/tunnel-push-pop.at  |2 -
 tests/tunnel.at   |2 -
 12 files changed, 102 insertions(+), 106 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 5fc312f8c..dbd4554d0 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -6204,6 +6204,11 @@ odp_flow_key_from_flow__(const struct odp_flow_key_parms 
*parms,
 const struct flow *mask = parms->mask;
 const struct flow *data = export_mask ? mask : flow;
 
+if (parms->support.recirc) {
+nl_msg_put_u32(buf, OVS_KEY_ATTR_RECIRC_ID, data->recirc_id);
+nl_msg_put_u32(buf, OVS_KEY_ATTR_DP_HASH, data->dp_hash);
+}
+
 nl_msg_put_u32(buf, OVS_KEY_ATTR_PRIORITY, data->skb_priority);
 
 if (flow_tnl_dst_is_set(>tunnel) ||
@@ -6212,6 +6217,12 @@ odp_flow_key_from_flow__(const struct odp_flow_key_parms 
*parms,
 parms->key_buf, NULL);
 }
 
+/* Add an ingress port attribute if this is a mask or 'in_port.odp_port'
+ * is not the magical value "ODPP_NONE". */
+if (export_mask || flow->in_port.odp_port != ODPP_NONE) {
+nl_msg_put_odp_port(buf, OVS_KEY_ATTR_IN_PORT, data->in_port.odp_port);
+}
+
 nl_msg_put_u32(buf, OVS_KEY_ATTR_SKB_MARK, data->pkt_mark);
 
 if (parms->support.ct_state) {
@@ -6255,16 +6266,6 @@ odp_flow_key_from_flow__(const struct odp_flow_key_parms 
*parms,
 ct->ipv6_proto = data->ct_nw_proto;
 }
 }
-if (parms->support.recirc) {
-nl_msg_put_u32(buf, OVS_KEY_ATTR_RECIRC_ID, data->recirc_id);
-nl_msg_put_u32(buf, OVS_KEY_ATTR_DP_HASH, data->dp_hash);
-}
-
-/* Add an ingress port attribute if this is a mask or 'in_port.odp_port'
- * is not the magical value "ODPP_NONE". */
-if (export_mask || flow->in_port.odp_port != ODPP_NONE) {
-nl_msg_put_odp_port(buf, OVS_KEY_ATTR_IN_PORT, data->in_port.odp_port);
-}
 
 nl_msg_put_be32(buf, OVS_KEY_ATTR_PACKET_TYPE, data->packet_type);
 
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index 9af70a68d..baab60a22 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -72,13 +72,13 @@ ovs-appctl time/warp 5000
 AT_CHECK([ovs-appctl netdev-dummy/receive p1 
'in_port(1),eth(src=50:54:00:00:00:01,dst=50:54:00:00:02:00),eth_type(0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)'])
OVS_WAIT_UNTIL([grep "miss upcall" ovs-vswitchd.log])
AT_CHECK([grep -A 1 'miss upcall' ovs-vswitchd.log | tail -n 1], [0], [dnl
-skb_priority(0),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),recirc_id(0),dp_hash(0),in_port(1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:01,dst=50:54:00:00:02:00),eth_type(0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)
+recirc_id(0),dp_hash(0),skb_priority(0),in_port(1),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:01,dst=50:54:00:00:02:00),eth_type(0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)
 ])
 
 AT_CHECK([ovs-appctl netdev-dummy/receive p1 
'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:06:00),eth_type(0x0800),ipv4(src=10.0.0.5,dst=10.0.0.6,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)'
 --len 1024])
OVS_WAIT_UNTIL([test `grep -c "miss upcall" ovs-vswitchd.log` -ge 2])
AT_CHECK([grep -A 1 'miss upcall' ovs-vswitchd.log | tail -n 1], [0], [dnl
-skb_priority(0),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),recirc_id(0),dp_hash(0),in_port(1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:06:00),eth_type(0x0800),ipv4(src=10.0.0.5,dst=10.0.0.6,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)
+recirc_id(0),dp_hash(0),skb_priority(0),in_port(1),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:06:00),eth_type(0x0800),ipv4(src=10.0.0.5,dst=10.0.0.6,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)
 ])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
@@ -139,7 +139,7 @@ m4_define([DPIF_NETDEV_MISS_FLOW_INSTALL],
 
OVS_WAIT_UNTIL([grep "miss upcall" 

[ovs-dev] [PATCH v8 11/15] test: Fix 'conntrack - Multiple ICMP traverse' for tc case.

[Eelco Chaudron]

tc does not include ethernet header length in packet byte count.
This fix will allow the packets that go trough tc to be 14 bytes less.

This difference in the TC implementation is already described in
tc-offload.rst.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |1 -
 tests/system-traffic.at  |2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index a7076c2c2..cc45f662a 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -76,7 +76,6 @@ conntrack - multi-stage pipeline, local
 conntrack - ICMP related with NAT
 conntrack - DNAT load balancing
 conntrack - DNAT load balancing with NC
-conntrack - Multiple ICMP traverse
 conntrack - can match and clear ct_state from outside OVS
 IGMP - flood under normal action"
 echo "$ovs_test_skip_list" | sed "s// /g"])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 8b8b63749..b140a1e26 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -7080,7 +7080,7 @@ AT_CHECK([DPCTL_DUMP_CONNTRACK | FORMAT_CT(10.1.1)], [0], 
[dnl
 
icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.1,id=,type=0,code=0)
 ])
 
-AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | 
OFPROTO_CLEAR_DURATION_IDLE],
+AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | 
OFPROTO_CLEAR_DURATION_IDLE | sed 's/n_bytes=70,/n_bytes=84,/'],
  [0], [dnl
  cookie=0x0, duration=, table=2, n_packets=2, n_bytes=84, 
idle_age=, priority=10,ct_state=+new+trk,in_port=1 actions=drop
  cookie=0x0, duration=, table=2, n_packets=0, n_bytes=0, 
idle_age=, priority=10,ct_state=+est+trk actions=drop

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

[ovs-dev] [PATCH v8 10/15] test: tc does not support conntrack timeout, skip the related test.

[Eelco Chaudron]

The tc conntrack implementation does not support the timeout option.
The current implementation is silently ignoring the timeout option
by adding a general conntrack entry.

This patch will skip the related test by overriding the support macro.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index c4ed3a171..a7076c2c2 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -49,6 +49,13 @@ m4_define([CHECK_CONNTRACK_ALG],
 ])
 
 
+# Conntrack timeout not supported for tc.
+m4_define([CHECK_CONNTRACK_TIMEOUT],
+[
+ AT_SKIP_IF([:])
+])
+
+
 # The list below are tests that will not pass for a "test environment" specific
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
@@ -64,7 +71,6 @@ conntrack - IPv4 fragmentation with fragments specified
 conntrack - IPv6 fragmentation + cvlan
 conntrack - Fragmentation over vxlan
 conntrack - IPv6 Fragmentation over vxlan
-conntrack - zone-based timeout policy
 conntrack - multiple zones, local
 conntrack - multi-stage pipeline, local
 conntrack - ICMP related with NAT

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

[ovs-dev] [PATCH v8 09/15] netdev-offload-tc: Conntrack ALGs are not supported with tc.

[Eelco Chaudron]

tc does not support conntrack ALGs. Even worse, with tc enabled, they
should not be used/configured at all. This is because even though TC
will ignore the rules with ALG configured, i.e., they will flow through
the kernel module, return traffic might flow through a tc conntrack
rule, and it will not invoke the ALG helper.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 Documentation/howto/tc-offload.rst |   11 +++
 lib/netdev-offload-tc.c|4 
 tests/system-offloads.at   |   27 +++
 3 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/Documentation/howto/tc-offload.rst 
b/Documentation/howto/tc-offload.rst
index f6482c8af..63687adc9 100644
--- a/Documentation/howto/tc-offload.rst
+++ b/Documentation/howto/tc-offload.rst
@@ -112,3 +112,14 @@ First flow packet not processed by meter
 Packets that are received by ovs-vswitchd through an upcall before the actual
 meter flow is installed, are not passing TC police action and therefore are
 not considered for policing.
+
+Conntrack Application Layer Gateways(ALG)
++
+
+TC does not support conntrack helpers, i.e., ALGs. TC will not offload flows if
+the ALG keyword is present within the ct() action. However, this will not allow
+ALGs to work within the datapath, as the return traffic without the ALG keyword
+might run through a TC rule, which internally will not call the conntrack
+helper required.
+
+So if ALG support is required, tc offload must be disabled.
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 915c45ed3..ba309c2b6 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1357,6 +1357,10 @@ parse_put_flow_ct_action(struct tc_flower *flower,
 action->ct.label_mask = ct_label->mask;
 }
 break;
+/* The following option we do not support in tc-ct, and should
+ * not be ignored for proper operation. */
+case OVS_CT_ATTR_HELPER:
+return EOPNOTSUPP;
 }
 }
 
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 9d1e80c8d..c4ed3a171 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -42,6 +42,13 @@ m4_define([OVS_REVALIDATOR_PURGE],
 m4_define([DPCTL_DUMP_CONNTRACK], [sleep 3; ovs-appctl dpctl/dump-conntrack])
 
 
+# Conntrack ALGs are not supported for tc.
+m4_define([CHECK_CONNTRACK_ALG],
+[
+ AT_SKIP_IF([:])
+])
+
+
 # The list below are tests that will not pass for a "test environment" specific
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
@@ -60,27 +67,7 @@ conntrack - IPv6 Fragmentation over vxlan
 conntrack - zone-based timeout policy
 conntrack - multiple zones, local
 conntrack - multi-stage pipeline, local
-conntrack - FTP
-conntrack - FTP over IPv6
-conntrack - IPv6 FTP Passive
-conntrack - FTP with multiple expectations
-conntrack - TFTP
 conntrack - ICMP related with NAT
-conntrack - FTP SNAT prerecirc
-conntrack - FTP SNAT prerecirc seqadj
-conntrack - FTP SNAT postrecirc
-conntrack - FTP SNAT postrecirc seqadj
-conntrack - FTP SNAT orig tuple
-conntrack - FTP SNAT orig tuple seqadj
-conntrack - IPv4 FTP Passive with SNAT
-conntrack - IPv4 FTP Passive with DNAT
-conntrack - IPv4 FTP Passive with DNAT 2
-conntrack - IPv4 FTP Active with DNAT
-conntrack - IPv4 FTP Active with DNAT with reverse skew
-conntrack - IPv6 FTP with SNAT
-conntrack - IPv6 FTP Passive with SNAT
-conntrack - IPv6 FTP with SNAT - orig tuple
-conntrack - IPv4 TFTP with SNAT
 conntrack - DNAT load balancing
 conntrack - DNAT load balancing with NC
 conntrack - Multiple ICMP traverse

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

[ovs-dev] [PATCH v8 08/15] test: Flush datapath when changing rules on the fly.

[Eelco Chaudron]

Flush datapath flows as TC flows take some more time to be flushed out.
The flush speeds this up.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |2 --
 tests/system-traffic.at  |6 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 593dc1c7a..9d1e80c8d 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -48,8 +48,6 @@ m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
-conntrack - zones from other field
-conntrack - zones from other field, more tests
 conntrack - multiple namespaces, internal ports
 conntrack - ct metadata, multiple zones
 conntrack - ICMP related
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 2497dcd60..8b8b63749 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2845,6 +2845,9 @@ AT_CHECK([ovs-appctl dpctl/dump-flows --names 
filter=in_port=ovs-p0 dnl
 AT_CHECK([ovs-ofctl mod-flows br0 dnl
 'priority=100,ct_state=-trk,tcp,in_port="ovs-p0" 
actions=ct(table=0,zone=15)'])
 
+dnl Force flush flows as some datapaths (read TC) might take time to clear.
+AT_CHECK([ovs-appctl dpctl/del-flows])
+
 NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o 
wget0.log])
 
 AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p0 dnl
@@ -2893,6 +2896,9 @@ AT_CHECK([ovs-appctl dpctl/dump-flows --names 
filter=in_port=ovs-p0 dnl
 
 AT_CHECK([ovs-ofctl mod-flows br0 
'priority=100,ct_state=-trk,tcp,in_port="ovs-p0" 
actions=ct(table=0,zone=15,commit,exec(load:0x000f->NXM_NX_CT_LABEL[[0..31]]))'])
 
+dnl Force flush flows as some datapaths (read TC) might take time to clear.
+AT_CHECK([ovs-appctl dpctl/del-flows])
+
 NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o 
wget0.log])
 
 AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p0 dnl

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

[ovs-dev] [PATCH v8 07/15] test: Fix "conntrack - floating IP" test for TC.

[Eelco Chaudron]

This change fixes the "conntrack - floating" test for the TC
offload case. In this scenario, the connection might move to
CLOSE_WAIT, which would fail the test as it only accepts
TIME_WAIT. However, both indicate the connection was
established, so the test should pass.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |1 -
 tests/system-traffic.at  |   13 +++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 1aca41825..593dc1c7a 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -85,7 +85,6 @@ conntrack - IPv6 FTP with SNAT - orig tuple
 conntrack - IPv4 TFTP with SNAT
 conntrack - DNAT load balancing
 conntrack - DNAT load balancing with NC
-conntrack - floating IP
 conntrack - Multiple ICMP traverse
 conntrack - can match and clear ct_state from outside OVS
 IGMP - flood under normal action"
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index dfaaf9d26..2497dcd60 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -6959,16 +6959,17 @@ AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
 
 dnl non-FIP case
 NS_CHECK_EXEC([at_ns1], [echo "foobar" |nc $NC_EOF_OPT 10.1.1.1 1234])
-OVS_WAIT_UNTIL([[ovs-appctl dpctl/dump-conntrack | sed -e 
's/port=[0-9]*/port=/g' -e 's/id=[0-9]*/id=/g' |
+OVS_WAIT_UNTIL([[ovs-appctl dpctl/dump-conntrack | sed -e 
's/port=[0-9]*/port=/g' -e 's/id=[0-9]*/id=/g' -e 
's/CLOSE_WAIT\|CLOSING/TIME_WAIT/g' |
 grep 
"tcp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=),reply=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),protoinfo=(state=TIME_WAIT)"
 ]])
 
-dnl Check that the full session ends as expected (i.e. TIME_WAIT). Otherwise it
-dnl means the datapath didn't process the ct_clear action. Ending in SYN_RECV
-dnl (OVS maps to ESTABLISHED) means the initial frame was committed, but not a
-dnl second time after the FIP translation (because ct_clear didn't occur).
+dnl Check that the full session ends as expected (i.e. TIME_WAIT, CLOSE_WAIT).
+dnl Otherwise it means the datapath didn't process the ct_clear action. Ending
+dnl in SYN_RECV (OVS maps to ESTABLISHED) means the initial frame was
+dnl committed, but not a second time after the FIP translation (because
+dnl ct_clear didn't occur).
 NS_CHECK_EXEC([at_ns1], [echo "foobar" |nc $NC_EOF_OPT 10.254.254.1 1234])
-OVS_WAIT_UNTIL([[ovs-appctl dpctl/dump-conntrack | sed -e 
's/port=[0-9]*/port=/g' -e 's/id=[0-9]*/id=/g' |
+OVS_WAIT_UNTIL([[ovs-appctl dpctl/dump-conntrack | sed -e 
's/port=[0-9]*/port=/g' -e 's/id=[0-9]*/id=/g'  -e 
's/CLOSE_WAIT\|CLOSING/TIME_WAIT/g' |
 grep 
"tcp,orig=(src=10.254.254.2,dst=10.1.1.1,sport=,dport=),reply=(src=10.1.1.1,dst=10.254.254.2,sport=,dport=),protoinfo=(state=TIME_WAIT)"
 ]])
 

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

[ovs-dev] [PATCH v8 06/15] tests: Add delay to dump-conntrack for tc test cases.

[Eelco Chaudron]

This patch adds a delay before dumping the conntrack table because with
tc it takes a bit longer before it gets synced.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-common-macros.at |3 +
 tests/system-offloads.at  |   25 +
 tests/system-traffic.at   |  199 -
 3 files changed, 107 insertions(+), 120 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index d95d79791..32b9ca0de 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -347,3 +347,6 @@ m4_define([OVS_CHECK_CT_CLEAR],
 # OVS_REVALIDATOR_PURGE()
 m4_define([OVS_REVALIDATOR_PURGE],
 [AT_CHECK([ovs-appctl revalidator/purge], [0])])
+
+# DPCTL_DUMP_CONNTRACK()
+m4_define([DPCTL_DUMP_CONNTRACK], [ovs-appctl dpctl/dump-conntrack])
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index d39997708..1aca41825 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -37,24 +37,20 @@ m4_define([OVS_REVALIDATOR_PURGE],
 [AT_CHECK([sleep 2; ovs-appctl revalidator/purge], [0])])
 
 
+# We override the DPCTL_DUMP_CONNTRACK macro, allowing a bit more time for the
+# tc-datapath conntrack entries to be installed/updated.
+m4_define([DPCTL_DUMP_CONNTRACK], [sleep 3; ovs-appctl dpctl/dump-conntrack])
+
+
 # The list below are tests that will not pass for a "test environment" specific
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
-conntrack - preserve registers
-conntrack - zones
-conntrack - zones from field
 conntrack - zones from other field
 conntrack - zones from other field, more tests
-conntrack - multiple zones
 conntrack - multiple namespaces, internal ports
-conntrack - ct_mark
-conntrack - ct_mark bit-fiddling
-conntrack - ct_mark from register
-conntrack - ct_label
-conntrack - ct_label bit-fiddling
 conntrack - ct metadata, multiple zones
 conntrack - ICMP related
 conntrack - ICMP related to original direction
@@ -64,8 +60,6 @@ conntrack - IPv6 fragmentation + cvlan
 conntrack - Fragmentation over vxlan
 conntrack - IPv6 Fragmentation over vxlan
 conntrack - zone-based timeout policy
-conntrack - IPv4 HTTP
-conntrack - IPv6 HTTP
 conntrack - multiple zones, local
 conntrack - multi-stage pipeline, local
 conntrack - FTP
@@ -73,14 +67,6 @@ conntrack - FTP over IPv6
 conntrack - IPv6 FTP Passive
 conntrack - FTP with multiple expectations
 conntrack - TFTP
-conntrack - simple SNAT
-conntrack - SNAT with port range
-conntrack - SNAT with port range with exhaustion
-conntrack - more complex SNAT
-conntrack - all-zero IP SNAT
-conntrack - simple DNAT
-conntrack - DNAT with additional SNAT
-conntrack - more complex DNAT
 conntrack - ICMP related with NAT
 conntrack - FTP SNAT prerecirc
 conntrack - FTP SNAT prerecirc seqadj
@@ -93,7 +79,6 @@ conntrack - IPv4 FTP Passive with DNAT
 conntrack - IPv4 FTP Passive with DNAT 2
 conntrack - IPv4 FTP Active with DNAT
 conntrack - IPv4 FTP Active with DNAT with reverse skew
-conntrack - IPv6 HTTP with DNAT
 conntrack - IPv6 FTP with SNAT
 conntrack - IPv6 FTP Passive with SNAT
 conntrack - IPv6 FTP with SNAT - orig tuple
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 3187c3ff5..dfaaf9d26 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2263,7 +2263,7 @@ 
udp,vlan_tci=0x,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10.
 dnl
 dnl Check that the directionality has been changed by force commit.
 dnl
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.2,"], 
[], [dnl
+AT_CHECK([DPCTL_DUMP_CONNTRACK | grep "orig=.src=10\.1\.1\.2,"], [], [dnl
 
udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2)
 ])
 
@@ -2271,7 +2271,7 @@ dnl OK, now send another packet from port 1 and see that 
it switches again
 AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 
packet=5054000a505400090800451c0011a4cd0a0101010a010102000100020008
 actions=resubmit(,0)"])
 OVS_REVALIDATOR_PURGE()
 
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.1,"], 
[], [dnl
+AT_CHECK([DPCTL_DUMP_CONNTRACK | grep "orig=.src=10\.1\.1\.1,"], [], [dnl
 
udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1)
 ])
 
@@ -2305,32 +2305,31 @@ AS_BOX([Testing with FLUSH_CMD])
 dnl Test UDP from port 1
 AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 
packet=5054000a505400090800451c0011a4cd0a0101010a010102000100020008
 actions=resubmit(,0)"])
 
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.1,"], 
[], [dnl
+AT_CHECK([DPCTL_DUMP_CONNTRACK | grep "orig=.src=10\.1\.1\.1,"], [], [dnl
 

[ovs-dev] [PATCH v8 05/15] netdev-offload-tc: Fix tc conntrack force commit support.

[Eelco Chaudron]

tc was not setting the OVS_CT_ATTR_FORCE_COMMIT flag when a forced
commit was requested. This patch will fix this.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 lib/netdev-offload-tc.c  |   13 +++--
 tests/system-offloads.at |1 -
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index ce7f8ad97..915c45ed3 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -825,7 +825,11 @@ parse_tc_flower_to_actions__(struct tc_flower *flower, 
struct ofpbuf *buf,
 ct_offset = nl_msg_start_nested(buf, OVS_ACTION_ATTR_CT);
 
 if (action->ct.commit) {
-nl_msg_put_flag(buf, OVS_CT_ATTR_COMMIT);
+if (action->ct.force) {
+nl_msg_put_flag(buf, OVS_CT_ATTR_FORCE_COMMIT);
+} else {
+nl_msg_put_flag(buf, OVS_CT_ATTR_COMMIT);
+}
 }
 
 if (action->ct.zone) {
@@ -1309,7 +1313,12 @@ parse_put_flow_ct_action(struct tc_flower *flower,
 NL_ATTR_FOR_EACH_UNSAFE (ct_attr, ct_left, ct, ct_len) {
 switch (nl_attr_type(ct_attr)) {
 case OVS_CT_ATTR_COMMIT: {
-action->ct.commit = true;
+action->ct.commit = true;
+}
+break;
+case OVS_CT_ATTR_FORCE_COMMIT: {
+action->ct.commit = true;
+action->ct.force = true;
 }
 break;
 case OVS_CT_ATTR_ZONE: {
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 7b6deccf0..d39997708 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -43,7 +43,6 @@ m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
-conntrack - force commit
 conntrack - preserve registers
 conntrack - zones
 conntrack - zones from field

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

[ovs-dev] [PATCH v8 04/15] test: Add delay on revalidator flush for offload test cases.

[Eelco Chaudron]

The revalidator/purge commands in the system test cases sometimes
get called immediately after a partial test is completed. This
could cause the revalidator thread to log an error that it can
not find/delete a flow due to the slower flow installation nature
of TC.

This patch uses a macro to call the revalidator/purge command,
which can be overwritten when the system tests are run on a tc
enabled datapath.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-common-macros.at |4 
 tests/system-offloads.at  |8 +++-
 tests/system-traffic.at   |   38 +++---
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 8b9f5c752..d95d79791 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -343,3 +343,7 @@ m4_define([OVS_CHECK_IPROUTE_ENCAP],
 # OVS_CHECK_CT_CLEAR()
 m4_define([OVS_CHECK_CT_CLEAR],
 [AT_SKIP_IF([! grep -q "Datapath supports ct_clear action" 
ovs-vswitchd.log])])
+
+# OVS_REVALIDATOR_PURGE()
+m4_define([OVS_REVALIDATOR_PURGE],
+[AT_CHECK([ovs-appctl revalidator/purge], [0])])
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index fbe1dc99a..7b6deccf0 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -30,11 +30,17 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| 
uuidfilt])], [0], [$2])
 ])
 
+
+# We override the OVS_REVALIDATOR_PURGE macro, allowing a bit more time for the
+# tc-datapath entries to be installed.
+m4_define([OVS_REVALIDATOR_PURGE],
+[AT_CHECK([sleep 2; ovs-appctl revalidator/purge], [0])])
+
+
 # The list below are tests that will not pass for a "test environment" specific
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
-datapath - basic truncate action
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
 conntrack - force commit
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 3e610bacb..3187c3ff5 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1565,12 +1565,12 @@ on_exit 'rm -f payload200.bin'
 NS_CHECK_EXEC([at_ns0], [nc $NC_EOF_OPT -u 10.1.1.2 1234 < payload200.bin])
 
 dnl packet with truncated size
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" |  sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=100
 ])
 dnl packet with original size
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=5" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=242
 ])
@@ -1587,7 +1587,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 NS_CHECK_EXEC([at_ns0], [nc $NC_EOF_OPT -u 10.1.1.2 1234 < payload200.bin])
 
 dnl 100 + 100 + 242 + min(65535,242) = 684
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=684
 ])
@@ -1617,7 +1617,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 NS_CHECK_EXEC([at_ns0], [nc $NC_EOF_OPT -u 10.1.1.2 1234 < payload200.bin])
 
 dnl 100 + 100 + 242 + min(65535,242) = 684
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=684
 ])
@@ -1701,7 +1701,7 @@ AT_CHECK([ovs-ofctl add-flows br-underlay 
flows-underlay.txt])
 
 dnl check tunnel push path, from at_ns1 to at_ns0
 NS_CHECK_EXEC([at_ns1], [nc $NC_EOF_OPT -u 10.1.1.1 1234 < payload200.bin])
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 
 dnl Before truncation = ETH(14) + IP(20) + UDP(8) + 200 = 242B
 AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=2" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
@@ -1717,7 +1717,7 @@ dnl This 200-byte packet is simulated on behalf of ns_gre0
 ovs-ofctl -O OpenFlow13 packet-out br-underlay "in_port=1 
packet=02908ca8a149faadfa25056008004500010a9e9d4000402f4084ac1f0101ac1f01646558e666c122e666c111080045e46f8e40004011b4760a0101010a010102e026162e00d016e6a366ebf904c74132c6fed42a9e9e46240b4d9fd13c9b47d9704a388e70a5e77db16934a6188dc01d86aa20007ace2cf9cdb111f208474b88ffc851c871f0e3fb4fff138c1d288d437efff487e2b86a9c99fbf4229a6485e133bcf3e16f6e345207fda0932d9eeb602740456fd077b4847d25481337bd716155cc245be129ccc11bf82b834767b3760b52fe913c0e24f31c0e1b27f88acf7bba6b985fb64ee2cd6fc6bba1a9c1f021e253e1728b046fd4d023307e3296361a37ea2617ebcb2537e0284a81050dd0ee
 actions=LOCAL"
 
 dnl After truncation = 100 byte at loopback device p2(4)
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 | grep 

[ovs-dev] [PATCH v8 03/15] test: Do not use MPLS implicit null label in test cases.

[Eelco Chaudron]

TC flower does not allow the push of the implicit null labels (RFC3032).
Avoid the use of such labels in the MPLS test cases.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |2 --
 tests/system-traffic.at  |8 
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index f6dd931b7..fbe1dc99a 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -34,8 +34,6 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
-datapath - mpls actions
-datapath - multiple mpls label pop
 datapath - basic truncate action
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 503455cc6..3e610bacb 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1215,8 +1215,8 @@ AT_CHECK([ovs-vsctl add-port br0 patch0])
 AT_CHECK([ovs-vsctl add-port br1 patch1])
 
 AT_DATA([flows.txt], [dnl
-table=0,priority=100,dl_type=0x0800 
actions=push_mpls:0x8847,set_mpls_label:3,resubmit(,1)
-table=0,priority=100,dl_type=0x8847,mpls_label=3 
actions=pop_mpls:0x0800,resubmit(,1)
+table=0,priority=100,dl_type=0x0800 
actions=push_mpls:0x8847,set_mpls_label:4,resubmit(,1)
+table=0,priority=100,dl_type=0x8847,mpls_label=4 
actions=pop_mpls:0x0800,resubmit(,1)
 table=0,priority=10 actions=resubmit(,1)
 table=1,priority=10 actions=normal
 ])
@@ -1252,10 +1252,10 @@ AT_CHECK([ovs-vsctl add-port br0 patch0])
 AT_CHECK([ovs-vsctl add-port br1 patch1])
 
 AT_DATA([flows.txt], [dnl
-table=0,priority=100,dl_type=0x0800 
actions=push_mpls:0x8847,set_mpls_label:3,push_mpls:0x8847,set_mpls_label:2,push_mpls:0x8847,set_mpls_label:1,resubmit(,3)
+table=0,priority=100,dl_type=0x0800 
actions=push_mpls:0x8847,set_mpls_label:4,push_mpls:0x8847,set_mpls_label:2,push_mpls:0x8847,set_mpls_label:1,resubmit(,3)
 table=0,priority=100,dl_type=0x8847,mpls_label=1 
actions=pop_mpls:0x8847,resubmit(,1)
 table=1,priority=100,dl_type=0x8847,mpls_label=2 
actions=pop_mpls:0x8847,resubmit(,2)
-table=2,priority=100,dl_type=0x8847,mpls_label=3 
actions=pop_mpls:0x0800,resubmit(,3)
+table=2,priority=100,dl_type=0x8847,mpls_label=4 
actions=pop_mpls:0x0800,resubmit(,3)
 table=0,priority=10 actions=resubmit(,3)
 table=3,priority=10 actions=normal
 ])

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

[ovs-dev] [PATCH v8 02/15] tests: Include working system-traffic tests into the system-offloads-testsuite.

[Eelco Chaudron]

Include and run the system-traffic.at tests as part of the system offload
testsuite. Exclude all the tests that will not run without any special
modifications.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/automake.mk  |1 
 tests/system-offloads-testsuite.at |1 
 tests/system-offloads.at   |  106 
 3 files changed, 108 insertions(+)
 create mode 100644 tests/system-offloads.at

diff --git a/tests/automake.mk b/tests/automake.mk
index c8de3fe28..79224b135 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -185,6 +185,7 @@ SYSTEM_TESTSUITE_AT = \
 
 SYSTEM_OFFLOADS_TESTSUITE_AT = \
tests/system-common-macros.at \
+   tests/system-offloads.at \
tests/system-offloads-traffic.at \
tests/system-offloads-testsuite.at
 
diff --git a/tests/system-offloads-testsuite.at 
b/tests/system-offloads-testsuite.at
index eb5d2d4b3..a2dfcbc94 100644
--- a/tests/system-offloads-testsuite.at
+++ b/tests/system-offloads-testsuite.at
@@ -23,3 +23,4 @@ m4_include([tests/system-common-macros.at])
 m4_include([tests/system-kmod-macros.at])
 
 m4_include([tests/system-offloads-traffic.at])
+m4_include([tests/system-offloads.at])
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
new file mode 100644
index 0..f6dd931b7
--- /dev/null
+++ b/tests/system-offloads.at
@@ -0,0 +1,106 @@
+AT_COPYRIGHT([Copyright (c) 2022 Red Hat, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at:
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.])
+
+# The goal is to run as many as possible of the system-traffic tests with
+# OVS tc offload enabled. We do this by overriding the
+# OVS_TRAFFIC_VSWITCHD_START() with offloading enabled.
+m4_define([OVS_TRAFFIC_VSWITCHD_START],
+  [AT_CHECK([modprobe openvswitch])
+   on_exit 'modprobe -r openvswitch'
+   m4_foreach([mod], [[vport_geneve], [vport_gre], [vport_lisp], [vport_stt], 
[vport_vxlan]],
+  [modprobe -q mod || echo "Module mod not loaded."
+   on_exit 'modprobe -q -r mod'
+  ])
+   on_exit 'ovs-dpctl del-dp ovs-system'
+   on_exit 'ovs-appctl dpctl/flush-conntrack'
+   _OVS_VSWITCHD_START([], [-- set Open_vSwitch . other_config:hw-offload=true
+   $3])
+   dnl Add bridges, ports, etc.
+   AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| 
uuidfilt])], [0], [$2])
+])
+
+# The list below are tests that will not pass for a "test environment" specific
+# issue.
+m4_define([OVS_TEST_SKIP_LIST],
+[ovs_test_skip_list="
+datapath - mpls actions
+datapath - multiple mpls label pop
+datapath - basic truncate action
+datapath - truncate and output to gre tunnel by simulated packets
+datapath - truncate and output to gre tunnel
+conntrack - force commit
+conntrack - preserve registers
+conntrack - zones
+conntrack - zones from field
+conntrack - zones from other field
+conntrack - zones from other field, more tests
+conntrack - multiple zones
+conntrack - multiple namespaces, internal ports
+conntrack - ct_mark
+conntrack - ct_mark bit-fiddling
+conntrack - ct_mark from register
+conntrack - ct_label
+conntrack - ct_label bit-fiddling
+conntrack - ct metadata, multiple zones
+conntrack - ICMP related
+conntrack - ICMP related to original direction
+conntrack - IPv4 fragmentation + cvlan
+conntrack - IPv4 fragmentation with fragments specified
+conntrack - IPv6 fragmentation + cvlan
+conntrack - Fragmentation over vxlan
+conntrack - IPv6 Fragmentation over vxlan
+conntrack - zone-based timeout policy
+conntrack - IPv4 HTTP
+conntrack - IPv6 HTTP
+conntrack - multiple zones, local
+conntrack - multi-stage pipeline, local
+conntrack - FTP
+conntrack - FTP over IPv6
+conntrack - IPv6 FTP Passive
+conntrack - FTP with multiple expectations
+conntrack - TFTP
+conntrack - simple SNAT
+conntrack - SNAT with port range
+conntrack - SNAT with port range with exhaustion
+conntrack - more complex SNAT
+conntrack - all-zero IP SNAT
+conntrack - simple DNAT
+conntrack - DNAT with additional SNAT
+conntrack - more complex DNAT
+conntrack - ICMP related with NAT
+conntrack - FTP SNAT prerecirc
+conntrack - FTP SNAT prerecirc seqadj
+conntrack - FTP SNAT postrecirc
+conntrack - FTP SNAT postrecirc seqadj
+conntrack - FTP SNAT orig tuple
+conntrack - FTP SNAT orig tuple seqadj
+conntrack - IPv4 FTP Passive with SNAT
+conntrack - IPv4 FTP Passive with DNAT
+conntrack - IPv4 FTP Passive with DNAT 2
+conntrack - IPv4 FTP Active with DNAT
+conntrack - IPv4 FTP Active with DNAT with reverse skew
+conntrack - 

[ovs-dev] [PATCH v8 01/15] tests: Allow system-traffic tests to be skipped based on a list.

[Eelco Chaudron]

When the test description is part of the OVS_TEST_SKIP_LIST
variable, the test is skipped.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/ofproto-macros.at |5 -
 tests/ovs-macros.at |7 +++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
index 676d55aa9..5c033f771 100644
--- a/tests/ofproto-macros.at
+++ b/tests/ofproto-macros.at
@@ -161,7 +161,10 @@ m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m'])
 # before starting ovs-vswitchd.
 #
 m4_define([_OVS_VSWITCHD_START],
-  [dnl Create database.
+  [dnl Check if test needs to be run.
+   OVS_SKIP_TEST_IF_REQUESTED()
+
+   dnl Create database.
touch .conf.db.~lock~
AT_CHECK([ovsdb-tool create conf.db 
$abs_top_srcdir/vswitchd/vswitch.ovsschema])
 
diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at
index 39fbfceeb..f3eff5c05 100644
--- a/tests/ovs-macros.at
+++ b/tests/ovs-macros.at
@@ -371,3 +371,10 @@ dnl Add a rule to always accept the traffic.
 m4_define([IPTABLES_ACCEPT],
   [AT_CHECK([iptables -I INPUT 1 -i $1 -j ACCEPT])
on_exit 'iptables -D INPUT 1 -i $1'])
+
+# OVS_TEST_SKIP_LIST()
+m4_define([OVS_TEST_SKIP_LIST], [ echo ""])
+
+# OVS_SKIP_TEST_IF_REQUESTED()
+m4_define([OVS_SKIP_TEST_IF_REQUESTED],
+[AT_SKIP_IF([OVS_TEST_SKIP_LIST() | grep -qx "$at_desc"])])

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

Re: [ovs-dev] [PATCH v7 00/15] tests: Add system-traffic.at tests to check-offloads.

[Eelco Chaudron]

On 24 Jan 2023, at 10:07, Eelco Chaudron wrote:

> This series makes it possible to include system-traffic.at tests into
> "make check-offloads" tests.
>
> The last patch of the series explains which tests are still not passing
> and might need some more work.
>
> I'll try to work on the remaining failing test cases or find someone
> who can work on them.
>
> v7:
>   - Removed left over merge comment, and re-run all tests.
> v6:
>   - Added ACKs from v5
>   - Changed 'netdev-offload-tc: If the flow has not been used, report
> it as such.' to also work on hardware offloaded flows.
> v5:
>   - Include all patches, v4 went out with missing two patches :(
> v4:
>   - Fix rename from system-traffic.at to sym-traffic.at in patch 11
> v3:
>   - Fixed missing MACRO's in patches 4, 6 and 10.
> v2:
>   - Fix commit message on last patch
>   - Moved handling of system-traffic.at tests to a separate file
> system-offloads.at
>   - Re-based to the latest ovs master branch
>   - Added Roi's ACKs

Looks like I had to rebase my branch for this to apply cleanly :)

Will send out a v8 in a second. Did a branch refresh, and a full test run.

//Eelco

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

[ovs-dev] [PATCH v8 00/15] tests: Add system-traffic.at tests to check-offloads.

[Eelco Chaudron]

This series makes it possible to include system-traffic.at tests into
"make check-offloads" tests.

The last patch of the series explains which tests are still not passing
and might need some more work.

I'll try to work on the remaining failing test cases or find someone
who can work on them.

v8:
  - Re-based on top of latest OVS master.
v7:
  - Removed left over merge comment, and re-run all tests.
v6:
  - Added ACKs from v5
  - Changed 'netdev-offload-tc: If the flow has not been used, report
it as such.' to also work on hardware offloaded flows.
v5:
  - Include all patches, v4 went out with missing two patches :(
v4:
  - Fix rename from system-traffic.at to sym-traffic.at in patch 11
v3:
  - Fixed missing MACRO's in patches 4, 6 and 10.
v2:
  - Fix commit message on last patch
  - Moved handling of system-traffic.at tests to a separate file
system-offloads.at
  - Re-based to the latest ovs master branch
  - Added Roi's ACKs

Eelco Chaudron (15):
  tests: Allow system-traffic tests to be skipped based on a list.
  tests: Include working system-traffic tests into the 
system-offloads-testsuite.
  test: Do not use MPLS implicit null label in test cases.
  test: Add delay on revalidator flush for offload test cases.
  netdev-offload-tc: Fix tc conntrack force commit support.
  tests: Add delay to dump-conntrack for tc test cases.
  test: Fix "conntrack - floating IP" test for TC.
  test: Flush datapath when changing rules on the fly.
  netdev-offload-tc: Conntrack ALGs are not supported with tc.
  test: tc does not support conntrack timeout, skip the related test.
  test: Fix 'conntrack - Multiple ICMP traverse' for tc case.
  odp-util: Make odp_flow_key_from_flow__ nlattr order the same as the 
kernel.
  netdev-offload-tc: If the flow has not been used, report it as such.
  tests: Fix reading of OpenFlow byte counters in GRE test cases.
  tests: Comment currently failing TC system-traffic tests.


 Documentation/howto/tc-offload.rst |  11 ++
 lib/netdev-offload-tc.c|  17 +-
 lib/odp-util.c |  21 ++-
 lib/tc.c   |  14 +-
 tests/automake.mk  |   1 +
 tests/dpif-netdev.at   |  28 +--
 tests/mcast-snooping.at|   4 +-
 tests/nsh.at   |  10 +-
 tests/odp.at   |  84 -
 tests/ofproto-dpif.at  |  30 +--
 tests/ofproto-macros.at|   5 +-
 tests/ovs-macros.at|   7 +
 tests/packet-type-aware.at |  22 +--
 tests/pmd.at   |   2 +-
 tests/system-common-macros.at  |   7 +
 tests/system-offloads-testsuite.at |   1 +
 tests/system-offloads.at   | 110 +++
 tests/system-traffic.at| 283 +++--
 tests/tunnel-push-pop-ipv6.at  |   2 +-
 tests/tunnel-push-pop.at   |   2 +-
 tests/tunnel.at|   2 +-
 21 files changed, 415 insertions(+), 248 deletions(-)
 create mode 100644 tests/system-offloads.at

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

Re: [ovs-dev] [PATCH] netdev-dpdk: config tx queue number to be the minimum between device and ovs request

[Ilya Maximets]

On 1/24/23 12:40, clevia...@marvell.com wrote:
> From: Chava Leviatan 
> 
> Tx queue are set each time port is added/removed or the cmask changes by
> reconfigure_datapath. The amount of TX queues is set according to PMD
> thread and does not take into consideration the device capabilities .
> As a result , when transmitting packet from OVS to device driver by
> rte_eth_tx_burst , the device driver is called with
> dev->data->tx_queues[queue_id] which is not a legal address , hence
> can cause a crash . This patch suggests to set the tx queue as a minimum
> between the device capability and the PMD thread number as requested
> by reconfigure_datapath.
> The patch was tested on Marvell setup include DPU running OVS-dpdk ,
> and Marvell DPDK Octeontx2 driver. DPU is bind to a host through PCI .
> Traffic is sent from associated host (VF) and remote host .
> 
> Fixes: 050c60bfb5b4 ("netdev-dpdk: Use ->reconfigure() call to change rx/tx 
> queues.")
> 
> Reviewed-by: Liron Himi lir...@marvell.com
> Signed-off-by: Chava Leviatan 
> ---
>  lib/netdev-dpdk.c | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

This seems identical to the previous submission.  I'm not sure
why it was sent twice.

Please, reply to comments for the previous submission:
  https://mail.openvswitch.org/pipermail/ovs-dev/2023-January/401418.html

Marking this one as 'Changes Requested' for now.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] netdev-dpdk: config tx queue number to be the minimum between device and ovs request

[cleviatan]

From: Chava Leviatan 

Tx queue are set each time port is added/removed or the cmask changes by
reconfigure_datapath. The amount of TX queues is set according to PMD
thread and does not take into consideration the device capabilities .
As a result , when transmitting packet from OVS to device driver by
rte_eth_tx_burst , the device driver is called with
dev->data->tx_queues[queue_id] which is not a legal address , hence
can cause a crash . This patch suggests to set the tx queue as a minimum
between the device capability and the PMD thread number as requested
by reconfigure_datapath.
The patch was tested on Marvell setup include DPU running OVS-dpdk ,
and Marvell DPDK Octeontx2 driver. DPU is bind to a host through PCI .
Traffic is sent from associated host (VF) and remote host .

Fixes: 050c60bfb5b4 ("netdev-dpdk: Use ->reconfigure() call to change rx/tx 
queues.")

Reviewed-by: Liron Himi lir...@marvell.com
Signed-off-by: Chava Leviatan 
---
 lib/netdev-dpdk.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index ab5b8223e..278da12d4 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -508,6 +508,7 @@ struct netdev_dpdk {
 int requested_n_rxq;
 int requested_rxq_size;
 int requested_txq_size;
+int device_n_txq;
 
 /* Number of rx/tx descriptors for physical devices */
 int rxq_size;
@@ -1124,6 +1125,7 @@ dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int 
n_rxq, int n_txq)
 
 dev->up.n_rxq = n_rxq;
 dev->up.n_txq = n_txq;
+dev->device_n_txq = n_txq;
 
 return 0;
 }
@@ -2133,15 +2135,20 @@ netdev_dpdk_get_numa_id(const struct netdev *netdev)
 static int
 netdev_dpdk_set_tx_multiq(struct netdev *netdev, unsigned int n_txq)
 {
+unsigned int req_n_txq;
 struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
 
 ovs_mutex_lock(>mutex);
 
-if (dev->requested_n_txq == n_txq) {
+req_n_txq = MIN(n_txq, dev->device_n_txq);
+if (dev->requested_n_txq == req_n_txq) {
 goto out;
 }
-
-dev->requested_n_txq = n_txq;
+if (!dev->device_n_txq) {
+   dev->device_n_txq = 1;
+   req_n_txq = 1;
+}
+dev->requested_n_txq = req_n_txq;
 netdev_request_reconfigure(netdev);
 
 out:
-- 
2.17.1

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

[ovs-dev] [PATCH ovn 1/2] controller: Store load balancer data in separate node

[Ales Musil]

In order to reuse parsed data keep hash map
of 'struct ovn_controller_lb', that is local for the
controller, in separate engine node called 'lb_data'.
Those data will be later on used to determine if we need
to flush CT for changfed/removed LB backends.

Reported-at: https://bugzilla.redhat.com/1839103
Signed-off-by: Ales Musil 
---
 controller/lflow.c  | 244 ++-
 controller/lflow.h  |  14 +-
 controller/local_data.c |  35 +++
 controller/local_data.h |   3 +
 controller/ovn-controller.c | 458 +++-
 lib/lb.c|  31 +++
 lib/lb.h|   9 +
 7 files changed, 552 insertions(+), 242 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index 4b1cfe318..1982df626 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -98,10 +98,7 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
   struct lflow_ctx_out *l_ctx_out);
 
 static void
-consider_lb_hairpin_flows(struct objdep_mgr *mgr,
-  const struct sbrec_load_balancer *sbrec_lb,
-  const struct hmap *local_datapaths,
-  const struct smap *template_vars,
+consider_lb_hairpin_flows(const struct ovn_controller_lb *lb,
   bool use_ct_mark,
   struct ovn_desired_flow_table *flow_table,
   struct simap *ids);
@@ -801,43 +798,6 @@ lflow_handle_changed_ref(enum objdep_type type, const char 
*res_name,
 return true;
 }
 
-bool
-lb_handle_changed_ref(enum objdep_type type, const char *res_name,
-  struct ovs_list *objs_todo,
-  const void *in_arg, void *out_arg)
-{
-struct lflow_ctx_in *l_ctx_in = CONST_CAST(struct lflow_ctx_in *, in_arg);
-struct lflow_ctx_out *l_ctx_out = out_arg;
-
-struct object_to_resources_list_node *resource_lb_uuid;
-LIST_FOR_EACH_POP (resource_lb_uuid, list_node, objs_todo) {
-VLOG_DBG("Reprocess LB "UUID_FMT" for resource type: %s, name: %s",
- UUID_ARGS(_lb_uuid->obj_uuid),
- objdep_type_name(type), res_name);
-
-const struct sbrec_load_balancer *lb =
-sbrec_load_balancer_table_get_for_uuid(
-l_ctx_in->lb_table, _lb_uuid->obj_uuid);
-if (!lb) {
-VLOG_DBG("Failed to find LB "UUID_FMT" referred by: %s",
- UUID_ARGS(_lb_uuid->obj_uuid), res_name);
-} else {
-ofctrl_remove_flows(l_ctx_out->flow_table,
-_lb_uuid->obj_uuid);
-
-consider_lb_hairpin_flows(l_ctx_out->lb_deps_mgr, lb,
-  l_ctx_in->local_datapaths,
-  l_ctx_in->template_vars,
-  l_ctx_in->lb_hairpin_use_ct_mark,
-  l_ctx_out->flow_table,
-  l_ctx_out->hairpin_lb_ids);
-}
-
-free(resource_lb_uuid);
-}
-return true;
-}
-
 static void
 lflow_parse_ctrl_meter(const struct sbrec_logical_flow *lflow,
struct ovn_extend_table *meter_table,
@@ -1646,10 +1606,9 @@ add_lb_vip_hairpin_reply_action(struct in6_addr *vip6, 
ovs_be32 vip,
  * original destination tuple stored by ovn-northd.
  */
 static void
-add_lb_vip_hairpin_flows(struct ovn_controller_lb *lb,
+add_lb_vip_hairpin_flows(const struct ovn_controller_lb *lb,
  struct ovn_lb_vip *lb_vip,
  struct ovn_lb_backend *lb_backend,
- uint8_t lb_proto,
  bool use_ct_mark,
  struct ovn_desired_flow_table *flow_table)
 {
@@ -1688,7 +1647,7 @@ add_lb_vip_hairpin_flows(struct ovn_controller_lb *lb,
   ntohl(vip4));
 }
 
-add_lb_vip_hairpin_reply_action(NULL, snat_vip4, lb_proto,
+add_lb_vip_hairpin_reply_action(NULL, snat_vip4, lb->proto,
 lb_backend->port,
 lb->slb->header_.uuid.parts[0],
 );
@@ -1713,17 +1672,17 @@ add_lb_vip_hairpin_flows(struct ovn_controller_lb *lb,
 ntoh128(vip6_value));
 }
 
-add_lb_vip_hairpin_reply_action(snat_vip6, 0, lb_proto,
+add_lb_vip_hairpin_reply_action(snat_vip6, 0, lb->proto,
 lb_backend->port,
 lb->slb->header_.uuid.parts[0],
 );
 }
 
 if (lb_backend->port) {
-match_set_nw_proto(_match, lb_proto);
+match_set_nw_proto(_match, lb->proto);
 match_set_tp_dst(_match, htons(lb_backend->port));
 if (!lb->hairpin_orig_tuple) {
-match_set_ct_nw_proto(_match, 

[ovs-dev] [PATCH ovn 2/2] controller: Flush CT for removed LB backends

[Ales Musil]

Remove CT for LB backends that are affected by any
LB change. This way there shouldn't be any stale entry
in the CT. This feature is enabled by default but is dependent
on OvS support for CT flush via OFP extension.

Reported-at: https://bugzilla.redhat.com/1839103
Signed-off-by: Ales Musil 
---
 controller/ofctrl.c |  43 
 controller/ofctrl.h |   1 +
 controller/ovn-controller.c |  61 +++-
 include/ovn/features.h  |   2 +
 lib/features.c  |   5 +
 lib/lb.c|  70 +
 lib/lb.h|  19 
 tests/ovn.at|  65 
 tests/system-ovn.at | 190 
 9 files changed, 455 insertions(+), 1 deletion(-)

diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 6c631ea41..d922f3206 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -30,6 +30,7 @@
 #include "openvswitch/match.h"
 #include "openvswitch/ofp-actions.h"
 #include "openvswitch/ofp-bundle.h"
+#include "openvswitch/ofp-ct.h"
 #include "openvswitch/ofp-flow.h"
 #include "openvswitch/ofp-group.h"
 #include "openvswitch/ofp-match.h"
@@ -43,6 +44,7 @@
 #include "ovn-controller.h"
 #include "ovn/actions.h"
 #include "lib/extend-table.h"
+#include "lib/lb.h"
 #include "openvswitch/poll-loop.h"
 #include "physical.h"
 #include "openvswitch/rconn.h"
@@ -2485,6 +2487,38 @@ update_installed_flows_by_track(struct 
ovn_desired_flow_table *flow_table,
 }
 }
 
+static void
+add_ct_flush_tuple(const struct ovn_lb_five_tuple *tuple,
+   struct ovs_list *msgs)
+{
+if (VLOG_IS_DBG_ENABLED()) {
+struct ds ds = DS_EMPTY_INITIALIZER;
+
+ds_put_cstr(, "Flushing CT for 5-tuple: vip=");
+ipv6_format_mapped(>vip_ip, );
+ds_put_format(, ":%"PRIu16", backend=", tuple->vip_port);
+ipv6_format_mapped(>backend_ip, );
+ds_put_format(, ":%"PRIu16", protocol=%"PRIu8,
+  tuple->backend_port, tuple->proto);
+VLOG_DBG("%s", ds_cstr());
+
+ds_destroy();
+}
+
+struct ofp_ct_match match = {0};
+
+match.ip_proto = tuple->proto;
+
+match.tuple_orig.dst = tuple->vip_ip;
+match.tuple_orig.dst_port = htons(tuple->vip_port);
+
+match.tuple_reply.src = tuple->backend_ip;
+match.tuple_reply.src_port = htons(tuple->backend_port);
+
+struct ofpbuf *msg = ofp_ct_match_encode(, NULL, OFP15_VERSION);
+ovs_list_push_back(msgs, >list_node);
+}
+
 bool
 ofctrl_has_backlog(void)
 {
@@ -2524,6 +2558,7 @@ void
 ofctrl_put(struct ovn_desired_flow_table *lflow_table,
struct ovn_desired_flow_table *pflow_table,
struct shash *pending_ct_zones,
+   struct hmap *pending_lb_tuples,
const struct sbrec_meter_table *meter_table,
uint64_t req_cfg,
bool lflows_changed,
@@ -2754,6 +2789,14 @@ ofctrl_put(struct ovn_desired_flow_table *lflow_table,
 /* Sync the contents of meters->desired to meters->existing. */
 ovn_extend_table_sync(meters);
 
+if (ovs_feature_is_supported(OVS_CT_TUPLE_FLUSH_SUPPORT)) {
+struct ovn_lb_five_tuple *tuple;
+HMAP_FOR_EACH_POP (tuple, hmap_node, pending_lb_tuples) {
+add_ct_flush_tuple(tuple, );
+free(tuple);
+}
+}
+
 if (!ovs_list_is_empty()) {
 /* Add a barrier to the list of messages. */
 struct ofpbuf *barrier = ofputil_encode_barrier_request(OFP15_VERSION);
diff --git a/controller/ofctrl.h b/controller/ofctrl.h
index 396824512..f5751e3ee 100644
--- a/controller/ofctrl.h
+++ b/controller/ofctrl.h
@@ -58,6 +58,7 @@ enum mf_field_id ofctrl_get_mf_field_id(void);
 void ofctrl_put(struct ovn_desired_flow_table *lflow_table,
 struct ovn_desired_flow_table *pflow_table,
 struct shash *pending_ct_zones,
+struct hmap *pending_lb_tuples,
 const struct sbrec_meter_table *,
 uint64_t nb_cfg,
 bool lflow_changed,
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index b3170973a..ec373215b 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -2660,6 +2660,8 @@ load_balancers_by_dp_cleanup(struct hmap *lbs)
 struct ed_type_lb_data {
 /* Locally installed 'struct ovn_controller_lb' by UUID. */
 struct hmap local_lbs;
+/* 'struct ovn_lb_five_tuple' removed during last run. */
+struct hmap removed_tuples;
 /* Load balancer <-> resource cross reference */
 struct objdep_mgr deps_mgr;
 /* Objects processed in the current engine execution.
@@ -2684,6 +2686,48 @@ struct lb_data_ctx_in {
 const struct smap *template_vars;
 };
 
+static void
+lb_data_removed_five_tuples_add(struct ed_type_lb_data *lb_data,
+const struct ovn_controller_lb *lb)
+{
+if (!ovs_feature_is_supported(OVS_CT_TUPLE_FLUSH_SUPPORT)) {
+return;
+}
+
+for (size_t i 

Re: [ovs-dev] [PATCH ovn v6] northd: Refactor build_lrouter_nat_flows_for_lb function

[Ales Musil]

On Tue, Jan 24, 2023 at 12:28 PM Ales Musil  wrote:

> To make it easier to add flows to this stage, refactor the function.
> This also has the benefit that we should see fewer allocations due to
> rearranging how we create flows and how we manipulate the match string.
>
> Signed-off-by: Ales Musil 
> ---
> v5: Rebase on top of current main.
> Add missing ");" if ct_lb_related feature is not supported.
> v6: Rebase on top of current main.
> Fix wrong enclosing for reject action and add missing test case.
> ---
>  northd/northd.c | 483 
>  tests/ovn-northd.at |  69 ++-
>  2 files changed, 280 insertions(+), 272 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 0944a7b56..4b0d37375 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3878,10 +3878,13 @@ static bool
>  build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
>   struct ovn_northd_lb_vip *lb_vip_nb,
>   struct ds *action, char *selection_fields,
> - bool ls_dp, bool ct_lb_mark)
> + struct ds *skip_snat_action, struct ds
> *force_snat_action,
> + bool ls_dp, const struct chassis_features *features)
>  {
> -const char *ct_lb_action = ct_lb_mark ? "ct_lb_mark" : "ct_lb";
> -bool skip_hash_fields = false, reject = false;
> +const char *ct_lb_action =
> +features->ct_no_masked_label ? "ct_lb_mark" : "ct_lb";
> +bool reject = !lb_vip->n_backends && lb_vip->empty_backend_rej;
> +bool drop = false;
>
>  if (lb_vip_nb->lb_health_check) {
>  ds_put_format(action, "%s(backends=", ct_lb_action);
> @@ -3902,23 +3905,12 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
>  ds_put_format(action, "%s:%"PRIu16",",
>backend->ip_str, backend->port);
>  }
> +ds_chomp(action, ',');
>
> -if (!n_active_backends) {
> -if (!lb_vip->empty_backend_rej) {
> -ds_clear(action);
> -ds_put_cstr(action, debug_drop_action());
> -skip_hash_fields = true;
> -} else {
> -reject = true;
> -}
> -} else {
> -ds_chomp(action, ',');
> -ds_put_cstr(action, ");");
> -}
> -} else if (lb_vip->empty_backend_rej && !lb_vip->n_backends) {
> -reject = true;
> +drop = !n_active_backends && !lb_vip->empty_backend_rej;
> +reject = !n_active_backends && lb_vip->empty_backend_rej;
>  } else {
> -ds_put_format(action, "%s(backends=%s);", ct_lb_action,
> +ds_put_format(action, "%s(backends=%s", ct_lb_action,
>lb_vip_nb->backend_ips);
>  }
>
> @@ -3927,12 +3919,29 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
>: ovn_stage_get_table(S_ROUTER_OUT_SNAT);
>  ds_clear(action);
>  ds_put_format(action, "reg0 = 0; reject { outport <-> inport; "
> -  "next(pipeline=egress,table=%d);};", stage);
> -} else if (!skip_hash_fields && selection_fields &&
> selection_fields[0]) {
> -ds_chomp(action, ';');
> -ds_chomp(action, ')');
> -ds_put_format(action, "; hash_fields=\"%s\");", selection_fields);
> +  "next(pipeline=egress,table=%d);};", stage);
> +} else if (drop) {
> +ds_clear(action);
> +ds_put_cstr(action, debug_drop_action());
> +} else if (selection_fields && selection_fields[0]) {
> +ds_put_format(action, "; hash_fields=\"%s\"", selection_fields);
> +}
> +
> +bool is_lb_action = !(reject || drop);
> +const char *enclose = is_lb_action ? ");" : "";
> +
> +if (!ls_dp) {
> +bool flag_supported = is_lb_action && features->ct_lb_related;
> +ds_put_format(skip_snat_action, "flags.skip_snat_for_lb = 1;
> %s%s",
> +  ds_cstr(action),
> +  flag_supported ? "; skip_snat);" : enclose);
> +ds_put_format(force_snat_action, "flags.force_snat_for_lb = 1;
> %s%s",
> +  ds_cstr(action),
> +  flag_supported ? "; force_snat);" : enclose);
>  }
> +
> +ds_put_cstr(action, enclose);
> +
>  return reject;
>  }
>
> @@ -7502,9 +7511,9 @@ build_lb_affinity_default_flows(struct ovn_datapath
> *od, struct hmap *lflows)
>  }
>
>  static void
> -build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb, bool
> ct_lb_mark,
> -   struct ds *match, struct ds *action,
> -   const struct shash *meter_groups)
> +build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb,
> +   const struct chassis_features *features, struct ds *match,
> +   struct ds *action, const struct shash *meter_groups)
>  {
>  for (size_t i = 0; i < lb->n_vips; i++) {
>  struct ovn_lb_vip *lb_vip = >vips[i];
> @@ -7527,8 

[ovs-dev] [PATCH ovn v6] northd: Refactor build_lrouter_nat_flows_for_lb function

[Ales Musil]

To make it easier to add flows to this stage, refactor the function.
This also has the benefit that we should see fewer allocations due to
rearranging how we create flows and how we manipulate the match string.

Signed-off-by: Ales Musil 
---
v5: Rebase on top of current main.
Add missing ");" if ct_lb_related feature is not supported.
v6: Rebase on top of current main.
Fix wrong enclosing for reject action and add missing test case.
---
 northd/northd.c | 483 
 tests/ovn-northd.at |  69 ++-
 2 files changed, 280 insertions(+), 272 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 0944a7b56..4b0d37375 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3878,10 +3878,13 @@ static bool
 build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
  struct ovn_northd_lb_vip *lb_vip_nb,
  struct ds *action, char *selection_fields,
- bool ls_dp, bool ct_lb_mark)
+ struct ds *skip_snat_action, struct ds *force_snat_action,
+ bool ls_dp, const struct chassis_features *features)
 {
-const char *ct_lb_action = ct_lb_mark ? "ct_lb_mark" : "ct_lb";
-bool skip_hash_fields = false, reject = false;
+const char *ct_lb_action =
+features->ct_no_masked_label ? "ct_lb_mark" : "ct_lb";
+bool reject = !lb_vip->n_backends && lb_vip->empty_backend_rej;
+bool drop = false;
 
 if (lb_vip_nb->lb_health_check) {
 ds_put_format(action, "%s(backends=", ct_lb_action);
@@ -3902,23 +3905,12 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
 ds_put_format(action, "%s:%"PRIu16",",
   backend->ip_str, backend->port);
 }
+ds_chomp(action, ',');
 
-if (!n_active_backends) {
-if (!lb_vip->empty_backend_rej) {
-ds_clear(action);
-ds_put_cstr(action, debug_drop_action());
-skip_hash_fields = true;
-} else {
-reject = true;
-}
-} else {
-ds_chomp(action, ',');
-ds_put_cstr(action, ");");
-}
-} else if (lb_vip->empty_backend_rej && !lb_vip->n_backends) {
-reject = true;
+drop = !n_active_backends && !lb_vip->empty_backend_rej;
+reject = !n_active_backends && lb_vip->empty_backend_rej;
 } else {
-ds_put_format(action, "%s(backends=%s);", ct_lb_action,
+ds_put_format(action, "%s(backends=%s", ct_lb_action,
   lb_vip_nb->backend_ips);
 }
 
@@ -3927,12 +3919,29 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip,
   : ovn_stage_get_table(S_ROUTER_OUT_SNAT);
 ds_clear(action);
 ds_put_format(action, "reg0 = 0; reject { outport <-> inport; "
-  "next(pipeline=egress,table=%d);};", stage);
-} else if (!skip_hash_fields && selection_fields && selection_fields[0]) {
-ds_chomp(action, ';');
-ds_chomp(action, ')');
-ds_put_format(action, "; hash_fields=\"%s\");", selection_fields);
+  "next(pipeline=egress,table=%d);};", stage);
+} else if (drop) {
+ds_clear(action);
+ds_put_cstr(action, debug_drop_action());
+} else if (selection_fields && selection_fields[0]) {
+ds_put_format(action, "; hash_fields=\"%s\"", selection_fields);
+}
+
+bool is_lb_action = !(reject || drop);
+const char *enclose = is_lb_action ? ");" : "";
+
+if (!ls_dp) {
+bool flag_supported = is_lb_action && features->ct_lb_related;
+ds_put_format(skip_snat_action, "flags.skip_snat_for_lb = 1; %s%s",
+  ds_cstr(action),
+  flag_supported ? "; skip_snat);" : enclose);
+ds_put_format(force_snat_action, "flags.force_snat_for_lb = 1; %s%s",
+  ds_cstr(action),
+  flag_supported ? "; force_snat);" : enclose);
 }
+
+ds_put_cstr(action, enclose);
+
 return reject;
 }
 
@@ -7502,9 +7511,9 @@ build_lb_affinity_default_flows(struct ovn_datapath *od, 
struct hmap *lflows)
 }
 
 static void
-build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb, bool ct_lb_mark,
-   struct ds *match, struct ds *action,
-   const struct shash *meter_groups)
+build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb,
+   const struct chassis_features *features, struct ds *match,
+   struct ds *action, const struct shash *meter_groups)
 {
 for (size_t i = 0; i < lb->n_vips; i++) {
 struct ovn_lb_vip *lb_vip = >vips[i];
@@ -7527,8 +7536,8 @@ build_lb_rules(struct hmap *lflows, struct ovn_northd_lb 
*lb, bool ct_lb_mark,
 /* New connections in Ingress table. */
 const char *meter = NULL;
 bool reject = build_lb_vip_actions(lb_vip, lb_vip_nb, action,
-  

Re: [ovs-dev] [PATCH v2] ofproto-dpif-xlate: Remove repeated function for judge garp

[Ilya Maximets]

On 1/20/23 10:49, Simon Horman wrote:
> Hi Han Ding,
> 
> On Thu, Jan 05, 2023 at 02:15:03PM +0800, Han Ding wrote:
>> Function is_gratuitous_arp() and function is_garp() are all used to judge
>> whether the flow is gratuitous arp. It is not necessary to use two functions
>> to do the same thing and just keep one.
> 
> I agree it is nice to consolidate things.
> 
> But, curiously, the logic of is_garp() and is_gratuitous_arp()
> are somewhat different. I wonder about the risk of regressions
> when consolidating them.

Yeah, I had the same concern for v1.

> 
>> Acked-by: Ilya Maximets

I didn't ACK this patch.  Please, don't add tags not explicitly given.

>> Signed-off-by: Han Ding 
>> ---
>>  lib/flow.h   |  3 ++-
>>  ofproto/ofproto-dpif-xlate.c | 32 ++--
>>  2 files changed, 4 insertions(+), 31 deletions(-)
>>
>> diff --git a/lib/flow.h b/lib/flow.h
>> index c647ad83c..ade64b52d 100644
>> --- a/lib/flow.h
>> +++ b/lib/flow.h
>> @@ -1132,7 +1132,8 @@ static inline bool is_arp(const struct flow *flow)
>>  static inline bool is_garp(const struct flow *flow,
>> struct flow_wildcards *wc)
>>  {
>> -if (is_arp(flow)) {
>> +if (is_arp(flow) &&
>> +eth_addr_is_broadcast(FLOW_WC_GET_AND_MASK_WC(flow, wc, dl_dst))) {
> 
> Are any users relying on is_garp() not enforcing the
> eth_addr_is_broadcast() check?
> 
>>  return (FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_src) ==
>>  FLOW_WC_GET_AND_MASK_WC(flow, wc, nw_dst));
>>  }
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index a9cf3cbee..b3c13f6bf 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -2543,34 +2543,6 @@ output_normal(struct xlate_ctx *ctx, const struct 
>> xbundle *out_xbundle,
>>  memcpy(>xin->flow.vlans, _vlans, sizeof(old_vlans));
>>  }
>>
>> -/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
>> - * migration.  Older Citrix-patched Linux DomU used gratuitous ARP replies 
>> to
>> - * indicate this; newer upstream kernels use gratuitous ARP requests. */
>> -static bool
>> -is_gratuitous_arp(const struct flow *flow, struct flow_wildcards *wc)
>> -{
>> -if (flow->dl_type != htons(ETH_TYPE_ARP)) {
>> -return false;
>> -}
>> -
>> -memset(>masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
>> -if (!eth_addr_is_broadcast(flow->dl_dst)) {
>> -return false;
>> -}
>> -
>> -memset(>masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>> -if (flow->nw_proto == ARP_OP_REPLY) {
>> -return true;
> 
> This behaviour, which I assume correlates to the Citrix portion
> of the comment above, does not seem to be present in is_garp().
> 
> Is that a problem?
> 
>> -} else if (flow->nw_proto == ARP_OP_REQUEST) {
>> -memset(>masks.nw_src, 0xff, sizeof wc->masks.nw_src);
>> -memset(>masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
>> -
>> -return flow->nw_src == flow->nw_dst;
>> -} else {
> 
> This behaviour does not seem to be present in is_garp().
> 
> Is that a problem?

I did some research that might answer or maybe clarify the questions above.
Namely reading ARP-related RFCs - 826 and 5227.  From these I carried
following:

 - Gratuitous ARP / ARP Announcement messages are generally link-level
   broadcast messages.  I didn't find any mentions of unicast GARP.
   This means that checking for a broadcast should generally be fine.

 - Gratuitous ARP messages carry the same IP in sender and target fields.

 - The opcode in the ARP header mostly doesn't matter.  Its only purpose
   is to tell the receiver that it has to generate a reply if the opcode
   is 'request'.  RFC 826 makes it clear that the opcode should be the
   absolutely last thing that is checked in the packet, and this is done
   after the sender information is already put into the table, i.e after
   the whole processing is done.  So, checking the opcode should be
   unnecessary and can even be incorrect in our case.

 - It's typical for ARP replies to be unicast.  However, broadcast replies
   are not prohibited and can be used in some network setups, e.g. for
   faster collision detection.
   Saying that, it seems that is_gratuitous_arp() function is not really
   correct, as it assumes that all broadcast ARP replies are gratuitous.
   However, I don't know how Citrix-patched Linux DomU ARP replies looked
   like.  But I tend to assume that they did have the same IP as sender
   and a target, so this check should be sufficient?  i.e. as long as we
   are comparing IPs without looking at the opcode, we should be fine.

Looking into this, the patch might be OK.  We might not even check for
an address to be broadcast as in v1, but I'm not sure about this.
Checking for a broadcast may save us from unwildcarding too many fields.

Thoughts?  Some fact-checking of above would be great.

Best regards, Ilya Maximets.

[ovs-dev] [PATCH 2/2] dpif-netdev-perf: Add metric averages when no iterations.

[Kevin Traynor]

pmd-perf-show with pmd-perf-metrics=true displays a histogram
with averages. However, averages were not displayed when there
is no iterations.

They will be all zero so it is not hiding useful information
but the stats look incomplete without them, especially when
they are displayed for some PMD thread cores and not others.

The histogram print is large and this is just an extra couple
of lines, so might as well print them all the time to ensure
that the user does not think there is something missing from
the display.

Before patch:
Histograms
   cycles/it
   499   0
   716   0
   1025  0
   1469  0


After patch:
Histograms
   cycles/it
   499   0
   716   0
   1025  0
   1469  0

---
   cycles/it
   0

Signed-off-by: Kevin Traynor 
---
 lib/dpif-netdev-perf.c | 49 ++
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/lib/dpif-netdev-perf.c b/lib/dpif-netdev-perf.c
index a552948ac..79ea5e3be 100644
--- a/lib/dpif-netdev-perf.c
+++ b/lib/dpif-netdev-perf.c
@@ -338,27 +338,30 @@ pmd_perf_format_histograms(struct ds *str, struct 
pmd_perf_stats *s)
   ">", s->upcalls.bin[i],
   ">", s->cycles_per_upcall.bin[i]);
-if (s->totals.iterations > 0) {
-ds_put_cstr(str,
-"-"
-"-"
-"\n");
-ds_put_format(str,
-  "   %-21s  %-21s  %-21s  %-21s  %-21s  %-21s  %-21s\n",
-  "cycles/it", "packets/it", "cycles/pkt", "pkts/batch",
-  "vhost qlen", "upcalls/it", "cycles/upcall");
-ds_put_format(str,
-  "   %-21"PRIu64"  %-21.5f  %-21"PRIu64
-  "  %-21.5f  %-21.5f  %-21.5f  %-21"PRIu32"\n",
-  s->totals.cycles / s->totals.iterations,
-  1.0 * s->totals.pkts / s->totals.iterations,
-  s->totals.pkts
-  ? s->totals.busy_cycles / s->totals.pkts : 0,
-  s->totals.batches
-  ? 1.0 * s->totals.pkts / s->totals.batches : 0,
-  1.0 * s->totals.max_vhost_qfill / s->totals.iterations,
-  1.0 * s->totals.upcalls / s->totals.iterations,
-  s->totals.upcalls
-  ? s->totals.upcall_cycles / s->totals.upcalls : 0);
-}
+ds_put_cstr(str,
+"-"
+"-"
+"\n");
+ds_put_format(str,
+  "   %-21s  %-21s  %-21s  %-21s  %-21s  %-21s  %-21s\n",
+  "cycles/it", "packets/it", "cycles/pkt", "pkts/batch",
+  "vhost qlen", "upcalls/it", "cycles/upcall");
+ds_put_format(str,
+  "   %-21"PRIu64"  %-21.5f  %-21"PRIu64
+  "  %-21.5f  %-21.5f  %-21.5f  %-21"PRIu32"\n",
+  s->totals.iterations
+  ? s->totals.cycles / s->totals.iterations : 0,
+  s->totals.iterations
+  ? 1.0 * s->totals.pkts / s->totals.iterations : 0,
+  s->totals.pkts
+  ? s->totals.busy_cycles / s->totals.pkts : 0,
+  s->totals.batches
+  ? 1.0 * s->totals.pkts / s->totals.batches : 0,
+  s->totals.iterations
+  ? 1.0 * s->totals.max_vhost_qfill / s->totals.iterations
+  : 0,
+  s->totals.iterations
+  ? 1.0 * s->totals.upcalls / s->totals.iterations : 0,
+  s->totals.upcalls
+  ? s->totals.upcall_cycles / s->totals.upcalls : 0);
 }
 
-- 
2.39.1

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

[ovs-dev] [PATCH 1/2] dpif-netdev-perf: Remove not a number stat value.

[Kevin Traynor]

Some stats in pmd-perf-show don't check for divide by zero
which results in not a number (-nan).

This is a normal case for some of the stats when there are
no Rx queues assigned to the PMD thread core.

It is not obvious what -nan is to a user so add a check for
divide by zero and set stat to 0 if present.

Before patch:
pmd thread numa_id 1 core_id 9:

  Iterations:0  (-nan us/it)
  - Used TSC cycles: 0  (  0.0 % of total cycles)
  - idle iterations: 0  ( -nan % of used cycles)
  - busy iterations: 0  ( -nan % of used cycles)

After patch:
pmd thread numa_id 1 core_id 9:

  Iterations:0  (0.00 us/it)
  - Used TSC cycles: 0  (  0.0 % of total cycles)
  - idle iterations: 0  (  0.0 % of used cycles)
  - busy iterations: 0  (  0.0 % of used cycles)

Signed-off-by: Kevin Traynor 
---
 lib/dpif-netdev-perf.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/dpif-netdev-perf.c b/lib/dpif-netdev-perf.c
index 1a7bab04c..a552948ac 100644
--- a/lib/dpif-netdev-perf.c
+++ b/lib/dpif-netdev-perf.c
@@ -242,10 +242,12 @@ pmd_perf_format_overall_stats(struct ds *str, struct 
pmd_perf_stats *s,
 "  Sleep time (us):%12.0f  (%3.0f us/iteration avg.)\n",
 tot_iter,
-(tot_cycles + tot_sleep_cycles) * us_per_cycle / tot_iter,
+tot_iter
+? (tot_cycles + tot_sleep_cycles) * us_per_cycle / tot_iter
+: 0,
 tot_cycles, 100.0 * (tot_cycles / duration) / tsc_hz,
 idle_iter,
-100.0 * stats[PMD_CYCLES_ITER_IDLE] / tot_cycles,
+tot_cycles ? 100.0 * stats[PMD_CYCLES_ITER_IDLE] / tot_cycles : 0,
 busy_iter,
-100.0 * stats[PMD_CYCLES_ITER_BUSY] / tot_cycles,
+tot_cycles ? 100.0 * stats[PMD_CYCLES_ITER_BUSY] / tot_cycles : 0,
 sleep_iter, tot_iter ? 100.0 * sleep_iter / tot_iter : 0,
 tot_sleep_cycles * us_per_cycle,
-- 
2.39.1

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

Re: [ovs-dev] [PATCH v7 15/15] tests: Comment currently failing TC system-traffic tests.

[0-day Robot]

Bleep bloop.  Greetings Eelco Chaudron, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


Patch skipped due to previous failure.

Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 14/15] tests: Fix reading of OpenFlow byte counters in GRE test cases.

[0-day Robot]

Bleep bloop.  Greetings Eelco Chaudron, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


Patch skipped due to previous failure.

Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 13/15] netdev-offload-tc: If the flow has not been used, report it as such.

[0-day Robot]

Bleep bloop.  Greetings Eelco Chaudron, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


Patch skipped due to previous failure.

Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 12/15] odp-util: Make odp_flow_key_from_flow__ nlattr order the same as the kernel.

[0-day Robot]

Bleep bloop.  Greetings Eelco Chaudron, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


Patch skipped due to previous failure.

Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 11/15] test: Fix 'conntrack - Multiple ICMP traverse' for tc case.

[0-day Robot]

Bleep bloop.  Greetings Eelco Chaudron, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


Patch skipped due to previous failure.

Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 10/15] test: tc does not support conntrack timeout, skip the related test.

[0-day Robot]

Bleep bloop.  Greetings Eelco Chaudron, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


Patch skipped due to previous failure.

Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 09/15] netdev-offload-tc: Conntrack ALGs are not supported with tc.

[0-day Robot]

Bleep bloop.  Greetings Eelco Chaudron, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


Patch skipped due to previous failure.

Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 07/15] test: Fix "conntrack - floating IP" test for TC.

[0-day Robot]

Bleep bloop.  Greetings Eelco Chaudron, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


Patch skipped due to previous failure.

Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 08/15] test: Flush datapath when changing rules on the fly.

[0-day Robot]

Bleep bloop.  Greetings Eelco Chaudron, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


Patch skipped due to previous failure.

Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v7 06/15] tests: Add delay to dump-conntrack for tc test cases.

[0-day Robot]

Bleep bloop.  Greetings Eelco Chaudron, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 tests: Add delay to dump-conntrack for tc test cases.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Patch skipped due to previous failure.

Please check this out.  If you feel there has been an error, please email 
acon...@redhat.com

Thanks,
0-day Robot
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v7 14/15] tests: Fix reading of OpenFlow byte counters in GRE test cases.

[Eelco Chaudron]

With some datapaths, read TC, it takes a bit longer to update the
OpenFlow statistics. Rather than adding an additional delay, try
to read the counters multiple times until we get the desired value.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |2 --
 tests/system-traffic.at  |   15 ++-
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index f4adac7b3..18e542aea 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -60,8 +60,6 @@ m4_define([CHECK_CONNTRACK_TIMEOUT],
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
-datapath - truncate and output to gre tunnel by simulated packets
-datapath - truncate and output to gre tunnel
 conntrack - multiple namespaces, internal ports
 conntrack - ct metadata, multiple zones
 conntrack - ICMP related
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 57ff83b51..ae21dace9 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1660,9 +1660,8 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=2" | 
sed -n 's/.*\(n\_bytes=[
 n_bytes=242
 ])
 dnl After truncation = outer ETH(14) + outer IP(20) + GRE(4) + 100 = 138B
-AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
-n_bytes=138
-])
+OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed 
-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p' | grep "n_bytes=138"],
+   [ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed 
-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'])
 
 dnl check tunnel pop path, from at_ns0 to at_ns1
 dnl This 200-byte packet is simulated on behalf of ns_gre0
@@ -1697,9 +1696,8 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=2" | 
sed -n 's/.*\(n\_bytes=[
 n_bytes=242
 ])
 dnl After truncation = outer ETH(14) + outer IP(20) + GRE(4) + 100 = 138B
-AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
-n_bytes=138
-])
+OVS_WAIT_UNTIL([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed 
-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p' | grep "n_bytes=138"],
+   [ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed 
-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'])
 
 dnl check tunnel pop path, from at_ns0 to at_ns1
 dnl This 200-byte packet is simulated on behalf of ns_gre0
@@ -1707,9 +1705,8 @@ ovs-ofctl -O OpenFlow13 packet-out br-underlay "in_port=1 
packet=02908ca8a149faa
 
 dnl After truncation = 100 byte at loopback device p2(4)
 OVS_REVALIDATOR_PURGE()
-AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip], [0], [dnl
- n_packets=1, n_bytes=100, priority=1,ip,in_port=4 actions=drop
-])
+OVS_WAIT_UNTIL([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip | 
grep "n_packets=1, n_bytes=100, priority=1,ip,in_port=4 actions=drop"],
+   [ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip])
 
 OVS_TRAFFIC_VSWITCHD_STOP
 AT_CLEANUP

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

[ovs-dev] [PATCH v7 15/15] tests: Comment currently failing TC system-traffic tests.

[Eelco Chaudron]

The goal was to run 200 successful tc tests in a row. To do this the
following was run:

  for i in {1..200}; do make check-offloads || break; \
echo "ALL_200_OK: $i"; done;

Unfortunately, a bunch of test cases showed occasional failures.
For now, they are excluded from the test cases and need further
investigation. They are:

  802.1ad - vlan_limit
  conntrack - DNAT load balancing
  conntrack - DNAT load balancing with NC
  conntrack - ICMP related
  conntrack - ICMP related to original direction
  conntrack - ICMP related with NAT
  conntrack - IPv4 fragmentation with fragments specified
  conntrack - multiple namespaces, internal ports
  conntrack - zones from other field
  conntrack - zones from other field, more tests
  datapath - basic truncate action
  datapath - multiple mpls label pop
  datapath - truncate and output to gre tunnel
  datapath - truncate and output to gre tunnel by simulated packets

Some other test cases also fail due to what looks like problems
in the tc kernel conntrack implementation. For details see the
details in the system-offloads.at exclusion list definition.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |   43 +--
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 18e542aea..edffdd73b 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -60,20 +60,51 @@ m4_define([CHECK_CONNTRACK_TIMEOUT],
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
+# TC does not support moving ports to a different namespace than vswitchd's
+# namespace, so we need to disable this test.
 conntrack - multiple namespaces, internal ports
+
+# When moving through different zones, it can take up to ~8 seconds before
+# the conntrack state gets updated causing these tests to fail.
 conntrack - ct metadata, multiple zones
-conntrack - ICMP related
-conntrack - ICMP related to original direction
+conntrack - multiple zones, local
+conntrack - multi-stage pipeline, local
+
+# The kernel's tcf_ct_act() function does not seem to take care of any (QinQ)
+# VLAN headers causing commits to fail. However, if this is solved, we have to
+# make sure conntrack does not break the VLAN boundary, i.e., putting together
+# two packets with different CVLAN+SVLAN values.
 conntrack - IPv4 fragmentation + cvlan
-conntrack - IPv4 fragmentation with fragments specified
 conntrack - IPv6 fragmentation + cvlan
+
+# Fragmentation handling in ct zone 9 does not seem to work correctly.
+# When moving this test over to the default zone all works fine.
 conntrack - Fragmentation over vxlan
 conntrack - IPv6 Fragmentation over vxlan
-conntrack - multiple zones, local
-conntrack - multi-stage pipeline, local
+
+# Occasionaly we fail on the 'execute ct(commit) failed (Invalid argument) on
+# packet...' log message being present
+conntrack - zones from other field
+conntrack - zones from other field, more tests
+conntrack - multiple namespaces, internal ports
+conntrack - IPv4 fragmentation with fragments specified
+
+# Occasionaly we fail on the 'failed to flow_get/flow_del (No such file or 
directory)
+# ufid:..' log message being present.
+datapath - multiple mpls label pop
+datapath - basic truncate action
+conntrack - ICMP related
+conntrack - ICMP related to original direction
 conntrack - ICMP related with NAT
 conntrack - DNAT load balancing
-conntrack - DNAT load balancing with NC"
+conntrack - DNAT load balancing with NC
+802.1ad - vlan_limit
+
+# Occasionalt we fail with extreme high byte counters, i.e.
+# n_bytes=18446744073705804134
+datapath - truncate and output to gre tunnel by simulated packets
+datapath - truncate and output to gre tunnel
+"
 echo "$ovs_test_skip_list" | sed "s// /g"])
 
 m4_include([tests/system-traffic.at])

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

[ovs-dev] [PATCH v7 13/15] netdev-offload-tc: If the flow has not been used, report it as such.

[Eelco Chaudron]

If a tc flow was installed but has not yet been used, report it as such.

In addition, add a delay to the "IGMP - flood under normal action" test
case to make it work with many repetitions. This delay is also present
in other ICMP/IGMP tests.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 lib/tc.c |   14 +-
 tests/system-offloads.at |3 +--
 tests/system-traffic.at  |2 ++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/lib/tc.c b/lib/tc.c
index a66dc432f..78b2fa797 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -1361,7 +1361,19 @@ get_user_hz(void)
 static void
 nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower)
 {
-uint64_t lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
+uint64_t lastused;
+
+/* On creation both tm->install and tm->lastuse are set to jiffies
+ * by the kernel. So if both values are the same, the flow has not been
+ * used yet.
+ *
+ * Note that tm->firstuse can not be used due to some kernel bug, i.e.,
+ * hardware offloaded flows do not update tm->firstuse. */
+if (tm->lastuse == tm->install) {
+lastused = 0;
+} else {
+lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz());
+}
 
 if (flower->lastused < lastused) {
 flower->lastused = lastused;
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 57be710a7..f4adac7b3 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -75,8 +75,7 @@ conntrack - multiple zones, local
 conntrack - multi-stage pipeline, local
 conntrack - ICMP related with NAT
 conntrack - DNAT load balancing
-conntrack - DNAT load balancing with NC
-IGMP - flood under normal action"
+conntrack - DNAT load balancing with NC"
 echo "$ovs_test_skip_list" | sed "s// /g"])
 
 m4_include([tests/system-traffic.at])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 07913a192..57ff83b51 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -7002,6 +7002,8 @@ f0 00 00 01 01 01 08 00 46 c0 00 28 00 00 40 00 01 02 d3 
49 45 65 eb 4a e0 dnl
 00 00 16 94 04 00 00 22 00 f9 02 00 00 00 01 04 00 00 00 e0 00 00 fb 00 00 dnl
 00 00 00 00 > /dev/null])
 
+sleep 1
+
 AT_CHECK([ovs-appctl dpctl/dump-flows --names | grep -e .*ipv4 | sort | dnl
   strip_stats | strip_used | strip_recirc | dnl
   sed 's/,packet_type(ns=[[0-9]]*,id=[[0-9]]*),/,/'],

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

[ovs-dev] [PATCH v7 12/15] odp-util: Make odp_flow_key_from_flow__ nlattr order the same as the kernel.

[Eelco Chaudron]

Make the order of the Netlink attributes for odp_flow_key_from_flow__()
the same as the kernel will return them.

This will make sure the attributes displayed in the dpctl/dump-flows
output appear in the same order for all datapath.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 lib/odp-util.c|   21 +-
 tests/dpif-netdev.at  |   28 +++---
 tests/mcast-snooping.at   |4 +-
 tests/nsh.at  |   10 ++---
 tests/odp.at  |   83 +++--
 tests/ofproto-dpif.at |   30 +++
 tests/packet-type-aware.at|   22 +--
 tests/pmd.at  |2 -
 tests/system-offloads.at  |1 
 tests/tunnel-push-pop-ipv6.at |2 -
 tests/tunnel-push-pop.at  |2 -
 tests/tunnel.at   |2 -
 12 files changed, 101 insertions(+), 106 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 72e076e1c..9baca12d8 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -6197,6 +6197,11 @@ odp_flow_key_from_flow__(const struct odp_flow_key_parms 
*parms,
 const struct flow *mask = parms->mask;
 const struct flow *data = export_mask ? mask : flow;
 
+if (parms->support.recirc) {
+nl_msg_put_u32(buf, OVS_KEY_ATTR_RECIRC_ID, data->recirc_id);
+nl_msg_put_u32(buf, OVS_KEY_ATTR_DP_HASH, data->dp_hash);
+}
+
 nl_msg_put_u32(buf, OVS_KEY_ATTR_PRIORITY, data->skb_priority);
 
 if (flow_tnl_dst_is_set(>tunnel) ||
@@ -6205,6 +6210,12 @@ odp_flow_key_from_flow__(const struct odp_flow_key_parms 
*parms,
 parms->key_buf, NULL);
 }
 
+/* Add an ingress port attribute if this is a mask or 'in_port.odp_port'
+ * is not the magical value "ODPP_NONE". */
+if (export_mask || flow->in_port.odp_port != ODPP_NONE) {
+nl_msg_put_odp_port(buf, OVS_KEY_ATTR_IN_PORT, data->in_port.odp_port);
+}
+
 nl_msg_put_u32(buf, OVS_KEY_ATTR_SKB_MARK, data->pkt_mark);
 
 if (parms->support.ct_state) {
@@ -6248,16 +6259,6 @@ odp_flow_key_from_flow__(const struct odp_flow_key_parms 
*parms,
 ct->ipv6_proto = data->ct_nw_proto;
 }
 }
-if (parms->support.recirc) {
-nl_msg_put_u32(buf, OVS_KEY_ATTR_RECIRC_ID, data->recirc_id);
-nl_msg_put_u32(buf, OVS_KEY_ATTR_DP_HASH, data->dp_hash);
-}
-
-/* Add an ingress port attribute if this is a mask or 'in_port.odp_port'
- * is not the magical value "ODPP_NONE". */
-if (export_mask || flow->in_port.odp_port != ODPP_NONE) {
-nl_msg_put_odp_port(buf, OVS_KEY_ATTR_IN_PORT, data->in_port.odp_port);
-}
 
 nl_msg_put_be32(buf, OVS_KEY_ATTR_PACKET_TYPE, data->packet_type);
 
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index 6aff1eda7..04c82c109 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -72,13 +72,13 @@ ovs-appctl time/warp 5000
 AT_CHECK([ovs-appctl netdev-dummy/receive p1 
'in_port(1),eth(src=50:54:00:00:00:01,dst=50:54:00:00:02:00),eth_type(0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)'])
OVS_WAIT_UNTIL([grep "miss upcall" ovs-vswitchd.log])
AT_CHECK([grep -A 1 'miss upcall' ovs-vswitchd.log | tail -n 1], [0], [dnl
-skb_priority(0),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),recirc_id(0),dp_hash(0),in_port(1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:01,dst=50:54:00:00:02:00),eth_type(0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)
+recirc_id(0),dp_hash(0),skb_priority(0),in_port(1),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:01,dst=50:54:00:00:02:00),eth_type(0x0800),ipv4(src=10.0.0.1,dst=10.0.0.2,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)
 ])
 
 AT_CHECK([ovs-appctl netdev-dummy/receive p1 
'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:06:00),eth_type(0x0800),ipv4(src=10.0.0.5,dst=10.0.0.6,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)'
 --len 1024])
OVS_WAIT_UNTIL([test `grep -c "miss upcall" ovs-vswitchd.log` -ge 2])
AT_CHECK([grep -A 1 'miss upcall' ovs-vswitchd.log | tail -n 1], [0], [dnl
-skb_priority(0),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),recirc_id(0),dp_hash(0),in_port(1),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:06:00),eth_type(0x0800),ipv4(src=10.0.0.5,dst=10.0.0.6,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)
+recirc_id(0),dp_hash(0),skb_priority(0),in_port(1),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),packet_type(ns=0,id=0),eth(src=50:54:00:00:00:05,dst=50:54:00:00:06:00),eth_type(0x0800),ipv4(src=10.0.0.5,dst=10.0.0.6,proto=6,tos=0,ttl=64,frag=no),tcp(src=8,dst=9),tcp_flags(ack)
 ])
 OVS_VSWITCHD_STOP
 AT_CLEANUP
@@ -139,7 +139,7 @@ m4_define([DPIF_NETDEV_MISS_FLOW_INSTALL],
 
OVS_WAIT_UNTIL([grep "miss upcall" 

[ovs-dev] [PATCH v7 11/15] test: Fix 'conntrack - Multiple ICMP traverse' for tc case.

[Eelco Chaudron]

tc does not include ethernet header length in packet byte count.
This fix will allow the packets that go trough tc to be 14 bytes less.

This difference in the TC implementation is already described in
tc-offload.rst.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |1 -
 tests/system-traffic.at  |2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index a7076c2c2..cc45f662a 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -76,7 +76,6 @@ conntrack - multi-stage pipeline, local
 conntrack - ICMP related with NAT
 conntrack - DNAT load balancing
 conntrack - DNAT load balancing with NC
-conntrack - Multiple ICMP traverse
 conntrack - can match and clear ct_state from outside OVS
 IGMP - flood under normal action"
 echo "$ovs_test_skip_list" | sed "s// /g"])
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 980b0bd70..07913a192 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -6929,7 +6929,7 @@ AT_CHECK([DPCTL_DUMP_CONNTRACK | FORMAT_CT(10.1.1)], [0], 
[dnl
 
icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.1,id=,type=0,code=0)
 ])
 
-AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | 
OFPROTO_CLEAR_DURATION_IDLE],
+AT_CHECK([ovs-ofctl dump-flows br0 | grep table=2, | 
OFPROTO_CLEAR_DURATION_IDLE | sed 's/n_bytes=70,/n_bytes=84,/'],
  [0], [dnl
  cookie=0x0, duration=, table=2, n_packets=2, n_bytes=84, 
idle_age=, priority=10,ct_state=+new+trk,in_port=1 actions=drop
  cookie=0x0, duration=, table=2, n_packets=0, n_bytes=0, 
idle_age=, priority=10,ct_state=+est+trk actions=drop

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

[ovs-dev] [PATCH v7 10/15] test: tc does not support conntrack timeout, skip the related test.

[Eelco Chaudron]

The tc conntrack implementation does not support the timeout option.
The current implementation is silently ignoring the timeout option
by adding a general conntrack entry.

This patch will skip the related test by overriding the support macro.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index c4ed3a171..a7076c2c2 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -49,6 +49,13 @@ m4_define([CHECK_CONNTRACK_ALG],
 ])
 
 
+# Conntrack timeout not supported for tc.
+m4_define([CHECK_CONNTRACK_TIMEOUT],
+[
+ AT_SKIP_IF([:])
+])
+
+
 # The list below are tests that will not pass for a "test environment" specific
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
@@ -64,7 +71,6 @@ conntrack - IPv4 fragmentation with fragments specified
 conntrack - IPv6 fragmentation + cvlan
 conntrack - Fragmentation over vxlan
 conntrack - IPv6 Fragmentation over vxlan
-conntrack - zone-based timeout policy
 conntrack - multiple zones, local
 conntrack - multi-stage pipeline, local
 conntrack - ICMP related with NAT

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

[ovs-dev] [PATCH v7 09/15] netdev-offload-tc: Conntrack ALGs are not supported with tc.

[Eelco Chaudron]

tc does not support conntrack ALGs. Even worse, with tc enabled, they
should not be used/configured at all. This is because even though TC
will ignore the rules with ALG configured, i.e., they will flow through
the kernel module, return traffic might flow through a tc conntrack
rule, and it will not invoke the ALG helper.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 Documentation/howto/tc-offload.rst |   11 +++
 lib/netdev-offload-tc.c|4 
 tests/system-offloads.at   |   27 +++
 3 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/Documentation/howto/tc-offload.rst 
b/Documentation/howto/tc-offload.rst
index f6482c8af..63687adc9 100644
--- a/Documentation/howto/tc-offload.rst
+++ b/Documentation/howto/tc-offload.rst
@@ -112,3 +112,14 @@ First flow packet not processed by meter
 Packets that are received by ovs-vswitchd through an upcall before the actual
 meter flow is installed, are not passing TC police action and therefore are
 not considered for policing.
+
+Conntrack Application Layer Gateways(ALG)
++
+
+TC does not support conntrack helpers, i.e., ALGs. TC will not offload flows if
+the ALG keyword is present within the ct() action. However, this will not allow
+ALGs to work within the datapath, as the return traffic without the ALG keyword
+might run through a TC rule, which internally will not call the conntrack
+helper required.
+
+So if ALG support is required, tc offload must be disabled.
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 915c45ed3..ba309c2b6 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1357,6 +1357,10 @@ parse_put_flow_ct_action(struct tc_flower *flower,
 action->ct.label_mask = ct_label->mask;
 }
 break;
+/* The following option we do not support in tc-ct, and should
+ * not be ignored for proper operation. */
+case OVS_CT_ATTR_HELPER:
+return EOPNOTSUPP;
 }
 }
 
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 9d1e80c8d..c4ed3a171 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -42,6 +42,13 @@ m4_define([OVS_REVALIDATOR_PURGE],
 m4_define([DPCTL_DUMP_CONNTRACK], [sleep 3; ovs-appctl dpctl/dump-conntrack])
 
 
+# Conntrack ALGs are not supported for tc.
+m4_define([CHECK_CONNTRACK_ALG],
+[
+ AT_SKIP_IF([:])
+])
+
+
 # The list below are tests that will not pass for a "test environment" specific
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
@@ -60,27 +67,7 @@ conntrack - IPv6 Fragmentation over vxlan
 conntrack - zone-based timeout policy
 conntrack - multiple zones, local
 conntrack - multi-stage pipeline, local
-conntrack - FTP
-conntrack - FTP over IPv6
-conntrack - IPv6 FTP Passive
-conntrack - FTP with multiple expectations
-conntrack - TFTP
 conntrack - ICMP related with NAT
-conntrack - FTP SNAT prerecirc
-conntrack - FTP SNAT prerecirc seqadj
-conntrack - FTP SNAT postrecirc
-conntrack - FTP SNAT postrecirc seqadj
-conntrack - FTP SNAT orig tuple
-conntrack - FTP SNAT orig tuple seqadj
-conntrack - IPv4 FTP Passive with SNAT
-conntrack - IPv4 FTP Passive with DNAT
-conntrack - IPv4 FTP Passive with DNAT 2
-conntrack - IPv4 FTP Active with DNAT
-conntrack - IPv4 FTP Active with DNAT with reverse skew
-conntrack - IPv6 FTP with SNAT
-conntrack - IPv6 FTP Passive with SNAT
-conntrack - IPv6 FTP with SNAT - orig tuple
-conntrack - IPv4 TFTP with SNAT
 conntrack - DNAT load balancing
 conntrack - DNAT load balancing with NC
 conntrack - Multiple ICMP traverse

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

[ovs-dev] [PATCH v7 08/15] test: Flush datapath when changing rules on the fly.

[Eelco Chaudron]

Flush datapath flows as TC flows take some more time to be flushed out.
The flush speeds this up.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |2 --
 tests/system-traffic.at  |6 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 593dc1c7a..9d1e80c8d 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -48,8 +48,6 @@ m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
-conntrack - zones from other field
-conntrack - zones from other field, more tests
 conntrack - multiple namespaces, internal ports
 conntrack - ct metadata, multiple zones
 conntrack - ICMP related
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 2f6d8f13f..980b0bd70 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2694,6 +2694,9 @@ AT_CHECK([ovs-appctl dpctl/dump-flows --names 
filter=in_port=ovs-p0 dnl
 AT_CHECK([ovs-ofctl mod-flows br0 dnl
 'priority=100,ct_state=-trk,tcp,in_port="ovs-p0" 
actions=ct(table=0,zone=15)'])
 
+dnl Force flush flows as some datapaths (read TC) might take time to clear.
+AT_CHECK([ovs-appctl dpctl/del-flows])
+
 NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o 
wget0.log])
 
 AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p0 dnl
@@ -2742,6 +2745,9 @@ AT_CHECK([ovs-appctl dpctl/dump-flows --names 
filter=in_port=ovs-p0 dnl
 
 AT_CHECK([ovs-ofctl mod-flows br0 
'priority=100,ct_state=-trk,tcp,in_port="ovs-p0" 
actions=ct(table=0,zone=15,commit,exec(load:0x000f->NXM_NX_CT_LABEL[[0..31]]))'])
 
+dnl Force flush flows as some datapaths (read TC) might take time to clear.
+AT_CHECK([ovs-appctl dpctl/del-flows])
+
 NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o 
wget0.log])
 
 AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p0 dnl

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

[ovs-dev] [PATCH v7 07/15] test: Fix "conntrack - floating IP" test for TC.

[Eelco Chaudron]

This change fixes the "conntrack - floating" test for the TC
offload case. In this scenario, the connection might move to
CLOSE_WAIT, which would fail the test as it only accepts
TIME_WAIT. However, both indicate the connection was
established, so the test should pass.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |1 -
 tests/system-traffic.at  |   13 +++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 1aca41825..593dc1c7a 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -85,7 +85,6 @@ conntrack - IPv6 FTP with SNAT - orig tuple
 conntrack - IPv4 TFTP with SNAT
 conntrack - DNAT load balancing
 conntrack - DNAT load balancing with NC
-conntrack - floating IP
 conntrack - Multiple ICMP traverse
 conntrack - can match and clear ct_state from outside OVS
 IGMP - flood under normal action"
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 48545f57d..2f6d8f13f 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -6808,16 +6808,17 @@ AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
 
 dnl non-FIP case
 NS_CHECK_EXEC([at_ns1], [echo "foobar" |nc $NC_EOF_OPT 10.1.1.1 1234])
-OVS_WAIT_UNTIL([[ovs-appctl dpctl/dump-conntrack | sed -e 
's/port=[0-9]*/port=/g' -e 's/id=[0-9]*/id=/g' |
+OVS_WAIT_UNTIL([[ovs-appctl dpctl/dump-conntrack | sed -e 
's/port=[0-9]*/port=/g' -e 's/id=[0-9]*/id=/g' -e 
's/CLOSE_WAIT\|CLOSING/TIME_WAIT/g' |
 grep 
"tcp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=,dport=),reply=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),protoinfo=(state=TIME_WAIT)"
 ]])
 
-dnl Check that the full session ends as expected (i.e. TIME_WAIT). Otherwise it
-dnl means the datapath didn't process the ct_clear action. Ending in SYN_RECV
-dnl (OVS maps to ESTABLISHED) means the initial frame was committed, but not a
-dnl second time after the FIP translation (because ct_clear didn't occur).
+dnl Check that the full session ends as expected (i.e. TIME_WAIT, CLOSE_WAIT).
+dnl Otherwise it means the datapath didn't process the ct_clear action. Ending
+dnl in SYN_RECV (OVS maps to ESTABLISHED) means the initial frame was
+dnl committed, but not a second time after the FIP translation (because
+dnl ct_clear didn't occur).
 NS_CHECK_EXEC([at_ns1], [echo "foobar" |nc $NC_EOF_OPT 10.254.254.1 1234])
-OVS_WAIT_UNTIL([[ovs-appctl dpctl/dump-conntrack | sed -e 
's/port=[0-9]*/port=/g' -e 's/id=[0-9]*/id=/g' |
+OVS_WAIT_UNTIL([[ovs-appctl dpctl/dump-conntrack | sed -e 
's/port=[0-9]*/port=/g' -e 's/id=[0-9]*/id=/g'  -e 
's/CLOSE_WAIT\|CLOSING/TIME_WAIT/g' |
 grep 
"tcp,orig=(src=10.254.254.2,dst=10.1.1.1,sport=,dport=),reply=(src=10.1.1.1,dst=10.254.254.2,sport=,dport=),protoinfo=(state=TIME_WAIT)"
 ]])
 

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

[ovs-dev] [PATCH v7 06/15] tests: Add delay to dump-conntrack for tc test cases.

[Eelco Chaudron]

This patch adds a delay before dumping the conntrack table because with
tc it takes a bit longer before it gets synced.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-common-macros.at |3 +
 tests/system-offloads.at  |   25 +
 tests/system-traffic.at   |  198 +
 3 files changed, 107 insertions(+), 119 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index d95d79791..32b9ca0de 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -347,3 +347,6 @@ m4_define([OVS_CHECK_CT_CLEAR],
 # OVS_REVALIDATOR_PURGE()
 m4_define([OVS_REVALIDATOR_PURGE],
 [AT_CHECK([ovs-appctl revalidator/purge], [0])])
+
+# DPCTL_DUMP_CONNTRACK()
+m4_define([DPCTL_DUMP_CONNTRACK], [ovs-appctl dpctl/dump-conntrack])
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index d39997708..1aca41825 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -37,24 +37,20 @@ m4_define([OVS_REVALIDATOR_PURGE],
 [AT_CHECK([sleep 2; ovs-appctl revalidator/purge], [0])])
 
 
+# We override the DPCTL_DUMP_CONNTRACK macro, allowing a bit more time for the
+# tc-datapath conntrack entries to be installed/updated.
+m4_define([DPCTL_DUMP_CONNTRACK], [sleep 3; ovs-appctl dpctl/dump-conntrack])
+
+
 # The list below are tests that will not pass for a "test environment" specific
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
-conntrack - preserve registers
-conntrack - zones
-conntrack - zones from field
 conntrack - zones from other field
 conntrack - zones from other field, more tests
-conntrack - multiple zones
 conntrack - multiple namespaces, internal ports
-conntrack - ct_mark
-conntrack - ct_mark bit-fiddling
-conntrack - ct_mark from register
-conntrack - ct_label
-conntrack - ct_label bit-fiddling
 conntrack - ct metadata, multiple zones
 conntrack - ICMP related
 conntrack - ICMP related to original direction
@@ -64,8 +60,6 @@ conntrack - IPv6 fragmentation + cvlan
 conntrack - Fragmentation over vxlan
 conntrack - IPv6 Fragmentation over vxlan
 conntrack - zone-based timeout policy
-conntrack - IPv4 HTTP
-conntrack - IPv6 HTTP
 conntrack - multiple zones, local
 conntrack - multi-stage pipeline, local
 conntrack - FTP
@@ -73,14 +67,6 @@ conntrack - FTP over IPv6
 conntrack - IPv6 FTP Passive
 conntrack - FTP with multiple expectations
 conntrack - TFTP
-conntrack - simple SNAT
-conntrack - SNAT with port range
-conntrack - SNAT with port range with exhaustion
-conntrack - more complex SNAT
-conntrack - all-zero IP SNAT
-conntrack - simple DNAT
-conntrack - DNAT with additional SNAT
-conntrack - more complex DNAT
 conntrack - ICMP related with NAT
 conntrack - FTP SNAT prerecirc
 conntrack - FTP SNAT prerecirc seqadj
@@ -93,7 +79,6 @@ conntrack - IPv4 FTP Passive with DNAT
 conntrack - IPv4 FTP Passive with DNAT 2
 conntrack - IPv4 FTP Active with DNAT
 conntrack - IPv4 FTP Active with DNAT with reverse skew
-conntrack - IPv6 HTTP with DNAT
 conntrack - IPv6 FTP with SNAT
 conntrack - IPv6 FTP Passive with SNAT
 conntrack - IPv6 FTP with SNAT - orig tuple
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 1d0d0dfd5..48545f57d 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2215,7 +2215,7 @@ 
udp,vlan_tci=0x,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10.
 dnl
 dnl Check that the directionality has been changed by force commit.
 dnl
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.2,"], 
[], [dnl
+AT_CHECK([DPCTL_DUMP_CONNTRACK | grep "orig=.src=10\.1\.1\.2,"], [], [dnl
 
udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2)
 ])
 
@@ -2223,7 +2223,7 @@ dnl OK, now send another packet from port 1 and see that 
it switches again
 AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 
packet=5054000a505400090800451c0011a4cd0a0101010a010102000100020008
 actions=resubmit(,0)"])
 OVS_REVALIDATOR_PURGE()
 
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.1,"], 
[], [dnl
+AT_CHECK([DPCTL_DUMP_CONNTRACK | grep "orig=.src=10\.1\.1\.1,"], [], [dnl
 
udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1)
 ])
 
@@ -2253,25 +2253,25 @@ AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
 dnl Test UDP from port 1
 AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 
packet=5054000a505400090800451c0011a4cd0a0101010a010102000100020008
 actions=resubmit(,0)"])
 
-AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.1,"], 
[], [dnl
+AT_CHECK([DPCTL_DUMP_CONNTRACK | grep "orig=.src=10\.1\.1\.1,"], [], [dnl
 

[ovs-dev] [PATCH v7 05/15] netdev-offload-tc: Fix tc conntrack force commit support.

[Eelco Chaudron]

tc was not setting the OVS_CT_ATTR_FORCE_COMMIT flag when a forced
commit was requested. This patch will fix this.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 lib/netdev-offload-tc.c  |   13 +++--
 tests/system-offloads.at |1 -
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index ce7f8ad97..915c45ed3 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -825,7 +825,11 @@ parse_tc_flower_to_actions__(struct tc_flower *flower, 
struct ofpbuf *buf,
 ct_offset = nl_msg_start_nested(buf, OVS_ACTION_ATTR_CT);
 
 if (action->ct.commit) {
-nl_msg_put_flag(buf, OVS_CT_ATTR_COMMIT);
+if (action->ct.force) {
+nl_msg_put_flag(buf, OVS_CT_ATTR_FORCE_COMMIT);
+} else {
+nl_msg_put_flag(buf, OVS_CT_ATTR_COMMIT);
+}
 }
 
 if (action->ct.zone) {
@@ -1309,7 +1313,12 @@ parse_put_flow_ct_action(struct tc_flower *flower,
 NL_ATTR_FOR_EACH_UNSAFE (ct_attr, ct_left, ct, ct_len) {
 switch (nl_attr_type(ct_attr)) {
 case OVS_CT_ATTR_COMMIT: {
-action->ct.commit = true;
+action->ct.commit = true;
+}
+break;
+case OVS_CT_ATTR_FORCE_COMMIT: {
+action->ct.commit = true;
+action->ct.force = true;
 }
 break;
 case OVS_CT_ATTR_ZONE: {
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index 7b6deccf0..d39997708 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -43,7 +43,6 @@ m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
-conntrack - force commit
 conntrack - preserve registers
 conntrack - zones
 conntrack - zones from field

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

[ovs-dev] [PATCH v7 04/15] test: Add delay on revalidator flush for offload test cases.

[Eelco Chaudron]

The revalidator/purge commands in the system test cases sometimes
get called immediately after a partial test is completed. This
could cause the revalidator thread to log an error that it can
not find/delete a flow due to the slower flow installation nature
of TC.

This patch uses a macro to call the revalidator/purge command,
which can be overwritten when the system tests are run on a tc
enabled datapath.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-common-macros.at |4 
 tests/system-offloads.at  |8 +++-
 tests/system-traffic.at   |   38 +++---
 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at
index 8b9f5c752..d95d79791 100644
--- a/tests/system-common-macros.at
+++ b/tests/system-common-macros.at
@@ -343,3 +343,7 @@ m4_define([OVS_CHECK_IPROUTE_ENCAP],
 # OVS_CHECK_CT_CLEAR()
 m4_define([OVS_CHECK_CT_CLEAR],
 [AT_SKIP_IF([! grep -q "Datapath supports ct_clear action" 
ovs-vswitchd.log])])
+
+# OVS_REVALIDATOR_PURGE()
+m4_define([OVS_REVALIDATOR_PURGE],
+[AT_CHECK([ovs-appctl revalidator/purge], [0])])
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index fbe1dc99a..7b6deccf0 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -30,11 +30,17 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| 
uuidfilt])], [0], [$2])
 ])
 
+
+# We override the OVS_REVALIDATOR_PURGE macro, allowing a bit more time for the
+# tc-datapath entries to be installed.
+m4_define([OVS_REVALIDATOR_PURGE],
+[AT_CHECK([sleep 2; ovs-appctl revalidator/purge], [0])])
+
+
 # The list below are tests that will not pass for a "test environment" specific
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
-datapath - basic truncate action
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
 conntrack - force commit
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index cd3ad0f68..1d0d0dfd5 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1517,12 +1517,12 @@ on_exit 'rm -f payload200.bin'
 NS_CHECK_EXEC([at_ns0], [nc $NC_EOF_OPT -u 10.1.1.2 1234 < payload200.bin])
 
 dnl packet with truncated size
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" |  sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=100
 ])
 dnl packet with original size
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=5" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=242
 ])
@@ -1539,7 +1539,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 NS_CHECK_EXEC([at_ns0], [nc $NC_EOF_OPT -u 10.1.1.2 1234 < payload200.bin])
 
 dnl 100 + 100 + 242 + min(65535,242) = 684
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=684
 ])
@@ -1569,7 +1569,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
 NS_CHECK_EXEC([at_ns0], [nc $NC_EOF_OPT -u 10.1.1.2 1234 < payload200.bin])
 
 dnl 100 + 100 + 242 + min(65535,242) = 684
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=684
 ])
@@ -1653,7 +1653,7 @@ AT_CHECK([ovs-ofctl add-flows br-underlay 
flows-underlay.txt])
 
 dnl check tunnel push path, from at_ns1 to at_ns0
 NS_CHECK_EXEC([at_ns1], [nc $NC_EOF_OPT -u 10.1.1.1 1234 < payload200.bin])
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 
 dnl Before truncation = ETH(14) + IP(20) + UDP(8) + 200 = 242B
 AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=2" | sed -n 
's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
@@ -1669,7 +1669,7 @@ dnl This 200-byte packet is simulated on behalf of ns_gre0
 ovs-ofctl -O OpenFlow13 packet-out br-underlay "in_port=1 
packet=02908ca8a149faadfa25056008004500010a9e9d4000402f4084ac1f0101ac1f01646558e666c122e666c111080045e46f8e40004011b4760a0101010a010102e026162e00d016e6a366ebf904c74132c6fed42a9e9e46240b4d9fd13c9b47d9704a388e70a5e77db16934a6188dc01d86aa20007ace2cf9cdb111f208474b88ffc851c871f0e3fb4fff138c1d288d437efff487e2b86a9c99fbf4229a6485e133bcf3e16f6e345207fda0932d9eeb602740456fd077b4847d25481337bd716155cc245be129ccc11bf82b834767b3760b52fe913c0e24f31c0e1b27f88acf7bba6b985fb64ee2cd6fc6bba1a9c1f021e253e1728b046fd4d023307e3296361a37ea2617ebcb2537e0284a81050dd0ee
 actions=LOCAL"
 
 dnl After truncation = 100 byte at loopback device p2(4)
-AT_CHECK([ovs-appctl revalidator/purge], [0])
+OVS_REVALIDATOR_PURGE()
 AT_CHECK([ovs-ofctl dump-flows br0 | grep 

[ovs-dev] [PATCH v7 03/15] test: Do not use MPLS implicit null label in test cases.

[Eelco Chaudron]

TC flower does not allow the push of the implicit null labels (RFC3032).
Avoid the use of such labels in the MPLS test cases.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/system-offloads.at |2 --
 tests/system-traffic.at  |8 
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/system-offloads.at b/tests/system-offloads.at
index f6dd931b7..fbe1dc99a 100644
--- a/tests/system-offloads.at
+++ b/tests/system-offloads.at
@@ -34,8 +34,6 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
 # issue.
 m4_define([OVS_TEST_SKIP_LIST],
 [ovs_test_skip_list="
-datapath - mpls actions
-datapath - multiple mpls label pop
 datapath - basic truncate action
 datapath - truncate and output to gre tunnel by simulated packets
 datapath - truncate and output to gre tunnel
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index e5403519f..cd3ad0f68 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1167,8 +1167,8 @@ AT_CHECK([ovs-vsctl add-port br0 patch0])
 AT_CHECK([ovs-vsctl add-port br1 patch1])
 
 AT_DATA([flows.txt], [dnl
-table=0,priority=100,dl_type=0x0800 
actions=push_mpls:0x8847,set_mpls_label:3,resubmit(,1)
-table=0,priority=100,dl_type=0x8847,mpls_label=3 
actions=pop_mpls:0x0800,resubmit(,1)
+table=0,priority=100,dl_type=0x0800 
actions=push_mpls:0x8847,set_mpls_label:4,resubmit(,1)
+table=0,priority=100,dl_type=0x8847,mpls_label=4 
actions=pop_mpls:0x0800,resubmit(,1)
 table=0,priority=10 actions=resubmit(,1)
 table=1,priority=10 actions=normal
 ])
@@ -1204,10 +1204,10 @@ AT_CHECK([ovs-vsctl add-port br0 patch0])
 AT_CHECK([ovs-vsctl add-port br1 patch1])
 
 AT_DATA([flows.txt], [dnl
-table=0,priority=100,dl_type=0x0800 
actions=push_mpls:0x8847,set_mpls_label:3,push_mpls:0x8847,set_mpls_label:2,push_mpls:0x8847,set_mpls_label:1,resubmit(,3)
+table=0,priority=100,dl_type=0x0800 
actions=push_mpls:0x8847,set_mpls_label:4,push_mpls:0x8847,set_mpls_label:2,push_mpls:0x8847,set_mpls_label:1,resubmit(,3)
 table=0,priority=100,dl_type=0x8847,mpls_label=1 
actions=pop_mpls:0x8847,resubmit(,1)
 table=1,priority=100,dl_type=0x8847,mpls_label=2 
actions=pop_mpls:0x8847,resubmit(,2)
-table=2,priority=100,dl_type=0x8847,mpls_label=3 
actions=pop_mpls:0x0800,resubmit(,3)
+table=2,priority=100,dl_type=0x8847,mpls_label=4 
actions=pop_mpls:0x0800,resubmit(,3)
 table=0,priority=10 actions=resubmit(,3)
 table=3,priority=10 actions=normal
 ])

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

[ovs-dev] [PATCH v7 02/15] tests: Include working system-traffic tests into the system-offloads-testsuite.

[Eelco Chaudron]

Include and run the system-traffic.at tests as part of the system offload
testsuite. Exclude all the tests that will not run without any special
modifications.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/automake.mk  |1 
 tests/system-offloads-testsuite.at |1 
 tests/system-offloads.at   |  106 
 3 files changed, 108 insertions(+)
 create mode 100644 tests/system-offloads.at

diff --git a/tests/automake.mk b/tests/automake.mk
index d509cf935..12435d2c1 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -183,6 +183,7 @@ SYSTEM_TESTSUITE_AT = \
 
 SYSTEM_OFFLOADS_TESTSUITE_AT = \
tests/system-common-macros.at \
+   tests/system-offloads.at \
tests/system-offloads-traffic.at \
tests/system-offloads-testsuite.at
 
diff --git a/tests/system-offloads-testsuite.at 
b/tests/system-offloads-testsuite.at
index eb5d2d4b3..a2dfcbc94 100644
--- a/tests/system-offloads-testsuite.at
+++ b/tests/system-offloads-testsuite.at
@@ -23,3 +23,4 @@ m4_include([tests/system-common-macros.at])
 m4_include([tests/system-kmod-macros.at])
 
 m4_include([tests/system-offloads-traffic.at])
+m4_include([tests/system-offloads.at])
diff --git a/tests/system-offloads.at b/tests/system-offloads.at
new file mode 100644
index 0..f6dd931b7
--- /dev/null
+++ b/tests/system-offloads.at
@@ -0,0 +1,106 @@
+AT_COPYRIGHT([Copyright (c) 2022 Red Hat, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at:
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.])
+
+# The goal is to run as many as possible of the system-traffic tests with
+# OVS tc offload enabled. We do this by overriding the
+# OVS_TRAFFIC_VSWITCHD_START() with offloading enabled.
+m4_define([OVS_TRAFFIC_VSWITCHD_START],
+  [AT_CHECK([modprobe openvswitch])
+   on_exit 'modprobe -r openvswitch'
+   m4_foreach([mod], [[vport_geneve], [vport_gre], [vport_lisp], [vport_stt], 
[vport_vxlan]],
+  [modprobe -q mod || echo "Module mod not loaded."
+   on_exit 'modprobe -q -r mod'
+  ])
+   on_exit 'ovs-dpctl del-dp ovs-system'
+   on_exit 'ovs-appctl dpctl/flush-conntrack'
+   _OVS_VSWITCHD_START([], [-- set Open_vSwitch . other_config:hw-offload=true
+   $3])
+   dnl Add bridges, ports, etc.
+   AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| 
uuidfilt])], [0], [$2])
+])
+
+# The list below are tests that will not pass for a "test environment" specific
+# issue.
+m4_define([OVS_TEST_SKIP_LIST],
+[ovs_test_skip_list="
+datapath - mpls actions
+datapath - multiple mpls label pop
+datapath - basic truncate action
+datapath - truncate and output to gre tunnel by simulated packets
+datapath - truncate and output to gre tunnel
+conntrack - force commit
+conntrack - preserve registers
+conntrack - zones
+conntrack - zones from field
+conntrack - zones from other field
+conntrack - zones from other field, more tests
+conntrack - multiple zones
+conntrack - multiple namespaces, internal ports
+conntrack - ct_mark
+conntrack - ct_mark bit-fiddling
+conntrack - ct_mark from register
+conntrack - ct_label
+conntrack - ct_label bit-fiddling
+conntrack - ct metadata, multiple zones
+conntrack - ICMP related
+conntrack - ICMP related to original direction
+conntrack - IPv4 fragmentation + cvlan
+conntrack - IPv4 fragmentation with fragments specified
+conntrack - IPv6 fragmentation + cvlan
+conntrack - Fragmentation over vxlan
+conntrack - IPv6 Fragmentation over vxlan
+conntrack - zone-based timeout policy
+conntrack - IPv4 HTTP
+conntrack - IPv6 HTTP
+conntrack - multiple zones, local
+conntrack - multi-stage pipeline, local
+conntrack - FTP
+conntrack - FTP over IPv6
+conntrack - IPv6 FTP Passive
+conntrack - FTP with multiple expectations
+conntrack - TFTP
+conntrack - simple SNAT
+conntrack - SNAT with port range
+conntrack - SNAT with port range with exhaustion
+conntrack - more complex SNAT
+conntrack - all-zero IP SNAT
+conntrack - simple DNAT
+conntrack - DNAT with additional SNAT
+conntrack - more complex DNAT
+conntrack - ICMP related with NAT
+conntrack - FTP SNAT prerecirc
+conntrack - FTP SNAT prerecirc seqadj
+conntrack - FTP SNAT postrecirc
+conntrack - FTP SNAT postrecirc seqadj
+conntrack - FTP SNAT orig tuple
+conntrack - FTP SNAT orig tuple seqadj
+conntrack - IPv4 FTP Passive with SNAT
+conntrack - IPv4 FTP Passive with DNAT
+conntrack - IPv4 FTP Passive with DNAT 2
+conntrack - IPv4 FTP Active with DNAT
+conntrack - IPv4 FTP Active with DNAT with reverse skew
+conntrack - 

[ovs-dev] [PATCH v7 01/15] tests: Allow system-traffic tests to be skipped based on a list.

[Eelco Chaudron]

When the test description is part of the OVS_TEST_SKIP_LIST
variable, the test is skipped.

Signed-off-by: Eelco Chaudron 
Acked-by: Roi Dayan 
---
 tests/ofproto-macros.at |5 -
 tests/ovs-macros.at |7 +++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
index 676d55aa9..5c033f771 100644
--- a/tests/ofproto-macros.at
+++ b/tests/ofproto-macros.at
@@ -161,7 +161,10 @@ m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m'])
 # before starting ovs-vswitchd.
 #
 m4_define([_OVS_VSWITCHD_START],
-  [dnl Create database.
+  [dnl Check if test needs to be run.
+   OVS_SKIP_TEST_IF_REQUESTED()
+
+   dnl Create database.
touch .conf.db.~lock~
AT_CHECK([ovsdb-tool create conf.db 
$abs_top_srcdir/vswitchd/vswitch.ovsschema])
 
diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at
index 39fbfceeb..f3eff5c05 100644
--- a/tests/ovs-macros.at
+++ b/tests/ovs-macros.at
@@ -371,3 +371,10 @@ dnl Add a rule to always accept the traffic.
 m4_define([IPTABLES_ACCEPT],
   [AT_CHECK([iptables -I INPUT 1 -i $1 -j ACCEPT])
on_exit 'iptables -D INPUT 1 -i $1'])
+
+# OVS_TEST_SKIP_LIST()
+m4_define([OVS_TEST_SKIP_LIST], [ echo ""])
+
+# OVS_SKIP_TEST_IF_REQUESTED()
+m4_define([OVS_SKIP_TEST_IF_REQUESTED],
+[AT_SKIP_IF([OVS_TEST_SKIP_LIST() | grep -qx "$at_desc"])])

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

[ovs-dev] [PATCH v7 00/15] tests: Add system-traffic.at tests to check-offloads.

[Eelco Chaudron]

This series makes it possible to include system-traffic.at tests into
"make check-offloads" tests.

The last patch of the series explains which tests are still not passing
and might need some more work.

I'll try to work on the remaining failing test cases or find someone
who can work on them.

v7:
  - Removed left over merge comment, and re-run all tests.
v6:
  - Added ACKs from v5
  - Changed 'netdev-offload-tc: If the flow has not been used, report
it as such.' to also work on hardware offloaded flows.
v5:
  - Include all patches, v4 went out with missing two patches :(
v4:
  - Fix rename from system-traffic.at to sym-traffic.at in patch 11
v3:
  - Fixed missing MACRO's in patches 4, 6 and 10.
v2:
  - Fix commit message on last patch
  - Moved handling of system-traffic.at tests to a separate file
system-offloads.at
  - Re-based to the latest ovs master branch
  - Added Roi's ACKs

Eelco Chaudron (15):
  tests: Allow system-traffic tests to be skipped based on a list.
  tests: Include working system-traffic tests into the 
system-offloads-testsuite.
  test: Do not use MPLS implicit null label in test cases.
  test: Add delay on revalidator flush for offload test cases.
  netdev-offload-tc: Fix tc conntrack force commit support.
  tests: Add delay to dump-conntrack for tc test cases.
  test: Fix "conntrack - floating IP" test for TC.
  test: Flush datapath when changing rules on the fly.
  netdev-offload-tc: Conntrack ALGs are not supported with tc.
  test: tc does not support conntrack timeout, skip the related test.
  test: Fix 'conntrack - Multiple ICMP traverse' for tc case.
  odp-util: Make odp_flow_key_from_flow__ nlattr order the same as the 
kernel.
  netdev-offload-tc: If the flow has not been used, report it as such.
  tests: Fix reading of OpenFlow byte counters in GRE test cases.
  tests: Comment currently failing TC system-traffic tests.


 Documentation/howto/tc-offload.rst |  11 ++
 lib/netdev-offload-tc.c|  17 +-
 lib/odp-util.c |  21 ++-
 lib/tc.c   |  14 +-
 tests/automake.mk  |   1 +
 tests/dpif-netdev.at   |  28 +--
 tests/mcast-snooping.at|   4 +-
 tests/nsh.at   |  10 +-
 tests/odp.at   |  83 -
 tests/ofproto-dpif.at  |  30 +--
 tests/ofproto-macros.at|   5 +-
 tests/ovs-macros.at|   7 +
 tests/packet-type-aware.at |  22 +--
 tests/pmd.at   |   2 +-
 tests/system-common-macros.at  |   7 +
 tests/system-offloads-testsuite.at |   1 +
 tests/system-offloads.at   | 110 +++
 tests/system-traffic.at| 282 +++--
 tests/tunnel-push-pop-ipv6.at  |   2 +-
 tests/tunnel-push-pop.at   |   2 +-
 tests/tunnel.at|   2 +-
 21 files changed, 414 insertions(+), 247 deletions(-)
 create mode 100644 tests/system-offloads.at

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

Re: [ovs-dev] [PATCH v6 09/15] netdev-offload-tc: Conntrack ALGs are not supported with tc.

[Eelco Chaudron]

On 24 Jan 2023, at 7:39, Roi Dayan wrote:

> On 13/12/2022 17:35, Eelco Chaudron wrote:
>> tc does not support conntrack ALGs. Even worse, with tc enabled, they
>> should not be used/configured at all. This is because even though TC
>> will ignore the rules with ALG configured, i.e., they will flow through
>> the kernel module, return traffic might flow through a tc conntrack
>> rule, and it will not invoke the ALG helper.
>>
>> Signed-off-by: Eelco Chaudron 
>> Acked-by: Roi Dayan 
>> ---
>>  Documentation/howto/tc-offload.rst |   11 +++
>>  lib/netdev-offload-tc.c|4 
>>  tests/system-offloads.at   |   28 
>>  3 files changed, 23 insertions(+), 20 deletions(-)
>>
>> diff --git a/Documentation/howto/tc-offload.rst 
>> b/Documentation/howto/tc-offload.rst
>> index f6482c8af..63687adc9 100644
>> --- a/Documentation/howto/tc-offload.rst
>> +++ b/Documentation/howto/tc-offload.rst
>> @@ -112,3 +112,14 @@ First flow packet not processed by meter
>>  Packets that are received by ovs-vswitchd through an upcall before the 
>> actual
>>  meter flow is installed, are not passing TC police action and therefore are
>>  not considered for policing.
>> +
>> +Conntrack Application Layer Gateways(ALG)
>> ++
>> +
>> +TC does not support conntrack helpers, i.e., ALGs. TC will not offload 
>> flows if
>> +the ALG keyword is present within the ct() action. However, this will not 
>> allow
>> +ALGs to work within the datapath, as the return traffic without the ALG 
>> keyword
>> +might run through a TC rule, which internally will not call the conntrack
>> +helper required.
>> +
>> +So if ALG support is required, tc offload must be disabled.
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>> index 915c45ed3..ba309c2b6 100644
>> --- a/lib/netdev-offload-tc.c
>> +++ b/lib/netdev-offload-tc.c
>> @@ -1357,6 +1357,10 @@ parse_put_flow_ct_action(struct tc_flower *flower,
>>  action->ct.label_mask = ct_label->mask;
>>  }
>>  break;
>> +/* The following option we do not support in tc-ct, and 
>> should
>> + * not be ignored for proper operation. */
>> +case OVS_CT_ATTR_HELPER:
>> +return EOPNOTSUPP;
>>  }
>>  }
>>
>> diff --git a/tests/system-offloads.at b/tests/system-offloads.at
>> index 9d1e80c8d..73a761316 100644
>> --- a/tests/system-offloads.at
>> +++ b/tests/system-offloads.at
>> @@ -30,6 +30,7 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
>> AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| 
>> uuidfilt])], [0], [$2])
>>  ])
>>
>> +<<< current
>
> Hi,
>
> I just noticed this. I guess from rebases as the file was supposed to be ok 
> since v2.
> I see it got added by mistake since v3
> also, beside this any reason to hold the series?

Thanks, I’ll send out a v7 to fix this. I think it’s not yet in because Ilya 
had no time to look at this before the 3.1 branch creation :(

> Thanks,
> Roi
>
>
>>
>>  # We override the OVS_REVALIDATOR_PURGE macro, allowing a bit more time for 
>> the
>>  # tc-datapath entries to be installed.
>> @@ -42,6 +43,13 @@ m4_define([OVS_REVALIDATOR_PURGE],
>>  m4_define([DPCTL_DUMP_CONNTRACK], [sleep 3; ovs-appctl 
>> dpctl/dump-conntrack])
>>
>>
>> +# Conntrack ALGs are not supported for tc.
>> +m4_define([CHECK_CONNTRACK_ALG],
>> +[
>> + AT_SKIP_IF([:])
>> +])
>> +
>> +
>>  # The list below are tests that will not pass for a "test environment" 
>> specific
>>  # issue.
>>  m4_define([OVS_TEST_SKIP_LIST],
>> @@ -60,27 +68,7 @@ conntrack - IPv6 Fragmentation over vxlan
>>  conntrack - zone-based timeout policy
>>  conntrack - multiple zones, local
>>  conntrack - multi-stage pipeline, local
>> -conntrack - FTP
>> -conntrack - FTP over IPv6
>> -conntrack - IPv6 FTP Passive
>> -conntrack - FTP with multiple expectations
>> -conntrack - TFTP
>>  conntrack - ICMP related with NAT
>> -conntrack - FTP SNAT prerecirc
>> -conntrack - FTP SNAT prerecirc seqadj
>> -conntrack - FTP SNAT postrecirc
>> -conntrack - FTP SNAT postrecirc seqadj
>> -conntrack - FTP SNAT orig tuple
>> -conntrack - FTP SNAT orig tuple seqadj
>> -conntrack - IPv4 FTP Passive with SNAT
>> -conntrack - IPv4 FTP Passive with DNAT
>> -conntrack - IPv4 FTP Passive with DNAT 2
>> -conntrack - IPv4 FTP Active with DNAT
>> -conntrack - IPv4 FTP Active with DNAT with reverse skew
>> -conntrack - IPv6 FTP with SNAT
>> -conntrack - IPv6 FTP Passive with SNAT
>> -conntrack - IPv6 FTP with SNAT - orig tuple
>> -conntrack - IPv4 TFTP with SNAT
>>  conntrack - DNAT load balancing
>>  conntrack - DNAT load balancing with NC
>>  conntrack - Multiple ICMP traverse
>>

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