Re: [ovs-dev] [v4 7/8] netdev-offload-tc: Offloading rules with police actions

[Jianbo Liu via dev]

On Mon, 2022-05-16 at 13:34 +0200, Eelco Chaudron wrote:
> 
> 
> On 3 May 2022, at 5:08, Jianbo Liu via dev wrote:
> 
> > When offloading rule, tc should be filled with police index, instead
> > of meter id. As meter is mapped to police action, and the mapping is
> > inserted into meter_id_to_police_idx hmap, this hmap is used to find
> > the police index. Besides, an action cookie is added to save meter id
> > on rule creation, so meter id can be retrieved from the cookie, and
> > pass to dpif while dumping rules.
> > 
> > Signed-off-by: Jianbo Liu 
> > ---
> >  lib/netdev-offload-tc.c | 16 +++-
> >  lib/tc.c    | 25 -
> >  2 files changed, 39 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > index bded4bc8c..fa3e8113b 100644
> > --- a/lib/netdev-offload-tc.c
> > +++ b/lib/netdev-offload-tc.c
> > @@ -169,6 +169,8 @@ static struct netlink_field set_flower_map[][4] =
> > {
> >  },
> >  };
> > 
> > +static int meter_id_lookup(uint32_t meter_id, uint32_t *police_idx);
> > +
> 
> Maybe move this up to the place where you also define the meter
> mutex/ids?
> 
> >  static struct ovs_mutex ufid_lock = OVS_MUTEX_INITIALIZER;
> > 
> >  /**
> > @@ -1038,7 +1040,8 @@ parse_tc_flower_to_match(struct tc_flower
> > *flower,
> >  }
> >  break;
> >  case TC_ACT_POLICE: {
> > -    /* Not supported yet */
> > +    nl_msg_put_u32(buf, OVS_ACTION_ATTR_METER,
> > +   action->police.meter_id);
> >  }
> >  break;
> >  }
> > @@ -1961,6 +1964,17 @@ netdev_tc_flow_put(struct netdev *netdev,
> > struct match *match,
> >  action->type = TC_ACT_GOTO;
> >  action->chain = 0;  /* 0 is reserved and not used by
> > recirc. */
> >  flower.action_count++;
> > +    } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_METER) {
> > +    uint32_t police_index;
> > +
> > +    action->type = TC_ACT_POLICE;
> > +    action->police.meter_id = nl_attr_get_u32(nla);
> > +    if (meter_id_lookup(action->police.meter_id,
> > _index)) {
> > +    return EOPNOTSUPP;
> > +    }
> > +
> > +    action->police.index = police_index;
> > +    flower.action_count++;
> >  } else {
> >  VLOG_DBG_RL(, "unsupported put action type: %d",
> >  nl_attr_type(nla));
> > diff --git a/lib/tc.c b/lib/tc.c
> > index 16d4ae3da..ecdcb676d 100644
> > --- a/lib/tc.c
> > +++ b/lib/tc.c
> > @@ -2421,6 +2421,22 @@ nl_msg_put_act_gact(struct ofpbuf *request,
> > uint32_t chain)
> >  nl_msg_end_nested(request, offset);
> >  }
> > 
> > +static void
> > +nl_msg_put_act_police_index(struct ofpbuf *request, uint32_t
> > police_idx)
> > +{
> > +    struct tc_police police;
> > +    size_t offset;
> > +
> > +    memset(, 0, sizeof police);
> > +    police.index = police_idx;
> > +
> > +    nl_msg_put_string(request, TCA_ACT_KIND, "police");
> > +    offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS);
> > +    nl_msg_put_unspec(request, TCA_POLICE_TBF, , sizeof
> > police);
> > +    nl_msg_put_u32(request, TCA_POLICE_RESULT, TC_ACT_PIPE);
> > +    nl_msg_end_nested(request, offset);
> > +}
> > +
> >  static void
> >  nl_msg_put_act_ct(struct ofpbuf *request, struct tc_action *action)
> >  {
> > @@ -2911,7 +2927,14 @@ nl_msg_put_flower_acts(struct ofpbuf *request,
> > struct tc_flower *flower)
> >  }
> >  break;
> >  case TC_ACT_POLICE: {
> > -    /* Not supported yet */
> > +    struct tc_cookie act_cookie;
> > +
> > +    act_offset = nl_msg_start_nested(request,
> > act_index++);
> > +    nl_msg_put_act_police_index(request, action-
> > >police.index);
> > +    act_cookie.data = >police.meter_id;
> > +    act_cookie.len = sizeof(action->police.meter_id);
> > +    nl_msg_put_act_cookie(request, _cookie);
> 
> I think we should only set the action cookie to flower->act_cookie as
> the entire infrastructure relies on this being the ufid. If you need to
> store some meter_id, you might need to do it in tcf_id somehow. See
> your colleague's sflow patch he has the same problem as he needs to
> store the sflow id. Guess this also requires changes in
> nl_parse_act_police() as you can not use the action_cookie.
> 

tcf_id is used to find ufid. But I don't think it's proper to put
meter_id in tcf_id, because not all rules have meter. From this logic,
meter_id can't be a part of the id or key for a rule.
There are several benifits to use action_cookie. First, we don't need
to searh HMAP, either by ufid, or police index (we can add the mapping
of meter id and police index). Secondly, it's helpful for debugging,
you can check police action and its meter id with tc command.

For example:
#tc -s action 

Re: [ovs-dev] [PATCH v2] dpif-netdev: ct maxconns config persistence

[lic121]

On Tue, May 17, 2022 at 09:06:24AM -0400, Aaron Conole wrote:
> lic121  writes:
> 
> > Max allowed userspace dp conntrack entries is configurable with
> > 'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios,
> > this configuration is expected to survive from host reboot, from
> > ovs service restart.
> >
> > Signed-off-by: lic121 
> > ---
> 
> Sorry - just one last comment, and then I think this can go in.
> 
> Please add a warning to dpctl_ct_set_maxconns that also flags the
> deprecation, and add a NEWS entry as well.  That way end users are aware
> of the change.  If you want to add NEWS entry as a separate patch
> (because those can generate git-am related issues) that's probably okay.

No worry, and thanks for your careful review. I will add warning and add NEWS in
the next version.

> 
> > Notes:
> > v2:
> >   - rename "ct-maxconns" to "userspace-ct-maxconns"
> >
> >  lib/dpctl.man   |  4 +++-
> >  lib/dpif-netdev.c   | 11 +++
> >  tests/system-traffic.at | 10 ++
> >  vswitchd/vswitch.xml|  7 +++
> >  4 files changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/dpctl.man b/lib/dpctl.man
> > index c100d0a..4f08a3f 100644
> > --- a/lib/dpctl.man
> > +++ b/lib/dpctl.man
> > @@ -343,7 +343,9 @@ system due to connection tracking or simply limiting 
> > connection
> >  tracking.  If the number of connections is already over the new maximum
> >  limit request then the new maximum limit will be enforced when the
> >  number of connections decreases to that limit, which normally happens
> > -due to connection expiry.  Only supported for userspace datapath.
> > +due to connection expiry.  Only supported for userspace datapath. This
> > +command is deprecated by ovsdb cfg other_config:userspace-ct-maxconns
> > +because of persistence capability.
> >  .
> >  .TP
> >  \*(DX\fBct\-get\-maxconns\fR [\fIdp\fR]
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 6764343..e2348a1 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -4827,6 +4827,17 @@ dpif_netdev_set_config(struct dpif *dpif, const 
> > struct smap *other_config)
> >  }
> >  }
> >  
> > +uint32_t ct_maxconns, cur_maxconns;
> > +ct_maxconns = smap_get_int(other_config, "userspace-ct-maxconns",
> > +   UINT32_MAX);
> > +/* Leave runtime value as it is when cfg is removed. */
> > +if (ct_maxconns < UINT32_MAX) {
> > +conntrack_get_maxconns(dp->conntrack, _maxconns);
> > +if (ct_maxconns != cur_maxconns) {
> > +conntrack_set_maxconns(dp->conntrack, ct_maxconns);
> > +}
> > +}
> > +
> >  bool smc_enable = smap_get_bool(other_config, "smc-enable", false);
> >  bool cur_smc;
> >  atomic_read_relaxed(>smc_enable_db, _smc);
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index 4a7fa49..82ea992 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -2258,6 +2258,16 @@ AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> >  10
> >  ])
> >  
> > +AT_CHECK([ovs-vsctl set Open_vswitch . 
> > other_config:userspace-ct-maxconns=20], [0])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> > +20
> > +])
> > +
> > +AT_CHECK([ovs-vsctl remove Open_vswitch . other_config 
> > userspace-ct-maxconns], [0])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> > +20
> > +])
> > +
> >  OVS_TRAFFIC_VSWITCHD_STOP
> >  AT_CLEANUP
> >  
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index 0c66326..48f05d7 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -183,6 +183,13 @@
> >  
> >
> >  
> > +   > +  type='{"type": "integer", "minInteger": 0}'>
> > +The maximum number of connection tracker entries allowed in the
> > +userspace datapath.  This deprecates "ovs-appctl 
> > dpctl/ct-set-maxconns"
> > +command.
> > +  
> > +
> > >type='{"type": "integer", "minInteger": 500}'>
> >  
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dynamic-string: Fix undefined behavior due to offsetting null pointer.

[Ilya Maximets]

On 4/26/22 17:16, Dumitru Ceara wrote:
> On 4/26/22 16:29, Adrian Moreno wrote:
>>
>>
>> On 4/12/22 15:29, Dumitru Ceara wrote:
>>> When compiled with '-fsanitize=undefined' running 'ovsdb-client
>>> --timestamp monitor Open_vSwitch' in a sandbox triggers the following
>>> undefined behavior (flagged by UBSan):
>>>
>>>    lib/dynamic-string.c:207:38: runtime error: applying zero offset to
>>> null pointer
>>>    #0 0x196c05b in ds_put_strftime_msec lib/dynamic-string.c:207:38
>>>    #1 0x196c2aa in xastrftime_msec lib/dynamic-string.c:225:5
>>>    #2 0x1935f33 in pmd_info_show_perf lib/dpif-netdev.c:799:17
>>>    #3 0x1935f33 in dpif_netdev_pmd_info lib/dpif-netdev.c:1487:13
>>>    #4 0x1938155 in pmd_perf_show_cmd lib/dpif-netdev.c:1537:5
>>>    #5 0x1d7634b in process_command lib/unixctl.c:310:13
>>>    #6 0x1d7634b in run_connection lib/unixctl.c:344:17
>>>    #7 0x1d7634b in unixctl_server_run lib/unixctl.c:395:21
>>>    #8 0x168b5bd in main vswitchd/ovs-vswitchd.c:130:9
>>>    #9 0x7f6fa2fd8492 in __libc_start_main (/lib64/libc.so.6+0x23492)
>>>    #10 0xe6911d in _start (vswitchd/ovs-vswitchd+0xe6911d)
>>>
>>> Reported-by: Ilya Maximets 
>>> Signed-off-by: Dumitru Ceara 
>>> ---
>>>   lib/dynamic-string.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
>>> index fd0127ed1740..3d415af4f4e0 100644
>>> --- a/lib/dynamic-string.c
>>> +++ b/lib/dynamic-string.c
>>> @@ -200,8 +200,8 @@ ds_put_strftime_msec(struct ds *ds, const char
>>> *template, long long int when,
>>>     for (;;) {
>>>   size_t avail = ds->string ? ds->allocated - ds->length + 1 : 0;
>>> -    size_t used = strftime_msec(>string[ds->length], avail,
>>> template,
>>> -    );
>>> +    char *dest = ds->string ? >string[ds->length] : NULL;
>>> +    size_t used = strftime_msec(dest, avail, template, );
>>>   if (used) {
>>>   ds->length += used;
>>>   return;
>>
>> The code looks OK to me. However, calling strftime_msec(NULL, 0,...)
>> once if the dynamic string is empty seems to me a bit obfuscated.
>>
>> I would personally just add:
>>
>> if (!ds->string) {
>>  ds_reserve(ds, 64);
>> }
>>
>> before the loop to mentally get rid of that corner case.
> 
> Sure, that's fine too, I have no preference either way.  Ilya, what do
> you think, does this need a v2?

Yeah, the ds_reserve() seems cleaner.  We don't really need to
check for !ds->string before calling it though, IIUC.

With that we may also remove the check while calculating 'avail'.

Best regards, Ilya Maximets.

> 
>>
>> In any case:
>>
>> Acked-by: Adrian Moreno 
>>
> 
> Thanks for the review!
> 

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

Re: [ovs-dev] [PATCH ovn] northd: Honor ct-snat-zone option for common case.

[Numan Siddique]

On Fri, May 6, 2022 at 9:47 AM Mark Michelson  wrote:

> Commit 4deac4509 introduced the new "czone" variants for ct_dnat and
> ct_snat. This makes it so that a single conntrack zone is used for both
> DNAT and SNAT on the datapath. The common CT zone chosen in this commit
> was the DNAT zone.
>
> The ct-snat-zone option for logical routers makes it so that an SNAT
> zone can be explicitly configured instead of having a zone selected at
> random by OVN. The problem is that since the DNAT zone is used as the
> common zone ID, it means that when SNAT is performed, the SNAT is not
> performed in the configured ct-snat-zone.
>
> The fix for this is to change to using the SNAT zone as the common zone
> if ct-snat-zone is configured on the logical router. This way, SNAT will
> use the configured zone as expected. DNAT will also use this zone when
> possible.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2061619
>
> Signed-off-by: Mark Michelson 
>

Thanks for fixing this.

Acked-by: Numan Siddique 

Numan

> ---
>  controller/lflow.c   | 18 +
>  include/ovn/actions.h|  2 +
>  include/ovn/logical-fields.h |  2 -
>  lib/actions.c|  4 +-
>  tests/ovn.at | 72 
>  tests/test-ovn.c |  1 +
>  6 files changed, 95 insertions(+), 4 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index b7ddeb1c0..3c953e18b 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -1104,6 +1104,23 @@ lflow_parse_ctrl_meter(const struct
> sbrec_logical_flow *lflow,
>  }
>  }
>
> +static int
> +get_common_nat_zone(const struct local_datapath *ldp)
> +{
> +/* Normally, the common NAT zone defaults to the DNAT zone. However,
> + * if the "snat-ct-zone" is set on the datapath, the user is
> + * expecting an explicit CT zone to be used for SNAT. If we default
> + * to the DNAT zone, then it means SNAT will not use the configured
> + * value. The way we get around this is to use the SNAT zone as the
> + * common zone if "snat-ct-zone" is set.
> + */
> +if (smap_get(>datapath->external_ids, "snat-ct-zone")) {
> +return MFF_LOG_SNAT_ZONE;
> +} else {
> +return MFF_LOG_DNAT_ZONE;
> +}
> +}
> +
>  static void
>  add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
>const struct local_datapath *ldp,
> @@ -1153,6 +1170,7 @@ add_matches_to_flow_table(const struct
> sbrec_logical_flow *lflow,
>  .fdb_ptable = OFTABLE_GET_FDB,
>  .fdb_lookup_ptable = OFTABLE_LOOKUP_FDB,
>  .ctrl_meter_id = ctrl_meter_id,
> +.common_nat_ct_zone = get_common_nat_zone(ldp),
>  };
>  ovnacts_encode(ovnacts->data, ovnacts->size, , );
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index f55d77d47..547797584 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -808,6 +808,8 @@ struct ovnact_encode_params {
>  * 'lookup_fdb' to resubmit. */
>  uint32_t ctrl_meter_id; /* Meter to be used if the resulting flow
> sends packets to controller. */
> +uint32_t common_nat_ct_zone; /* When performing NAT in a common CT
> zone,
> +this determines which CT zone to use
> */
>  };
>
>  void ovnacts_encode(const struct ovnact[], size_t ovnacts_len,
> diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h
> index 25b5a62a3..18516634e 100644
> --- a/include/ovn/logical-fields.h
> +++ b/include/ovn/logical-fields.h
> @@ -36,8 +36,6 @@ enum ovn_controller_event {
> * (32 bits). */
>  #define MFF_LOG_SNAT_ZONE  MFF_REG12  /* conntrack snat zone for gateway
> router
> * (32 bits). */
> -#define MFF_LOG_NAT_ZONE   MFF_LOG_DNAT_ZONE /* conntrack zone for both
> snat
> -  * and dnat. */
>  #define MFF_LOG_CT_ZONEMFF_REG13  /* Logical conntrack zone for lports
> * (32 bits). */
>  #define MFF_LOG_INPORT MFF_REG14  /* Logical input port (32 bits). */
> diff --git a/lib/actions.c b/lib/actions.c
> index a3cf747db..a9c27600c 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -1131,7 +1131,7 @@ encode_CT_DNAT_IN_CZONE(const struct ovnact_ct_nat
> *cn,
>  const struct ovnact_encode_params *ep,
>  struct ofpbuf *ofpacts)
>  {
> -encode_ct_nat(cn, ep, false, MFF_LOG_NAT_ZONE, ofpacts);
> +encode_ct_nat(cn, ep, false, ep->common_nat_ct_zone, ofpacts);
>  }
>
>  static void
> @@ -1139,7 +1139,7 @@ encode_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat
> *cn,
>  const struct ovnact_encode_params *ep,
>  struct ofpbuf *ofpacts)
>  {
> -encode_ct_nat(cn, ep, true, MFF_LOG_NAT_ZONE, 

Re: [ovs-dev] [PATCH ovn v3] northd: add configuration option for enabling parallelization

[Mark Michelson]

Hi Xavier,

I have a few small comments below.

Thanks,
Mark

On 5/16/22 17:11, Xavier Simonart wrote:

This patch is intended to change the way to enable northd lflow build
parallelization, as well as enable runtime change of number of threads.
Before this patch, the following was needed to use parallelization:
- enable parallelization through use_parallel_build in NBDB
- use --dummy-numa to select number of threads.
This second part was needed as otherwise as many threads as cores in the system
were used, while parallelization showed some performance improvement only until
using around 4 (or maybe 8) threads.

With this patch, the number of threads used for lflow parallel build can be
specified either:
- at startup, using --n-threads= as ovn-northd command line option
- using unixctl
If the number of threads specified is > 1, then parallelization is enabled.
If the number is 1, parallelization is disabled.
If the number is < 1 or > 256, parallelization is disabled at startup and
a warning is logged.

Th following unixctl have been added:
- set-n-threads : set the number of treads used.
- get-n-threads: returns the number of threads used
If the number of threads is within <2-256> bounds, parallelization is enabled.
If the number of thread is 1, parallelization is disabled.
Otherwise an error is thrown.

Note that, if set-n-threads failed for any reason (e.g. failure to setup some
semaphore), parallelization is disabled, and get-n-thread will return 1.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2078552
Signed-off-by: Xavier Simonart 

---
v2:  - handled Dumitru's comments
  - added missing mutex_destroy
  - fixed issue when use_logical_dp_group is enabled after northd startup
  - rebased on top of main
v3:
  - fix mutex_destroy issue
---
  NEWS  |   7 +
  lib/ovn-parallel-hmap.c   | 290 +-
  lib/ovn-parallel-hmap.h   |  24 ++--
  northd/northd.c   | 176 ---
  northd/northd.h   |   1 +
  northd/ovn-northd-ddlog.c |   6 -
  northd/ovn-northd.8.xml   |  68 -
  northd/ovn-northd.c   |  68 -
  tests/ovn-macros.at   |  59 ++--
  tests/ovn-northd.at   | 109 ++
  10 files changed, 486 insertions(+), 322 deletions(-)

diff --git a/NEWS b/NEWS
index 244824e3f..6e489df32 100644
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,13 @@ Post v22.03.0
  implicit drop behavior on logical switches with ACLs applied.
- Support (LSP.options:qos_min_rate) to guarantee minimal bandwidth 
available
  for a logical port.
+  - Changed the way to enable northd parallelization.
+Removed support for:
+- use_parallel_build in NBDB.
+- --dummy-numa in northd cmdline.
+Added support for:
+-  --n-threads= in northd cmdline.
+- set-n-threads/get-n-threads unixctls.
  
  OVN v22.03.0 - 11 Mar 2022

  --
diff --git a/lib/ovn-parallel-hmap.c b/lib/ovn-parallel-hmap.c
index 7edc4c0b6..8738fba4c 100644
--- a/lib/ovn-parallel-hmap.c
+++ b/lib/ovn-parallel-hmap.c
@@ -38,14 +38,10 @@ VLOG_DEFINE_THIS_MODULE(ovn_parallel_hmap);
  
  #ifndef OVS_HAS_PARALLEL_HMAP
  
-#define WORKER_SEM_NAME "%x-%p-%x"

+#define WORKER_SEM_NAME "%x-%p-%"PRIxSIZE
  #define MAIN_SEM_NAME "%x-%p-main"
  
-/* These are accessed under mutex inside add_worker_pool().

- * They do not need to be atomic.
- */
  static atomic_bool initial_pool_setup = ATOMIC_VAR_INIT(false);
-static bool can_parallelize = false;
  
  /* This is set only in the process of exit and the set is

   * accompanied by a fence. It does not need to be atomic or be
@@ -57,18 +53,18 @@ static struct ovs_list worker_pools = 
OVS_LIST_INITIALIZER(_pools);
  
  static struct ovs_mutex init_mutex = OVS_MUTEX_INITIALIZER;
  
-static int pool_size;

+static size_t pool_size = 1;
  
  static int sembase;
  
  static void worker_pool_hook(void *aux OVS_UNUSED);

-static void setup_worker_pools(bool force);
+static void setup_worker_pools(void);
  static void merge_list_results(struct worker_pool *pool OVS_UNUSED,
 void *fin_result, void *result_frags,
-   int index);
+   size_t index);
  static void merge_hash_results(struct worker_pool *pool OVS_UNUSED,
 void *fin_result, void *result_frags,
-   int index);
+   size_t index);
  
  bool

  ovn_stop_parallel_processing(void)
@@ -76,107 +72,183 @@ ovn_stop_parallel_processing(void)
  return workers_must_exit;
  }
  
-bool

-ovn_can_parallelize_hashes(bool force_parallel)
+size_t
+ovn_get_worker_pool_size(void)
  {
-bool test = false;
+return pool_size;
+}
  
-if (atomic_compare_exchange_strong(

-_pool_setup,
-,
-true)) {
-ovs_mutex_lock(_mutex);
-setup_worker_pools(force_parallel);
-

Re: [ovs-dev] [PATCH ovn] tests: fixed multiple flaky tests

[Numan Siddique]

On Tue, May 17, 2022 at 1:47 PM Mark Michelson  wrote:

> Thanks for this, Xavier. I really like seeing new macros being added to
> the testsuite to increase reliability.
>
> Acked-by: Mark Michelson 
>

Thanks for improving the test cases.

Applied to the main branch.

Numan

>
> On 5/12/22 05:23, Xavier Simonart wrote:
> > - Multiple "ovn-controller" tests
> > - I-P handle northd_internal_version change
> > - 2 HVs, 1 lport/HV, localport ports
> > - 1 LR with distributed router gateway port
> > - send gratuitous arp for NAT rules on distributed router
> > - send gratuitous arp on localnet
> > - 4 HV, 3 LS, 2 LR, packet test with HA distributed router gateway port
> > - external logical port
> > - 3 HVs, 1 VIFs/HV, 1 GW, 1 LS
> > - VLAN transparency, passthru=true, ARP responder disabled
> >
> > Some other minor fixes:
> > - removed multiple definition of OVN_CHECK_PACKETS_UNIQ
> > - Added a check on ovs-pcap.in to prevent, in case of non existing
> input,
> >filling the logs with "detected unhandled Python exception"
> >
> > Signed-off-by: Xavier Simonart 
> > ---
> >   tests/ovn-controller.at |  30 +-
> >   tests/ovn.at| 125 +---
> >   2 files changed, 96 insertions(+), 59 deletions(-)
> >
> > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > index 88ec2f269..3339e58ce 100644
> > --- a/tests/ovn-controller.at
> > +++ b/tests/ovn-controller.at
> > @@ -901,7 +901,7 @@ read_counter() {
> >   }
> >
> >   ovn-nbctl create address_set name=as1
> > -check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" &&
> ip4.src == $as1' drop
> > +check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport ==
> "ls1-lp1" && ip4.src == $as1' drop
> >
> >   # Add IPs to as1 for 10 times, 1 IP each time.
> >   reprocess_count_old=$(read_counter consider_logical_flow)
> > @@ -1074,7 +1074,7 @@ read_counter() {
> >   }
> >
> >   ovn-nbctl create address_set name=as1
> > -check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" &&
> ip4.src == $as1 && tcp && tcp.dst == {111, 222, 333}' drop
> > +check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport ==
> "ls1-lp1" && ip4.src == $as1 && tcp && tcp.dst == {111, 222, 333}' drop
> >
> >   # Add IPs to as1 for 10 times, 1 IP each time.
> >   reprocess_count_old=$(read_counter consider_logical_flow)
> > @@ -1269,7 +1269,7 @@ read_counter() {
> >
> >   ovn-nbctl create address_set name=as1
> >   ovn-nbctl create address_set name=as2
> > -check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" &&
> ip4.src == $as1 && ip4.dst == $as2' drop
> > +check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport ==
> "ls1-lp1" && ip4.src == $as1 && ip4.dst == $as2' drop
> >
> >   # Add IPs to as1 and as2, with some of the IPs overlapping
> >   reprocess_count_old=$(read_counter consider_logical_flow)
> > @@ -1426,7 +1426,7 @@ ovn-nbctl create address_set name=as1
> >   ovn-nbctl create address_set name=as2
> >
> >   # OR on different fields
> > -check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" &&
> (ip4.src == $as1 || ip4.dst == $as2)' drop
> > +check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport ==
> "ls1-lp1" && (ip4.src == $as1 || ip4.dst == $as2)' drop
> >
> >   # Add IPs to as1 and as2, with some of the IPs overlapping
> >   reprocess_count_old=$(read_counter consider_logical_flow)
> > @@ -1522,7 +1522,7 @@ ovn-nbctl create address_set name=as1
> >   ovn-nbctl create address_set name=as2
> >
> >   # OR on the same field
> > -check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" &&
> ip4.src == {$as1, $as2}' drop
> > +check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport ==
> "ls1-lp1" && ip4.src == {$as1, $as2}' drop
> >
> >   # Add IPs to as1 and as2, with some of the IPs overlapping
> >   reprocess_count_old=$(read_counter consider_logical_flow)
> > @@ -1622,7 +1622,7 @@ read_counter() {
> >   }
> >
> >   ovn-nbctl create address_set name=as1
> > -check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" &&
> ip4.src == $as1 && ip4.dst == $as1' drop
> > +check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport ==
> "ls1-lp1" && ip4.src == $as1 && ip4.dst == $as1' drop
> >
> >   # Add IPs to as1 for 10 times, 1 IP each time.
> >   reprocess_count_old=$(read_counter consider_logical_flow)
> > @@ -1926,7 +1926,7 @@ read_counter() {
> >   }
> >
> >   ovn-nbctl create address_set name=as1
> > -check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" &&
> eth.src == $as1' drop
> > +check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport ==
> "ls1-lp1" && eth.src == $as1' drop
> >
> >   # Add MACs to as1 for 5 times.
> >   reprocess_count_old=$(read_counter consider_logical_flow)
> > @@ -2007,7 +2007,7 @@ read_counter() {
> >   }
> >
> >   ovn-nbctl create address_set name=as1
> > -check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" &&
> ip6.src == $as1' drop
> > +check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport ==
> 

[ovs-dev] [PATCH v1] Pmd.at: fix dpcls and dpif configuration test cases.

[Kumar Amber]

Without running set command first the string matching
fails on get command beacuse DPCLS prio value is different
for different default builds like with --enable-autovalidator
build auto-validator prio is set to 255 and if the build
is a scalar than generic value is default 255.

The same problem is seen with dpif where re-arranging the get
command after set makes it consistent across any builds.

Fixes: cc0a87b11c (pmd.at: Add test-cases for DPCLS and DPIF commands.)
Signed-off-by: Kumar Amber 
---
 tests/pmd.at | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/tests/pmd.at b/tests/pmd.at
index 0a451f33c..3962dd2bd 100644
--- a/tests/pmd.at
+++ b/tests/pmd.at
@@ -1113,15 +1113,15 @@ AT_SETUP([PMD - dpif configuration])
 OVS_VSWITCHD_START([], [], [], [--dummy-numa 0,0])
 AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy-pmd])
 
+AT_CHECK([ovs-appctl dpif-netdev/dpif-impl-set dpif_scalar], [0], [dnl
+DPIF implementation set to dpif_scalar.
+])
+
 AT_CHECK([ovs-vsctl show], [], [stdout])
 AT_CHECK([ovs-appctl dpif-netdev/dpif-impl-get | grep "dpif_scalar"], [], [dnl
   dpif_scalar (pmds: 0)
 ])
 
-AT_CHECK([ovs-appctl dpif-netdev/dpif-impl-set dpif_scalar], [0], [dnl
-DPIF implementation set to dpif_scalar.
-])
-
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
@@ -1130,13 +1130,6 @@ OVS_VSWITCHD_START([], [], [], [--dummy-numa 0,0])
 AT_CHECK([ovs-vsctl add-port br0 p1 -- set Interface p1 type=dummy-pmd])
 
 AT_CHECK([ovs-vsctl show], [], [stdout])
-AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep generic], [], 
[dnl
-  1 : generic
-])
-
-AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-get | grep 
autovalidator], [], [dnl
-  0 : autovalidator
-])
 
 AT_CHECK([ovs-appctl dpif-netdev/subtable-lookup-prio-set autovalidator 3], 
[0], [dnl
 Lookup priority change affected 0 dpcls ports and 0 subtables.
-- 
2.25.1

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

Re: [ovs-dev] [PATCH ovn] tests: fixed multiple flaky tests

[Mark Michelson]

Thanks for this, Xavier. I really like seeing new macros being added to 
the testsuite to increase reliability.


Acked-by: Mark Michelson 

On 5/12/22 05:23, Xavier Simonart wrote:

- Multiple "ovn-controller" tests
- I-P handle northd_internal_version change
- 2 HVs, 1 lport/HV, localport ports
- 1 LR with distributed router gateway port
- send gratuitous arp for NAT rules on distributed router
- send gratuitous arp on localnet
- 4 HV, 3 LS, 2 LR, packet test with HA distributed router gateway port
- external logical port
- 3 HVs, 1 VIFs/HV, 1 GW, 1 LS
- VLAN transparency, passthru=true, ARP responder disabled

Some other minor fixes:
- removed multiple definition of OVN_CHECK_PACKETS_UNIQ
- Added a check on ovs-pcap.in to prevent, in case of non existing input,
   filling the logs with "detected unhandled Python exception"

Signed-off-by: Xavier Simonart 
---
  tests/ovn-controller.at |  30 +-
  tests/ovn.at| 125 +---
  2 files changed, 96 insertions(+), 59 deletions(-)

diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 88ec2f269..3339e58ce 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -901,7 +901,7 @@ read_counter() {
  }
  
  ovn-nbctl create address_set name=as1

-check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src == 
$as1' drop
+check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport == "ls1-lp1" && 
ip4.src == $as1' drop
  
  # Add IPs to as1 for 10 times, 1 IP each time.

  reprocess_count_old=$(read_counter consider_logical_flow)
@@ -1074,7 +1074,7 @@ read_counter() {
  }
  
  ovn-nbctl create address_set name=as1

-check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src == $as1 && 
tcp && tcp.dst == {111, 222, 333}' drop
+check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src == $as1 
&& tcp && tcp.dst == {111, 222, 333}' drop
  
  # Add IPs to as1 for 10 times, 1 IP each time.

  reprocess_count_old=$(read_counter consider_logical_flow)
@@ -1269,7 +1269,7 @@ read_counter() {
  
  ovn-nbctl create address_set name=as1

  ovn-nbctl create address_set name=as2
-check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src == $as1 
&& ip4.dst == $as2' drop
+check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src == 
$as1 && ip4.dst == $as2' drop
  
  # Add IPs to as1 and as2, with some of the IPs overlapping

  reprocess_count_old=$(read_counter consider_logical_flow)
@@ -1426,7 +1426,7 @@ ovn-nbctl create address_set name=as1
  ovn-nbctl create address_set name=as2
  
  # OR on different fields

-check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && (ip4.src == 
$as1 || ip4.dst == $as2)' drop
+check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport == "ls1-lp1" && 
(ip4.src == $as1 || ip4.dst == $as2)' drop
  
  # Add IPs to as1 and as2, with some of the IPs overlapping

  reprocess_count_old=$(read_counter consider_logical_flow)
@@ -1522,7 +1522,7 @@ ovn-nbctl create address_set name=as1
  ovn-nbctl create address_set name=as2
  
  # OR on the same field

-check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src == 
{$as1, $as2}' drop
+check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport == "ls1-lp1" && 
ip4.src == {$as1, $as2}' drop
  
  # Add IPs to as1 and as2, with some of the IPs overlapping

  reprocess_count_old=$(read_counter consider_logical_flow)
@@ -1622,7 +1622,7 @@ read_counter() {
  }
  
  ovn-nbctl create address_set name=as1

-check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src == $as1 
&& ip4.dst == $as1' drop
+check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip4.src == 
$as1 && ip4.dst == $as1' drop
  
  # Add IPs to as1 for 10 times, 1 IP each time.

  reprocess_count_old=$(read_counter consider_logical_flow)
@@ -1926,7 +1926,7 @@ read_counter() {
  }
  
  ovn-nbctl create address_set name=as1

-check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && eth.src == 
$as1' drop
+check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport == "ls1-lp1" && 
eth.src == $as1' drop
  
  # Add MACs to as1 for 5 times.

  reprocess_count_old=$(read_counter consider_logical_flow)
@@ -2007,7 +2007,7 @@ read_counter() {
  }
  
  ovn-nbctl create address_set name=as1

-check ovn-nbctl acl-add ls1 to-lport 100 'outport == "ls1-lp1" && ip6.src == 
$as1' drop
+check ovn-nbctl --wait=hv acl-add ls1 to-lport 100 'outport == "ls1-lp1" && 
ip6.src == $as1' drop
  
  # Add IPs to as1 for 5 times, 1 IP each time.

  reprocess_count_old=$(read_counter consider_logical_flow)
@@ -2093,11 +2093,15 @@ AT_CHECK([echo $(($lflow_run_new - $lflow_run_old))], 
[0], [0
  # The below steps should cause northd_internal_version change twice. One by
  # ovn-sbctl, and the other by ovn-northd to change it back.
  
-lflow_run_old=$(read_counter lflow_run)

-check ovn-sbctl set SB_Global . 

Re: [ovs-dev] [PATCH ovn] controller: avoid recomputes triggered by SBDB Port_Binding updates.

[Han Zhou]

On Tue, May 17, 2022 at 2:41 AM Xavier Simonart  wrote:
>
> Hi Han
> Thanks for looking into this and for your feedback.
> Please see comments below
>
> On Tue, May 17, 2022 at 8:03 AM Han Zhou  wrote:
>>
>>
>>
>> On Thu, May 12, 2022 at 2:04 AM Xavier Simonart 
wrote:
>> >
>> > When VIF ports are claimed on a chassis, SBDB Port_Binding table is
updated.
>> > If the SBDB IDL is still is read-only ("in transaction") when such a
update
>> > is required, the update is not possible and recompute is triggered
through
>> > I+P failure.
>> >
>> > This situation can happen:
>> > - after updating Port_Binding->chassis to SBDB for one port, in a
following
>> >   iteration, ovn-controller handles
Interface:external_ids:ovn-installed
>> >   (for the same port) while SBDB is still read-only.
>> > - after updating Port_Binding->chassis to SBDB for one port, in a
following
>> >   iteration, ovn-controller updates Port_Binding->chassis for another
port,
>> >   while SBDB is still read-only.
>> >
>> > This patch prevent the recompute, by having the if-status module
>> > updating the Port_Binding chassis (if needed), when possible.
>> > This does not delay Port_Binding chassis update compared to before this
>> > patch: Port_Binding chassis will be updated as soon as SBDB is again
>> > writable, as it was the case, through a recompute, before this patch.
>> >
>> Thanks Xavier. I think the approach is good: moving the SB update from
the I-P engine to if-status module, so it wouldn't need to trigger I-P
engine recompute, and just let the if-status module update the SB as soon
as it is writable, through the if-status's state-machine.
>> However, I have some comments/questions regarding the implementation
details, primarily confusion on the state transitions. Please see below.
>>
>> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2059253
>> > Signed-off-by: Xavier Simonart 
>> > ---
>> >  controller/binding.c| 124 ++--
>> >  controller/binding.h|   9 ++-
>> >  controller/if-status.c  |  31 +++--
>> >  controller/if-status.h  |   6 +-
>> >  controller/ovn-controller.c |   6 +-
>> >  tests/ovn.at|  55 +++-
>> >  6 files changed, 184 insertions(+), 47 deletions(-)
>> >
>> > diff --git a/controller/binding.c b/controller/binding.c
>> > index e5ba56b25..d917d8775 100644
>> > --- a/controller/binding.c
>> > +++ b/controller/binding.c
>> > @@ -657,11 +657,15 @@ local_binding_get_lport_ofport(const struct
shash *local_bindings,
>> >  }
>> >
>> >  bool
>> > -local_binding_is_up(struct shash *local_bindings, const char *pb_name)
>> > +local_binding_is_up(struct shash *local_bindings, const char *pb_name,
>> > +const struct sbrec_chassis *chassis_rec)
>> >  {
>> >  struct local_binding *lbinding =
>> >  local_binding_find(local_bindings, pb_name);
>> >  struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
>> > +if (b_lport && b_lport->pb->chassis != chassis_rec) {
>> > +return false;
>> > +}
>>
>> Why need the change here? I understand that it is obvious that the
binding should not be considered up if the chassis is not updated in SB
port_binding, but I wonder why we need the change in *this* patch.
>> The function is called to see if an interface state should be moved from
MARK_UP to INSTALLED, but if the chassis is not set, it shouldn't even be
moved to MARK_UP.
>>
> I probably need to provide a better explanation of the states.
> In fact, my two goals were: 1) avoid/decrease recomputes and 2) do not
increase latency (time between interface is claimed and it's up in SB and
ovn-installed in OVS).
> There is not really an additional state covering the fact that the
chassis is set in sbdb.
> As soon as the state is CLAIMED, we try to set the pb->chassis (by 'try'
I mean we update pb->chassis if sb is not readonly).
> The fact that sb is readonly in CLAIMED state does not prevent us to move
to INSTALL_FLOWS state, where we are installing flows in OVS.

This may be a problem. If we haven't updated pb->chassis, then the physical
flows being installed are incomplete, and if we move the state forward, we
would end up updating the port status (SB pb->up and OVS
interface->external_ids:installed) too early, which completely void the
original purpose of the if-statue module, which is to manage the interface
state and tell CMS when a lport is ready to receive traffic.

> As soon as the seqno for those flows was received, we moved to MARK_UP
state.
> In those three states, we can update pb->chassis (if not already done).
> In MARK_UP we set the interface up.
> We moved to INSTALLED when both up and chassis have been written.
>
>> >  if (lbinding && b_lport && lbinding->iface) {
>> >  if (b_lport->pb->n_up && !b_lport->pb->up[0]) {
>> >  return false;
>> > @@ -673,13 +677,23 @@ local_binding_is_up(struct shash
*local_bindings, const char *pb_name)
>> >  }

Re: [ovs-dev] [PATCH ovn v3] controller/binding: prevent claiming container lport to ovs bridge

[Numan Siddique]

On Wed, Apr 27, 2022 at 8:39 AM Mohammad Heib  wrote:

> currently ovn-controller allow users to claim lport of type container
> to ovs bridge which is invalid use-case.
>
> This patch will prevent such invalid use-cases by ignoring the claiming
> requests for container lports and will throw a warning message to the
> controller logs.
>
> Signed-off-by: Mohammad Heib 
>

Hi Mohammad,

Thanks for v3.

Looks like we don't need this patch anymore after your patch -
https://github.com/ovn-org/ovn/commit/cd3b685043fa9758df3665bf3e3fc972048698a6

Also I do have a few concerns with this patch.
lport_lookup_by_name() will be called one extra time for every port binding
claim.  This could be expensive.
Presently when we have an ovs port with external_ids:iface-id set,  we
create a local_binding object for it.
And this patch now skips it.

Can you please test it out and see if the issue is still seen with the
present main branch ?

Thanks
Numan


---
>  controller/binding.c | 34 ++
>  tests/ovn.at | 11 +++
>  2 files changed, 45 insertions(+)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 7281b0485..db0a6f118 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -970,6 +970,32 @@ claim_lport(const struct sbrec_port_binding *pb,
>  return true;
>  }
>
> +/* Validate that "iface_rec:external_ids:iface-id" belongs to a lport
> + * that can be claimed to OVS bridge.
> + *
> + * If the lport type is LP_CONTAINER this function will throw a warning
> + * message to the logs and return "false".
> + *
> + * Otherwise, return "true".
> + */
> +static bool
> +valid_iface_claim(const char *iface_id,
> +  struct binding_ctx_in *b_ctx_in)
> +{
> +const struct sbrec_port_binding *pb = NULL;
> +pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name,
> +  iface_id);
> +if (pb && (get_lport_type(pb) == LP_CONTAINER)) {
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +VLOG_WARN_RL(, "Can't claim lport %s of type container to"
> + " OVS bridge,\nplease remove the lport parent_name"
> + " before claiming it.", pb->logical_port);
> +return false;
> +}
> +
> +return true;
> +}
> +
>  /* Returns false if lport is not released due to 'sb_readonly'.
>   * Returns true otherwise.
>   *
> @@ -1497,6 +1523,10 @@ build_local_bindings(struct binding_ctx_in
> *b_ctx_in,
>  struct local_binding *lbinding =
>  local_binding_find(local_bindings, iface_id);
>  if (!lbinding) {
> +if (!valid_iface_claim(iface_id, b_ctx_in)) {
> +continue;
> +}
> +
>  lbinding = local_binding_create(iface_id, iface_rec);
>  local_binding_add(local_bindings, lbinding);
>  } else {
> @@ -2022,6 +2052,10 @@ binding_handle_ovs_interface_changes(struct
> binding_ctx_in *b_ctx_in,
>  int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
>  if (iface_id && ofport > 0 &&
>  is_iface_in_int_bridge(iface_rec, b_ctx_in->br_int)) {
> +if (!valid_iface_claim(iface_id, b_ctx_in)) {
> +continue;
> +}
> +
>  handled = consider_iface_claim(iface_rec, iface_id, b_ctx_in,
> b_ctx_out, qos_map_ptr);
>  if (!handled) {
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 34b6abfc0..bc59f3f13 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -28535,6 +28535,11 @@ check ovn-nbctl --wait=sb set logical_switch_port
> vm1 parent_name=vm-cont1
>
>  wait_for_ports_up
>
> +# Try to claim container port to ovs
> +check ovn-nbctl set logical_switch_port vm-cont2 parent_name=vm2
> +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm-cont2
> +AT_CHECK([test 1 = `cat hv1/ovn-controller.log |grep -c "claim lport
> vm-cont2 of type container"`])
> +
>  # Delete vm1, vm-cont1 and vm-cont2 and recreate again.
>  check ovn-nbctl lsp-del vm1
>  check ovn-nbctl lsp-del vm-cont1
> @@ -28546,6 +28551,12 @@ check ovn-nbctl lsp-add ls vm-cont1 vm1 1
>  check ovn-nbctl lsp-add ls vm-cont2 vm1 2
>
>  wait_for_ports_up
> +check as hv1 ovn-appctl -t ovn-controller debug/pause
> +# Try to claim container port to ovs with recompute
> +check ovn-nbctl set logical_switch_port vm-cont2 parent_name=vm2
> +check as hv1 ovs-vsctl set Interface vm1 external_ids:iface-id=vm-cont2
> +check as hv1 ovn-appctl -t ovn-controller debug/resume
> +AT_CHECK([test 1 = `cat hv1/ovn-controller.log |grep -c "claim lport
> vm-cont2 of type container"`])
>
>  # Make vm1 as a child port of some non existent lport - foo. vm1,
> vm1-cont1 and
>  # vm1-cont2 should be released.
> --
> 2.34.1
>
> ___
> dev mailing list
> 

Re: [ovs-dev] [PATCH ovn] physical.c: Avoid NULL ptr deref in populate_remote_chassis_macs

[Numan Siddique]

On Tue, May 10, 2022 at 1:40 AM Ales Musil  wrote:

> When the "ovn-chassis-mac-mappings" had wrong format it would
> crash the ovn-controller as it tried to deref NULL pointer.
>
> Add check if the token is a valid pointer, if not log warning
> and skip this value.
>
> Reported-at: https://bugzilla.redhat.com/2082341
> Signed-off-by: Ales Musil 
>

Thanks for fixing this.

---
>  controller/physical.c   |  6 ++
>  tests/ovn-controller.at | 28 
>  2 files changed, 34 insertions(+)
>
> diff --git a/controller/physical.c b/controller/physical.c
> index 73320..71e22cbf0 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -426,6 +426,12 @@ populate_remote_chassis_macs(const struct
> sbrec_chassis *my_chassis,
>  char *save_ptr2 = NULL;
>  char *chassis_mac_bridge = strtok_r(token, ":", _ptr2);
>  char *chassis_mac_str = strtok_r(NULL, "", _ptr2);
> +if(!chassis_mac_str) {
>

As per the coding guidelines,  this should be
if (!chassis_mac_str)

I think Mark/Han can fix it before applying it.

Acked-by: Numan Siddique 

Numan

+VLOG_WARN("Parsing of ovn-chassis-mac-mappings failed for:
> "
> +  "\"%s\", the correct format is
> \"br-name1:MAC1\".",
> +  token);
> +continue;
> +}
>  struct remote_chassis_mac *remote_chassis_mac = NULL;
>  remote_chassis_mac = xmalloc(sizeof *remote_chassis_mac);
>  hmap_insert(_chassis_macs,
> _chassis_mac->hmap_node,
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 88ec2f269..dcadab1f3 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -2154,3 +2154,31 @@ AT_CHECK_UNQUOTED([echo $lflow_run_1], [0],
> [$lflow_run_2
>
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
> +
> +
> +AT_SETUP([ovn-controller - check ovn-chassis-mac-mappings])
> +
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +as hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +pid=$(cat hv1/ovn-controller.pid)
> +
> +# Add chassis with some ovn-chassis-mac-mappings
> +AT_CHECK([ovn-sbctl chassis-add foo geneve 127.0.0.2])
> +AT_CHECK([ovn-sbctl set chassis foo
> other_config:ovn-chassis-mac-mappings="invalid1,invalid2,br1:00:00:00:00:00:00"])
> +AT_CHECK([ovn-nbctl --wait=hv sync])
> +
> +# Check if ovn-controller is still alive
> +AT_CHECK([ps $pid], [0], [ignore])
> +# Check if we got warnings for invalid
> +AT_CHECK([grep "Parsing of ovn-chassis-mac-mappings failed"
> hv1/ovn-controller.log | grep -q invalid1])
> +AT_CHECK([grep "Parsing of ovn-chassis-mac-mappings failed"
> hv1/ovn-controller.log | grep -q invalid2])
> +AT_CHECK([grep "Parsing of ovn-chassis-mac-mappings failed"
> hv1/ovn-controller.log | grep -q br1], [1])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> --
> 2.35.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCHv2 ovn] ovn-controller: Allow configuration of tunnel DF bits

[Numan Siddique]

On Fri, May 13, 2022 at 8:34 AM Jochen Friedrich via dev <
ovs-dev@openvswitch.org> wrote:

> Add ovn-encap-df_default configuration option to set df_default on OVN
> tunnels.
>
> Signed-off-by: Jochen Friedrich 
>

Thanks for the v2.

The patch LGTM.  Can you please also add the documentation about the new
option somewhere here -
https://github.com/ovn-org/ovn/blob/main/controller/ovn-controller.8.xml#L178

Thanks
Numan

> ---
>  controller/encaps.c | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/controller/encaps.c b/controller/encaps.c
> index ed01b1368..0ad8fe16c 100644
> --- a/controller/encaps.c
> +++ b/controller/encaps.c
> @@ -190,6 +190,13 @@ tunnel_add(struct tunnel_ctx *tc, const struct
> sbrec_sb_global *sbg,
>  smap_add(, "tos", encap_tos);
>  }
>
> +/* If the df_default option is configured, get it */
> +
> +const char *encap_df = smap_get(>external_ids,
> "ovn-encap-df_default");
> +if (encap_df) {
> +smap_add(, "df_default", encap_df);
> +}
> +
>  /* If ovn-set-local-ip option is configured, get it */
>  set_local_ip = smap_get_bool(>external_ids,
> "ovn-set-local-ip",
>   false);
> --
> 2.27.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v3 0/2] Fix ALB parameters type and value mismatch.

[Kevin Traynor]

On 15/05/2022 08:09, mit...@outlook.com wrote:

From: Lin Huang 

This series fix ALB parameters type mismatch and set a hard limit
to rebalance_intvl.

It also includes some self-tests.

Changes since v2:
   1. Fix type mismatch int dpif-netdev.c.

Changes since v1:
   1. Fix the commit message and formatting.
   2. Fix typo in unit tests.

Lin Huang (2):
   dpif-netdev: Fix ALB parameters type mismatch.
   dpif-netdev : Fix ALB 'rebalance_intvl' max hard limit.

  lib/dpif-netdev.c | 28 
  tests/alb.at  | 20 +++-
  2 files changed, 35 insertions(+), 13 deletions(-)



The changes lgtm, just need the checkpatch CI warnings fixed and I can Ack.

fyi - you can check these before submission with the checkpatch script e.g.
$ git format-patch -2
$ ./utilities/checkpatch.py *.patch

thanks,
Kevin.

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

Re: [ovs-dev] [PATCH v3 2/2] dpif-netdev : Fix ALB 'rebalance_intvl' max hard limit.

[Kevin Traynor]

On 15/05/2022 08:09, mit...@outlook.com wrote:

From: linhuang 

Currently the pmd-auto-lb-rebal-interval's value was not been
checked properly.
It maybe a negative, or too big value (>2 weeks between rebalances),
which will be lead to a big unsigned value. So reset it to default
if the value exceeds the max permitted as described in vswitchd.xml.

Fixes: 5bf84282482a ("Adding support for PMD auto load balancing")
Signed-off-by: Lin Huang linhu...@ruijie.com.cn


The CI is complaining here too that the emails don't match (and <> are 
missing from the email here too.)



---
  lib/dpif-netdev.c |  6 +-
  tests/alb.at  | 20 +++-
  2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 904f4ec92..503539841 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -93,7 +93,8 @@ VLOG_DEFINE_THIS_MODULE(dpif_netdev);
  /* Auto Load Balancing Defaults */
  #define ALB_IMPROVEMENT_THRESHOLD25
  #define ALB_LOAD_THRESHOLD   95
-#define ALB_REBALANCE_INTERVAL   1 /* 1 Min */
+#define ALB_REBALANCE_INTERVAL   1 /* 1 Min */
+#define MAX_ALB_REBALANCE_INTERVAL   2 /* 2 Min */
  #define MIN_TO_MSEC  6
  
  #define FLOW_DUMP_MAX_BATCH 50

@@ -4881,6 +4882,9 @@ dpif_netdev_set_config(struct dpif *dpif, const struct 
smap *other_config)
  
  rebalance_intvl = smap_get_ullong(other_config, "pmd-auto-lb-rebal-interval",

ALB_REBALANCE_INTERVAL);
+if (rebalance_intvl > MAX_ALB_REBALANCE_INTERVAL) {
+rebalance_intvl = ALB_REBALANCE_INTERVAL;
+}
  
  /* Input is in min, convert it to msec. */

  rebalance_intvl =
diff --git a/tests/alb.at b/tests/alb.at
index 0036bd1f2..922185d61 100644
--- a/tests/alb.at
+++ b/tests/alb.at
@@ -243,7 +243,25 @@ get_log_next_line_num
  AT_CHECK([ovs-vsctl set open_vswitch . 
other_config:pmd-auto-lb-rebal-interval="0"])
  CHECK_ALB_PARAM([interval], [1 mins], [+$LINENUM])
  
-# No check for above max as it is only a documented max value and not a hard limit

+# Set new value
+get_log_next_line_num
+AT_CHECK([ovs-vsctl set open_vswitch . 
other_config:pmd-auto-lb-rebal-interval="100"])
+CHECK_ALB_PARAM([interval], [100 mins], [+$LINENUM])
+
+# Set above max value
+get_log_next_line_num
+AT_CHECK([ovs-vsctl set open_vswitch . 
other_config:pmd-auto-lb-rebal-interval="20001"])
+CHECK_ALB_PARAM([interval], [1 mins], [+$LINENUM])
+
+# Set new value
+get_log_next_line_num
+AT_CHECK([ovs-vsctl set open_vswitch . 
other_config:pmd-auto-lb-rebal-interval="1000"])
+CHECK_ALB_PARAM([interval], [1000 mins], [+$LINENUM])
+
+# Set Negative value
+get_log_next_line_num
+AT_CHECK([ovs-vsctl set open_vswitch . 
other_config:pmd-auto-lb-rebal-interval="-1"])
+CHECK_ALB_PARAM([interval], [1 mins], [+$LINENUM])
  
  OVS_VSWITCHD_STOP

  AT_CLEANUP


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

Re: [ovs-dev] [PATCH v3 1/2] dpif-netdev: Fix ALB parameters type mismatch.

[Kevin Traynor]

Hi Lin,

On 15/05/2022 08:09, mit...@outlook.com wrote:

From: linhuang 

The ALB parameters should never be negative.
So it's to use unsigned smap_get versions to get it properly, and
update VLOG formatting.

Fixes: 5bf84282482a ("Adding support for PMD auto load balancing")
Signed-off-by: Lin Huang 


The CI is complaining about some items:
http://patchwork.ozlabs.org/project/openvswitch/list/?series=300268

Here the From: and Signed-off-by: email addresses should match exactly


---
  lib/dpif-netdev.c | 22 +++---
  1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 61929049c..904f4ec92 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4777,8 +4777,8 @@ dpif_netdev_set_config(struct dpif *dpif, const struct 
smap *other_config)
  uint32_t insert_min, cur_min;
  uint32_t tx_flush_interval, cur_tx_flush_interval;
  uint64_t rebalance_intvl;
-uint8_t rebalance_load, cur_rebalance_load;
-uint8_t rebalance_improve;
+uint8_t cur_rebalance_load;
+uint32_t rebalance_load, rebalance_improve;
  bool log_autolb = false;
  enum sched_assignment_type pmd_rxq_assign_type;
  
@@ -4879,8 +4879,8 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config)
  
  struct pmd_auto_lb *pmd_alb = >pmd_alb;
  
-rebalance_intvl = smap_get_int(other_config, "pmd-auto-lb-rebal-interval",

-   ALB_REBALANCE_INTERVAL);
+rebalance_intvl = smap_get_ullong(other_config, 
"pmd-auto-lb-rebal-interval",


This line is getting flagged as being too long now that it has some 
extra characters, so it needs an extra break.



+  ALB_REBALANCE_INTERVAL);
  
  /* Input is in min, convert it to msec. */

  rebalance_intvl =
@@ -4893,21 +4893,21 @@ dpif_netdev_set_config(struct dpif *dpif, const struct 
smap *other_config)
  log_autolb = true;
  }
  
-rebalance_improve = smap_get_int(other_config,

- "pmd-auto-lb-improvement-threshold",
- ALB_IMPROVEMENT_THRESHOLD);
+rebalance_improve = smap_get_uint(other_config,
+  "pmd-auto-lb-improvement-threshold",
+  ALB_IMPROVEMENT_THRESHOLD);
  if (rebalance_improve > 100) {
  rebalance_improve = ALB_IMPROVEMENT_THRESHOLD;
  }
  if (rebalance_improve != pmd_alb->rebalance_improve_thresh) {
  pmd_alb->rebalance_improve_thresh = rebalance_improve;
  VLOG_INFO("PMD auto load balance improvement threshold set to "
-  "%"PRIu8"%%", rebalance_improve);
+  "%"PRIu32"%%", rebalance_improve);
  log_autolb = true;
  }
  
-rebalance_load = smap_get_int(other_config, "pmd-auto-lb-load-threshold",

-  ALB_LOAD_THRESHOLD);
+rebalance_load = smap_get_uint(other_config, "pmd-auto-lb-load-threshold",
+   ALB_LOAD_THRESHOLD);
  if (rebalance_load > 100) {
  rebalance_load = ALB_LOAD_THRESHOLD;
  }
@@ -4915,7 +4915,7 @@ dpif_netdev_set_config(struct dpif *dpif, const struct 
smap *other_config)
  if (rebalance_load != cur_rebalance_load) {
  atomic_store_relaxed(_alb->rebalance_load_thresh,
   rebalance_load);
-VLOG_INFO("PMD auto load balance load threshold set to %"PRIu8"%%",
+VLOG_INFO("PMD auto load balance load threshold set to %"PRIu32"%%",
rebalance_load);
  log_autolb = true;
  }


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

Re: [ovs-dev] [PATCH dpdk-latest] system-dpdk: Update vhost tests to be compatible with DPDK 22.03.

[Maxime Coquelin]

Hi Sunil;

On 5/17/22 16:11, Sunil Pai G wrote:

The DPDK commit [1] improves the socket layer logs in the vhost library
to ease log filtering and debugging.
Update the system-dpdk vhost tests to reflect this change.

[1] c85c35b1d447 ("vhost: improve socket layer logs")

Signed-off-by: Sunil Pai G 
---
  tests/system-dpdk.at | 22 +++---
  1 file changed, 11 insertions(+), 11 deletions(-)




Thanks for taking care of the adaptation:
Acked-by: Maxime Coquelin 

Maxime

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

Re: [ovs-dev] [PATCH v3 5/5] tests: Add check_pkt_len action test to system-offload-traffic.

[Phelan, Michael]

> -Original Message-
> From: Aaron Conole 
> Sent: Tuesday 17 May 2022 15:38
> To: Eelco Chaudron 
> Cc: 0-day Robot ; d...@openvswitch.org; Phelan, Michael
> 
> Subject: Re: [ovs-dev] [PATCH v3 5/5] tests: Add check_pkt_len action test to
> system-offload-traffic.
> 
> Eelco Chaudron  writes:
> 
> > On 16 May 2022, at 18:04, 0-day Robot wrote:
> >
> >> 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' to see the failed patch Patch
> >> failed at 0001 tests: Add check_pkt_len action test to 
> >> system-offload-traffic.
> >> 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
> >
> >
> > This failure was due to a merge conflict in the NEWS file. I will not
> > send a v4 for now as this will continue to happen. If the maintainers
> > do want a v4, I'll send it.
> 
> Understood that we know where the conflict is.
> 
> +Michael Phelan
> 
> Is it possible to get the test results for the previous patches?
Hey Aaron/Eelco,

The test was aborted due to the "netdev" in the series name, by trying to 
filter out OVN patches sometimes other patches give a false positive and get 
aborted incorrectly. I'll rerun the job and ensure the tests are run correctly 
and you can expect the results on the Build mailing list.

Thanks,
Michael.
> 
> > //Eelco

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

Re: [ovs-dev] [PATCH v5 5/5] acinclude: Add seperate checks for AVX512 ISA.

[Pai G, Sunil]

> Checking for each of the required AVX512 ISA separately will allow the
> compiler to generate some AVX512 code where there is some support in the
> compiler rather than only generating all AVX512 code when all of it is
> supported or no AVX512 code at all.
> 
> For example, in GCC 4.9 where there is just support for AVX512F, this
> patch will allow building the AVX512 DPIF.
> 
> Another example, in GCC 5 and 6, most AVX512 code can be generated, just
> without AVX512VPOPCNTDQ support.
> 
> Signed-off-by: Cian Ferriter 
> 
> ---
> v5:
> * Create a selector function for the permutexvar implementations based
>   on Sunil's feedback on the v4.  This hides the complexity of compile
>   time and run time selection of permutexvar implementations.
> * Add a comment explaining why VPOPCNTDQ_TARGET is defined and used.
> 
> v4:
> * Combine the 3 commits which added checks for AVX512 ISA into this
>   single commit since the first 2 commits were only useful and active
>   when the 3rd commit was applied. This also takes care of Sunil's
>   comment about explaining that the first 2 commits are precursors.
> * Don't check for AVX512DQ availability in the compiler. This ISA isn't
>   used in OVS.
> * Put all AVX512 ISA checks in the OVS_CHECK_AVX512 macro as per Sunil's
>   feedback.
> * Define a function in acinclude.m4, (OVS_CONDITIONAL_CC_OPTION_DEFINE),
>   to help with checking for AVX512 ISA support in the compiler.
> * Remove the '__AVX512VPOPCNTDQ__' check. Use the HAVE_AVX512* pattern
>   consistently with all AVX512 ISA checks instead. Fixup the comment
>   explaining the _mm512_popcnt_epi64_wrapper() function to reflect this.


Based on the comments and responses to v4 and changes implemented in v5 , 

Acked-by: Sunil Pai G 

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

Re: [ovs-dev] [PATCH v2] tests: Add ovs-dpdk rate limiting unit tests.

[Aaron Conole]

Michael Phelan  writes:

> From: Seamus Ryan 
>
> This adds 4 new unit tests to the 'check-dpdk' subsystem that will
> test rate limiting functionality.
>
> Signed-off-by: Seamus Ryan 
> Signed-off-by: Michael Phelan 
> Co-authored-by: Michael Phelan 
>
> ---

Acked-by: Aaron Conole 

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

Re: [ovs-dev] [PATCH v3 5/5] tests: Add check_pkt_len action test to system-offload-traffic.

[Aaron Conole]

Eelco Chaudron  writes:

> On 16 May 2022, at 18:04, 0-day Robot wrote:
>
>> 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' to see the failed patch
>> Patch failed at 0001 tests: Add check_pkt_len action test to 
>> system-offload-traffic.
>> 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
>
>
> This failure was due to a merge conflict in the NEWS file. I will not
> send a v4 for now as this will continue to happen. If the maintainers
> do want a v4, I'll send it.

Understood that we know where the conflict is.

+Michael Phelan

Is it possible to get the test results for the previous patches?

> //Eelco

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

[ovs-dev] [PATCH dpdk-latest] system-dpdk: Update vhost tests to be compatible with DPDK 22.03.

[Sunil Pai G]

The DPDK commit [1] improves the socket layer logs in the vhost library
to ease log filtering and debugging.
Update the system-dpdk vhost tests to reflect this change.

[1] c85c35b1d447 ("vhost: improve socket layer logs")

Signed-off-by: Sunil Pai G 
---
 tests/system-dpdk.at | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
index 7d2715c4a..f73612f67 100644
--- a/tests/system-dpdk.at
+++ b/tests/system-dpdk.at
@@ -63,14 +63,14 @@ AT_CHECK([ovs-vsctl show], [], [stdout])
 sleep 2
 
 dnl Parse log file
-AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created" 
ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user 
client: socket created" ovs-vswitchd.log], [], [stdout])
 AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' 
mode, using client socket" ovs-vswitchd.log], [], [stdout])
-AT_CHECK([grep "VHOST_CONFIG: $OVS_RUNDIR/dpdkvhostclient0: reconnecting..." 
ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." 
ovs-vswitchd.log], [], [stdout])
 
 dnl Clean up
 AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], 
[stderr])
 OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
-\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0: No such 
file or directory@d
+\@VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) failed to connect: No such file 
or directory@d
 ])")
 AT_CLEANUP
 dnl --
@@ -97,11 +97,11 @@ AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuser0 -- set 
Interface dpdkvhostuser0
 AT_CHECK([ovs-vsctl show], [], [stdout])
 
 dnl Parse log file
-AT_CHECK([grep "VHOST_CONFIG: vhost-user server: socket created" \
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) vhost-user server: 
socket created" \
   ovs-vswitchd.log], [], [stdout])
 AT_CHECK([grep "Socket $OVS_RUNDIR/dpdkvhostuser0 created for vhost-user port 
dpdkvhostuser0" \
   ovs-vswitchd.log], [], [stdout])
-AT_CHECK([grep "VHOST_CONFIG: bind to $OVS_RUNDIR/dpdkvhostuser0" 
ovs-vswitchd.log], [],
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) binding succeeded" 
ovs-vswitchd.log], [],
  [stdout])
 
 dnl Set up namespaces
@@ -142,8 +142,8 @@ pkill -f -x -9 'tail -f /dev/null'
 dnl Clean up
 AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuser0], [], [stdout], [stderr])
 OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
-\@VHOST_CONFIG: recvmsg failed@d
-\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostuser0: No such file 
or directory@d
+\@VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) recvmsg failed@d
+\@VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostuser0) failed to connect: No such file 
or directory@d
 \@dpdkvhostuser ports are considered deprecated;  please migrate to 
dpdkvhostuserclient ports.@d
 \@failed to enumerate system datapaths: No such file or directory@d
 ])")
@@ -172,9 +172,9 @@ AT_CHECK([ovs-vsctl add-port br10 dpdkvhostuserclient0 -- 
set Interface \
 AT_CHECK([ovs-vsctl show], [], [stdout])
 
 dnl Parse log file
-AT_CHECK([grep "VHOST_CONFIG: vhost-user client: socket created" 
ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) vhost-user 
client: socket created" ovs-vswitchd.log], [], [stdout])
 AT_CHECK([grep "vHost User device 'dpdkvhostuserclient0' created in 'client' 
mode, using client socket" ovs-vswitchd.log], [], [stdout])
-AT_CHECK([grep "VHOST_CONFIG: $OVS_RUNDIR/dpdkvhostclient0: reconnecting..." 
ovs-vswitchd.log], [], [stdout])
+AT_CHECK([grep "VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) reconnecting..." 
ovs-vswitchd.log], [], [stdout])
 
 dnl Set up namespaces
 ADD_NAMESPACES(ns1, ns2)
@@ -214,8 +214,8 @@ pkill -f -x -9 'tail -f /dev/null'
 dnl Clean up
 AT_CHECK([ovs-vsctl del-port br10 dpdkvhostuserclient0], [], [stdout], 
[stderr])
 OVS_VSWITCHD_STOP("m4_join([], [SYSTEM_DPDK_ALLOWED_LOGS], [
-\@VHOST_CONFIG: recvmsg failed@d
-\@VHOST_CONFIG: failed to connect to $OVS_RUNDIR/dpdkvhostclient0: No such 
file or directory@d
+\@VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) recvmsg failed@d
+\@VHOST_CONFIG: ($OVS_RUNDIR/dpdkvhostclient0) failed to connect: No such file 
or directory@d
 \@dpdkvhostuser ports are considered deprecated;  please migrate to 
dpdkvhostuserclient ports.@d
 \@failed to enumerate system datapaths: No such file or directory@d
 ])")
-- 
2.25.1

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

[ovs-dev] OVN - flushing conntrack entries for newly allocated zones

[Dumitru Ceara]

Hi Justin,

I know it's a while ago since this patch [0][1] made it in OVN but I was
wondering if you remembered some of the reasons behind doing this.

More recently, OVN got a new feature to allow the CMS to explicitly
specify a zone ID to be used for conntrack for gateway routers [2].

The reason behind that was that, in some cases, ovn-kubernetes shares
the gateway router conntrack zone with the host [3].

The fact that an ovn-controller restart flushes the shared zone becomes
an issue sometimes.  We're trying to see if there's a way to avoid that,
potentially via an ovn-controller cmdline argument or configuration.

But before that, I wonder if you know of any CMS that relies on the
flush-on-use behavior.

Thanks,
Dumitru

[0]
https://github.com/ovn-org/ovn/commit/11b75557aebd7caa433658895ff5c5e0381c34a0
[1]
https://mail.openvswitch.org/pipermail/ovs-dev/2016-September/323402.html
[2]
https://github.com/ovn-org/ovn/commit/f9cab11d5fabe2ae321a3b4bad5972b61df958c0
[3]
https://github.com/ovn-org/ovn-kubernetes/commit/f2cc64dcfb9d3205eda5fd0cb0de340a7abcb565

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

Re: [ovs-dev] [PATCH v2 ovn 0/5] optimize lb lflow generation for gw router

[Mark Michelson]

Thank you Dumitru and Lorenzo. I added the "Reported-at" note to the 
final patch of the series.


I pushed the series to main.

On 5/17/22 07:14, Dumitru Ceara wrote:

Hi Lorenzo,

On 5/13/22 19:03, Lorenzo Bianconi wrote:

ovn-main:
-
Statistics for 'lflows_lbs'
   Total samples: 51
   Maximum: 1319 msec
   Minimum: 1282 msec
   95th percentile: 1309.836278 msec
   Short term average: 1296.054252 msec
   Long term average: 1295.728676 msec

ovn-main+optimization:
--
Statistics for 'lflows_lbs'
   Total samples: 51
   Maximum: 526 msec
   Minimum: 503 msec
   95th percentile: 524.794986 msec
   Short term average: 518.826313 msec
   Long term average: 509.005444 msec


This is a nice improvement!  I had a look at the changes and they seem
OK to me.  I acked the patches but maybe others can have another look
too before merging these.



Changes since v1:
- group together distributed routers similar to gw router
- fix force_snat flag bug

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2079834


Maybe the maintainers can do this when applying the series, otherwise
can you please add the "Reported-at" tag to the individual patches?
Like that our automation will pick up the reference between the OVN
commits and the BZ.



Lorenzo Bianconi (5):
   northd: move nat_flows_for_lb for skip_snat in a dedicated routine
   northd: move nat_flows_for_lb for force_nat in a dedicated routine
   northd: move nat_flows_for_lb for gw router in a dedicated routine
   northd: refactor logical router loop in build_lrouter_nat_flows_for_lb
   northd: optimize build_gw_lrouter_nat_flows_for_lb code

  northd/northd.c | 170 
  1 file changed, 129 insertions(+), 41 deletions(-)



Thanks,
Dumitru

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



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

Re: [ovs-dev] [PATCH ovn v2] Do not set 'up' attribute in the port-binding table if ovn-northd is at an older version.

[Mark Michelson]

OK thanks for the explanations, Xavier and Mary. This is pretty 
important since we expect that ovn-controller should be upgraded before 
ovn-northd.


I'm acking the patch because the content is correct. I suggest that when 
this is merged, the person that merges should expand on the commit 
message a bit to explain why the commit is necessary. If nothing else, 
make it clear that 20.09 is the cutoff between an "older" and "newer" 
version of ovn-northd in this case.


Acked-by: Mark Michelson

On 5/12/22 11:59, Mary Manohar wrote:

Thanks Xavier for clarifying.

Hi Mark,

Xavier’s explanation is bang on. The ‘up’ field is introduced in the 
Port Binding table post 20.09.


So, when the southbound is at 20.09 version, the set operation fails in 
ovn-controller.


Thanks

Mary

*From: *Xavier Simonart 
*Date: *Wednesday, May 11, 2022 at 8:10 AM
*To: *Mark Michelson 
*Cc: *Mary Manohar , ovs-dev@openvswitch.org 

*Subject: *Re: [ovs-dev] [PATCH ovn v2] Do not set 'up' attribute in the 
port-binding table if ovn-northd is at an older version.


Hi Mark

This is fixing a backward compatibility issue when using an older 
ovn-northd.


ovn-controller should only set Port_Binding.up field if the Southbound 
DB is aware of this field (or transaction would fail).


We already do this when setting pb up through notification (if-status 
module).


More explanations on commit 8b45fc9b2 from Dumitru.

Thanks

Xavier

On Tue, May 10, 2022 at 8:12 PM Mark Michelson > wrote:


Hi Mary,

I'm confused by this change. The summary mentions ovn-northd being
at an
older version, but there's no version check being performed in the
patch. Also, what constitutes an "older" version of ovn-northd? Why
shouldn't we set the port up in this case? What problem is being solved?

On 5/9/22 15:28, mary.mano...@nutanix.com
 wrote:
> From: Mary Manohar mailto:mary.mano...@nutanix.com>>
> 
> Signed-off-by: Mary Manohar mailto:mary.mano...@nutanix.com>>

> ---
>   controller/binding.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/controller/binding.c b/controller/binding.c

> index e284704..e7dc537 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -908,7 +908,9 @@ claimed_lport_set_up(const struct sbrec_port_binding 
*pb,
>       if (!notify_up) {
>           bool up = true;
>           if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) {
> -            sbrec_port_binding_set_up(pb, , 1);
> +            if (pb->n_up) {
> +                sbrec_port_binding_set_up(pb, , 1);
> +            }
>           }
>           return;
>       }
> 


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





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

Re: [ovs-dev] [PATCH v2] dpif-netdev: ct maxconns config persistence

[Aaron Conole]

lic121  writes:

> Max allowed userspace dp conntrack entries is configurable with
> 'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios,
> this configuration is expected to survive from host reboot, from
> ovs service restart.
>
> Signed-off-by: lic121 
> ---

Sorry - just one last comment, and then I think this can go in.

Please add a warning to dpctl_ct_set_maxconns that also flags the
deprecation, and add a NEWS entry as well.  That way end users are aware
of the change.  If you want to add NEWS entry as a separate patch
(because those can generate git-am related issues) that's probably okay.

> Notes:
> v2:
>   - rename "ct-maxconns" to "userspace-ct-maxconns"
>
>  lib/dpctl.man   |  4 +++-
>  lib/dpif-netdev.c   | 11 +++
>  tests/system-traffic.at | 10 ++
>  vswitchd/vswitch.xml|  7 +++
>  4 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index c100d0a..4f08a3f 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -343,7 +343,9 @@ system due to connection tracking or simply limiting 
> connection
>  tracking.  If the number of connections is already over the new maximum
>  limit request then the new maximum limit will be enforced when the
>  number of connections decreases to that limit, which normally happens
> -due to connection expiry.  Only supported for userspace datapath.
> +due to connection expiry.  Only supported for userspace datapath. This
> +command is deprecated by ovsdb cfg other_config:userspace-ct-maxconns
> +because of persistence capability.
>  .
>  .TP
>  \*(DX\fBct\-get\-maxconns\fR [\fIdp\fR]
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 6764343..e2348a1 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -4827,6 +4827,17 @@ dpif_netdev_set_config(struct dpif *dpif, const struct 
> smap *other_config)
>  }
>  }
>  
> +uint32_t ct_maxconns, cur_maxconns;
> +ct_maxconns = smap_get_int(other_config, "userspace-ct-maxconns",
> +   UINT32_MAX);
> +/* Leave runtime value as it is when cfg is removed. */
> +if (ct_maxconns < UINT32_MAX) {
> +conntrack_get_maxconns(dp->conntrack, _maxconns);
> +if (ct_maxconns != cur_maxconns) {
> +conntrack_set_maxconns(dp->conntrack, ct_maxconns);
> +}
> +}
> +
>  bool smc_enable = smap_get_bool(other_config, "smc-enable", false);
>  bool cur_smc;
>  atomic_read_relaxed(>smc_enable_db, _smc);
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 4a7fa49..82ea992 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -2258,6 +2258,16 @@ AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
>  10
>  ])
>  
> +AT_CHECK([ovs-vsctl set Open_vswitch . 
> other_config:userspace-ct-maxconns=20], [0])
> +AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> +20
> +])
> +
> +AT_CHECK([ovs-vsctl remove Open_vswitch . other_config 
> userspace-ct-maxconns], [0])
> +AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> +20
> +])
> +
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>  
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 0c66326..48f05d7 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -183,6 +183,13 @@
>  
>
>  
> +   +  type='{"type": "integer", "minInteger": 0}'>
> +The maximum number of connection tracker entries allowed in the
> +userspace datapath.  This deprecates "ovs-appctl 
> dpctl/ct-set-maxconns"
> +command.
> +  
> +
>type='{"type": "integer", "minInteger": 500}'>
>  

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

Re: [ovs-dev] [ovs-dev v4 3/3] checkpatch.py: add checks for experimental API.

[Aaron Conole]

Peng He  writes:

> Signed-off-by: Peng He 
> Acked-by: Eelco Chaudron 
> ---

Sorry it took so long to review.  I like this feature, and thanks for
the concise implementation.

Please add tests into tests/checkpatch.at to ensure that a future
change doesn't break it :)

With such a test, you can also add my

Acked-by: Aaron Conole 

>  utilities/checkpatch.py | 36 +---
>  1 file changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 8c7faa419..8c02ac3ce 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -620,6 +620,10 @@ def regex_error_factory(description):
>  return lambda: print_error(description)
>  
>  
> +def regex_warn_factory(description):
> +return lambda: print_warning(description)
> +
> +
>  std_functions = [
>  ('malloc', 'Use xmalloc() in place of malloc()'),
>  ('calloc', 'Use xcalloc() in place of calloc()'),
> @@ -636,6 +640,7 @@ std_functions = [
>  ('assert', 'Use ovs_assert() in place of assert()'),
>  ('error', 'Use ovs_error() in place of error()'),
>  ]
> +
>  checks += [
>  {'regex': r'(\.c|\.h)(\.in)?$',
>   'match_name': None,
> @@ -644,6 +649,21 @@ checks += [
>   'print': regex_error_factory(description)}
>  for (function_name, description) in std_functions]
>  
> +easy_to_misuse_api = [
> +('ovsrcu_barrier',
> +'lib/ovs-rcu.c',
> +'Are you sure you need to use ovsrcu_barrier(), '
> +'in most cases ovsrcu_synchronize() will be fine?'),
> +]
> +
> +checks += [
> +{'regex': r'(\.c)(\.in)?$',
> + 'match_name': lambda x: x != location,
> + 'prereq': lambda x: not is_comment_line(x),
> + 'check': regex_function_factory(function_name),
> + 'print': regex_warn_factory(description)}
> +for (function_name, location, description) in easy_to_misuse_api]
> +
>  
>  def regex_operator_factory(operator):
>  regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' % operator)
> @@ -676,12 +696,22 @@ def get_file_type_checks(filename):
>  global checks
>  checkList = []
>  for check in checks:
> +regex_check = True
> +match_check = True
> +
>  if check['regex'] is None and check['match_name'] is None:
>  checkList.append(check)
> +continue
> +
>  if check['regex'] is not None and \
> -   re.compile(check['regex']).search(filename) is not None:
> -checkList.append(check)
> -elif check['match_name'] is not None and 
> check['match_name'](filename):
> +   re.compile(check['regex']).search(filename) is None:
> +regex_check = False
> +
> +if check['match_name'] is not None and \
> +not check['match_name'](filename):
> +match_check = False
> +
> +if regex_check and match_check:
>  checkList.append(check)
>  return checkList

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

Re: [ovs-dev] [v4 4/8] netdev-linux: Add functions to manipulate tc police action

[Jianbo Liu via dev]

On Fri, 2022-05-13 at 16:55 +0200, Eelco Chaudron wrote:
> 
> 
> On 3 May 2022, at 5:08, Jianbo Liu via dev wrote:
> 
> > Add helpers to add, delete and get stats of police action with
> > the specified index.
> 
> See inline comments… This is the last patch for this week, I’ll
> continue the review sometime next week!
> 
> 
> > Signed-off-by: Jianbo Liu 
> > ---
> >  lib/netdev-linux.c | 133
> > +
> >  lib/netdev-linux.h |   6 ++
> >  lib/tc.c   |  21 +++
> >  lib/tc.h   |   6 ++
> >  4 files changed, 166 insertions(+)
> > 
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index eb05153c0..ef6c7312f 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -5664,6 +5664,139 @@ tc_add_policer(struct netdev *netdev,
> > uint32_t kbits_rate,
> >  return 0;
> >  }
> > 
> > +int
> > +tc_add_policer_action(uint32_t index, uint32_t kbits_rate,
> > +  uint32_t kbits_burst, uint32_t pkts_rate,
> > +  uint32_t pkts_burst, bool update)
> > +{
> > +    struct tc_police tc_police;
> > +    struct ofpbuf request;
> > +    struct tcamsg *tcamsg;
> > +    size_t offset;
> > +    int flags;
> > +
> > +    tc_policer_init(_police, kbits_rate, kbits_burst);
> > +    tc_police.index = index;
> > +
> > +    flags = (update ? NLM_F_REPLACE : NLM_F_EXCL) | NLM_F_CREATE;
> > +    tcamsg = tc_make_action_request(RTM_NEWACTION, flags,
> > );
> > +    if (!tcamsg) {
> > +    return ENODEV;
> > +    }
> > +
> > +    offset = nl_msg_start_nested(, TCA_ACT_TAB);
> > +    nl_msg_put_act_police(, _police, pkts_rate,
> > pkts_burst);
> > +    nl_msg_end_nested(, offset);
> > +
> > +    return tc_transact(, NULL);
> > +}
> > +
> > +static int
> > +tc_update_policer_action_stats(struct ofpbuf *msg,
> > +   struct ofputil_meter_stats *stats)
> > +{
> > +    const struct nlattr *act = NULL;
> > +    struct tc_flower flower;
> > +    struct nlattr *prio;
> > +    struct tcamsg *tca;
> > +    int error;
> > +
> 
> If would also do the if (!stats) return check here so none of the
> APIs will crash if messed up.
> 
> > +    if (NLMSG_HDRLEN + sizeof *tca > msg->size) {
> > +    return EPROTO;
> > +    }
> > +
> > +    tca = ofpbuf_at_assert(msg, NLMSG_HDRLEN, sizeof *tca);
> > +
> > +    act = nl_attr_find(msg, NLMSG_HDRLEN + sizeof *tca,
> > TCA_ACT_TAB);
> > +    if (!act) {
> > +    return EPROTO;
> > +    }
> > +
> > +    prio = (struct nlattr *) act + 1;
> > +    memset(, 0, sizeof(struct tc_flower));
> > +    error = tc_parse_single_action(prio, , false);
> 
> I do not like this approach, we zero out a complex data structure
> pass into a general function, and hope it gives us a counter we need.
> I think we should separate out the statistics handling from
> nl_parse_single_action() and make it available to use here.
> 
> > +    if (!error) {
> > +    stats->packet_in_count +=
> > +    get_32aligned_u64(_sw.n_packets);
> > +    stats->byte_in_count +=
> > get_32aligned_u64(_sw.n_bytes);
> > +    stats->packet_in_count +=
> > +    get_32aligned_u64(_hw.n_packets);
> > +    stats->byte_in_count +=
> > get_32aligned_u64(_hw.n_bytes);
> 
> What about the band stats on dropped packets? We need this to be in
> line with the kernel dp.
> 

It looks something wrong with tc police stats for the dropped packets,
and someone is working on the kernel. Maybe we have to ignore this
issue now?

> > +    }
> > +
> > +    return error;
> > +}
> > +
> > +int
> > +tc_get_policer_action(uint32_t index, struct ofputil_meter_stats
> > *stats)
> > +{
> > +    struct ofpbuf *replyp = NULL;
> > +    struct ofpbuf request;
> > +    struct tcamsg *tcamsg;
> > +    size_t root_offset;
> > +    size_t prio_offset;
> > +    int prio = 0;
> > +    int error;
> 
> If you do not add an “if !stats” check in
> tc_update_policer_action_stats(), I would add it here to avoid
> crashes with invalid API callback arguments.
> 
> > +    tcamsg = tc_make_action_request(RTM_GETACTION, 0, );
> > +    if (!tcamsg) {
> > +    return ENODEV;
> > +    }
> > +
> > +    root_offset = nl_msg_start_nested(, TCA_ACT_TAB);
> > +    prio_offset = nl_msg_start_nested(, ++prio);
> 
> Why do we need the ++prio variable? Can we just not call it with 1?
> 
> > +    nl_msg_put_string(, TCA_ACT_KIND, "police");
> > +    nl_msg_put_u32(, TCA_ACT_INDEX, index);
> > +    nl_msg_end_nested(, prio_offset);
> > +    nl_msg_end_nested(, root_offset);
> > +
> > +    error = tc_transact(, );
> > +    if (error) {
> > +    VLOG_ERR_RL(, "failed to dump police action (index:
> > %u), err=%d",
> 
> Capital F for Failed.
> 
> > +    index, error);
> > +    return error;
> > +    }
> > +
> > +    error = tc_update_policer_action_stats(replyp, stats);
> > +    if (error) {
> > +    VLOG_ERR_RL(, "failed to update police stats (index:
> > %u), err=%d",
> > +    index, 

Re: [ovs-dev] [EXT] Re: OVS DPDK DMA-Dev library/Design Discussion

[Radha Chintakuntla]

> -Original Message-
> From: Bruce Richardson 
> Sent: Friday, May 13, 2022 3:34 AM
> To: fengchengwen 
> Cc: Pai G, Sunil ; Ilya Maximets
> ; Radha Chintakuntla ;
> Veerasenareddy Burru ; Gagandeep Singh
> ; Nipun Gupta ; Stokes, Ian
> ; Hu, Jiayu ; Ferriter, Cian
> ; Van Haaren, Harry
> ; Maxime Coquelin
> (maxime.coque...@redhat.com) ; ovs-
> d...@openvswitch.org; d...@dpdk.org; Mcnamara, John
> ; O'Driscoll, Tim ;
> Finn, Emma 
> Subject: [EXT] Re: OVS DPDK DMA-Dev library/Design Discussion
> 
> External Email
> 
> --
> On Fri, May 13, 2022 at 05:48:35PM +0800, fengchengwen wrote:
> > On 2022/5/13 17:10, Bruce Richardson wrote:
> > > On Fri, May 13, 2022 at 04:52:10PM +0800, fengchengwen wrote:
> > >> On 2022/4/8 14:29, Pai G, Sunil wrote:
> >  -Original Message-
> >  From: Richardson, Bruce 
> >  Sent: Tuesday, April 5, 2022 5:38 PM
> >  To: Ilya Maximets ; Chengwen Feng
> >  ; Radha Mohan Chintakuntla
> >  ; Veerasenareddy Burru
> ;
> >  Gagandeep Singh ; Nipun Gupta
> >  
> >  Cc: Pai G, Sunil ; Stokes, Ian
> >  ; Hu, Jiayu ; Ferriter,
> >  Cian ; Van Haaren, Harry
> >  ; Maxime Coquelin
> >  (maxime.coque...@redhat.com) ;
> >  ovs-dev@openvswitch.org; d...@dpdk.org; Mcnamara, John
> >  ; O'Driscoll, Tim
> >  ; Finn, Emma 
> >  Subject: Re: OVS DPDK DMA-Dev library/Design Discussion
> > 
> >  On Tue, Apr 05, 2022 at 01:29:25PM +0200, Ilya Maximets wrote:
> > > On 3/30/22 16:09, Bruce Richardson wrote:
> > >> On Wed, Mar 30, 2022 at 01:41:34PM +0200, Ilya Maximets wrote:
> > >>> On 3/30/22 13:12, Bruce Richardson wrote:
> >  On Wed, Mar 30, 2022 at 12:52:15PM +0200, Ilya Maximets
> wrote:
> > > On 3/30/22 12:41, Ilya Maximets wrote:
> > >> Forking the thread to discuss a memory consistency/ordering
> model.
> > >>
> > >> AFAICT, dmadev can be anything from part of a CPU to a
> > >> completely separate PCI device.  However, I don't see any
> > >> memory ordering being enforced or even described in the
> > >> dmadev API or
> >  documentation.
> > >> Please, point me to the correct documentation, if I somehow
> > >> missed
> >  it.
> > >>
> > >> We have a DMA device (A) and a CPU core (B) writing
> > >> respectively the data and the descriptor info.  CPU core
> > >> (C) is reading the descriptor and the data it points too.
> > >>
> > >> A few things about that process:
> > >>
> > >> 1. There is no memory barrier between writes A and B (Did I
> miss
> > >>them?).  Meaning that those operations can be seen by C in
> a
> > >>different order regardless of barriers issued by C and
> >  regardless
> > >>of the nature of devices A and B.
> > >>
> > >> 2. Even if there is a write barrier between A and B, there is
> > >>no guarantee that C will see these writes in the same order
> > >>as C doesn't use real memory barriers because vhost
> > >> advertises
> > >
> > > s/advertises/does not advertise/
> > >
> > >>VIRTIO_F_ORDER_PLATFORM.
> > >>
> > >> So, I'm getting to conclusion that there is a missing write
> > >> barrier on the vhost side and vhost itself must not
> > >> advertise the
> > >
> > > s/must not/must/
> > >
> > > Sorry, I wrote things backwards. :)
> > >
> > >> VIRTIO_F_ORDER_PLATFORM, so the virtio driver can use
> > >> actual memory barriers.
> > >>
> > >> Would like to hear some thoughts on that topic.  Is it a
> > >> real
> >  issue?
> > >> Is it an issue considering all possible CPU architectures
> > >> and DMA HW variants?
> > >>
> > 
> >  In terms of ordering of operations using dmadev:
> > 
> >  * Some DMA HW will perform all operations strictly in order e.g.
> >  Intel
> >    IOAT, while other hardware may not guarantee order of
> >  operations/do
> >    things in parallel e.g. Intel DSA. Therefore the dmadev API
> >  provides the
> >    fence operation which allows the order to be enforced. The
> >  fence
> >  can be
> >    thought of as a full memory barrier, meaning no jobs after
> >  the
> >  barrier can
> >    be started until all those before it have completed.
> >  Obviously,
> >  for HW
> >    where order is always enforced, this will be a no-op, but
> >  for
> >  hardware that
> >    parallelizes, we want to reduce the fences to get best
> >  performance.
> > 
> >  * For synchronization between DMA devices and CPUs, where a
> >  CPU 

Re: [ovs-dev] [PATCH v2 ovn 5/5] northd: optimize build_gw_lrouter_nat_flows_for_lb code

[Dumitru Ceara]

On 5/13/22 19:03, Lorenzo Bianconi wrote:
> Optimize lflow generation code in build_gw_lrouter_nat_flows_for_lb when
> the system is running with datapath group enabled.
> The following tests have been carried out on running ovn-northd on a real
> openshift db composed of 120 nodes, 10K pods, and 10K load
> balancers applied to all node logical switches and routers:
> 
> ovn-main:
> -
> Statistics for 'lflows_lbs'
>   Total samples: 51
>   Maximum: 1319 msec
>   Minimum: 1282 msec
>   95th percentile: 1309.836278 msec
>   Short term average: 1296.054252 msec
>   Long term average: 1295.728676 msec
> 
> ovn-main+optimization:
> --
> Statistics for 'lflows_lbs'
>   Total samples: 51
>   Maximum: 526 msec
>   Minimum: 503 msec
>   95th percentile: 524.794986 msec
>   Short term average: 518.826313 msec
>   Long term average: 509.005444 msec
> 
> Signed-off-by: Lorenzo Bianconi 
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

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

Re: [ovs-dev] [PATCH v2 ovn 0/5] optimize lb lflow generation for gw router

[Dumitru Ceara]

Hi Lorenzo,

On 5/13/22 19:03, Lorenzo Bianconi wrote:
> ovn-main:
> -
> Statistics for 'lflows_lbs'
>   Total samples: 51
>   Maximum: 1319 msec
>   Minimum: 1282 msec
>   95th percentile: 1309.836278 msec
>   Short term average: 1296.054252 msec
>   Long term average: 1295.728676 msec
> 
> ovn-main+optimization:
> --
> Statistics for 'lflows_lbs'
>   Total samples: 51
>   Maximum: 526 msec
>   Minimum: 503 msec
>   95th percentile: 524.794986 msec
>   Short term average: 518.826313 msec
>   Long term average: 509.005444 msec

This is a nice improvement!  I had a look at the changes and they seem
OK to me.  I acked the patches but maybe others can have another look
too before merging these.

> 
> Changes since v1:
> - group together distributed routers similar to gw router
> - fix force_snat flag bug
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2079834

Maybe the maintainers can do this when applying the series, otherwise
can you please add the "Reported-at" tag to the individual patches?
Like that our automation will pick up the reference between the OVN
commits and the BZ.

> 
> Lorenzo Bianconi (5):
>   northd: move nat_flows_for_lb for skip_snat in a dedicated routine
>   northd: move nat_flows_for_lb for force_nat in a dedicated routine
>   northd: move nat_flows_for_lb for gw router in a dedicated routine
>   northd: refactor logical router loop in build_lrouter_nat_flows_for_lb
>   northd: optimize build_gw_lrouter_nat_flows_for_lb code
> 
>  northd/northd.c | 170 
>  1 file changed, 129 insertions(+), 41 deletions(-)
> 

Thanks,
Dumitru

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

Re: [ovs-dev] [PATCH v2 ovn 4/5] northd: refactor logical router loop in build_lrouter_nat_flows_for_lb

[Dumitru Ceara]

On 5/13/22 19:03, Lorenzo Bianconi wrote:
> Similar to gw routers, group together distributed router for processing
> them in build_lrouter_nat_flows_for_lb routine.
> 
> Signed-off-by: Lorenzo Bianconi 
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

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

Re: [ovs-dev] [PATCH v2 ovn 3/5] northd: move nat_flows_for_lb for gw router in a dedicated routine

[Dumitru Ceara]

On 5/13/22 19:03, Lorenzo Bianconi wrote:
> This is a preliminary patch to reduce load balancer logical flows
> computation cost.
> 
> Signed-off-by: Lorenzo Bianconi 
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

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

Re: [ovs-dev] [PATCH v2 ovn 2/5] northd: move nat_flows_for_lb for force_nat in a dedicated routine

[Dumitru Ceara]

On 5/13/22 19:03, Lorenzo Bianconi wrote:
> Similar to skip_snat counterpart, move force_nat gw router where a
> given lb is installed in a dedicated routine.
> This is a preliminary patch to reduce load balancer logical flows
> computation cost.
> 
> Signed-off-by: Lorenzo Bianconi 
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

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

Re: [ovs-dev] [PATCH v2 ovn 1/5] northd: move nat_flows_for_lb for skip_snat in a dedicated routine

[Dumitru Ceara]

On 5/13/22 19:03, Lorenzo Bianconi wrote:
> Group togheter gw routers where a given load balancer is installed
> dividing them according to snat_type flag (skip_snat, force_snat).
> This is a preliminary patch to reduce load balancer logical flows
> computation cost.
> 
> Signed-off-by: Lorenzo Bianconi 
> ---

Looks good to me, thanks!

Acked-by: Dumitru Ceara 

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

Re: [ovs-dev] [PATCH net-next v2] net: openvswitch: fix uAPI incompatibility with existing user space

[Eelco Chaudron]

On 12 May 2022, at 12:08, Vlad Buslov wrote:

> On Thu 12 May 2022 at 12:19, Eelco Chaudron  wrote:
>> On 7 Apr 2022, at 12:22, Ilya Maximets wrote:
>>
>>> On 4/7/22 10:02, Vlad Buslov wrote:
 On Mon 14 Mar 2022 at 20:40, Ilya Maximets  wrote:
> On 3/14/22 19:33, Roi Dayan wrote:
>>
>>
>> On 2022-03-10 8:44 PM, Aaron Conole wrote:
>>> Ilya Maximets  writes:
>>>
 Few years ago OVS user space made a strange choice in the commit [1]
 to define types only valid for the user space inside the copy of a
 kernel uAPI header.  '#ifndef __KERNEL__' and another attribute was
 added later.

 This leads to the inevitable clash between user space and kernel types
 when the kernel uAPI is extended.  The issue was unveiled with the
 addition of a new type for IPv6 extension header in kernel uAPI.

 When kernel provides the OVS_KEY_ATTR_IPV6_EXTHDRS attribute to the
 older user space application, application tries to parse it as
 OVS_KEY_ATTR_PACKET_TYPE and discards the whole netlink message as
 malformed.  Since OVS_KEY_ATTR_IPV6_EXTHDRS is supplied along with
 every IPv6 packet that goes to the user space, IPv6 support is fully
 broken.

 Fixing that by bringing these user space attributes to the kernel
 uAPI to avoid the clash.  Strictly speaking this is not the problem
 of the kernel uAPI, but changing it is the only way to avoid breakage
 of the older user space applications at this point.

 These 2 types are explicitly rejected now since they should not be
 passed to the kernel.  Additionally, OVS_KEY_ATTR_TUNNEL_INFO moved
 out from the '#ifdef __KERNEL__' as there is no good reason to hide
 it from the userspace.  And it's also explicitly rejected now, because
 it's for in-kernel use only.

 Comments with warnings were added to avoid the problem coming back.

 (1 << type) converted to (1ULL << type) to avoid integer overflow on
 OVS_KEY_ATTR_IPV6_EXTHDRS, since it equals 32 now.

   [1] beb75a40fdc2 ("userspace: Switching of L3 packets in L2 
 pipeline")

 Fixes: 28a3f0601727 ("net: openvswitch: IPv6: Add IPv6 extension 
 header support")
 Link: 
 https://lore.kernel.org/netdev/3adf00c7-fe65-3ef4-b6d7-6d8a0cad8...@nvidia.com
 Link: 
 https://github.com/openvswitch/ovs/commit/beb75a40fdc295bfd6521b0068b4cd12f6de507c
 Reported-by: Roi Dayan 
 Signed-off-by: Ilya Maximets 
 ---
>>>
>>> Acked-by: Aaron Conole 
>>>
>>
>>
>>
>> I got to check traffic with the fix and I do get some traffic
>> but something is broken. I didn't investigate much but the quick
>> test shows me rules are not offloaded and dumping ovs rules gives
>> error like this
>>
>> recirc_id(0),in_port(enp8s0f0_1),ct_state(-trk),eth(),eth_type(0x86dd),ipv6(frag=no)(bad
>> key length 2, expected -1)(00 00/(bad mask length 2, expected -1)(00 00),
>> packets:2453, bytes:211594, used:0.004s, flags:S., actions:ct,recirc(0x2)
>
> Such a dump is expected, because kernel parses fields that current
> userspace doesn't understand, and at the same time OVS by design is
> using kernel provided key/mask while installing datapath rules, IIRC.
> It should be possible to make these dumps a bit more friendly though.
>
> For the offloading not working, see my comment in the v2 patch email
> I sent (top email of this thread).  In short, it's a problem in user
> space and it can not be fixed from the kernel side, unless we revert
> IPv6 extension header support and never add any new types, which is
> unreasonable.  I didn't test any actual offloading, but I had a
> successful run of 'make check-offloads' with my quick'n'dirty fix from
> the top email.

 Hi Ilya,

 I can confirm that with latest OvS master IPv6 rules offload still fails
 without your pastebin code applied.

>
> Since we're here:
>
> Toms, do you plan to submit user space patches for this feature?

 I see there is a patch from you that is supposed to fix compatibility
 issues caused by this change in OvS d96d14b14733 ("openvswitch.h: Align
 uAPI definition with the kernel."), but it doesn't fix offload for me
 without pastebin patch.
>>>
>>> Yes.  OVS commit d96d14b14733 is intended to only fix the uAPI.
>>> Issue with offload is an OVS bug that should be fixed separately.
>>> The fix will also need to be backported to OVS stable branches.
>>>
 Do you plan to merge that code into OvS or you
 require some help from our side?
>>>
>>> I could do that, but I don't really have enough time.  So, if you
>>> can work on that fix, it would be great.  Note that comments 

[ovs-dev] [PATCH ovn] ovs-sandbox: Allow specifying initial contents for OVS and VTEP databases.

[Dumitru Ceara]

This makes it easier to troubleshoot OVN in a sandbox when initial
contents for the NB/SB/OVS DBs are provided (e.g., from a production
environment).

A way to load the DBs locally and run OVN against them is:
$ SANDBOXFLAGS="--nbdb-source=path-to-nb.db --sbdb-source=path-to-sb.db 
--ovs-source=path-to-conf.db" make sandbox

Signed-off-by: Dumitru Ceara 
---
 tutorial/ovs-sandbox | 28 ++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/tutorial/ovs-sandbox b/tutorial/ovs-sandbox
index d81e00496..2219b0e99 100755
--- a/tutorial/ovs-sandbox
+++ b/tutorial/ovs-sandbox
@@ -79,6 +79,8 @@ ovn_rbac=true
 n_northds=1
 n_ics=1
 n_controllers=1
+ovs_source=
+vtep_source=
 nbdb_model=standalone
 nbdb_servers=3
 nbdb_source=
@@ -307,6 +309,18 @@ EOF
 --sbdb-so*)
 prev=ovnsb_source
 ;;
+--ovs-so*=*)
+ovs_source=$optarg
+;;
+--ovs-so*)
+prev=ovs_source
+;;
+--vtep-so*=*)
+vtep_source=$optarg
+;;
+--vtep-so*)
+prev=vtep_source
+;;
 --ic-nb-s*=*)
 ic_nb_servers=$optarg
 ic_nb_model=clustered
@@ -474,10 +488,20 @@ trap 'kill `cat "$sandbox"/*.pid`' 0 1 2 3 13 14 15
 
 # Create database and start ovsdb-server.
 touch "$sandbox"/.conf.db.~lock~
-run ovsdb-tool create conf.db "$schema"
+if test ! -e "$ovs_source"; then
+run ovsdb-tool create conf.db "$schema"
+else
+run cp "$ovs_source" conf.db
+run ovsdb-tool convert conf.db "$schema"
+fi
 ovsdb_server_args=
 
-run ovsdb-tool create vtep.db "$vtep_schema"
+if test ! -e "$vtep_source"; then
+run ovsdb-tool create vtep.db "$vtep_schema"
+else
+run cp "$vtep_source" vtep.db
+run ovsdb-tool convert vtep.db "$vtep_schema"
+fi
 ovsdb_server_args="vtep.db conf.db"
 
 if [ "$HAVE_OPENSSL" = yes ]; then
-- 
2.27.0

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

Re: [ovs-dev] [PATCH v4 5/5] acinclude: Add seperate checks for AVX512 ISA.

[Ferriter, Cian]

> -Original Message-
> From: Pai G, Sunil 
> Sent: Thursday 5 May 2022 11:26
> To: Ferriter, Cian ; ovs-dev@openvswitch.org
> Cc: Van Haaren, Harry 
> Subject: RE: [PATCH v4 5/5] acinclude: Add seperate checks for AVX512 ISA.
> 
> Hi Cian,
> 
> Few comments inline.
> 
> 
> 

Hi Sunil,

Thanks for the review. Responses inline.

> > diff --git a/acinclude.m4 b/acinclude.m4 index 61e88105f..7b2889a40 100644
> > --- a/acinclude.m4
> > +++ b/acinclude.m4
> > @@ -73,16 +73,13 @@ AC_DEFUN([OVS_CHECK_DPIF_AVX512_DEFAULT], [
> >
> >  dnl OVS_CHECK_AVX512
> >  dnl
> > -dnl Checks if compiler and binutils supports AVX512.
> > +dnl Checks if compiler and binutils supports various AVX512 ISA.
> >  AC_DEFUN([OVS_CHECK_AVX512], [
> >OVS_CHECK_BINUTILS_AVX512
> > -  OVS_CHECK_CC_OPTION(
> > -[-mavx512f -mavx512vpopcntdq], [ovs_have_cc_mavx512f=yes],
> > [ovs_have_cc_mavx512f=no])
> > -  AM_CONDITIONAL([HAVE_AVX512F], [test $ovs_have_cc_mavx512f = yes])
> > -  if test "$ovs_have_cc_mavx512f" = yes; then
> > -AC_DEFINE([HAVE_AVX512F], [1],
> > -  [Define to 1 if compiler supports AVX512.])
> > -  fi
> > +  OVS_CONDITIONAL_CC_OPTION_DEFINE([-mavx512f], [HAVE_AVX512F])
> > + OVS_CONDITIONAL_CC_OPTION_DEFINE([-mavx512bw], [HAVE_AVX512BW])
> > + OVS_CONDITIONAL_CC_OPTION_DEFINE([-mavx512vbmi], [HAVE_AVX512VBMI])
> > + OVS_CONDITIONAL_CC_OPTION_DEFINE([-mavx512vpopcntdq],
> > + [HAVE_AVX512VPOPCNTDQ])
> >  ])
> >
> >  dnl OVS_ENABLE_WERROR
> > @@ -1360,6 +1357,19 @@ AC_DEFUN([OVS_CONDITIONAL_CC_OPTION],
> > AM_CONDITIONAL([$2], [test $ovs_have_cc_option = yes])])  dnl 
> > --
> >
> > +dnl OVS_CONDITIONAL_CC_OPTION_DEFINE([OPTION], [CONDITIONAL]) dnl Check
> > +whether the given C compiler OPTION is accepted.
> > +dnl If so, enable the given Automake CONDITIONAL and define it.
> > +dnl Example: OVS_CONDITIONAL_CC_OPTION_DEFINE([-mavx512f],
> > +[HAVE_AVX512F]) AC_DEFUN([OVS_CONDITIONAL_CC_OPTION_DEFINE],
> > +  [OVS_CHECK_CC_OPTION(
> > +[$1], [ovs_have_cc_option=yes], [ovs_have_cc_option=no])
> > +   AM_CONDITIONAL([$2], [test $ovs_have_cc_option = yes])
> > +   if test "$ovs_have_cc_option" = yes; then
> > + AC_DEFINE([$2], [1],
> > +   [Define to 1 if compiler supports the '$1' option.])
> > +   fi])
> > +
> 
> Thanks for creating a common version to check the CPUID flags.
> 
> 
> 
> >  # Build core vswitch libraries as before  lib_libopenvswitch_la_SOURCES =
> > \ diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-
> > avx512.c
> > index 2815bf480..7011eb00f 100644
> > --- a/lib/dpif-netdev-extract-avx512.c
> > +++ b/lib/dpif-netdev-extract-avx512.c
> 
> 
> > @@ -110,7 +110,9 @@ _mm512_maskz_permutex2var_epi8_skx(__mmask64 k_mask,
> 
> 
> Since we now include this file only when AVX512BW is available, I guess we 
> can remove the
> __attribute__((target("avx512bw"))) for _mm512_maskz_permutex2var_epi8_skx ?
> 
> 

Yes, good catch, we can and should remove this attribute target. This attribute 
target was unnecessary even before the changes in this patch. The -mavx512bw 
CFLAG is set for the entire AVX512 OVS library before and after the patch set. 

I'll remove this attribute target in the "dpif-netdev-extract: Remove 
unnecessary compiler targets." patch (3/5). I'll keep your Ack on patch 3/5 
since you suggested this change.

> >
> >  /* Wrapper function required to enable ISA. */  static inline __m512i
> > +#if HAVE_AVX512VBMI
> >  __attribute__((__target__("avx512vbmi")))
> > +#endif
> >  _mm512_maskz_permutexvar_epi8_wrap(__mmask64 kmask, __m512i idx, __m512i
> > a)  {
> >  return _mm512_maskz_permutexvar_epi8(kmask, idx, a); @@ -481,7 +483,7
> > @@ mfex_avx512_process(struct dp_packet_batch *packets,
> >  odp_port_t in_port,
> >  void *pmd_handle OVS_UNUSED,
> >  const enum MFEX_PROFILES profile_id,
> > -const uint32_t use_vbmi)
> > +const uint32_t use_vbmi OVS_UNUSED)
> >  {
> >  uint32_t hitmask = 0;
> >  struct dp_packet *packet;
> > @@ -544,7 +546,11 @@ mfex_avx512_process(struct dp_packet_batch *packets,
> >   */
> >  __m512i v512_zeros = _mm512_setzero_si512();
> >  __m512i v_blk0;
> > +#if HAVE_AVX512VBMI
> >  if (__builtin_constant_p(use_vbmi) && use_vbmi) {
> > +#else
> > +if (0) {
> > +#endif
> 
> 
> In my opinion, this affects the readability of the code.
> I'd rather define a separate inline wrapper for this and keep this lengthy 
> function clean, may be
> something like:
> 
> 
> #define OVS_SET_USED(x) (void)(x)
> 
> static inline __m512i
> __attribute__((target("avx512vbmi")))
> _mm512_maskz_permutexvar_epi8_wrap(__mmask64 k_shuf, __m512i v_shuf,
>__m512i v_pkt0, const uint32_t use_vbmi)
> {
> /* Permute the packet layout into miniflow blocks shape. */
> __m512i 

[ovs-dev] [PATCH v5 5/5] acinclude: Add seperate checks for AVX512 ISA.

[Cian Ferriter]

Checking for each of the required AVX512 ISA separately will allow the
compiler to generate some AVX512 code where there is some support in the
compiler rather than only generating all AVX512 code when all of it is
supported or no AVX512 code at all.

For example, in GCC 4.9 where there is just support for AVX512F, this
patch will allow building the AVX512 DPIF.

Another example, in GCC 5 and 6, most AVX512 code can be generated, just
without AVX512VPOPCNTDQ support.

Signed-off-by: Cian Ferriter 

---
v5:
* Create a selector function for the permutexvar implementations based
  on Sunil's feedback on the v4.  This hides the complexity of compile
  time and run time selection of permutexvar implementations.
* Add a comment explaining why VPOPCNTDQ_TARGET is defined and used.

v4:
* Combine the 3 commits which added checks for AVX512 ISA into this
  single commit since the first 2 commits were only useful and active
  when the 3rd commit was applied. This also takes care of Sunil's
  comment about explaining that the first 2 commits are precursors.
* Don't check for AVX512DQ availability in the compiler. This ISA isn't
  used in OVS.
* Put all AVX512 ISA checks in the OVS_CHECK_AVX512 macro as per Sunil's
  feedback.
* Define a function in acinclude.m4, (OVS_CONDITIONAL_CC_OPTION_DEFINE),
  to help with checking for AVX512 ISA support in the compiler.
* Remove the '__AVX512VPOPCNTDQ__' check. Use the HAVE_AVX512* pattern
  consistently with all AVX512 ISA checks instead. Fixup the comment
  explaining the _mm512_popcnt_epi64_wrapper() function to reflect this.

v3:
* Preserve the order of the mfex impl list. v2 changed this order. We
  want the order to be preserved because VBMI functions should be chosen
  by the mfex study impl where possible.

v2:
* Don't register vbmi specialized mfex impls unless VBMI is actually
  available.
  * This required some re-ordering of the mfex impl lists.
---
 acinclude.m4   | 26 +++
 lib/automake.mk| 14 --
 lib/dpif-netdev-extract-avx512.c   | 64 ++
 lib/dpif-netdev-lookup-avx512-gather.c | 33 +
 lib/dpif-netdev-lookup.c   |  3 +-
 lib/dpif-netdev-private-extract.c  | 18 
 lib/dpif-netdev-private-extract.h  | 19 +++-
 7 files changed, 127 insertions(+), 50 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 61e88105f..7b2889a40 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -73,16 +73,13 @@ AC_DEFUN([OVS_CHECK_DPIF_AVX512_DEFAULT], [
 
 dnl OVS_CHECK_AVX512
 dnl
-dnl Checks if compiler and binutils supports AVX512.
+dnl Checks if compiler and binutils supports various AVX512 ISA.
 AC_DEFUN([OVS_CHECK_AVX512], [
   OVS_CHECK_BINUTILS_AVX512
-  OVS_CHECK_CC_OPTION(
-[-mavx512f -mavx512vpopcntdq], [ovs_have_cc_mavx512f=yes], 
[ovs_have_cc_mavx512f=no])
-  AM_CONDITIONAL([HAVE_AVX512F], [test $ovs_have_cc_mavx512f = yes])
-  if test "$ovs_have_cc_mavx512f" = yes; then
-AC_DEFINE([HAVE_AVX512F], [1],
-  [Define to 1 if compiler supports AVX512.])
-  fi
+  OVS_CONDITIONAL_CC_OPTION_DEFINE([-mavx512f], [HAVE_AVX512F])
+  OVS_CONDITIONAL_CC_OPTION_DEFINE([-mavx512bw], [HAVE_AVX512BW])
+  OVS_CONDITIONAL_CC_OPTION_DEFINE([-mavx512vbmi], [HAVE_AVX512VBMI])
+  OVS_CONDITIONAL_CC_OPTION_DEFINE([-mavx512vpopcntdq], [HAVE_AVX512VPOPCNTDQ])
 ])
 
 dnl OVS_ENABLE_WERROR
@@ -1360,6 +1357,19 @@ AC_DEFUN([OVS_CONDITIONAL_CC_OPTION],
AM_CONDITIONAL([$2], [test $ovs_have_cc_option = yes])])
 dnl --
 
+dnl OVS_CONDITIONAL_CC_OPTION_DEFINE([OPTION], [CONDITIONAL])
+dnl Check whether the given C compiler OPTION is accepted.
+dnl If so, enable the given Automake CONDITIONAL and define it.
+dnl Example: OVS_CONDITIONAL_CC_OPTION_DEFINE([-mavx512f], [HAVE_AVX512F])
+AC_DEFUN([OVS_CONDITIONAL_CC_OPTION_DEFINE],
+  [OVS_CHECK_CC_OPTION(
+[$1], [ovs_have_cc_option=yes], [ovs_have_cc_option=no])
+   AM_CONDITIONAL([$2], [test $ovs_have_cc_option = yes])
+   if test "$ovs_have_cc_option" = yes; then
+ AC_DEFINE([$2], [1],
+   [Define to 1 if compiler supports the '$1' option.])
+   fi])
+
 dnl Check for too-old XenServer.
 AC_DEFUN([OVS_CHECK_XENSERVER_VERSION],
   [AC_CACHE_CHECK([XenServer release], [ovs_cv_xsversion],
diff --git a/lib/automake.mk b/lib/automake.mk
index 14347bac6..cb50578eb 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -31,7 +31,6 @@ lib_LTLIBRARIES += lib/libopenvswitchavx512.la
 lib_libopenvswitch_la_LIBADD += lib/libopenvswitchavx512.la
 lib_libopenvswitchavx512_la_CFLAGS = \
-mavx512f \
-   -mavx512bw \
-mbmi \
-mbmi2 \
-fPIC \
@@ -39,13 +38,18 @@ lib_libopenvswitchavx512_la_CFLAGS = \
 lib_libopenvswitchavx512_la_SOURCES = \
lib/cpu.c \
lib/cpu.h \
-   lib/dpif-netdev-lookup-avx512-gather.c \
-   lib/dpif-netdev-extract-avx512.c \
lib/dpif-netdev-avx512.c

[ovs-dev] [PATCH v5 3/5] dpif-netdev-extract: Remove unnecessary compiler targets.

[Cian Ferriter]

No instructions from the AVX512VL ISA are used.

Compilation for AVX512F and AVX512 BW ISA are already enabled in
lib/automake.mk for the dpif-netdev-lookup-avx512-gather.c file because
it's part of the libopenvswitchavx512.la library. They don't need to be
enabled at a function level.

Remove these unnecessary function-level compiler target attributes.

Signed-off-by: Cian Ferriter 
Acked-by: Sunil Pai G 

---
v5:
* Remove an 'avx512bw' target since it's also unnecessary as per Sunil's
  suggestion.
* Add Sunil's Acked-by tag.

v4:
* Remove the 'avx512f' target since it's also unnecessary.
* Sunil acked the v3 version of this commit, but since it's changed, I'm
  not carrying the ack over.
---
 lib/dpif-netdev-extract-avx512.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c
index 6b6fe07db..4a94dfcfd 100644
--- a/lib/dpif-netdev-extract-avx512.c
+++ b/lib/dpif-netdev-extract-avx512.c
@@ -52,7 +52,6 @@
 
 /* AVX512-BW level permutex2var_epi8 emulation. */
 static inline __m512i
-__attribute__((target("avx512bw")))
 _mm512_maskz_permutex2var_epi8_skx(__mmask64 k_mask,
__m512i v_data_0,
__m512i v_shuf_idxs,
@@ -632,8 +631,6 @@ mfex_avx512_process(struct dp_packet_batch *packets,
 
 #define DECLARE_MFEX_FUNC(name, profile)\
 uint32_t\
-__attribute__((__target__("avx512f")))  \
-__attribute__((__target__("avx512vl"))) \
 __attribute__((__target__("avx512vbmi")))   \
 mfex_avx512_vbmi_##name(struct dp_packet_batch *packets,\
 struct netdev_flow_key *keys, uint32_t keys_size,\
@@ -645,8 +642,6 @@ mfex_avx512_vbmi_##name(struct dp_packet_batch *packets,
\
 }   \
 \
 uint32_t\
-__attribute__((__target__("avx512f")))  \
-__attribute__((__target__("avx512vl"))) \
 mfex_avx512_##name(struct dp_packet_batch *packets, \
struct netdev_flow_key *keys, uint32_t keys_size,\
odp_port_t in_port, struct dp_netdev_pmd_thread  \
-- 
2.25.1

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

[ovs-dev] [PATCH v5 4/5] automake.mk: Remove -mavx512dq CFLAG from AVX512 library.

[Cian Ferriter]

No instructions from the AVX512DQ ISA are used anywhere in OVS. Remove
this unnecessary CFLAG.

Signed-off-by: Cian Ferriter 
Acked-by: Sunil Pai G 

---
v5:
* Add Sunil's Acked-by tag.

v4:
* Add this commit to the series.
---
 lib/automake.mk | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/automake.mk b/lib/automake.mk
index a23cdc4ad..14347bac6 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -32,7 +32,6 @@ lib_libopenvswitch_la_LIBADD += lib/libopenvswitchavx512.la
 lib_libopenvswitchavx512_la_CFLAGS = \
-mavx512f \
-mavx512bw \
-   -mavx512dq \
-mbmi \
-mbmi2 \
-fPIC \
-- 
2.25.1

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

[ovs-dev] [PATCH v5 2/5] dpif-netdev-lookup: Fix GCC 5 warning.

[Cian Ferriter]

GCC 5 gave an incompatible pointer type warning for pkt_blocks when it's
passed to _mm512_mask_i64gather_epi64().

Follow the same pattern used for tbl_blocks where the 'const uint64_t *'
is cast to a 'const void *' when passed in to avx512_blocks_gather().

Fixes: 47a2a8f4138e ("dpif-netdev/dpcls-avx512: Enable 16 block processing.")
Signed-off-by: Cian Ferriter 
Acked-by: Sunil Pai G 

---
v4:
* Added Sunil's Fixes and Acked-by tags.

v3:
* Add this commit to the series.
---
 lib/dpif-netdev-lookup-avx512-gather.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dpif-netdev-lookup-avx512-gather.c 
b/lib/dpif-netdev-lookup-avx512-gather.c
index 7bc1e9e9a..b396772bc 100644
--- a/lib/dpif-netdev-lookup-avx512-gather.c
+++ b/lib/dpif-netdev-lookup-avx512-gather.c
@@ -155,7 +155,7 @@ netdev_rule_matches_key(const struct dpcls_rule *rule,
 static inline ALWAYS_INLINE __m512i
 avx512_blocks_gather(__m512i v_u0,
  __m512i v_u1,
- const uint64_t *pkt_blocks,
+ const void *pkt_blocks,
  const void *tbl_blocks,
  const void *tbl_mf_masks,
  __mmask64 u1_bcast_msk,
-- 
2.25.1

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

[ovs-dev] [PATCH v5 1/5] dpif-netdev-private-extract: Fix typo VMBI -> VBMI.

[Cian Ferriter]

Fixes: 250ceddcc2d0 ("dpif-netdev/mfex: Add AVX512 based optimized miniflow 
extract")
Fixes: aa85a25095ae ("dpif-netdev/mfex: Add more AVX512 traffic profiles")
Signed-off-by: Cian Ferriter 
Acked-by: Sunil Pai G 

---
v4:
* Added Sunil's Fixes and Acked-by tags.
---
 lib/dpif-netdev-private-extract.c |  8 
 lib/dpif-netdev-private-extract.h | 10 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/lib/dpif-netdev-private-extract.c 
b/lib/dpif-netdev-private-extract.c
index 4b2f12015..b7f094dac 100644
--- a/lib/dpif-netdev-private-extract.c
+++ b/lib/dpif-netdev-private-extract.c
@@ -55,7 +55,7 @@ static struct dpif_miniflow_extract_impl mfex_impls[] = {
 
 /* Compile in implementations only if the compiler ISA checks pass. */
 #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
-[MFEX_IMPL_VMBI_IPv4_UDP] = {
+[MFEX_IMPL_VBMI_IPv4_UDP] = {
 .probe = mfex_avx512_vbmi_probe,
 .extract_func = mfex_avx512_vbmi_ip_udp,
 .name = "avx512_vbmi_ipv4_udp", },
@@ -65,7 +65,7 @@ static struct dpif_miniflow_extract_impl mfex_impls[] = {
 .extract_func = mfex_avx512_ip_udp,
 .name = "avx512_ipv4_udp", },
 
-[MFEX_IMPL_VMBI_IPv4_TCP] = {
+[MFEX_IMPL_VBMI_IPv4_TCP] = {
 .probe = mfex_avx512_vbmi_probe,
 .extract_func = mfex_avx512_vbmi_ip_tcp,
 .name = "avx512_vbmi_ipv4_tcp", },
@@ -75,7 +75,7 @@ static struct dpif_miniflow_extract_impl mfex_impls[] = {
 .extract_func = mfex_avx512_ip_tcp,
 .name = "avx512_ipv4_tcp", },
 
-[MFEX_IMPL_VMBI_DOT1Q_IPv4_UDP] = {
+[MFEX_IMPL_VBMI_DOT1Q_IPv4_UDP] = {
 .probe = mfex_avx512_vbmi_probe,
 .extract_func = mfex_avx512_vbmi_dot1q_ip_udp,
 .name = "avx512_vbmi_dot1q_ipv4_udp", },
@@ -85,7 +85,7 @@ static struct dpif_miniflow_extract_impl mfex_impls[] = {
 .extract_func = mfex_avx512_dot1q_ip_udp,
 .name = "avx512_dot1q_ipv4_udp", },
 
-[MFEX_IMPL_VMBI_DOT1Q_IPv4_TCP] = {
+[MFEX_IMPL_VBMI_DOT1Q_IPv4_TCP] = {
 .probe = mfex_avx512_vbmi_probe,
 .extract_func = mfex_avx512_vbmi_dot1q_ip_tcp,
 .name = "avx512_vbmi_dot1q_ipv4_tcp", },
diff --git a/lib/dpif-netdev-private-extract.h 
b/lib/dpif-netdev-private-extract.h
index f9a757ba4..ae5c161b4 100644
--- a/lib/dpif-netdev-private-extract.h
+++ b/lib/dpif-netdev-private-extract.h
@@ -82,13 +82,13 @@ enum dpif_miniflow_extract_impl_idx {
 MFEX_IMPL_SCALAR,
 MFEX_IMPL_STUDY,
 #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
-MFEX_IMPL_VMBI_IPv4_UDP,
+MFEX_IMPL_VBMI_IPv4_UDP,
 MFEX_IMPL_IPv4_UDP,
-MFEX_IMPL_VMBI_IPv4_TCP,
+MFEX_IMPL_VBMI_IPv4_TCP,
 MFEX_IMPL_IPv4_TCP,
-MFEX_IMPL_VMBI_DOT1Q_IPv4_UDP,
+MFEX_IMPL_VBMI_DOT1Q_IPv4_UDP,
 MFEX_IMPL_DOT1Q_IPv4_UDP,
-MFEX_IMPL_VMBI_DOT1Q_IPv4_TCP,
+MFEX_IMPL_VBMI_DOT1Q_IPv4_TCP,
 MFEX_IMPL_DOT1Q_IPv4_TCP,
 #endif
 MFEX_IMPL_MAX
@@ -101,7 +101,7 @@ extern struct ovs_mutex dp_netdev_mutex;
  */
 #if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD && __SSE4_2__)
 
-#define MFEX_IMPL_START_IDX MFEX_IMPL_VMBI_IPv4_UDP
+#define MFEX_IMPL_START_IDX MFEX_IMPL_VBMI_IPv4_UDP
 #else
 
 #define MFEX_IMPL_START_IDX MFEX_IMPL_MAX
-- 
2.25.1

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

[ovs-dev] [PATCH v5 0/5] Build some AVX512 code on older compilers.

[Cian Ferriter]

Support for more AVX512 ISA was added in various compiler releases. For
example in GCC, increasing AVX512 ISA support was added in GCC 4.9, 5
and then 7. Depending on the GCC version, none, some or all of the
AVX512 code in OVS can be built. This patchset adds support for the
"some" case, where currently, we build all or none of the AVX512 code.

For my testing on Ubuntu 20.04, the below steps were very helpful for
installing and switching between different GCC versions.
https://askubuntu.com/a/1313032

v5:
* Add Sunil's Acked-by tags to patches 3 and 4.
* Remove an 'avx512bw' target attribute as per Sunil's suggestion.
* Create a selector function for the permutexvar implementations based on
  Sunil's feedback on the v4. This hides the complexity of compile time and run
  time selection of permutexvar implementations.

v4:
* Added Sunil's Fixes and Acked-by tags where appropriate.
* Remove the 'avx512f' target in commit 3 since it's also unnecessary. I didn't
  carry Sunil's Acked-by tag over because of the change.
* Added the "automake.mk: Remove -mavx512dq CFLAG from AVX512 library." commit
  to the series at commit 4/5.
* Combine the 3 commits which added checks for AVX512 ISA into this
  single commit since the first 2 commits were only useful and active
  when the 3rd commit was applied. This also takes care of Sunil's
  comment about explaining that the first 2 commits are precursors.

v3:
* Add a patch which fixes a warning in the AVX512 code when building with GCC
  5.
* Preserve the order of the mfex impl list. v2 changed this order. We want the
  order to be preserved because VBMI functions should be chosen by the mfex
  study impl where possible.

v2:
* Add a commit to fix a typo present on OVS master code VMBI -> VBMI.
* Don't register vbmi specialized mfex impls unless VBMI is actually
  available.
  * This required some re-ordering of the mfex impl lists.


Cian Ferriter (5):
  dpif-netdev-private-extract: Fix typo VMBI -> VBMI.
  dpif-netdev-lookup: Fix GCC 5 warning.
  dpif-netdev-extract: Remove unnecessary compiler targets.
  automake.mk: Remove -mavx512dq CFLAG from AVX512 library.
  acinclude: Add seperate checks for AVX512 ISA.

 acinclude.m4   | 26 +++---
 lib/automake.mk| 15 +++---
 lib/dpif-netdev-extract-avx512.c   | 69 +-
 lib/dpif-netdev-lookup-avx512-gather.c | 35 +
 lib/dpif-netdev-lookup.c   |  3 +-
 lib/dpif-netdev-private-extract.c  | 26 +-
 lib/dpif-netdev-private-extract.h  | 29 ---
 7 files changed, 137 insertions(+), 66 deletions(-)

-- 
2.25.1

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

Re: [ovs-dev] [PATCH ovn] controller: avoid recomputes triggered by SBDB Port_Binding updates.

[Xavier Simonart]

Hi Han
Thanks for looking into this and for your feedback.
Please see comments below

On Tue, May 17, 2022 at 8:03 AM Han Zhou  wrote:

>
>
> On Thu, May 12, 2022 at 2:04 AM Xavier Simonart 
> wrote:
> >
> > When VIF ports are claimed on a chassis, SBDB Port_Binding table is
> updated.
> > If the SBDB IDL is still is read-only ("in transaction") when such a
> update
> > is required, the update is not possible and recompute is triggered
> through
> > I+P failure.
> >
> > This situation can happen:
> > - after updating Port_Binding->chassis to SBDB for one port, in a
> following
> >   iteration, ovn-controller handles Interface:external_ids:ovn-installed
> >   (for the same port) while SBDB is still read-only.
> > - after updating Port_Binding->chassis to SBDB for one port, in a
> following
> >   iteration, ovn-controller updates Port_Binding->chassis for another
> port,
> >   while SBDB is still read-only.
> >
> > This patch prevent the recompute, by having the if-status module
> > updating the Port_Binding chassis (if needed), when possible.
> > This does not delay Port_Binding chassis update compared to before this
> > patch: Port_Binding chassis will be updated as soon as SBDB is again
> > writable, as it was the case, through a recompute, before this patch.
> >
> Thanks Xavier. I think the approach is good: moving the SB update from the
> I-P engine to if-status module, so it wouldn't need to trigger I-P engine
> recompute, and just let the if-status module update the SB as soon as it is
> writable, through the if-status's state-machine.
> However, I have some comments/questions regarding the implementation
> details, primarily confusion on the state transitions. Please see below.
>
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2059253
> > Signed-off-by: Xavier Simonart 
> > ---
> >  controller/binding.c| 124 ++--
> >  controller/binding.h|   9 ++-
> >  controller/if-status.c  |  31 +++--
> >  controller/if-status.h  |   6 +-
> >  controller/ovn-controller.c |   6 +-
> >  tests/ovn.at|  55 +++-
> >  6 files changed, 184 insertions(+), 47 deletions(-)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index e5ba56b25..d917d8775 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -657,11 +657,15 @@ local_binding_get_lport_ofport(const struct shash
> *local_bindings,
> >  }
> >
> >  bool
> > -local_binding_is_up(struct shash *local_bindings, const char *pb_name)
> > +local_binding_is_up(struct shash *local_bindings, const char *pb_name,
> > +const struct sbrec_chassis *chassis_rec)
> >  {
> >  struct local_binding *lbinding =
> >  local_binding_find(local_bindings, pb_name);
> >  struct binding_lport *b_lport =
> local_binding_get_primary_lport(lbinding);
> > +if (b_lport && b_lport->pb->chassis != chassis_rec) {
> > +return false;
> > +}
>
> Why need the change here? I understand that it is obvious that the binding
> should not be considered up if the chassis is not updated in SB
> port_binding, but I wonder why we need the change in *this* patch.
> The function is called to see if an interface state should be moved from
> MARK_UP to INSTALLED, but if the chassis is not set, it shouldn't even be
> moved to MARK_UP.
>
> I probably need to provide a better explanation of the states.
In fact, my two goals were: 1) avoid/decrease recomputes and 2) do not
increase latency (time between interface is claimed and it's up in SB and
ovn-installed in OVS).
There is not really an additional state covering the fact that the chassis
is set in sbdb.
As soon as the state is CLAIMED, we try to set the pb->chassis (by 'try' I
mean we update pb->chassis if sb is not readonly).
The fact that sb is readonly in CLAIMED state does not prevent us to move
to INSTALL_FLOWS state, where we are installing flows in OVS.
As soon as the seqno for those flows was received, we moved to MARK_UP
state.
In those three states, we can update pb->chassis (if not already done).
In MARK_UP we set the interface up.
We moved to INSTALLED when both up and chassis have been written.

>  if (lbinding && b_lport && lbinding->iface) {
> >  if (b_lport->pb->n_up && !b_lport->pb->up[0]) {
> >  return false;
> > @@ -673,13 +677,23 @@ local_binding_is_up(struct shash *local_bindings,
> const char *pb_name)
> >  }
> >
> >  bool
> > -local_binding_is_down(struct shash *local_bindings, const char *pb_name)
> > +local_binding_is_down(struct shash *local_bindings, const char *pb_name,
> > +  const struct sbrec_chassis *chassis_rec)
> >  {
> >  struct local_binding *lbinding =
> >  local_binding_find(local_bindings, pb_name);
> >
> >  struct binding_lport *b_lport =
> local_binding_get_primary_lport(lbinding);
> >
> > +if (b_lport) {
> > +if (b_lport->pb->chassis == chassis_rec) {
> > +

Re: [ovs-dev] [v4 2/8] tc: Add support parsing tc police action

[Jianbo Liu via dev]

On Fri, 2022-05-13 at 14:28 +0200, Eelco Chaudron wrote:
> 
> 
> On 3 May 2022, at 5:08, Jianbo Liu via dev wrote:
> 
> > Add function to parse police action from netlink message, and meter
> > id
> > can be retrieved from action cockie as it will be saved there in
> > later
> > patch.
> 
> See comments inline below…
> 
> > Signed-off-by: Jianbo Liu 
> > ---
> >  lib/netdev-offload-tc.c |  4 +++
> >  lib/tc.c    | 59
> > +
> >  lib/tc.h    |  6 +
> >  3 files changed, 69 insertions(+)
> > 
> > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > index a41b62758..83d57c63b 100644
> > --- a/lib/netdev-offload-tc.c
> > +++ b/lib/netdev-offload-tc.c
> > @@ -1017,6 +1017,10 @@ parse_tc_flower_to_match(struct tc_flower
> > *flower,
> >  nl_msg_put_u32(buf, OVS_ACTION_ATTR_RECIRC,
> > action->chain);
> >  }
> >  break;
> > +    case TC_ACT_POLICE: {
> > +    /* Not supported yet */
> > +    }
> > +    break;
> >  }
> >  }
> >  }
> > diff --git a/lib/tc.c b/lib/tc.c
> > index df73a43d4..af7a7bc6d 100644
> > --- a/lib/tc.c
> > +++ b/lib/tc.c
> > @@ -1339,6 +1339,59 @@ nl_parse_act_gact(struct nlattr *options,
> > struct tc_flower *flower)
> >  return 0;
> >  }
> > 
> > +static const struct nl_policy police_policy[] = {
> > +    [TCA_POLICE_TBF] = { .type = NL_A_UNSPEC,
> > + .min_len = sizeof(struct tc_police),
> > + .optional = false, },
> > +    [TCA_POLICE_RATE] = { .type = NL_A_UNSPEC,
> > +  .min_len = 1024,
> > +  .optional = true, },
> > +    [TCA_POLICE_PEAKRATE] = { .type = NL_A_UNSPEC,
> > +  .min_len = 1024,
> > +  .optional = true, },
> > +    [TCA_POLICE_AVRATE] = { .type = NL_A_U32,
> > +    .optional = true, },
> > +    [TCA_POLICE_RESULT] = { .type = NL_A_U32,
> > +    .optional = true, },
> > +    [TCA_POLICE_TM] = { .type = NL_A_UNSPEC,
> > +    .min_len = sizeof(struct tcf_t),
> > +    .optional = true, },
> > +};
> > +
> > +static int
> > +nl_parse_act_police(const struct nlattr *options, struct tc_flower
> > *flower,
> > +    struct nlattr *act_cookie)
> > +{
> > +    struct nlattr *police_attrs[ARRAY_SIZE(police_policy)] = {};
> > +    struct tc_action *action;
> > +    const struct tc_police *police;
> 
> For the new code, I review I try to stick to reverse Christmas tree
> style, so as you might need to make changes may also swap the two-
> line?
> 
> > +    struct nlattr *police_tm;
> > +    const struct tcf_t *tm;
> > +
> > +    if (!nl_parse_nested(options, police_policy, police_attrs,
> > + ARRAY_SIZE(police_policy))) {
> > +    VLOG_ERR_RL(_rl, "failed to parse police action
> > options");
> 
> Change failed to a capital, so “Failed to parse..."
> 
> > +    return EPROTO;
> > +    }
> > +
> > +    police = nl_attr_get(police_attars[TCA_POLICE_TBF]);
> 
> police = nl_attr_get_unspec(police_attrs[TCA_POLICE_TBF], sizeof
> *police);
> 
> > +    action = >actions[flower->action_count++];
> > +    action->type = TC_ACT_POLICE;
> > +    action->police.index = police->index;
> > +
> > +    if (act_cookie) {
> > +    action->police.meter_id = nl_attr_get_u32(act_cookie);
> > +    }
> > +
> > +    police_tm = police_attrs[TCA_POLICE_TM];
> 
> If you use optional attributes, you have to make sure they are
> present. Guess the fix would be to set .optional = false for
> TCA_POLICE_TM.
> 

I checked police_tm at below line, to make sure they are present. So
why need to set .optional=false?

> > +    if (police_tm) {
> > +    tm = nl_attr_get_unspec(police_tm, sizeof *tm);
> > +    nl_parse_tcf(tm, flower);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static const struct nl_policy mirred_policy[] = {
> >  [TCA_MIRRED_PARMS] = { .type = NL_A_UNSPEC,
> >     .min_len = sizeof(struct tc_mirred),
> > @@ -1761,6 +1814,8 @@ nl_parse_single_action(struct nlattr *action,
> > struct tc_flower *flower,
> >  /* Added for TC rule only (not in OvS rule) so ignore. */
> >  } else if (!strcmp(act_kind, "ct")) {
> >  nl_parse_act_ct(act_options, flower);
> > +    } else if (!strcmp(act_kind, "police")) {
> > +    nl_parse_act_police(act_options, flower, act_cookie);
> >  } else {
> >  VLOG_ERR_RL(_rl, "unknown tc action kind: %s",
> > act_kind);
> >  err = EINVAL;
> > @@ -2773,6 +2828,10 @@ nl_msg_put_flower_acts(struct ofpbuf
> > *request, struct tc_flower *flower)
> >  nl_msg_end_nested(request, act_offset);
> >  }
> >  break;
> > +    case TC_ACT_POLICE: {
> > +    /* Not supported yet */
> > +   

Re: [ovs-dev] [PATCH] netdev-dpdk: Delay vhost mempool creation on multi-NUMA.

[David Marchand]

On Fri, May 13, 2022 at 5:35 PM Kevin Traynor  wrote:
>
> A mempool is currently created for a vhost port when it is added.
>
> The NUMA info of the vhost port is not known until a device is added to
> the port, so on multi-NUMA systems the initial NUMA node for the mempool
> is a best guess based on vswitchd affinity.
>
> When a device is added to the vhost port, the NUMA info can be checked
> and if the guess was incorrect a mempool on the correct NUMA node
> created.
>
> The current scheme can have the effect of creating a mempool on a NUMA
> node that will not be needed and at least for a certain time period
> requires memory.
>
> It is also difficult for a user trying to provision memory on different
> NUMA nodes, if they are not sure which NUMA node the initial mempool
> for a vhost port will be on.
>
> This patch delays the creation of the mempool for a vhost port on
> multi-NUMA systems until the vhost NUMA info is known.

I prefer having a single behavior for mono and multi numa (=> no
question about which behavior resulted in mempool
creation/attribution).
Though I don't have a strong opinion against having this difference in behavior.


>
> Signed-off-by: Kevin Traynor 

Otherwise, this new behavior for multi numa and the patch lgtm.

I tested with a dual numa system, ovs running with pmd threads on numa
1, one bridge with vhost-user ports serving one vm in numa0 and one vm
in numa1.

Running ovs-appctl netdev-dpdk/get-mempool-info | grep ovs,
- before patch, no vm started
mempool @0x17f703180
^^
We can notice that a numa0 mempool was created
even if PMD threads are on numa1 and no port uses this mempool.
- before patch, once vm in numa1 is started,
mempool @0x17f703180
mempool @0x11ffe01e40
- before patch, once vm in numa0 is started too,
mempool @0x17f703180
mempool @0x11ffe01e40


- after patch, no vm started

- after patch, once vm in numa1 is started,
mempool @0x11ffe01e40
^^
Only one mempool in numa1
- after patch, once vm in numa0 is started too,
mempool @0x11ffe01e40
mempool @0x17f51dcc0


Reviewed-by: David Marchand 

-- 
David Marchand

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

Re: [ovs-dev] [ovs-dev v3 3/3] checkpatch.py: add checks for experimental API.

[Eelco Chaudron]

On 17 May 2022, at 9:20, Peng He wrote:

> You mean I can ignore the following warnings?
>
> "
> checkpatch:
> ERROR: Author Peng He  needs to sign off.
> WARNING: Unexpected sign-offs from developers who are not authors or
> co-authors or committers: Peng He 
> Lines checked: 98, Warnings: 1, Errors: 1
> "

I think these are because of your mixed email address. Ilya can probably answer 
this, but I think he will fix this on commit, Ilya?

>
> Eelco Chaudron  于2022年5月17日周二 14:26写道：
>
>>
>>
>> On 17 May 2022, at 7:49, Peng He wrote:
>>
>>> The only issue I have is that I have to use the company's email address
>> to
>>> submit patches
>>> while I am using the personal email to subscribe to the maillist.
>>>
>>> So the bot will always warn about the author needing to sign-off.
>>> Perhaps in the future I would use both email addresses to avoid this
>>> warning.
>>
>> There is this error on patch 1/3:
>>
>> ERROR: Co-author Eelco Chaudron  needs to sign off.
>>
>> So I guess you need to add my sign-off also in that patch.
>>>
>>>
>>> Eelco Chaudron  于2022年5月16日周一 19:36写道：
>>>


 On 16 May 2022, at 12:19, Peng He wrote:

> Eelco Chaudron  于2022年5月16日周一 17:12写道：
>
>>
>>
>> On 14 May 2022, at 10:40, Peng He wrote:
>>
>>> Signed-off-by: Peng He 
>>> ---
>>>  utilities/checkpatch.py | 32 +---
>>>  1 file changed, 29 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>>> index 8c7faa419..67c517b69 100755
>>> --- a/utilities/checkpatch.py
>>> +++ b/utilities/checkpatch.py
>>> @@ -619,6 +619,8 @@ def regex_function_factory(func_name):
>>>  def regex_error_factory(description):
>>>  return lambda: print_error(description)
>>>
>>
>> Need extra newline, install flake8 and you will get the error at
>> compile
>> time.
>>
>>> +def regex_warn_factory(description):
>>> +return lambda: print_warning(description)
>>
>> Need extra newline, install flake8 and you will get the error at
>> compile
>> time.
>>
>>>
>>>  std_functions = [
>>>  ('malloc', 'Use xmalloc() in place of malloc()'),
>>> @@ -636,6 +638,7 @@ std_functions = [
>>>  ('assert', 'Use ovs_assert() in place of assert()'),
>>>  ('error', 'Use ovs_error() in place of error()'),
>>>  ]
>>> +
>>>  checks += [
>>>  {'regex': r'(\.c|\.h)(\.in)?$',
>>>   'match_name': None,
>>> @@ -644,6 +647,21 @@ checks += [
>>>   'print': regex_error_factory(description)}
>>>  for (function_name, description) in std_functions]
>>>
>>> +experimental_api = [
>>
>> I do not think this is an experimental API, I would call it something
 like
>> a suspicious API maybe?
>>
>
> or change to easy_to_misused_api?

 This is fine by me to.

>>
>>> +('ovsrcu_barrier',
>>> +'lib/ovs-rcu.c',
>>> +'Are you sure you need to use ovsrcu_barrier(),'
>>
>> Here you need to add a space at the end, as the error message now
>> looks
>> like:
>>
>>   WARNING: Are you sure you need to use ovsrcu_barrier(),in most cases
>> ovsrcu_synchronize() will be fine?
>>
>>> +'in most cases ovsrcu_synchronize() will be fine?'),
>>> +]
>>> +
>>> +checks += [
>>> +{'regex': r'(\.c)(\.in)?$',
>>> + 'match_name': lambda x: x != location,
>>> + 'prereq': lambda x: not is_comment_line(x),
>>> + 'check': regex_function_factory(function_name),
>>> + 'print': regex_warn_factory(description)}
>>> +for (function_name, location, description) in experimental_api]
>>> +
>>>
>>>  def regex_operator_factory(operator):
>>>  regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' %
>> operator)
>>> @@ -676,12 +694,20 @@ def get_file_type_checks(filename):
>>>  global checks
>>>  checkList = []
>>>  for check in checks:
>>> +regex_check = True
>>> +match_check = True
>>> +
>>>  if check['regex'] is None and check['match_name'] is None:
>>>  checkList.append(check)
>>> +continue
>>> +
>>>  if check['regex'] is not None and \
>>> -   re.compile(check['regex']).search(filename) is not None:
>>> -checkList.append(check)
>>> -elif check['match_name'] is not None and
>> check['match_name'](filename):
>>> +   re.compile(check['regex']).search(filename) is None:
>>> +regex_check = False
>>> +elif check['match_name'] is not None and not
>> check['match_name'](filename):
>>
>> Line is too long so you need to break it up:
>>
>> utilities/checkpatch.py:709:80: E501 line too long (83 > 79
>> characters)

Re: [ovs-dev] [PATCH] classifier: adjust segment boundary to execute prerequisite processing

[Eelco Chaudron]

On 16 May 2022, at 22:04, Aaron Conole wrote:

> During flow processing, the flow wildcards are checked as a series of
> stages, and these stages are intended to carry dependencies in a single
> direction.  But when the neighbor discovery processing, for example, was
> executed there is an incorrect dependency chain - we need fields from
> stage 4 to determine whether we need fields from stage 3.
>
> We can build a set of flow rules to demonstrate this:
>   table=0,priority=100,ipv6,ipv6_src=1000::/10 actions=resubmit(,1)
>   table=0,priority=0 actions=NORMAL
>   table=1,priority=110,ipv6,ipv6_dst=1000::3 actions=resubmit(,2)
>   table=1,priority=100,ipv6,ipv6_dst=1000::4 actions=resubmit(,2)
>   table=1,priority=0 actions=NORMAL
>   
> table=2,priority=120,icmp6,nw_ttl=255,icmp_type=135,icmp_code=0,nd_sll=10:de:ad:be:ef:10
>  actions=NORMAL
>   table=2,priority=100,tcp actions=NORMAL
>   table=2,priority=100,icmp6 actions=NORMAL
>   table=2,priority=0 actions=NORMAL
>
> With this set of flows, any IPv6 packet that executes through this pipeline
> will have the corresponding nd_sll field flagged as required match for
> classification even if that field doesn't make sense in such a context
> (for example, TCP packets).  When the corresponding flow is installed into
> the kernel datapath, this field is not reflected when the revalidator
> executes the dump stage (see net/openvswitch/flow_netlink.c for more details).
>
> During the sweep stage, revalidator will compare the dumped WC with a
> generated WC - these will mismatch because the generated WC will match on
> the Neighbor Discovery fields, while the datapath WC will not match on
> these fields.  We will then invalidate the flow and as a side effect
> force an upcall.
>
> By redefining the boundary, we shift these fields to the l4 subtable, and
> cause masks to be generated matching just the requisite fields.  The list
> of fields being shifted:
>
> struct in6_addr nd_target;
> struct eth_addr arp_sha;
> struct eth_addr arp_tha;
> ovs_be16 tcp_flags;
> ovs_be16 pad2;
> struct ovs_key_nsh nsh;
>
> A standout field would be tcp_flags moving from l3 subtable matches to
> the l4 subtable matches.  This reverts a partial performance optimization
> in the case of stateless firewalling.  The tcp_flags field might have
> been a good candidate to retain in the l3 segment, but it got overloaded
> with ICMPv6 ND matching, and therefore we can't preserve this kind of
> optimization.
>
> Two other approaches were considered - moving the nd_target field alone
> and collapsing the l3/l4 segments into a single subtable for matching.
> Moving any field individually introduces ABI mismatch, and doesn't
> completely address the problems with other neighbor discovery related
> fields (such as nd_sll/nd_tll).  Collapsing the two subtables creates
> an issue with datapath flow explosion, since the l3 and l4 fields will
> be unwildcarded together (this can be seen with some of the existing
> classifier tests).
>
> A simple test is added to showcase the behavior.
>
> Fixes: 476f36e83bc5 ("Classifier: Staged subtable matching.")
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2081773
> Reported-by: Numan Siddique 
> Suggested-by: Ilya Maximets 
> Signed-off-by: Aaron Conole 
> ---

Bit later response (was planning on replying to the RFC patch), but I needed a 
bit more time (and experimenting) with this area of the code :)

The change looks good, and thanks for adding so many details in the commit 
message!

Acked-by: Eelco Chaudron 


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

Re: [ovs-dev] [ovs-dev v3 3/3] checkpatch.py: add checks for experimental API.

[Peng He]

You mean I can ignore the following warnings?

"
checkpatch:
ERROR: Author Peng He  needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or
co-authors or committers: Peng He 
Lines checked: 98, Warnings: 1, Errors: 1
"


Eelco Chaudron  于2022年5月17日周二 14:26写道：

>
>
> On 17 May 2022, at 7:49, Peng He wrote:
>
> > The only issue I have is that I have to use the company's email address
> to
> > submit patches
> > while I am using the personal email to subscribe to the maillist.
> >
> > So the bot will always warn about the author needing to sign-off.
> > Perhaps in the future I would use both email addresses to avoid this
> > warning.
>
> There is this error on patch 1/3:
>
> ERROR: Co-author Eelco Chaudron  needs to sign off.
>
> So I guess you need to add my sign-off also in that patch.
> >
> >
> > Eelco Chaudron  于2022年5月16日周一 19:36写道：
> >
> >>
> >>
> >> On 16 May 2022, at 12:19, Peng He wrote:
> >>
> >>> Eelco Chaudron  于2022年5月16日周一 17:12写道：
> >>>
> 
> 
>  On 14 May 2022, at 10:40, Peng He wrote:
> 
> > Signed-off-by: Peng He 
> > ---
> >  utilities/checkpatch.py | 32 +---
> >  1 file changed, 29 insertions(+), 3 deletions(-)
> >
> > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> > index 8c7faa419..67c517b69 100755
> > --- a/utilities/checkpatch.py
> > +++ b/utilities/checkpatch.py
> > @@ -619,6 +619,8 @@ def regex_function_factory(func_name):
> >  def regex_error_factory(description):
> >  return lambda: print_error(description)
> >
> 
>  Need extra newline, install flake8 and you will get the error at
> compile
>  time.
> 
> > +def regex_warn_factory(description):
> > +return lambda: print_warning(description)
> 
>  Need extra newline, install flake8 and you will get the error at
> compile
>  time.
> 
> >
> >  std_functions = [
> >  ('malloc', 'Use xmalloc() in place of malloc()'),
> > @@ -636,6 +638,7 @@ std_functions = [
> >  ('assert', 'Use ovs_assert() in place of assert()'),
> >  ('error', 'Use ovs_error() in place of error()'),
> >  ]
> > +
> >  checks += [
> >  {'regex': r'(\.c|\.h)(\.in)?$',
> >   'match_name': None,
> > @@ -644,6 +647,21 @@ checks += [
> >   'print': regex_error_factory(description)}
> >  for (function_name, description) in std_functions]
> >
> > +experimental_api = [
> 
>  I do not think this is an experimental API, I would call it something
> >> like
>  a suspicious API maybe?
> 
> >>>
> >>> or change to easy_to_misused_api?
> >>
> >> This is fine by me to.
> >>
> 
> > +('ovsrcu_barrier',
> > +'lib/ovs-rcu.c',
> > +'Are you sure you need to use ovsrcu_barrier(),'
> 
>  Here you need to add a space at the end, as the error message now
> looks
>  like:
> 
>    WARNING: Are you sure you need to use ovsrcu_barrier(),in most cases
>  ovsrcu_synchronize() will be fine?
> 
> > +'in most cases ovsrcu_synchronize() will be fine?'),
> > +]
> > +
> > +checks += [
> > +{'regex': r'(\.c)(\.in)?$',
> > + 'match_name': lambda x: x != location,
> > + 'prereq': lambda x: not is_comment_line(x),
> > + 'check': regex_function_factory(function_name),
> > + 'print': regex_warn_factory(description)}
> > +for (function_name, location, description) in experimental_api]
> > +
> >
> >  def regex_operator_factory(operator):
> >  regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' %
> operator)
> > @@ -676,12 +694,20 @@ def get_file_type_checks(filename):
> >  global checks
> >  checkList = []
> >  for check in checks:
> > +regex_check = True
> > +match_check = True
> > +
> >  if check['regex'] is None and check['match_name'] is None:
> >  checkList.append(check)
> > +continue
> > +
> >  if check['regex'] is not None and \
> > -   re.compile(check['regex']).search(filename) is not None:
> > -checkList.append(check)
> > -elif check['match_name'] is not None and
>  check['match_name'](filename):
> > +   re.compile(check['regex']).search(filename) is None:
> > +regex_check = False
> > +elif check['match_name'] is not None and not
>  check['match_name'](filename):
> 
>  Line is too long so you need to break it up:
> 
>  utilities/checkpatch.py:709:80: E501 line too long (83 > 79
> characters)
> 
> 
> > +match_check = False
> > +
> > +if regex_check and match_check:
> >  checkList.append(check)
> 
>  Would it not make more sense to re-write 

Re: [ovs-dev] [ovs-dev v4 3/3] checkpatch.py: add checks for experimental API.

[Eelco Chaudron]

On 17 May 2022, at 3:47, Peng He wrote:

> Signed-off-by: Peng He 
> Acked-by: Eelco Chaudron 
> ---

Changes look good to me.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [ovs-dev v4 2/3] ofproto-dpif: fix meter use-after-free

[Eelco Chaudron]

On 17 May 2022, at 3:47, Peng He wrote:

> add a rcu_barrier before close_dpif_backer to ensure that
> all meters has been freed before id_pool_destory meter's
> id-pool.
>
> Signed-off-by: Peng He 
> Tested-by: David Marchand 
> Acked-by: Eelco Chaudron 
> ---
Changes look good to me.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [ovs-dev v4 1/3] ovs-rcu: add rcu_barrier

[Eelco Chaudron]

On 17 May 2022, at 3:47, Peng He wrote:

> rcu_barrier will block the current thread until all the postponed
> rcu job has been finished. it's like the OVS's version of
> the kernel rcu_barrier()
>
> Signed-off-by: Peng He 
> Co-authored-by: Eelco Chaudron 
> Signed-off-by: Eelco Chaudron 

Changes look good to me.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [ovs-dev v3 3/3] checkpatch.py: add checks for experimental API.

[Eelco Chaudron]

On 17 May 2022, at 7:49, Peng He wrote:

> The only issue I have is that I have to use the company's email address to
> submit patches
> while I am using the personal email to subscribe to the maillist.
>
> So the bot will always warn about the author needing to sign-off.
> Perhaps in the future I would use both email addresses to avoid this
> warning.

There is this error on patch 1/3:

ERROR: Co-author Eelco Chaudron  needs to sign off.

So I guess you need to add my sign-off also in that patch.
>
>
> Eelco Chaudron  于2022年5月16日周一 19:36写道：
>
>>
>>
>> On 16 May 2022, at 12:19, Peng He wrote:
>>
>>> Eelco Chaudron  于2022年5月16日周一 17:12写道：
>>>


 On 14 May 2022, at 10:40, Peng He wrote:

> Signed-off-by: Peng He 
> ---
>  utilities/checkpatch.py | 32 +---
>  1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 8c7faa419..67c517b69 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -619,6 +619,8 @@ def regex_function_factory(func_name):
>  def regex_error_factory(description):
>  return lambda: print_error(description)
>

 Need extra newline, install flake8 and you will get the error at compile
 time.

> +def regex_warn_factory(description):
> +return lambda: print_warning(description)

 Need extra newline, install flake8 and you will get the error at compile
 time.

>
>  std_functions = [
>  ('malloc', 'Use xmalloc() in place of malloc()'),
> @@ -636,6 +638,7 @@ std_functions = [
>  ('assert', 'Use ovs_assert() in place of assert()'),
>  ('error', 'Use ovs_error() in place of error()'),
>  ]
> +
>  checks += [
>  {'regex': r'(\.c|\.h)(\.in)?$',
>   'match_name': None,
> @@ -644,6 +647,21 @@ checks += [
>   'print': regex_error_factory(description)}
>  for (function_name, description) in std_functions]
>
> +experimental_api = [

 I do not think this is an experimental API, I would call it something
>> like
 a suspicious API maybe?

>>>
>>> or change to easy_to_misused_api?
>>
>> This is fine by me to.
>>

> +('ovsrcu_barrier',
> +'lib/ovs-rcu.c',
> +'Are you sure you need to use ovsrcu_barrier(),'

 Here you need to add a space at the end, as the error message now looks
 like:

   WARNING: Are you sure you need to use ovsrcu_barrier(),in most cases
 ovsrcu_synchronize() will be fine?

> +'in most cases ovsrcu_synchronize() will be fine?'),
> +]
> +
> +checks += [
> +{'regex': r'(\.c)(\.in)?$',
> + 'match_name': lambda x: x != location,
> + 'prereq': lambda x: not is_comment_line(x),
> + 'check': regex_function_factory(function_name),
> + 'print': regex_warn_factory(description)}
> +for (function_name, location, description) in experimental_api]
> +
>
>  def regex_operator_factory(operator):
>  regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' % operator)
> @@ -676,12 +694,20 @@ def get_file_type_checks(filename):
>  global checks
>  checkList = []
>  for check in checks:
> +regex_check = True
> +match_check = True
> +
>  if check['regex'] is None and check['match_name'] is None:
>  checkList.append(check)
> +continue
> +
>  if check['regex'] is not None and \
> -   re.compile(check['regex']).search(filename) is not None:
> -checkList.append(check)
> -elif check['match_name'] is not None and
 check['match_name'](filename):
> +   re.compile(check['regex']).search(filename) is None:
> +regex_check = False
> +elif check['match_name'] is not None and not
 check['match_name'](filename):

 Line is too long so you need to break it up:

 utilities/checkpatch.py:709:80: E501 line too long (83 > 79 characters)


> +match_check = False
> +
> +if regex_check and match_check:
>  checkList.append(check)

 Would it not make more sense to re-write the above elif cases to a
>> single
 case?

>  return checkList
>
> --
> 2.25.1


>>> will send a new version， thanks for the detailed review.
>>
>> You can also add my Signed-off-by: if you make the suggested changes to
>> keep the robot happy.
>>
>> //Eelco
>>
>>
>
> -- 
> hepeng

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

Re: [ovs-dev] [PATCH v3 5/5] tests: Add check_pkt_len action test to system-offload-traffic.

[Eelco Chaudron]

On 16 May 2022, at 18:04, 0-day Robot wrote:

> 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' to see the failed patch
> Patch failed at 0001 tests: Add check_pkt_len action test to 
> system-offload-traffic.
> 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


This failure was due to a merge conflict in the NEWS file. I will not send a v4 
for now as this will continue to happen. If the maintainers do want a v4, I'll 
send it.

//Eelco

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

Re: [ovs-dev] [PATCH ovn] controller: avoid recomputes triggered by SBDB Port_Binding updates.

[Han Zhou]

On Thu, May 12, 2022 at 2:04 AM Xavier Simonart  wrote:
>
> When VIF ports are claimed on a chassis, SBDB Port_Binding table is
updated.
> If the SBDB IDL is still is read-only ("in transaction") when such a
update
> is required, the update is not possible and recompute is triggered through
> I+P failure.
>
> This situation can happen:
> - after updating Port_Binding->chassis to SBDB for one port, in a
following
>   iteration, ovn-controller handles Interface:external_ids:ovn-installed
>   (for the same port) while SBDB is still read-only.
> - after updating Port_Binding->chassis to SBDB for one port, in a
following
>   iteration, ovn-controller updates Port_Binding->chassis for another
port,
>   while SBDB is still read-only.
>
> This patch prevent the recompute, by having the if-status module
> updating the Port_Binding chassis (if needed), when possible.
> This does not delay Port_Binding chassis update compared to before this
> patch: Port_Binding chassis will be updated as soon as SBDB is again
> writable, as it was the case, through a recompute, before this patch.
>
Thanks Xavier. I think the approach is good: moving the SB update from the
I-P engine to if-status module, so it wouldn't need to trigger I-P engine
recompute, and just let the if-status module update the SB as soon as it is
writable, through the if-status's state-machine.
However, I have some comments/questions regarding the implementation
details, primarily confusion on the state transitions. Please see below.

> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2059253
> Signed-off-by: Xavier Simonart 
> ---
>  controller/binding.c| 124 ++--
>  controller/binding.h|   9 ++-
>  controller/if-status.c  |  31 +++--
>  controller/if-status.h  |   6 +-
>  controller/ovn-controller.c |   6 +-
>  tests/ovn.at|  55 +++-
>  6 files changed, 184 insertions(+), 47 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index e5ba56b25..d917d8775 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -657,11 +657,15 @@ local_binding_get_lport_ofport(const struct shash
*local_bindings,
>  }
>
>  bool
> -local_binding_is_up(struct shash *local_bindings, const char *pb_name)
> +local_binding_is_up(struct shash *local_bindings, const char *pb_name,
> +const struct sbrec_chassis *chassis_rec)
>  {
>  struct local_binding *lbinding =
>  local_binding_find(local_bindings, pb_name);
>  struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
> +if (b_lport && b_lport->pb->chassis != chassis_rec) {
> +return false;
> +}

Why need the change here? I understand that it is obvious that the binding
should not be considered up if the chassis is not updated in SB
port_binding, but I wonder why we need the change in *this* patch.
The function is called to see if an interface state should be moved from
MARK_UP to INSTALLED, but if the chassis is not set, it shouldn't even be
moved to MARK_UP.

>  if (lbinding && b_lport && lbinding->iface) {
>  if (b_lport->pb->n_up && !b_lport->pb->up[0]) {
>  return false;
> @@ -673,13 +677,23 @@ local_binding_is_up(struct shash *local_bindings,
const char *pb_name)
>  }
>
>  bool
> -local_binding_is_down(struct shash *local_bindings, const char *pb_name)
> +local_binding_is_down(struct shash *local_bindings, const char *pb_name,
> +  const struct sbrec_chassis *chassis_rec)
>  {
>  struct local_binding *lbinding =
>  local_binding_find(local_bindings, pb_name);
>
>  struct binding_lport *b_lport =
local_binding_get_primary_lport(lbinding);
>
> +if (b_lport) {
> +if (b_lport->pb->chassis == chassis_rec) {
> +return false;
> +} else if (b_lport->pb->chassis) {
> +VLOG_DBG("lport %s already claimed by other chassis",
> + b_lport->pb->logical_port);
> +}
> +}
> +

Same as above, it is not clear to me why this change here. It would be
better to clarify the state transition first.

>  if (!lbinding) {
>  return true;
>  }
> @@ -894,6 +908,48 @@ get_lport_type_str(enum en_lport_type lport_type)
>  OVS_NOT_REACHED();
>  }
>
> +static void set_pb_chassis_in_sbrec(const struct sbrec_port_binding *pb,
> +  const struct sbrec_chassis *chassis_rec)
> +{
> +if (pb->chassis != chassis_rec) {
> +if (pb->chassis) {
> +VLOG_INFO("Changing chassis for lport %s from %s to %s.",
> +pb->logical_port, pb->chassis->name,
> +chassis_rec->name);
> +} else {
> +VLOG_INFO("Claiming lport %s for this chassis.",
pb->logical_port);
> +}
> +for (int i = 0; i < pb->n_mac; i++) {
> +VLOG_INFO("%s: Claiming %s", pb->logical_port, pb->mac[i]);
> +}
> +