Re: [ovs-dev] [ovs-dev v3 3/4] ofproto-dpif-upcall: new ukey needs to take the old ukey's dump seq

[Peng He]

Hi，Eelco,

Which kernel version you have tested on?
I've also tested the #50 for several hundreds times, but only get 0 packets
once.

I have put your patches upon on commits 49efc63ad: ofproto-dpif-xlate: Fix
error messages for nonexistent ports/recirc_ids.
not sure if it's related to the modification on master branch.




Eelco Chaudron  于2022年10月7日周五 18:54写道：

> Hi hepeng,
>
> I tried your changes, but I also do get a lot of zero packet failures on
> test 50. Not sure what happened, maybe it was related to syncing to the
> latest master.
>
> However I focused on test 49, and I can replicate it every x test, but
> within 40 tries:
>
> ## -- ##
> ## openvswitch 3.0.90 test suite. ##
> ## -- ##
> 49: datapath - truncate and output to gre tunnel by simulated packets
> FAILED (ovs-macros.at:247)
>
> ...
> ...
> system-traffic.at:1663: waiting until ovs-ofctl dump-flows br-underlay |
> grep "in_port=LOCAL" | sed -n 's/.*\(n\_bytes=[0-9]*\).*/\1/p' | grep
> "n_bytes=138"...
> system-traffic.at:1663: wait failed after 30 seconds
> n_bytes=18446744073707612670
> ./ovs-macros.at:247: hard failure
>
> I’ll try to dig into this later once I get some time...
>
> //Eelco
>
> On 2 Oct 2022, at 8:40, Peng He wrote:
>
> Hi,
>
> I have tried test #50 for 200 times, the overflow as well as stats of 0
> did not show up.
>
> the command line is:
> for i in {1..100}; do echo $i; make check-offloads TESTSUITEFLAGS="-v 50"
> || break; done
>
> I am using Debian 10, and my kernel version is
> root@ubuntu:~/ovs# uname -r
> 5.4.0-121-generic
>
> I doubt the overflow might be related to kernel datapath.
> IIRC, the kernel datapath does not support some IPv6 ND/NA match.
>
> In the logs, there are some warning saying that "failed to get/del/put
> some datapath flow", which is all
> related the ICMPv6. It seems to prove the missing of the support of NA/ND
> match in datapath.
>
> since the ICMPv6 is not related to the test itself. I did some
> modification to the test to drop all IPv6 traffic.
>
> Below is my complete diff to the #50:
>
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index dfbc30e47..b759c4bb9 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -1762,7 +1762,7 @@ on_exit 'rm -f payload200.bin'
>
>  AT_CHECK([ovs-ofctl del-flows br0])
>  AT_DATA([flows.txt], [dnl
>
> -priority=99,in_port=1,actions=output(port=2,max_len=100),output(port=3,max_len=100)
>
> +priority=99,in_port=1,ip,actions=output(port=2,max_len=100),output(port=3,max_len=100)
>  priority=99,in_port=2,udp,actions=output(port=1,max_len=100)
>  priority=1,in_port=4,ip,actions=drop
>  priority=1,actions=drop
> @@ -1771,6 +1771,8 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>
>  AT_CHECK([ovs-ofctl del-flows br-underlay])
>  AT_DATA([flows-underlay.txt], [dnl
> +priority=99,arp,in_port=1,actions=LOCAL
> +priority=99,arp,in_port=LOCAL,actions=1
>  priority=99,dl_type=0x0800,nw_proto=47,in_port=1,actions=LOCAL
>  priority=99,dl_type=0x0800,nw_proto=47,in_port=LOCAL,ip_dst=
> 172.31.1.1/24,actions=1
>  priority=1,actions=drop
> @@ -1787,7 +1789,7 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep
> "in_port=2" | sed -n 's/.*\(n\_bytes=[
>  n_bytes=242
>  ])
>  dnl After truncation = outer ETH(14) + outer IP(20) + GRE(4) + 100 = 138B
> -AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed
> -n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
> +AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "ip,in_port=LOCAL" |
> sed -n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
>  n_bytes=138
>  ])
>
> @@ -1822,7 +1824,7 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep
> "in_port=2" | sed -n 's/.*\(n\_bytes=[
>  n_bytes=242
>  ])
>  dnl After truncation = outer ETH(14) + outer IP(20) + GRE(4) + 100 = 138B
> -AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | sed
> -n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
> +AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "ip,in_port=LOCAL" |
> sed -n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
>  n_bytes=138
>  ])
>
> @@ -1834,7 +1836,11 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep
> "in_port=4" | ofctl_strip], [0], [dnl
>   n_packets=1, n_bytes=100, priority=1,ip,in_port=4 actions=drop
>  ])
>
> -OVS_TRAFFIC_VSWITCHD_STOP
> +OVS_TRAFFIC_VSWITCHD_STOP(["dnl
> +/.*lost packet on port.*/d
> +/.failed to flow_get.*/d
> +/.failed to flow_del.*/d
> +/.*Failed to acquire udpif_key corresponding to unexpected flow.*/d"])
>  AT_CLEANUP
>
>
> It seems that the overflow here is not related to the race in
> revalidators, but it is related to the design of
> the testsuite and also the missing of the support of NA/ND in kernel
> datapath.
>
>
>
> Eelco Chaudron  于2022年9月30日周五 23:47写道：
>
>> On 30 Sep 2022, at 17:41, Peng He wrote:
>>
>> > It's so easy to reproduce ?
>> >
>> > Thanks! then I should have to dig it again.
>>
>> It’s been on my to-do for a while but did not get to it. Sometimes it
>> reproduced 

Re: [ovs-dev] [PATCH v4 3/3] dpif-netdev: fix inconsistent processing between ukey and megaflow

[Peng He]

Hi，Eelco

after a second thought, I think this patch is not needed neither,
the code here is trying to find a rule which cover the packet,
it does not mean the match and action of rule equals to the ones
of the ukey.

So the code here is just a prevention, no need to make it consistent
with ukey.

but the comments above are really misleading, so I sent a new patch fixing
it.

Peng He  于2022年10月3日周一 20:41写道：

> When PMDs perform upcalls, the newly generated ukey will replace
> the old, however, the newly generated mageflow will be discard
> to reuse the old one without checking if the actions of new and
> old are equal.
>
> This code prevents in case someone runs dpctl/add-flow to add
> a dp flow with inconsistent actions with the actions of ukey,
> and causes more confusion.
>
> Signed-off-by: Peng He 
> ---
>  lib/dpif-netdev.c | 17 -
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index a45b46014..b316e59ef 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -8304,7 +8304,22 @@ handle_packet_upcall(struct dp_netdev_pmd_thread
> *pmd,
>   * to be locking revalidators out of making flow modifications. */
>  ovs_mutex_lock(>flow_mutex);
>  netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL);
> -if (OVS_LIKELY(!netdev_flow)) {
> +if (OVS_UNLIKELY(netdev_flow)) {
> +struct dp_netdev_actions *old_act =
> +dp_netdev_flow_get_actions(netdev_flow);
> +
> +if ((add_actions->size != old_act->size) ||
> +memcmp(old_act->actions, add_actions->data,
> + add_actions->size)) {
> +
> +   struct dp_netdev_actions *new_act =
> +   dp_netdev_actions_create(add_actions->data,
> +add_actions->size);
> +
> +   ovsrcu_set(_flow->actions, new_act);
> +   ovsrcu_postpone(dp_netdev_actions_free, old_act);
> +}
> +} else {
>  netdev_flow = dp_netdev_flow_add(pmd, , ,
>   add_actions->data,
>   add_actions->size,
> orig_in_port);
> --
> 2.25.1
>
>

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

Re: [ovs-dev] [PATCH ovn 5/8] controller: Add support for templated actions and matches.

[Han Zhou]

On Fri, Oct 7, 2022 at 12:23 PM Ilya Maximets  wrote:
>
> On 10/7/22 20:31, Han Zhou wrote:
> >
> >
> > On Fri, Oct 7, 2022 at 10:24 AM Ilya Maximets mailto:i.maxim...@ovn.org>> wrote:
> >>
> >> On 10/7/22 10:26, Dumitru Ceara wrote:
> >> > On 10/7/22 04:03, Han Zhou wrote:
> >> >> On Fri, Sep 30, 2022 at 7:02 AM Dumitru Ceara mailto:dce...@redhat.com>> wrote:
> >> >>>
> >> >>> Expand SB.Template_Var records in two stages:
> >> >>> 1. first expand them to local values in match/action strings
> >> >>> 2. then reparse the expanded strings
> >> >>>
> >> >>> For the case when a lflow references a Template_Var also track
> >> >>> references (similar to the ones maintained for multicast groups,
address
> >> >>> sets, port_groups, port bindings).
> >> >>>
> >> >>> Signed-off-by: Dumitru Ceara >
> >> >>
> >> >> Hi Dumitru,
> >> >>
> >> >
> >> > Hi Han,
> >> >
> >> >> In addition to the two-stage parsing concerns we are discussing in
the
> >> >
> >> > I'm doing some more targetted performance testing.  I'll update the
> >> > other thread when I have more data to share.
> >> >
> >> >> other thread, I have some minor comments below. The major one is
whether we
> >> >> should allow matching hostname or not.
> >> >>
> >> >>> ---
> >> >>>  controller/lflow.c  |   59 +++-
> >> >>>  controller/lflow.h  |1
> >> >>>  controller/lport.c  |3
> >> >>>  controller/ofctrl.c |9 +
> >> >>>  controller/ofctrl.h |3
> >> >>>  controller/ovn-controller.c |  317
> >> >> +++
> >> >>>  include/ovn/expr.h  |4 -
> >> >>>  include/ovn/lex.h   |   14 +-
> >> >>>  lib/actions.c   |9 +
> >> >>>  lib/expr.c  |   18 ++
> >> >>>  lib/lex.c   |   55 +++
> >> >>>  lib/objdep.c|1
> >> >>>  lib/objdep.h|1
> >> >>>  lib/ovn-util.c  |7 +
> >> >>>  lib/ovn-util.h  |3
> >> >>>  tests/ovn.at |2
> >> >>>  tests/test-ovn.c|   16 ++
> >> >>>  utilities/ovn-trace.c   |   26 +++-
> >> >>>  18 files changed, 512 insertions(+), 36 deletions(-)
> >> >>>
> >>
> >> ...
> >>
> >> >>> +struct ed_type_template_vars {
> >> >>> +struct local_templates var_table;
> >> >>> +
> >> >>> +bool change_tracked;
> >> >>> +struct sset new;
> >> >>> +struct sset deleted;
> >> >>> +struct sset updated;
> >> >>> +};
> >> >>> +
> >> >>> +static void
> >> >>> +template_vars_init(const struct sbrec_template_var_table
*tv_table,
> >> >>> +   const struct sbrec_chassis *chassis,
> >> >>> +   struct local_templates *var_table)
> >> >>> +{
> >> >>> +const struct sbrec_template_var *tv;
> >> >>> +SBREC_TEMPLATE_VAR_TABLE_FOR_EACH (tv, tv_table) {
> >> >>> +if (chassis_name_equals(tv->chassis_name, chassis)) {
> >> >>
> >> >> I am not sure if it is a good idea to allow using hostname to match
the
> >> >> template var name. It provides flexibility to CMS, but we will need
more
> >> >> complexity to protect against corner cases.
> >> >> For example, if there are two records:
> >> >> r1: name="abc", value="v1"
> >> >> r2: hostname="abc", value="v2"
> >> >>
> >> >> Now with the current logic, whichever is handled later will take
precedence
> >> >> (in the local_templates.vars) and the value will be used (assume r2
"v2" is
> >> >> used). This may be fine, because the user should be responsible for
the
> >> >> inconsistent configurations.
> >> >>
> >> >> Later, when the user detects the problem and wants to correct the
> >> >> configuration. He/she deletes the r2 and expects the var "abc" to be
> >> >> expanded as "v1". But the logic in template_vars_update() would call
> >> >> local_templates_remove() which simply deletes the var ("abc" ->
"v2")
> >> >> instead of replacing it with ("abc" -> "v1"). The uuid of "abc" ->
"v1"
> >> >> will still be left in the uuidset, which is useless. This is an
unexpected
> >> >> behavior.
> >> >>
> >> >> Similar behavior would happen if there are duplicate hostnames,
e.g.:
> >> >> r1: hostname="abc", value="v1"
> >> >> r2: hostname="abc", value="v2"
> >> >>
> >> >
> >> > Very good point, nice catch!
> >>
> >> In general it might make sense to choose a bit different database
schema,
> >> e.g.:
> >>
> >> "Chassis_Template_Vars": {
> >> "columns": {
> >> "chassis": {"type": "string"},
> >> "variables": {
> >> "type": {"key": "string", "value": "string",
> >>  "min": 0, "max": "unlimited"}},
> >> "indexes": [["chassis"]],
> >> "isRoot": true}
> >>
> >> Here 'variables' or 'templates' or whatever you want to call it is a
> >> Var->Value map.
> >>
> >> Index on the 'chassis' column will provide uniqueness of chassis names,
> >> map has unique keys, so all variable names are unique within a 

Re: [ovs-dev] [PATCH ovn 5/8] controller: Add support for templated actions and matches.

[Ilya Maximets]

On 10/7/22 20:31, Han Zhou wrote:
> 
> 
> On Fri, Oct 7, 2022 at 10:24 AM Ilya Maximets  > wrote:
>>
>> On 10/7/22 10:26, Dumitru Ceara wrote:
>> > On 10/7/22 04:03, Han Zhou wrote:
>> >> On Fri, Sep 30, 2022 at 7:02 AM Dumitru Ceara > >> > wrote:
>> >>>
>> >>> Expand SB.Template_Var records in two stages:
>> >>> 1. first expand them to local values in match/action strings
>> >>> 2. then reparse the expanded strings
>> >>>
>> >>> For the case when a lflow references a Template_Var also track
>> >>> references (similar to the ones maintained for multicast groups, address
>> >>> sets, port_groups, port bindings).
>> >>>
>> >>> Signed-off-by: Dumitru Ceara > >>> >
>> >>
>> >> Hi Dumitru,
>> >>
>> >
>> > Hi Han,
>> >
>> >> In addition to the two-stage parsing concerns we are discussing in the
>> >
>> > I'm doing some more targetted performance testing.  I'll update the
>> > other thread when I have more data to share.
>> >
>> >> other thread, I have some minor comments below. The major one is whether 
>> >> we
>> >> should allow matching hostname or not.
>> >>
>> >>> ---
>> >>>  controller/lflow.c          |   59 +++-
>> >>>  controller/lflow.h          |    1
>> >>>  controller/lport.c          |    3
>> >>>  controller/ofctrl.c         |    9 +
>> >>>  controller/ofctrl.h         |    3
>> >>>  controller/ovn-controller.c |  317
>> >> +++
>> >>>  include/ovn/expr.h          |    4 -
>> >>>  include/ovn/lex.h           |   14 +-
>> >>>  lib/actions.c               |    9 +
>> >>>  lib/expr.c                  |   18 ++
>> >>>  lib/lex.c                   |   55 +++
>> >>>  lib/objdep.c                |    1
>> >>>  lib/objdep.h                |    1
>> >>>  lib/ovn-util.c              |    7 +
>> >>>  lib/ovn-util.h              |    3
>> >>>  tests/ovn.at                 |    2
>> >>>  tests/test-ovn.c            |   16 ++
>> >>>  utilities/ovn-trace.c       |   26 +++-
>> >>>  18 files changed, 512 insertions(+), 36 deletions(-)
>> >>>
>>
>> ...
>>
>> >>> +struct ed_type_template_vars {
>> >>> +    struct local_templates var_table;
>> >>> +
>> >>> +    bool change_tracked;
>> >>> +    struct sset new;
>> >>> +    struct sset deleted;
>> >>> +    struct sset updated;
>> >>> +};
>> >>> +
>> >>> +static void
>> >>> +template_vars_init(const struct sbrec_template_var_table *tv_table,
>> >>> +                   const struct sbrec_chassis *chassis,
>> >>> +                   struct local_templates *var_table)
>> >>> +{
>> >>> +    const struct sbrec_template_var *tv;
>> >>> +    SBREC_TEMPLATE_VAR_TABLE_FOR_EACH (tv, tv_table) {
>> >>> +        if (chassis_name_equals(tv->chassis_name, chassis)) {
>> >>
>> >> I am not sure if it is a good idea to allow using hostname to match the
>> >> template var name. It provides flexibility to CMS, but we will need more
>> >> complexity to protect against corner cases.
>> >> For example, if there are two records:
>> >> r1: name="abc", value="v1"
>> >> r2: hostname="abc", value="v2"
>> >>
>> >> Now with the current logic, whichever is handled later will take 
>> >> precedence
>> >> (in the local_templates.vars) and the value will be used (assume r2 "v2" 
>> >> is
>> >> used). This may be fine, because the user should be responsible for the
>> >> inconsistent configurations.
>> >>
>> >> Later, when the user detects the problem and wants to correct the
>> >> configuration. He/she deletes the r2 and expects the var "abc" to be
>> >> expanded as "v1". But the logic in template_vars_update() would call
>> >> local_templates_remove() which simply deletes the var ("abc" -> "v2")
>> >> instead of replacing it with ("abc" -> "v1"). The uuid of "abc" -> "v1"
>> >> will still be left in the uuidset, which is useless. This is an unexpected
>> >> behavior.
>> >>
>> >> Similar behavior would happen if there are duplicate hostnames, e.g.:
>> >> r1: hostname="abc", value="v1"
>> >> r2: hostname="abc", value="v2"
>> >>
>> >
>> > Very good point, nice catch!
>>
>> In general it might make sense to choose a bit different database schema,
>> e.g.:
>>
>>         "Chassis_Template_Vars": {
>>             "columns": {
>>                 "chassis": {"type": "string"},
>>                 "variables": {
>>                     "type": {"key": "string", "value": "string",
>>                              "min": 0, "max": "unlimited"}},
>>             "indexes": [["chassis"]],
>>             "isRoot": true}
>>
>> Here 'variables' or 'templates' or whatever you want to call it is a
>> Var->Value map.
>>
>> Index on the 'chassis' column will provide uniqueness of chassis names,
>> map has unique keys, so all variable names are unique within a chassis
>> as well.  This should cover all the possible misconfigurations on the
>> database level.
>>
>> As a bonus we will also save a lot of database space by not storing
>> millions of copies of chassis and variable names.  

Re: [ovs-dev] [PATCH ovn 5/8] controller: Add support for templated actions and matches.

[Han Zhou]

On Fri, Oct 7, 2022 at 10:24 AM Ilya Maximets  wrote:
>
> On 10/7/22 10:26, Dumitru Ceara wrote:
> > On 10/7/22 04:03, Han Zhou wrote:
> >> On Fri, Sep 30, 2022 at 7:02 AM Dumitru Ceara 
wrote:
> >>>
> >>> Expand SB.Template_Var records in two stages:
> >>> 1. first expand them to local values in match/action strings
> >>> 2. then reparse the expanded strings
> >>>
> >>> For the case when a lflow references a Template_Var also track
> >>> references (similar to the ones maintained for multicast groups,
address
> >>> sets, port_groups, port bindings).
> >>>
> >>> Signed-off-by: Dumitru Ceara 
> >>
> >> Hi Dumitru,
> >>
> >
> > Hi Han,
> >
> >> In addition to the two-stage parsing concerns we are discussing in the
> >
> > I'm doing some more targetted performance testing.  I'll update the
> > other thread when I have more data to share.
> >
> >> other thread, I have some minor comments below. The major one is
whether we
> >> should allow matching hostname or not.
> >>
> >>> ---
> >>>  controller/lflow.c  |   59 +++-
> >>>  controller/lflow.h  |1
> >>>  controller/lport.c  |3
> >>>  controller/ofctrl.c |9 +
> >>>  controller/ofctrl.h |3
> >>>  controller/ovn-controller.c |  317
> >> +++
> >>>  include/ovn/expr.h  |4 -
> >>>  include/ovn/lex.h   |   14 +-
> >>>  lib/actions.c   |9 +
> >>>  lib/expr.c  |   18 ++
> >>>  lib/lex.c   |   55 +++
> >>>  lib/objdep.c|1
> >>>  lib/objdep.h|1
> >>>  lib/ovn-util.c  |7 +
> >>>  lib/ovn-util.h  |3
> >>>  tests/ovn.at|2
> >>>  tests/test-ovn.c|   16 ++
> >>>  utilities/ovn-trace.c   |   26 +++-
> >>>  18 files changed, 512 insertions(+), 36 deletions(-)
> >>>
>
> ...
>
> >>> +struct ed_type_template_vars {
> >>> +struct local_templates var_table;
> >>> +
> >>> +bool change_tracked;
> >>> +struct sset new;
> >>> +struct sset deleted;
> >>> +struct sset updated;
> >>> +};
> >>> +
> >>> +static void
> >>> +template_vars_init(const struct sbrec_template_var_table *tv_table,
> >>> +   const struct sbrec_chassis *chassis,
> >>> +   struct local_templates *var_table)
> >>> +{
> >>> +const struct sbrec_template_var *tv;
> >>> +SBREC_TEMPLATE_VAR_TABLE_FOR_EACH (tv, tv_table) {
> >>> +if (chassis_name_equals(tv->chassis_name, chassis)) {
> >>
> >> I am not sure if it is a good idea to allow using hostname to match the
> >> template var name. It provides flexibility to CMS, but we will need
more
> >> complexity to protect against corner cases.
> >> For example, if there are two records:
> >> r1: name="abc", value="v1"
> >> r2: hostname="abc", value="v2"
> >>
> >> Now with the current logic, whichever is handled later will take
precedence
> >> (in the local_templates.vars) and the value will be used (assume r2
"v2" is
> >> used). This may be fine, because the user should be responsible for the
> >> inconsistent configurations.
> >>
> >> Later, when the user detects the problem and wants to correct the
> >> configuration. He/she deletes the r2 and expects the var "abc" to be
> >> expanded as "v1". But the logic in template_vars_update() would call
> >> local_templates_remove() which simply deletes the var ("abc" -> "v2")
> >> instead of replacing it with ("abc" -> "v1"). The uuid of "abc" -> "v1"
> >> will still be left in the uuidset, which is useless. This is an
unexpected
> >> behavior.
> >>
> >> Similar behavior would happen if there are duplicate hostnames, e.g.:
> >> r1: hostname="abc", value="v1"
> >> r2: hostname="abc", value="v2"
> >>
> >
> > Very good point, nice catch!
>
> In general it might make sense to choose a bit different database schema,
> e.g.:
>
> "Chassis_Template_Vars": {
> "columns": {
> "chassis": {"type": "string"},
> "variables": {
> "type": {"key": "string", "value": "string",
>  "min": 0, "max": "unlimited"}},
> "indexes": [["chassis"]],
> "isRoot": true}
>
> Here 'variables' or 'templates' or whatever you want to call it is a
> Var->Value map.
>
> Index on the 'chassis' column will provide uniqueness of chassis names,
> map has unique keys, so all variable names are unique within a chassis
> as well.  This should cover all the possible misconfigurations on the
> database level.
>
> As a bonus we will also save a lot of database space by not storing
> millions of copies of chassis and variable names.  May speed up the
> Nb->Sb synchronization as well.
>
> One downside is that we can't have true I-P for that table due to
> inability to track which values in a map actually changed.  Though
> it should be possible to figure out the diff in a semi-linear time
> from the number of variables.  

Re: [ovs-dev] [PATCH 1/2] netdev-offload: Expose error when init_flow_api() returns EBUSY.

[Mike Pattrick]

On Mon, Aug 8, 2022 at 12:54 PM Han Zhou  wrote:
>
> init_flow_api() can fail due to errors, such as netlink "Device or
> resource busy". Today the errors are ignored except just an INFO log.
> This ends up with an interface created without offload enabled, and
> application won't notice it. This patch expose the error to the OVSDB
> record, and reverts the dpif port changes upon failure.
>
> Signed-off-by: Han Zhou 

This seems reasonable to me.

Acked-by: Mike Pattrick 

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

Re: [ovs-dev] [OVSCON] Talk descriptions and abstracts are up

[Aaron Conole]

Apologies for the truncation.

Remeber that for the hotel, we have partnered with Mariott to provide a
block of discounted rooms.  These are available on a first-come,
first-served basis and are limited in quantity.  The link to register is at:

https://www.marriott.com/event-reservations/reservation-link.mi?id=1659447787685=GRP=resvlink

Aaron Conole  writes:

> Greetings,
>
> The initial list of talks can now be found on https://ovscon.site along
> with a registration link to register.  Remember that the hotel 
>
> We have a number of workshops to announce in the coming days as well, so
> stay tuned.
>
> -OVS+OvN Planning committee

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

[ovs-dev] [OVSCON] Talk descriptions and abstracts are up

[Aaron Conole]

Greetings,

The initial list of talks can now be found on https://ovscon.site along
with a registration link to register.  Remember that the hotel 

We have a number of workshops to announce in the coming days as well, so
stay tuned.

-OVS+OvN Planning committee

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

Re: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats

[Marcelo Leitner]

On Fri, Oct 07, 2022 at 11:59:42AM -0400, Jamal Hadi Salim wrote:
> On Fri, Oct 7, 2022 at 11:01 AM Marcelo Leitner  wrote:
> >
> > On Fri, Oct 07, 2022 at 04:39:25PM +0200, Davide Caratti wrote:
> > > On Fri, Oct 7, 2022 at 3:21 PM Marcelo Leitner  
> > > wrote:
> > > >
> > > > (+TC folks and netdev@)
> > > >
> > > > On Fri, Oct 07, 2022 at 02:42:56PM +0200, Ilya Maximets wrote:
> > > > > On 10/7/22 13:37, Eelco Chaudron wrote:
> > >
> > > [...]
> > >
> > > > I don't see how we could achieve this without breaking much of the
> > > > user experience.
> > > >
> > > > >
> > > > > - or create something like act_count - a dummy action that only
> > > > >   counts packets, and put it in every datapath action from OVS.
> > > >
> > > > This seems the easiest and best way forward IMHO. It's actually the
> > > > 3rd option below but "on demand", considering that tc will already use
> > > > the stats of the first action as the flow stats (in
> > > > tcf_exts_dump_stats()), then we can patch ovs to add such action if a
> > > > meter is also being used (or perhaps even always, because other
> > > > actions may also drop packets, and for OVS we would really be at the
> > > > 3rd option below).
> > >
> > > Correct me if I'm wrong, but actually act_gact action with "pipe"
> > > control action should already do this counting job.
> >
> > act_gact is so transparent that I never see it/remembers about it :)
> > Yup, although it's not offloadabe with pipe control actio AFAICT.
> >
>
> It's mostly how people who offload (not sure about OVS) do it;
> example some of the switches out there.

You mean with OK, DROP, TRAP or GOTO actions, right?

Because for PIPE, it has:
} else if (is_tcf_gact_pipe(act)) {
NL_SET_ERR_MSG_MOD(extack, "Offload of
\"pipe\" action is not supported");
return -EOPNOTSUPP;

Or maybe I'm missing something.

> The action index, whatever that action is, could be easily mapped
> to a stats index in hardware. If you are sharing action instances
> (eg policer index 32) across multiple flows then of course in hw
> you are using only that one instance of the meter. If you want
> to have extra stats that differentiate between the flows then
> add a gact (PIPE as Davide mentioned) and the only thing it
> will do is count and nothing else.

Yup, makes sense. And talking with Ilya, that's what OVS already does for DPDK
as well.

Thanks,
Marcelo

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

Re: [ovs-dev] [PATCH ovn 5/8] controller: Add support for templated actions and matches.

[Ilya Maximets]

On 10/7/22 10:26, Dumitru Ceara wrote:
> On 10/7/22 04:03, Han Zhou wrote:
>> On Fri, Sep 30, 2022 at 7:02 AM Dumitru Ceara  wrote:
>>>
>>> Expand SB.Template_Var records in two stages:
>>> 1. first expand them to local values in match/action strings
>>> 2. then reparse the expanded strings
>>>
>>> For the case when a lflow references a Template_Var also track
>>> references (similar to the ones maintained for multicast groups, address
>>> sets, port_groups, port bindings).
>>>
>>> Signed-off-by: Dumitru Ceara 
>>
>> Hi Dumitru,
>>
> 
> Hi Han,
> 
>> In addition to the two-stage parsing concerns we are discussing in the
> 
> I'm doing some more targetted performance testing.  I'll update the
> other thread when I have more data to share.
> 
>> other thread, I have some minor comments below. The major one is whether we
>> should allow matching hostname or not.
>>
>>> ---
>>>  controller/lflow.c  |   59 +++-
>>>  controller/lflow.h  |1
>>>  controller/lport.c  |3
>>>  controller/ofctrl.c |9 +
>>>  controller/ofctrl.h |3
>>>  controller/ovn-controller.c |  317
>> +++
>>>  include/ovn/expr.h  |4 -
>>>  include/ovn/lex.h   |   14 +-
>>>  lib/actions.c   |9 +
>>>  lib/expr.c  |   18 ++
>>>  lib/lex.c   |   55 +++
>>>  lib/objdep.c|1
>>>  lib/objdep.h|1
>>>  lib/ovn-util.c  |7 +
>>>  lib/ovn-util.h  |3
>>>  tests/ovn.at|2
>>>  tests/test-ovn.c|   16 ++
>>>  utilities/ovn-trace.c   |   26 +++-
>>>  18 files changed, 512 insertions(+), 36 deletions(-)
>>>

...

>>> +struct ed_type_template_vars {
>>> +struct local_templates var_table;
>>> +
>>> +bool change_tracked;
>>> +struct sset new;
>>> +struct sset deleted;
>>> +struct sset updated;
>>> +};
>>> +
>>> +static void
>>> +template_vars_init(const struct sbrec_template_var_table *tv_table,
>>> +   const struct sbrec_chassis *chassis,
>>> +   struct local_templates *var_table)
>>> +{
>>> +const struct sbrec_template_var *tv;
>>> +SBREC_TEMPLATE_VAR_TABLE_FOR_EACH (tv, tv_table) {
>>> +if (chassis_name_equals(tv->chassis_name, chassis)) {
>>
>> I am not sure if it is a good idea to allow using hostname to match the
>> template var name. It provides flexibility to CMS, but we will need more
>> complexity to protect against corner cases.
>> For example, if there are two records:
>> r1: name="abc", value="v1"
>> r2: hostname="abc", value="v2"
>>
>> Now with the current logic, whichever is handled later will take precedence
>> (in the local_templates.vars) and the value will be used (assume r2 "v2" is
>> used). This may be fine, because the user should be responsible for the
>> inconsistent configurations.
>>
>> Later, when the user detects the problem and wants to correct the
>> configuration. He/she deletes the r2 and expects the var "abc" to be
>> expanded as "v1". But the logic in template_vars_update() would call
>> local_templates_remove() which simply deletes the var ("abc" -> "v2")
>> instead of replacing it with ("abc" -> "v1"). The uuid of "abc" -> "v1"
>> will still be left in the uuidset, which is useless. This is an unexpected
>> behavior.
>>
>> Similar behavior would happen if there are duplicate hostnames, e.g.:
>> r1: hostname="abc", value="v1"
>> r2: hostname="abc", value="v2"
>>
> 
> Very good point, nice catch!

In general it might make sense to choose a bit different database schema,
e.g.:

"Chassis_Template_Vars": {
"columns": {
"chassis": {"type": "string"},
"variables": {
"type": {"key": "string", "value": "string",
 "min": 0, "max": "unlimited"}},
"indexes": [["chassis"]],
"isRoot": true}

Here 'variables' or 'templates' or whatever you want to call it is a
Var->Value map.

Index on the 'chassis' column will provide uniqueness of chassis names,
map has unique keys, so all variable names are unique within a chassis
as well.  This should cover all the possible misconfigurations on the
database level.

As a bonus we will also save a lot of database space by not storing
millions of copies of chassis and variable names.  May speed up the
Nb->Sb synchronization as well.

One downside is that we can't have true I-P for that table due to
inability to track which values in a map actually changed.  Though
it should be possible to figure out the diff in a semi-linear time
from the number of variables.  OpenFlow rules can still be processed
incrementally after that.  So, I'm not sure if that is a big performance
concern.  Testing is needed, I guess.

Conditional monitoring will be very simple.  Chassis will receive only
one row in most cases.  update2 will cover variable updates.

What do you 

Re: [ovs-dev] [PATCH branch-3.0 0/2] Release patches for v3.0.1.

[Ilya Maximets]

On 10/7/22 16:57, Flavio Leitner wrote:
> On Fri, Oct 07, 2022 at 01:17:33PM +0200, Ilya Maximets wrote:
>>
>> Ilya Maximets (2):
>>   Set release date for 3.0.1.
>>   Prepare for 3.0.2.
>>
>>  NEWS | 6 +-
>>  configure.ac | 2 +-
>>  debian/changelog | 8 +++-
>>  3 files changed, 13 insertions(+), 3 deletions(-)
>>
>> -- 
>> 2.37.3
>>
> 
> 
> To the set:
> Acked-by: Flavio Leitner 
> 
> Thanks Ilya,
> fbl


Thanks, Flavio!  I applied all sets to appropriate branches
and tagged new releases.  Will update the website and send
an announce soon.

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

[ovs-dev] [PATCH v4 ovn 6/7] Add connectivity test for 2 controllers on the same host

[Ihar Hrachyshka]

Signed-off-by: Ihar Hrachyshka 
---
 tests/ovn.at | 123 +++
 1 file changed, 123 insertions(+)

diff --git a/tests/ovn.at b/tests/ovn.at
index 3c76ba49c..1d69de96e 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -33170,3 +33170,126 @@ OVS_WAIT_UNTIL([ovs-vsctl --columns _uuid --bare find 
Port \
 OVN_CLEANUP([hv1])
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([multiple controllers on the same host can talk to each other])
+ovn_start
+net_add n1
+
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys-1
+ovs-vsctl add-br br-phys-2
+ovs-vsctl set open . external-ids:ovn-bridge-mappings-hv1=phys:br-phys-1
+ovs-vsctl set open . external-ids:ovn-bridge-mappings-hv2=phys:br-phys-2
+
+ovn_attach n1 br-phys-1 192.168.1.1 24
+
+# the file is read once at startup so it's safe to write it
+# here after the first ovn-controller has started
+echo hv2 > ${OVN_SYSCONFDIR}/system-id-override
+
+# This function is similar to ovn_attach but makes sure it doesn't
+# mess with another controller settings
+start_virtual_controller() {
+local net=$1 bridge=$2 int_bridge=$3 ip=$4 masklen=${5-24} 
encap=${6-geneve,vxlan} systemid=${7-$sandbox} cli_args=${@:8}
+net_attach $net $bridge || return 1
+
+mac=`ovs-vsctl get Interface $bridge mac_in_use | sed s/\"//g`
+arp_table="$arp_table $sandbox,$bridge,$ip,$mac"
+ovs-appctl netdev-dummy/ip4addr $bridge $ip/$masklen >/dev/null || return 1
+ovs-appctl ovs/route/add $ip/$masklen $bridge >/dev/null || return 1
+
+local ovn_remote
+if test X$HAVE_OPENSSL = Xyes; then
+ovn_remote=$SSL_OVN_SB_DB
+else
+ovn_remote=unix:$ovs_base/ovn-sb/ovn-sb.sock
+fi
+ovs-vsctl \
+-- set Open_vSwitch . external-ids:ovn-remote-$systemid=$ovn_remote \
+-- set Open_vSwitch . external-ids:ovn-encap-type-$systemid=$encap \
+-- set Open_vSwitch . external-ids:ovn-encap-ip-$systemid=$ip \
+-- set Open_vSwitch . external-ids:ovn-bridge-$systemid=$int_bridge \
+-- --may-exist add-br $int_bridge \
+-- set bridge $int_bridge fail-mode=secure 
other-config:disable-in-band=true \
+|| return 1
+
+ovn-controller --enable-dummy-vif-plug ${cli_args} -vconsole:off --detach 
--no-chdir
+}
+
+# for some reason SSL ovsdb configuration overrides CLI, so
+# delete ssl config from ovsdb to give CLI arguments priority
+ovs-vsctl del-ssl
+
+start_virtual_controller n1 br-phys-2 br-int-2 192.168.2.1 24 geneve,vxlan hv2 
\
+--pidfile=${OVS_RUNDIR}/ovn-controller-2.pid \
+--log-file=${OVS_RUNDIR}/ovn-controller-2.log \
+-p $PKIDIR/testpki-hv2-privkey.pem \
+-c $PKIDIR/testpki-hv2-cert.pem \
+-C $PKIDIR/testpki-cacert.pem
+pidfile="$OVS_RUNDIR"/ovn-controller-2.pid
+on_exit "test -e \"$pidfile\" && kill \`cat \"$pidfile\"\`"
+
+# Disable local ARP responder to pass ARP requests through tunnels
+check ovn-nbctl \
+ls-add ls \
+-- add Logical_Switch ls other_config vlan-passthru=true
+ovn-nbctl lsp-add ls lp1
+ovn-nbctl lsp-add ls lp2
+ovn-nbctl lsp-set-addresses lp1 "00:00:00:00:00:01 10.0.0.1"
+ovn-nbctl lsp-set-addresses lp2 "00:00:00:00:00:02 10.0.0.2"
+
+ovn-nbctl lsp-add ls ln_port
+ovn-nbctl lsp-set-addresses ln_port unknown
+ovn-nbctl lsp-set-type ln_port localnet
+ovn-nbctl lsp-set-options ln_port network_name=phys
+
+ovs-vsctl -- add-port br-int vif1 -- \
+set interface vif1 external-ids:iface-id=lp1 \
+options:tx_pcap=hv1/vif1-tx.pcap \
+options:rxq_pcap=hv1/vif1-rx.pcap
+ovs-vsctl -- add-port br-int-2 vif2 -- \
+set interface vif2 external-ids:iface-id=lp2 \
+options:tx_pcap=hv1/vif2-tx.pcap \
+options:rxq_pcap=hv1/vif2-rx.pcap
+
+reset_env() {
+as hv1 reset_pcap_file vif1 hv1/vif1
+as hv1 reset_pcap_file vif2 hv1/vif2
+for port in hv1/vif1 hv1/vif2; do
+: > $port.expected
+done
+}
+
+check_packets() {
+OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv1/vif1-tx.pcap], [hv1/vif1.expected])
+OVN_CHECK_PACKETS_REMOVE_BROADCAST([hv1/vif2-tx.pcap], [hv1/vif2.expected])
+}
+
+send_arp() {
+local hv=$1 inport=$2 eth_src=$3 eth_dst=$4 spa=$5 tpa=$6
+local 
request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa}
+as ${hv} ovs-appctl netdev-dummy/receive $inport $request
+echo "${request}"
+}
+
+wait_for_ports_up
+ovn-nbctl --wait=hv sync
+
+OVN_POPULATE_ARP
+
+reset_env
+
+lp1_spa=$(ip_to_hex 10 0 0 1)
+lp2_spa=$(ip_to_hex 10 0 0 2)
+request=$(send_arp hv1 vif1 0001 0002 $lp1_spa $lp2_spa)
+echo $request >> hv1/vif2.expected
+request=$(send_arp hv1 vif2 0002 0001 $lp2_spa $lp1_spa)
+echo $request >> hv1/vif1.expected
+
+check_packets
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
-- 
2.34.1

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

[ovs-dev] [PATCH v4 ovn 5/7] Don't touch tunnel ports from a different br-int

[Ihar Hrachyshka]

When multiple controllers are running using the same vswitchd,
controllers should delete only those tunnel ports that belong to the
integration bridge that is managed by the controller instance.

This makes sure multiple controllers don't step on each other when
running using the same vswitchd instance.

Signed-off-by: Ihar Hrachyshka 
---
 controller/encaps.c | 42 ++
 controller/encaps.h |  1 -
 controller/ovn-controller.c |  3 +-
 tests/ovn.at| 70 +
 4 files changed, 90 insertions(+), 26 deletions(-)

diff --git a/controller/encaps.c b/controller/encaps.c
index c9aaeb40c..4af25de3e 100644
--- a/controller/encaps.c
+++ b/controller/encaps.c
@@ -396,7 +396,6 @@ chassis_tzones_overlap(const struct sset *transport_zones,
 
 void
 encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
-   const struct ovsrec_bridge_table *bridge_table,
const struct ovsrec_bridge *br_int,
const struct sbrec_chassis_table *chassis_table,
const struct sbrec_chassis *this_chassis,
@@ -409,7 +408,6 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
 }
 
 const struct sbrec_chassis *chassis_rec;
-const struct ovsrec_bridge *br;
 
 struct tunnel_ctx tc = {
 .chassis = SHASH_INITIALIZER(),
@@ -427,27 +425,25 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
 /* Collect all port names into tc.port_names.
  *
  * Collect all the OVN-created tunnels into tc.tunnel_hmap. */
-OVSREC_BRIDGE_TABLE_FOR_EACH (br, bridge_table) {
-for (size_t i = 0; i < br->n_ports; i++) {
-const struct ovsrec_port *port = br->ports[i];
-sset_add(_names, port->name);
-
-/*
- * note that the id here is not just the chassis name, but the
- * combination of 
- */
-const char *id = smap_get(>external_ids, "ovn-chassis-id");
-if (id) {
-if (!shash_find(, id)) {
-struct chassis_node *chassis = xzalloc(sizeof *chassis);
-chassis->bridge = br;
-chassis->port = port;
-shash_add_assert(, id, chassis);
-} else {
-/* Duplicate port for ovn-chassis-id.  Arbitrarily choose
- * to delete this one. */
-ovsrec_bridge_update_ports_delvalue(br, port);
-}
+for (size_t i = 0; i < br_int->n_ports; i++) {
+const struct ovsrec_port *port = br_int->ports[i];
+sset_add(_names, port->name);
+
+/*
+ * note that the id here is not just the chassis name, but the
+ * combination of 
+ */
+const char *id = smap_get(>external_ids, "ovn-chassis-id");
+if (id) {
+if (!shash_find(, id)) {
+struct chassis_node *chassis = xzalloc(sizeof *chassis);
+chassis->bridge = br_int;
+chassis->port = port;
+shash_add_assert(, id, chassis);
+} else {
+/* Duplicate port for ovn-chassis-id.  Arbitrarily choose
+ * to delete this one. */
+ovsrec_bridge_update_ports_delvalue(br_int, port);
 }
 }
 }
diff --git a/controller/encaps.h b/controller/encaps.h
index 16032f15b..7197050b6 100644
--- a/controller/encaps.h
+++ b/controller/encaps.h
@@ -32,7 +32,6 @@ struct sset;
 
 void encaps_register_ovs_idl(struct ovsdb_idl *);
 void encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
-const struct ovsrec_bridge_table *,
 const struct ovsrec_bridge *br_int,
 const struct sbrec_chassis_table *,
 const struct sbrec_chassis *,
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index e26d46535..6a3aec317 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -4230,8 +4230,7 @@ main(int argc, char *argv[])
 }
 
 if (chassis) {
-encaps_run(ovs_idl_txn,
-   bridge_table, br_int,
+encaps_run(ovs_idl_txn, br_int,
sbrec_chassis_table_get(ovnsb_idl_loop.idl),
chassis,
sbrec_sb_global_first(ovnsb_idl_loop.idl),
diff --git a/tests/ovn.at b/tests/ovn.at
index 2508a224e..3c76ba49c 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -33100,3 +33100,73 @@ AT_CHECK(test x$encap_hv1_ip == x)
 OVN_CLEANUP([hv1],[hv2])
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([controllers don't touch tunnels that are not on br-int])
+ovn_start
+net_add n1
+
+sim_add hv1
+as hv1
+ovs-vsctl add-br br-phys1
+ovn_attach n1 br-phys1 192.168.0.1
+
+# the file is read once at startup so it's safe to write it
+# here after the first ovn-controller has started
+echo hv2 > 

[ovs-dev] [PATCH v4 ovn 2/7] Support ovn-...- specific global ovsdb options

[Ihar Hrachyshka]

Before the patch, all controller instances were reading configuration
from the same external-ids:ovn-* options. This patch adds support for
distinct config otions for different chassis names stored in the same
ovsdb global config object.

To configure an option for a distinct chassis name, an admin may add a
suffix with the desired chassis name to a config option. For example, if
the following is configured in ovsdb, only a controller with the
corresponding chassis name (either 'hv1' or 'hv2') would read just one
of the following options:

ovs-vsctl set open . external-ids:ovn-bridge-mappings-hv1=phys:br-phys-1
ovs-vsctl set open . external-ids:ovn-bridge-mappings-hv2=phys:br-phys-2

Chassis specific config options override any global settings, so for
example if the following configuration is used, then controller 'hv1'
will use the first setting but not the latter. Any other controllers
will use the global setting, which is the second setting..

ovs-vsctl set open . external-ids:ovn-bridge-mappings-hv1=phys:br-phys-1
ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys-2

This is supported for other options too.

This is in preparation to support running multiple controller instances
using the same vswitchd instance.

Signed-off-by: Ihar Hrachyshka 
---
 controller/chassis.c| 153 +---
 controller/chassis.h|   6 +-
 controller/encaps.c |  19 ++--
 controller/ovn-controller.8.xml |  12 +++
 controller/ovn-controller.c | 123 ++---
 controller/patch.c  |   6 +-
 controller/physical.c   |   2 +-
 lib/ovn-util.c  |  87 ++
 lib/ovn-util.h  |  26 ++
 tests/automake.mk   |   1 +
 tests/ovn-macros.at |   4 +-
 tests/ovn.at|  41 +
 12 files changed, 361 insertions(+), 119 deletions(-)

diff --git a/controller/chassis.c b/controller/chassis.c
index 685d9b2ae..c8a332786 100644
--- a/controller/chassis.c
+++ b/controller/chassis.c
@@ -93,9 +93,10 @@ chassis_register_ovs_idl(struct ovsdb_idl *ovs_idl)
 }
 
 static const char *
-get_hostname(const struct smap *ext_ids)
+get_hostname(const struct smap *ext_ids, const char *chassis_id)
 {
-const char *hostname = smap_get_def(ext_ids, "hostname", "");
+const char *hostname = get_chassis_external_id_value(ext_ids, chassis_id,
+ "hostname", "");
 
 if (strlen(hostname) == 0) {
 static char hostname_[HOST_NAME_MAX + 1];
@@ -111,69 +112,81 @@ get_hostname(const struct smap *ext_ids)
 }
 
 static const char *
-get_bridge_mappings(const struct smap *ext_ids)
+get_bridge_mappings(const struct smap *ext_ids, const char *chassis_id)
 {
-return smap_get_def(ext_ids, "ovn-bridge-mappings", "");
+return get_chassis_external_id_value(ext_ids, chassis_id,
+ "ovn-bridge-mappings", "");
 }
 
 const char *
-get_chassis_mac_mappings(const struct smap *ext_ids)
+get_chassis_mac_mappings(const struct smap *ext_ids, const char *chassis_id)
 {
-return smap_get_def(ext_ids, "ovn-chassis-mac-mappings", "");
+return get_chassis_external_id_value(ext_ids, chassis_id,
+ "ovn-chassis-mac-mappings", "");
 }
 
 static const char *
-get_cms_options(const struct smap *ext_ids)
+get_cms_options(const struct smap *ext_ids, const char *chassis_id)
 {
-return smap_get_def(ext_ids, "ovn-cms-options", "");
+return get_chassis_external_id_value(ext_ids, chassis_id,
+ "ovn-cms-options", "");
 }
 
 static const char *
-get_monitor_all(const struct smap *ext_ids)
+get_monitor_all(const struct smap *ext_ids, const char *chassis_id)
 {
-return smap_get_def(ext_ids, "ovn-monitor-all", "false");
+return get_chassis_external_id_value(ext_ids, chassis_id,
+ "ovn-monitor-all", "false");
 }
 
 static const char *
-get_enable_lflow_cache(const struct smap *ext_ids)
+get_enable_lflow_cache(const struct smap *ext_ids, const char *chassis_id)
 {
-return smap_get_def(ext_ids, "ovn-enable-lflow-cache", "true");
+return get_chassis_external_id_value(ext_ids, chassis_id,
+ "ovn-enable-lflow-cache", "true");
 }
 
 static const char *
-get_limit_lflow_cache(const struct smap *ext_ids)
+get_limit_lflow_cache(const struct smap *ext_ids, const char *chassis_id)
 {
-return smap_get_def(ext_ids, "ovn-limit-lflow-cache", "");
+return get_chassis_external_id_value(ext_ids, chassis_id,
+ "ovn-limit-lflow-cache", "");
 }
 
 static const char *
-get_memlimit_lflow_cache(const struct smap *ext_ids)
+get_memlimit_lflow_cache(const struct smap *ext_ids, const char *chassis_id)
 {
-return smap_get_def(ext_ids, "ovn-memlimit-lflow-cache-kb", "");
+return 

[ovs-dev] [PATCH v4 ovn 4/7] Support passing chassis name via CLI

[Ihar Hrachyshka]

This patch adds support for the desired system-id (chassis name) to be
passed via CLI:

$ ovn-controller -n 

If passed, CLI overrides any settings stored in ovsdb or in
system-id-override file.

This may be useful when running multiple controller instances using the
same vswitchd instance.

Signed-off-by: Ihar Hrachyshka 
---
 controller/chassis.c|  5 
 controller/chassis.h|  1 +
 controller/ovn-controller.8.xml |  8 ---
 controller/ovn-controller.c |  9 +++
 tests/ovn-macros.at |  4 ++--
 tests/ovn.at| 42 +
 6 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/controller/chassis.c b/controller/chassis.c
index f121af3c3..521bec2bd 100644
--- a/controller/chassis.c
+++ b/controller/chassis.c
@@ -37,6 +37,7 @@ VLOG_DEFINE_THIS_MODULE(chassis);
 #define HOST_NAME_MAX 255
 #endif /* HOST_NAME_MAX */
 
+char *cli_system_id = NULL;
 char *file_system_id = NULL;
 
 /*
@@ -279,6 +280,10 @@ chassis_parse_ovs_iface_types(char **iface_types, size_t 
n_iface_types,
 const char *
 get_ovs_chassis_id(const struct ovsrec_open_vswitch_table *ovs_table)
 {
+if (cli_system_id) {
+return cli_system_id;
+}
+
 if (file_system_id) {
 return file_system_id;
 }
diff --git a/controller/chassis.h b/controller/chassis.h
index baa327059..309ced28f 100644
--- a/controller/chassis.h
+++ b/controller/chassis.h
@@ -31,6 +31,7 @@ struct sset;
 struct eth_addr;
 struct smap;
 
+extern char *cli_system_id;
 extern char *file_system_id;
 
 void chassis_register_ovs_idl(struct ovsdb_idl *);
diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
index fef6309d5..2f852d39f 100644
--- a/controller/ovn-controller.8.xml
+++ b/controller/ovn-controller.8.xml
@@ -74,9 +74,11 @@
   gracefully stop ovn-controller or manually delete the
   stale Chassis and Chassis_Private records
   after changing the system-id. Note that the chassis name can
-  also be provided via the system-id-override file. The file
-  configuration overrides the one from the database, if both are
-  present.
+  also be provided via the system-id-override file or via the
+  -n command-line option. The following precedence is used:
+  first, the command-line option is read; if not present, the
+  system-id-override file is read; if not present, then the
+  name configured in the database is used.
 
   external_ids:hostname
   The hostname to use in the Chassis table.
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index af1332f12..e26d46535 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -4613,6 +4613,9 @@ loop_done:
 if (file_system_id) {
 free(file_system_id);
 }
+if (cli_system_id) {
+free(cli_system_id);
+}
 service_stop();
 
 exit(retval);
@@ -4638,6 +4641,7 @@ parse_options(int argc, char *argv[])
 STREAM_SSL_LONG_OPTIONS,
 {"peer-ca-cert", required_argument, NULL, OPT_PEER_CA_CERT},
 {"bootstrap-ca-cert", required_argument, NULL, OPT_BOOTSTRAP_CA_CERT},
+{"chassis", required_argument, NULL, 'n'},
 {"enable-dummy-vif-plug", no_argument, NULL,
  OPT_ENABLE_DUMMY_VIF_PLUG},
 {NULL, 0, NULL, 0}
@@ -4689,6 +4693,10 @@ parse_options(int argc, char *argv[])
 vif_plug_dummy_enable();
 break;
 
+case 'n':
+cli_system_id = xstrdup(optarg);
+break;
+
 case '?':
 exit(EXIT_FAILURE);
 
@@ -4724,6 +4732,7 @@ usage(void)
 daemon_usage();
 vlog_usage();
 printf("\nOther options:\n"
+   "  -n  custom chassis name\n"
"  -h, --help  display this help message\n"
"  -V, --version   display version information\n");
 exit(EXIT_SUCCESS);
diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
index 35b7b3872..b7f236fa8 100644
--- a/tests/ovn-macros.at
+++ b/tests/ovn-macros.at
@@ -290,7 +290,7 @@ net_attach () {
 
 # ovn_az_attach AZ NETWORK BRIDGE IP [MASKLEN]
 ovn_az_attach() {
-local az=$1 net=$2 bridge=$3 ip=$4 masklen=${5-24} encap=${6-geneve,vxlan} 
systemid=${7-$sandbox}
+local az=$1 net=$2 bridge=$3 ip=$4 masklen=${5-24} encap=${6-geneve,vxlan} 
systemid=${7-$sandbox} cli_args=${@:8}
 net_attach $net $bridge || return 1
 
 mac=`ovs-vsctl get Interface $bridge mac_in_use | sed s/\"//g`
@@ -326,7 +326,7 @@ ovn_az_attach() {
 ovs-vsctl set open . external_ids:ovn-monitor-all=true
 fi
 
-start_daemon ovn-controller --enable-dummy-vif-plug || return 1
+start_daemon ovn-controller --enable-dummy-vif-plug ${cli_args} || return 1
 }
 
 # ovn_attach NETWORK BRIDGE IP [MASKLEN]
diff --git a/tests/ovn.at b/tests/ovn.at
index 35c853618..2508a224e 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -33058,3 +33058,45 @@ AT_CHECK(test 

[ovs-dev] [PATCH v4 ovn 7/7] Document experimental support for co-hosted controllers

[Ihar Hrachyshka]

Signed-off-by: Ihar Hrachyshka 
---
 NEWS|  2 ++
 controller/ovn-controller.8.xml | 12 
 2 files changed, 14 insertions(+)

diff --git a/NEWS b/NEWS
index 224a7b83e..63afcea9d 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,7 @@
 Post v22.09.0
 -
+  - ovn-controller: Experimental support for co-hosting multiple controller
+instances on the same host.
 
 OVN v22.09.0 - 16 Sep 2022
 --
diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
index 2f852d39f..b258b27c1 100644
--- a/controller/ovn-controller.8.xml
+++ b/controller/ovn-controller.8.xml
@@ -371,6 +371,18 @@
 set in the database.
 
 
+
+Chassis-specific configuration options in the database plus the ability
+to configure the chassis name to use via the
+system-id-override file or command line allows to run
+multiple ovn-controller instances with unique chassis
+names on the same host using the same vswitchd instance.
+This may be useful when running a hybrid setup with more than one CMS
+managing ports on the host, or to use different datapath types on the
+same host. Note that this ability is highly experimental and has known
+limitations. Use at your own risk.
+
+
 
   ovn-controller reads the following values from the
   Open_vSwitch database of the local OVS instance:
-- 
2.34.1

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

[ovs-dev] [PATCH v4 ovn 1/7] Include "chassis index" into tunnel port name

[Ihar Hrachyshka]

This is in preparation to support multiple separate controller instances
with distinct chassis names operating on the same vswitchd instance.

To avoid conflicts, this patch introduces a unique "index" (from 0-9a-z
range) into the port name. Each chassis allocates a separate index for
itself on startup. The index is then stored in
Open_vSwitch:other_config:ovn-chassis-idx- key.

An alternative would be including source chassis name into the port
name, but the length is limited by IFNAMSIZ defined in kernel, which is
15.

Signed-off-by: Ihar Hrachyshka 
---
 controller/encaps.c | 23 +++
 controller/encaps.h |  2 ++
 controller/ovn-controller.c | 57 +
 3 files changed, 76 insertions(+), 6 deletions(-)

diff --git a/controller/encaps.c b/controller/encaps.c
index 9647ba507..4bf945af3 100644
--- a/controller/encaps.c
+++ b/controller/encaps.c
@@ -22,6 +22,7 @@
 #include "lib/vswitch-idl.h"
 #include "openvswitch/vlog.h"
 #include "lib/ovn-sb-idl.h"
+#include "lib/ovsdb-idl.h"
 #include "ovn-controller.h"
 #include "smap.h"
 
@@ -59,6 +60,7 @@ struct tunnel_ctx {
 struct sset port_names;
 
 struct ovsdb_idl_txn *ovs_txn;
+const struct ovsrec_open_vswitch_table *ovs_table;
 const struct ovsrec_bridge *br_int;
 const struct sbrec_chassis *this_chassis;
 };
@@ -68,14 +70,22 @@ struct chassis_node {
 const struct ovsrec_bridge *bridge;
 };
 
+static const char *get_chassis_idx(struct tunnel_ctx *tc)
+{
+const struct ovsrec_open_vswitch *cfg =
+ovsrec_open_vswitch_table_first(tc->ovs_table);
+char *idx_key = xasprintf(CHASSIS_IDX_PREFIX "%s", tc->this_chassis->name);
+return smap_get_def(>other_config, idx_key, "");
+free(idx_key);
+}
+
 static char *
 tunnel_create_name(struct tunnel_ctx *tc, const char *chassis_id)
 {
-int i;
-
-for (i = 0; i < UINT16_MAX; i++) {
-char *port_name;
-port_name = xasprintf("ovn-%.6s-%x", chassis_id, i);
+for (int i = 0; i < UINT16_MAX; i++) {
+const char *idx = get_chassis_idx(tc);
+char *port_name = xasprintf(
+"ovn%s-%.*s-%x", idx, strlen(idx) ? 5 : 6, chassis_id, i);
 
 if (!sset_contains(>port_names, port_name)) {
 return port_name;
@@ -400,7 +410,8 @@ encaps_run(struct ovsdb_idl_txn *ovs_idl_txn,
 .chassis = SHASH_INITIALIZER(),
 .port_names = SSET_INITIALIZER(_names),
 .br_int = br_int,
-.this_chassis = this_chassis
+.this_chassis = this_chassis,
+.ovs_table = ovs_table,
 };
 
 tc.ovs_txn = ovs_idl_txn;
diff --git a/controller/encaps.h b/controller/encaps.h
index 25d44b034..16032f15b 100644
--- a/controller/encaps.h
+++ b/controller/encaps.h
@@ -18,6 +18,8 @@
 
 #include 
 
+#define CHASSIS_IDX_PREFIX "ovn-chassis-idx-"
+
 struct ovsdb_idl;
 struct ovsdb_idl_txn;
 struct ovsrec_bridge;
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index 9969d317f..5c2a4b6ef 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -3527,6 +3527,57 @@ check_northd_version(struct ovsdb_idl *ovs_idl, struct 
ovsdb_idl *ovnsb_idl,
 return true;
 }
 
+static void
+store_chassis_index_if_needed(
+const struct ovsrec_open_vswitch_table *ovs_table)
+{
+const struct ovsrec_open_vswitch *cfg =
+ovsrec_open_vswitch_table_first(ovs_table);
+const char *chassis_id = get_ovs_chassis_id(ovs_table);
+
+char *idx_key = xasprintf(CHASSIS_IDX_PREFIX "%s", chassis_id);
+const char *chassis_idx = smap_get(>other_config, idx_key);
+if (!chassis_idx) {
+/* collect all indices so far consumed by other chassis */
+struct sset used_indices = SSET_INITIALIZER(_indices);
+struct smap_node *node;
+SMAP_FOR_EACH (node, >other_config) {
+if (!strncmp(node->key, CHASSIS_IDX_PREFIX, 16)) {
+sset_add(_indices, node->value);
+}
+}
+/* first chassis on the host */
+if (!sset_contains(_indices, "")) {
+ovsrec_open_vswitch_update_other_config_setkey(
+cfg, idx_key, "");
+goto out;
+}
+/* next chassis get an alphanum index allocated */
+char idx[] = "0";
+for (char i = '0'; i <= '9'; i++) {
+idx[0] = i;
+if (!sset_contains(_indices, idx)) {
+ovsrec_open_vswitch_update_other_config_setkey(
+cfg, idx_key, idx);
+goto out;
+}
+}
+for (char i = 'a'; i <= 'z'; i++) {
+idx[0] = i;
+if (!sset_contains(_indices, idx)) {
+ovsrec_open_vswitch_update_other_config_setkey(
+cfg, idx_key, idx);
+goto out;
+}
+}
+/* all indices consumed; it's safer to abort */
+VLOG_ERR("All unique controller indices consumed. Exiting.");
+

[ovs-dev] [PATCH v4 ovn 3/7] Allow to override system-id via file

[Ihar Hrachyshka]

Before the patch, system-id could be configured via a global config
option in ovsdb. This patch adds another option - configure system-id
via a file. This is achieved by writing the desired system-id into the
following file location: ${OVN_SYSCONFDIR}/system-id-override.

The file is read on controller startup. The file setting overrides
configuration stored in ovsdb, if any.

This may be useful when running multiple containerized controller
instances using the same vswitchd.

Signed-off-by: Ihar Hrachyshka 
---
 controller/chassis.c|  6 +
 controller/chassis.h|  2 ++
 controller/ovn-controller.8.xml |  5 +++-
 controller/ovn-controller.c | 34 ++
 tests/ovn.at| 42 +
 tests/ovs-macros.at |  2 ++
 6 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/controller/chassis.c b/controller/chassis.c
index c8a332786..f121af3c3 100644
--- a/controller/chassis.c
+++ b/controller/chassis.c
@@ -37,6 +37,8 @@ VLOG_DEFINE_THIS_MODULE(chassis);
 #define HOST_NAME_MAX 255
 #endif /* HOST_NAME_MAX */
 
+char *file_system_id = NULL;
+
 /*
  * Structure for storing the chassis config parsed from the ovs table.
  */
@@ -277,6 +279,10 @@ chassis_parse_ovs_iface_types(char **iface_types, size_t 
n_iface_types,
 const char *
 get_ovs_chassis_id(const struct ovsrec_open_vswitch_table *ovs_table)
 {
+if (file_system_id) {
+return file_system_id;
+}
+
 const struct ovsrec_open_vswitch *cfg =
 ovsrec_open_vswitch_table_first(ovs_table);
 const char *chassis_id = cfg
diff --git a/controller/chassis.h b/controller/chassis.h
index 05a96bb0c..baa327059 100644
--- a/controller/chassis.h
+++ b/controller/chassis.h
@@ -31,6 +31,8 @@ struct sset;
 struct eth_addr;
 struct smap;
 
+extern char *file_system_id;
+
 void chassis_register_ovs_idl(struct ovsdb_idl *);
 const struct sbrec_chassis *chassis_run(
 struct ovsdb_idl_txn *ovnsb_idl_txn,
diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml
index 19dfa0ff5..fef6309d5 100644
--- a/controller/ovn-controller.8.xml
+++ b/controller/ovn-controller.8.xml
@@ -73,7 +73,10 @@
   not directly supported.  Users have two options: either first
   gracefully stop ovn-controller or manually delete the
   stale Chassis and Chassis_Private records
-  after changing the system-id.
+  after changing the system-id. Note that the chassis name can
+  also be provided via the system-id-override file. The file
+  configuration overrides the one from the database, if both are
+  present.
 
   external_ids:hostname
   The hostname to use in the Chassis table.
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index f40633517..af1332f12 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -18,10 +18,14 @@
 #include "ovn-controller.h"
 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
 
 #include "bfd.h"
 #include "binding.h"
@@ -55,6 +59,7 @@
 #include "lib/ip-mcast-index.h"
 #include "lib/mac-binding-index.h"
 #include "lib/mcast-group-index.h"
+#include "lib/ovn-dirs.h"
 #include "lib/ovn-sb-idl.h"
 #include "lib/ovn-util.h"
 #include "patch.h"
@@ -152,6 +157,29 @@ struct pending_pkt {
 /* Registered ofctrl seqno type for nb_cfg propagation. */
 static size_t ofctrl_seq_type_nb_cfg;
 
+static char *get_file_system_id(void)
+{
+char *ret = NULL;
+char *filename = xasprintf("%s/system-id-override", ovn_sysconfdir());
+errno = 0;
+int fd = open(filename, O_RDONLY);
+if (fd != -1) {
+char system_id[64];
+int nread = read(fd, system_id, sizeof system_id);
+if (nread) {
+system_id[nread] = '\0';
+if (system_id[nread - 1] == '\n') {
+system_id[nread - 1] = '\0';
+}
+ret = xstrdup(system_id);
+}
+close(fd);
+}
+
+free(filename);
+return ret;
+}
+
 static unsigned int
 update_sb_monitors(struct ovsdb_idl *ovnsb_idl,
const struct sbrec_chassis *chassis,
@@ -3604,6 +3632,9 @@ main(int argc, char *argv[])
 struct ovn_controller_exit_args exit_args = {, };
 int retval;
 
+/* Read from system-id-override file once on startup. */
+file_system_id = get_file_system_id();
+
 ovs_cmdl_proctitle_init(argc, argv);
 ovn_set_program_name(argv[0]);
 service_start(, );
@@ -4579,6 +4610,9 @@ loop_done:
 
 ovs_feature_support_destroy();
 free(ovs_remote);
+if (file_system_id) {
+free(file_system_id);
+}
 service_stop();
 
 exit(retval);
diff --git a/tests/ovn.at b/tests/ovn.at
index 9aac857f4..35c853618 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -33016,3 +33016,45 @@ AT_CHECK(test x$encap_hv1_ip == x)
 OVN_CLEANUP([hv1],[hv2])
 AT_CLEANUP
 ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([chassis name override via 

[ovs-dev] [PATCH v4 ovn 0/7] Support 2+ controllers on the same vswitchd

[Ihar Hrachyshka]

This series adds support to run multiple ovn-controller instances using
the same vswitchd instance. This may be used to reuse a single host
level vswitchd installation to run multiple CMS (e.g. k8s and
openstack), each having its own OVN stack running on a separate
integration bridge.

This setup may, in some instances, simplify administration of the
system, since the admin no longer needs to maintain separate vswitchd
installations (e.g. in separate containers). This is also helpful when
running different datapath types for the mixed setup.

v1: initial series
v2: change tunnel port naming scheme: include "chassis index" instead of
its name for source chassis.
v2: formatting adjustments.
v3: fixed build due to ovs_abort missing arguments.
v3: added documentation to CLI and system-id-override file.
v3: added documentation for chassis specific db config options.
v3: documented the ability to run multiple controllers on the same host,
while mentioning that this support is highly experimental.
v3: updated NEWS file to include the note about the new experimental
issue.
v3: rebased.
v4: fixed a memory leak in get_chassis_idx.

Ihar Hrachyshka (7):
  Include "chassis index" into tunnel port name
  Support ovn-...- specific global ovsdb options
  Allow to override system-id via file
  Support passing chassis name via CLI
  Don't touch tunnel ports from a different br-int
  Add connectivity test for 2 controllers on the same host
  Document experimental support for co-hosted controllers

 NEWS|   2 +
 controller/chassis.c| 164 ++--
 controller/chassis.h|   9 +-
 controller/encaps.c |  84 +
 controller/encaps.h |   3 +-
 controller/ovn-controller.8.xml |  31 +++-
 controller/ovn-controller.c | 226 +--
 controller/patch.c  |   6 +-
 controller/physical.c   |   2 +-
 lib/ovn-util.c  |  87 +
 lib/ovn-util.h  |  26 +++
 tests/automake.mk   |   1 +
 tests/ovn-macros.at |   6 +-
 tests/ovn.at| 318 
 tests/ovs-macros.at |   2 +
 15 files changed, 814 insertions(+), 153 deletions(-)

-- 
2.34.1

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

Re: [ovs-dev] [PATCH v6 9/9] mfex-avx512: Add support for tunnel packets in avx512 MFEX.

[Pai G, Sunil]

> -Original Message-
> From: dev  On Behalf Of Kumar Amber
> Sent: Thursday, October 6, 2022 3:54 PM
> To: ovs-dev@openvswitch.org
> Cc: i.maxim...@ovn.org; f...@sysclose.org; Amber, Kumar
> 
> Subject: [ovs-dev] [PATCH v6 9/9] mfex-avx512: Add support for tunnel
> packets in avx512 MFEX.
> 
> This patch adds the necessary support to avx512 mfex to support handling
> of tunnel packet type.
> 
> Signed-off-by: Kumar Amber 
> 
> ---
> v6:
> - Fix minor comments from Cian.
> - Deduce magic bits through protocol sizes.
> v5:
> - check metadata IP address to find tunneling is valid or not.
>   As dummy-pmd often passes garbage data to dpif.
> ---
> ---
>  lib/dpif-netdev-avx512.c  |  32 ++---
>  lib/dpif-netdev-extract-avx512.c  | 206 --
>  lib/dpif-netdev-private-extract.c |   4 +-
>  3 files changed, 187 insertions(+), 55 deletions(-)
> 

Thanks for working on this patch as well as making the magic numbers more 
meaningful.

Acked-by: Sunil Pai G 

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

Re: [ovs-dev] [PATCH v6 8/9] mfex-study: Modify study func to select outer and inner MFEX funcs.

[Pai G, Sunil]

> -Original Message-
> From: dev  On Behalf Of Kumar Amber
> Sent: Thursday, October 6, 2022 3:54 PM
> To: ovs-dev@openvswitch.org
> Cc: i.maxim...@ovn.org; f...@sysclose.org; Amber, Kumar
> 
> Subject: [ovs-dev] [PATCH v6 8/9] mfex-study: Modify study func to select
> outer and inner MFEX funcs.
> 
> The MFEX study function is split into outer and inner to allow for
> independent selection and studying of packets in outer and inner flows to
> different ISA optimized miniflow extract implementations.
> 
> Signed-off-by: Kumar Amber 
> 
> ---
> v6:
> - Fix minor comments from Cian.
> ---
> ---
>  lib/dpif-netdev-extract-study.c | 127 +---
>  1 file changed, 82 insertions(+), 45 deletions(-)
> 

LGTM, 
Acked-by: Sunil Pai G 

Thanks and regards
Sunil

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

Re: [ovs-dev] [PATCH v6 7/9] dpif-mfex: Change MFEX fn pointer prototype to include md_is_valid.

[Pai G, Sunil]

> -Original Message-
> From: dev  On Behalf Of Kumar Amber
> Sent: Thursday, October 6, 2022 3:54 PM
> To: ovs-dev@openvswitch.org
> Cc: i.maxim...@ovn.org; f...@sysclose.org; Amber, Kumar
> 
> Subject: [ovs-dev] [PATCH v6 7/9] dpif-mfex: Change MFEX fn pointer
> prototype to include md_is_valid.
> 
> The md_is_valid parameter is passed from DPIF to MFEX to allow MFEX
> functions to detect the tunneling and decide the processing of Inner
> packets in static predictable branches.
> 
> Signed-off-by: Kumar Amber 
> ---
>  lib/dpif-netdev-avx512.c  |  3 ++-
>  lib/dpif-netdev-extract-avx512.c  |  9 +
>  lib/dpif-netdev-extract-study.c   |  6 --
>  lib/dpif-netdev-private-extract.c |  6 --  lib/dpif-netdev-private-
> extract.h | 13 -
>  5 files changed, 23 insertions(+), 14 deletions(-)
> 

Change looks OK to me, 

Acked-by: Sunil Pai G 

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

Re: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats

[Jamal Hadi Salim]

On Fri, Oct 7, 2022 at 11:01 AM Marcelo Leitner  wrote:
>
> On Fri, Oct 07, 2022 at 04:39:25PM +0200, Davide Caratti wrote:
> > On Fri, Oct 7, 2022 at 3:21 PM Marcelo Leitner  wrote:
> > >
> > > (+TC folks and netdev@)
> > >
> > > On Fri, Oct 07, 2022 at 02:42:56PM +0200, Ilya Maximets wrote:
> > > > On 10/7/22 13:37, Eelco Chaudron wrote:
> >
> > [...]
> >
> > > I don't see how we could achieve this without breaking much of the
> > > user experience.
> > >
> > > >
> > > > - or create something like act_count - a dummy action that only
> > > >   counts packets, and put it in every datapath action from OVS.
> > >
> > > This seems the easiest and best way forward IMHO. It's actually the
> > > 3rd option below but "on demand", considering that tc will already use
> > > the stats of the first action as the flow stats (in
> > > tcf_exts_dump_stats()), then we can patch ovs to add such action if a
> > > meter is also being used (or perhaps even always, because other
> > > actions may also drop packets, and for OVS we would really be at the
> > > 3rd option below).
> >
> > Correct me if I'm wrong, but actually act_gact action with "pipe"
> > control action should already do this counting job.
>
> act_gact is so transparent that I never see it/remembers about it :)
> Yup, although it's not offloadabe with pipe control actio AFAICT.
>

It's mostly how people who offload (not sure about OVS) do it;
example some of the switches out there.
The action index, whatever that action is, could be easily mapped
to a stats index in hardware. If you are sharing action instances
(eg policer index 32) across multiple flows then of course in hw
you are using only that one instance of the meter. If you want
to have extra stats that differentiate between the flows then
add a gact (PIPE as Davide mentioned) and the only thing it
will do is count and nothing else.

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

Re: [ovs-dev] [RFC dpdk-latest v2 3/3] netdev-dpdk: Add per virtqueue statistics.

[Maxime Coquelin]

Hi David,

On 10/7/22 13:16, David Marchand wrote:

Request per virtqueue statistics from the vhost library and expose them
as per port OVS custom stats.


Thanks for the patch!


Note:
- the vhost stats API is experimental at the moment, this patch is
   sent as a demonstrator,


I'm going to send a patch to mark the API stable, since it has been used
in Vhost PMD for a while, and the API seems good for OVS


- we may drop maintaining rx stats in OVS itself and instead aggregate
   the per vq stats, this is something to investigate,
- a unit test is missing,

Signed-off-by: David Marchand 
---
  lib/netdev-dpdk.c | 203 --
  1 file changed, 194 insertions(+), 9 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 132ebb2843..3db5944977 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -532,11 +532,20 @@ struct netdev_dpdk {
  );
  
  PADDED_MEMBERS(CACHE_LINE_SIZE,

-/* Names of all XSTATS counters */
-struct rte_eth_xstat_name *rte_xstats_names;
-int rte_xstats_names_size;
-int rte_xstats_ids_size;
-uint64_t *rte_xstats_ids;
+union {
+struct {
+/* Names of all XSTATS counters */
+struct rte_eth_xstat_name *rte_xstats_names;
+int rte_xstats_names_size;
+int rte_xstats_ids_size;
+uint64_t *rte_xstats_ids;
+};
+struct {
+/* Names of all vhost stats */
+struct rte_vhost_stat_name *vhost_stat_names;
+int vhost_stat_size;
+};
+};
  );
  };
  
@@ -552,6 +561,7 @@ static int netdev_dpdk_get_sw_custom_stats(const struct netdev *,

 struct netdev_custom_stats *);
  static void netdev_dpdk_configure_xstats(struct netdev_dpdk *dev);
  static void netdev_dpdk_clear_xstats(struct netdev_dpdk *dev);
+static void netdev_dpdk_vhost_clear_stats(struct netdev_dpdk *dev);
  
  int netdev_dpdk_get_vid(const struct netdev_dpdk *dev);
  
@@ -1586,6 +1596,7 @@ netdev_dpdk_vhost_destruct(struct netdev *netdev)

  dev->vhost_id = NULL;
  rte_free(dev->vhost_rxq_enabled);
  
+netdev_dpdk_vhost_clear_stats(dev);

  common_destruct(dev);
  
  ovs_mutex_unlock(_mutex);

@@ -3039,6 +3050,80 @@ netdev_dpdk_vhost_get_stats(const struct netdev *netdev,
  return 0;
  }
  
+static int

+netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev,
+   struct netdev_custom_stats *custom_stats)
+{
+netdev_dpdk_get_sw_custom_stats(netdev, custom_stats);
+
+#ifdef ALLOW_EXPERIMENTAL_API
+struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+
+ovs_mutex_lock(>mutex);
+
+int vid = netdev_dpdk_get_vid(dev);
+
+if (vid >= 0 && dev->vhost_stat_size > 0) {
+struct rte_vhost_stat *vhost_stats;
+int stat_offset;
+int sw_stats_size;
+
+vhost_stats = xcalloc(dev->vhost_stat_size, sizeof *vhost_stats);
+
+stat_offset = 0;
+
+for (int q = 0; q < dev->up.n_rxq; q++) {
+int qid = q * VIRTIO_QNUM + VIRTIO_TXQ;
+int err;
+
+err = rte_vhost_vring_stats_get(vid, qid,
+_stats[stat_offset],
+dev->vhost_stat_size
+- stat_offset);
+if (err < 0 || stat_offset + err > dev->vhost_stat_size) {


I don't know if it would have a noticeable impact, but maybe you could
use OVS_UNLIKELY?


+goto fail;
+}
+stat_offset += err;
+}
+
+for (int q = 0; q < dev->up.n_txq; q++) {
+int qid = q * VIRTIO_QNUM;
+int err;
+
+err = rte_vhost_vring_stats_get(vid, qid,
+_stats[stat_offset],
+dev->vhost_stat_size
+- stat_offset);
+if (err < 0 || stat_offset + err > dev->vhost_stat_size) {
+goto fail;
+}
+stat_offset += err;
+}
+
+sw_stats_size = custom_stats->size;
+custom_stats->size += dev->vhost_stat_size;
+custom_stats->counters = xrealloc(custom_stats->counters,
+  custom_stats->size *
+  sizeof *custom_stats->counters);
+
+for (int i = 0; i < stat_offset; i++) {
+ovs_strlcpy(custom_stats->counters[sw_stats_size + i].name,
+dev->vhost_stat_names[i].name,
+NETDEV_CUSTOM_STATS_NAME_SIZE);
+custom_stats->counters[sw_stats_size + i].value =
+vhost_stats[i].value;
+}
+
+fail:
+free(vhost_stats);
+}
+
+ ovs_mutex_unlock(>mutex);
+#endif
+
+   

Re: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats

[Ilya Maximets]

On 10/7/22 16:49, Eelco Chaudron wrote:
> 
> 
> On 7 Oct 2022, at 14:42, Ilya Maximets wrote:
> 
>> On 10/7/22 13:37, Eelco Chaudron wrote:
>>>
>>>
>>> On 15 Sep 2022, at 14:03, Ilya Maximets wrote:
>>>
 On 9/15/22 13:56, Ilya Maximets wrote:
> On 9/15/22 13:38, Tianyu Yuan wrote:
>>
>> On 9/15/22 19:28,  Ilya Maximets wrote:
>>> On 9/14/22 16:19, Simon Horman wrote:
 From: Tianyu Yuan 

 Since dd9881ed55e6 ("tc: Fix stats dump when using same meter table")
 rule stats update will ignore meter police. Correspondingly, the
 reference stats of the test should also be modified to ensure the test
 could pass correctly.

 Fixes: dd9881ed55e6 ("tc: Fix stats dump when using same meter table")
 Signed-off-by: Tianyu Yuan 
 Signed-off-by: Simon Horman 
 ---
  tests/system-offloads-traffic.at | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/tests/system-offloads-traffic.at
 b/tests/system-offloads-traffic.at
 index d9b815a5ddf4..24e49d42f589 100644
 --- a/tests/system-offloads-traffic.at
 +++ b/tests/system-offloads-traffic.at
 @@ -264,7 +264,7 @@ NS_CHECK_EXEC([at_ns0], [echo "mark" | nc
 $NC_EOF_OPT -u 10.1.1.2 5678 -p 6789])  done

  AT_CHECK([ovs-appctl dpctl/dump-flows | grep "meter" |
 DUMP_CLEAN_SORTED], [0], [dnl
 -in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no),
 packets:10, bytes:330, used:0.001s, actions:meter(0),3
 +in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no),
 +packets:1, bytes:33, used:0.001s, actions:meter(0),3
>>>
>>> This looks very strange to me.  The test does send 10 packets.
>>> Why the flow should report only one?
>>>
>> Thanks for your review Ilya.
>> The test does send 10 packets but 9 of them are dropped by
>> meter action. As we descript in commit (dd9881ed55e6), the
>> flow stats should not report the police stats.

 "In previous patch, after parsing police action, the flower stats will
  be updated by dumped meter table stats"

 I suppose, that is the root cause.  We should not mix these two
 completely different types of statistics.  I didn't look at the
 code though to say why this was implemented in a way it is or
 how to fix that.
>>>
>>> Tianyu I might have missed this, but is there a follow on fix for this? 
>>> Asking as the test in the current master is broken.
>>>
>>> Ilya/Simon should we undo “dd9881ed5 tc: Fix stats dump when using same 
>>> meter table” and get a proper fix, fixing these counter issues?
>>
>> As far as I understand, kernel act_police is just not suitable
>> to represent an OpenFlow meter.  The main problem is that with
>> OVS we need two separate entities - actual meter and the meter
>> action that is using some actual meter.  And these two entities
>> should have separate sets of statistics.  Actions should count
>> how many packets went through these actions (ideally, TC chain
>> as a whole should count the number of successful matches, but that
>> is anther inconsistency between TC and OVS) and the actual meter
>> should count how many packets went through/dropped/etc.
> 
> Isn’t that the case? I think the basic statistics return the total number of 
> packets hitting the rule, and the additional TCA_STATS_QUEUE stats give you 
> the dropped packets.

AFAICT, we use the same instance of the act_police in multiple
datapath flows, so the statistics is aggregated across all of
them, hence cannot be used as flow stats.

> 
> I’ll need to go over this, and the code again, as this is all from memory.
> 
>> Multiple meter actions may reference the same actual meter.
>> Each of these actions should have statistics separate from the
>> statistics of the actual meter.  Stats of actions should be
>> reported as flow stats, stats of the meter should be reported
>> as meter stats.
> 
> This might be wrong, but I thought the stats are not per linked action, but 
> from the rule that has the linked action.

In normal OVS yes.  But I was thinking in TC abstractions here.
In TC stats are on each action.

> 
>> But with the act_police the actual meter and the action are the
>> same entity, so the only statistics we have is the statistics
>> of the actual meter.  So, there is no way to get the accurate
>> flow statistics if the meter is the first action.  This is just
>> a broken API by design.
>>
>> Saying that, commit dd9881ed5 seems to be correct, because we
>> should not use statistics of the actual meter as flow stats.
>> At the same time we have no way to get flow stats.  They are
>> broken either way but differently.
> 
> If my story above not correct and we share the TCA_STATS_BASIC* with all 
> instance where this meter is used we are out of luck.
> 
>> For the time being we may "fix" the test by adding some 

Re: [ovs-dev] [PATCH v6 5/9] dpif-netdev: Add function pointer for dpif re-circulate.

[Pai G, Sunil]

> -Original Message-
> From: dev  On Behalf Of Kumar Amber
> Sent: Thursday, October 6, 2022 3:54 PM
> To: ovs-dev@openvswitch.org
> Cc: i.maxim...@ovn.org; f...@sysclose.org; Amber, Kumar
> 
> Subject: [ovs-dev] [PATCH v6 5/9] dpif-netdev: Add function pointer for
> dpif re-circulate.
> 
> This patch adds support for selecting the recirculation implementation
> based on the DPIF implementation.
> 
> Signed-off-by: Kumar Amber 
> Signed-off-by: Cian Ferriter 
> Co-authored-by: Cian Ferriter 
> 
> ---
> v6:
> - Refactor common function from default function.
> v3:
> - Add description  to the dpif recirc function.
> - Fix use of return value to fall back to scalar dpif.
> ---
> ---
>  lib/dpif-netdev-private-dpif.c   | 73 +++-
>  lib/dpif-netdev-private-dpif.h   | 18 
>  lib/dpif-netdev-private-thread.h |  3 ++
>  lib/dpif-netdev.c| 14 +-
>  4 files changed, 88 insertions(+), 20 deletions(-)
> 

The changes look OK to me, 

Acked-by: Sunil Pai G 

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

Re: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats

[Eelco Chaudron]

On 7 Oct 2022, at 16:39, Davide Caratti wrote:

> On Fri, Oct 7, 2022 at 3:21 PM Marcelo Leitner  wrote:
>>
>> (+TC folks and netdev@)
>>
>> On Fri, Oct 07, 2022 at 02:42:56PM +0200, Ilya Maximets wrote:
>>> On 10/7/22 13:37, Eelco Chaudron wrote:
>
> [...]
>
>> I don't see how we could achieve this without breaking much of the
>> user experience.
>>
>>>
>>> - or create something like act_count - a dummy action that only
>>>   counts packets, and put it in every datapath action from OVS.
>>
>> This seems the easiest and best way forward IMHO. It's actually the
>> 3rd option below but "on demand", considering that tc will already use
>> the stats of the first action as the flow stats (in
>> tcf_exts_dump_stats()), then we can patch ovs to add such action if a
>> meter is also being used (or perhaps even always, because other
>> actions may also drop packets, and for OVS we would really be at the
>> 3rd option below).

Guess we have to add this extra action to each datapath flow offloaded due to 
the way flows back and forth translations are handled. Maybe we can do it 
selectively, but the code might become messier than it already is :(

> Correct me if I'm wrong, but actually act_gact action with "pipe"
> control action should already do this counting job.

I think we could use that, as we only use TC_ACT_GOTO_CHAIN and TC_ACT_SHOT. 
And it looks like TC_ACT_SHOT is not decoded correctly :(

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

Re: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats

[Marcelo Leitner]

On Fri, Oct 07, 2022 at 04:39:25PM +0200, Davide Caratti wrote:
> On Fri, Oct 7, 2022 at 3:21 PM Marcelo Leitner  wrote:
> >
> > (+TC folks and netdev@)
> >
> > On Fri, Oct 07, 2022 at 02:42:56PM +0200, Ilya Maximets wrote:
> > > On 10/7/22 13:37, Eelco Chaudron wrote:
>
> [...]
>
> > I don't see how we could achieve this without breaking much of the
> > user experience.
> >
> > >
> > > - or create something like act_count - a dummy action that only
> > >   counts packets, and put it in every datapath action from OVS.
> >
> > This seems the easiest and best way forward IMHO. It's actually the
> > 3rd option below but "on demand", considering that tc will already use
> > the stats of the first action as the flow stats (in
> > tcf_exts_dump_stats()), then we can patch ovs to add such action if a
> > meter is also being used (or perhaps even always, because other
> > actions may also drop packets, and for OVS we would really be at the
> > 3rd option below).
>
> Correct me if I'm wrong, but actually act_gact action with "pipe"
> control action should already do this counting job.

act_gact is so transparent that I never see it/remembers about it :)
Yup, although it's not offloadabe with pipe control actio AFAICT.

  Marcelo

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

Re: [ovs-dev] [PATCH branch-3.0 0/2] Release patches for v3.0.1.

[Flavio Leitner]

On Fri, Oct 07, 2022 at 01:17:33PM +0200, Ilya Maximets wrote:
> 
> Ilya Maximets (2):
>   Set release date for 3.0.1.
>   Prepare for 3.0.2.
> 
>  NEWS | 6 +-
>  configure.ac | 2 +-
>  debian/changelog | 8 +++-
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> -- 
> 2.37.3
> 


To the set:
Acked-by: Flavio Leitner 

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

Re: [ovs-dev] [PATCH branch-2.17 0/2] Release patches for v2.17.3.

[Flavio Leitner]

On Fri, Oct 07, 2022 at 01:17:21PM +0200, Ilya Maximets wrote:
> 
> Ilya Maximets (2):
>   Set release date for 2.17.3.
>   Prepare for 2.17.4.
> 
>  NEWS | 6 +-
>  configure.ac | 2 +-
>  debian/changelog | 8 +++-
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> -- 
> 2.37.3
> 

To the set:
Acked-by: Flavio Leitner 

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

Re: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats

[Eelco Chaudron]

On 7 Oct 2022, at 14:42, Ilya Maximets wrote:

> On 10/7/22 13:37, Eelco Chaudron wrote:
>>
>>
>> On 15 Sep 2022, at 14:03, Ilya Maximets wrote:
>>
>>> On 9/15/22 13:56, Ilya Maximets wrote:
 On 9/15/22 13:38, Tianyu Yuan wrote:
>
> On 9/15/22 19:28,  Ilya Maximets wrote:
>> On 9/14/22 16:19, Simon Horman wrote:
>>> From: Tianyu Yuan 
>>>
>>> Since dd9881ed55e6 ("tc: Fix stats dump when using same meter table")
>>> rule stats update will ignore meter police. Correspondingly, the
>>> reference stats of the test should also be modified to ensure the test
>>> could pass correctly.
>>>
>>> Fixes: dd9881ed55e6 ("tc: Fix stats dump when using same meter table")
>>> Signed-off-by: Tianyu Yuan 
>>> Signed-off-by: Simon Horman 
>>> ---
>>>  tests/system-offloads-traffic.at | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/system-offloads-traffic.at
>>> b/tests/system-offloads-traffic.at
>>> index d9b815a5ddf4..24e49d42f589 100644
>>> --- a/tests/system-offloads-traffic.at
>>> +++ b/tests/system-offloads-traffic.at
>>> @@ -264,7 +264,7 @@ NS_CHECK_EXEC([at_ns0], [echo "mark" | nc
>>> $NC_EOF_OPT -u 10.1.1.2 5678 -p 6789])  done
>>>
>>>  AT_CHECK([ovs-appctl dpctl/dump-flows | grep "meter" |
>>> DUMP_CLEAN_SORTED], [0], [dnl
>>> -in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no),
>>> packets:10, bytes:330, used:0.001s, actions:meter(0),3
>>> +in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no),
>>> +packets:1, bytes:33, used:0.001s, actions:meter(0),3
>>
>> This looks very strange to me.  The test does send 10 packets.
>> Why the flow should report only one?
>>
> Thanks for your review Ilya.
> The test does send 10 packets but 9 of them are dropped by
> meter action. As we descript in commit (dd9881ed55e6), the
> flow stats should not report the police stats.
>>>
>>> "In previous patch, after parsing police action, the flower stats will
>>>  be updated by dumped meter table stats"
>>>
>>> I suppose, that is the root cause.  We should not mix these two
>>> completely different types of statistics.  I didn't look at the
>>> code though to say why this was implemented in a way it is or
>>> how to fix that.
>>
>> Tianyu I might have missed this, but is there a follow on fix for this? 
>> Asking as the test in the current master is broken.
>>
>> Ilya/Simon should we undo “dd9881ed5 tc: Fix stats dump when using same 
>> meter table” and get a proper fix, fixing these counter issues?
>
> As far as I understand, kernel act_police is just not suitable
> to represent an OpenFlow meter.  The main problem is that with
> OVS we need two separate entities - actual meter and the meter
> action that is using some actual meter.  And these two entities
> should have separate sets of statistics.  Actions should count
> how many packets went through these actions (ideally, TC chain
> as a whole should count the number of successful matches, but that
> is anther inconsistency between TC and OVS) and the actual meter
> should count how many packets went through/dropped/etc.

Isn’t that the case? I think the basic statistics return the total number of 
packets hitting the rule, and the additional TCA_STATS_QUEUE stats give you the 
dropped packets.

I’ll need to go over this, and the code again, as this is all from memory.

> Multiple meter actions may reference the same actual meter.
> Each of these actions should have statistics separate from the
> statistics of the actual meter.  Stats of actions should be
> reported as flow stats, stats of the meter should be reported
> as meter stats.

This might be wrong, but I thought the stats are not per linked action, but 
from the rule that has the linked action.

> But with the act_police the actual meter and the action are the
> same entity, so the only statistics we have is the statistics
> of the actual meter.  So, there is no way to get the accurate
> flow statistics if the meter is the first action.  This is just
> a broken API by design.
>
> Saying that, commit dd9881ed5 seems to be correct, because we
> should not use statistics of the actual meter as flow stats.
> At the same time we have no way to get flow stats.  They are
> broken either way but differently.

If my story above not correct and we share the TCA_STATS_BASIC* with all 
instance where this meter is used we are out of luck.

> For the time being we may "fix" the test by adding some action
> before the meter in OpenFlow rules, so we can get accurate
> stats from it.
>
> For the real solution - I don't see any that doesn't involve
> kernel changes.  We either need to:
>
> - separate act_police from the actual policer in the kernel,
>   so we can query stats for them independently.
>
> - or create something like act_count - a dummy action that only
>   counts packets, and put it in every datapath action from 

Re: [ovs-dev] [PATCH branch-2.16 0/2] Release patches for v2.16.5.

[Flavio Leitner]

On Fri, Oct 07, 2022 at 01:17:06PM +0200, Ilya Maximets wrote:
> 
> Ilya Maximets (2):
>   Set release date for 2.16.5.
>   Prepare for 2.16.6.
> 
>  NEWS | 6 +-
>  configure.ac | 2 +-
>  debian/changelog | 8 +++-
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> -- 
> 2.37.3
> 

To the set:
Acked-by: Flavio Leitner 

Thanks Ilya,
fbl

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

Re: [ovs-dev] [PATCH branch-2.15 0/2] Release patches for v2.15.6.

[Flavio Leitner]

On Fri, Oct 07, 2022 at 01:16:54PM +0200, Ilya Maximets wrote:
> 
> Ilya Maximets (2):
>   Set release date for 2.15.6.
>   Prepare for 2.15.7.
> 
>  NEWS | 6 +-
>  configure.ac | 2 +-
>  debian/changelog | 8 +++-
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> -- 
> 2.37.3
> 

To the set:
Acked-by: Flavio Leitner 

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

Re: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats

[Davide Caratti]

On Fri, Oct 7, 2022 at 3:21 PM Marcelo Leitner  wrote:
>
> (+TC folks and netdev@)
>
> On Fri, Oct 07, 2022 at 02:42:56PM +0200, Ilya Maximets wrote:
> > On 10/7/22 13:37, Eelco Chaudron wrote:

[...]

> I don't see how we could achieve this without breaking much of the
> user experience.
>
> >
> > - or create something like act_count - a dummy action that only
> >   counts packets, and put it in every datapath action from OVS.
>
> This seems the easiest and best way forward IMHO. It's actually the
> 3rd option below but "on demand", considering that tc will already use
> the stats of the first action as the flow stats (in
> tcf_exts_dump_stats()), then we can patch ovs to add such action if a
> meter is also being used (or perhaps even always, because other
> actions may also drop packets, and for OVS we would really be at the
> 3rd option below).

Correct me if I'm wrong, but actually act_gact action with "pipe"
control action should already do this counting job.

any feedback appreciated, thanks!
-- 
davide

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

Re: [ovs-dev] [PATCH v2] dpif-netlink: add revalidator for offload of meters

[Eelco Chaudron]

On 30 Sep 2022, at 13:12, Simon Horman wrote:

> From: Yifan Li 
>
> Allow revalidator for hardware offload of meters via OVS-TC.
> Without revalidator, tc meter action can not be deleted while
> flow exists. The revalidator fix this bug by continuously
> checking existing meters and delete the unneeded ones. The
> autotest cases of revalidator are also added.

Hi Yifan,

I understand the problem, but I’m wondering why you decided to revalidate all 
the meters. Would it not be better to just keep track of a list of meters where 
deletion failed, and only handle those? Or maybe even better, check if the 
meter needs cleaning up when a flow with a meter action gets deleted. Maybe 
there are better/other ways but I need to give it some thought. Jainbo?

As mentioned before I think we should avoid doing all this extra work when 
there can be a simpler solution to the problem.


Some comments are below, which was not because I did a full review, but just 
noticed them when glancing over it.

> Signed-off-by: Yifan Li 
> Signed-off-by: Simon Horman 
> ---
>  lib/dpif-netdev.c|   1 +
>  lib/dpif-netlink.c   | 201 +++
>  lib/dpif-netlink.h   |   2 +
>  lib/dpif-provider.h  |   4 +
>  lib/dpif.c   |   7 ++
>  lib/dpif.h   |   2 +
>  lib/id-pool.c|  13 ++
>  lib/id-pool.h|   3 +
>  lib/netdev-linux.c   |   6 +
>  lib/netdev-offload-tc.c  |  11 +-
>  lib/tc.c |   5 -
>  lib/tc.h |   9 ++
>  ofproto/ofproto-dpif-upcall.c|   5 +
>  tests/system-offloads-traffic.at |   4 +
>  14 files changed, 267 insertions(+), 6 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index a45b460145c6..365aacadb03a 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -9583,6 +9583,7 @@ const struct dpif_class dpif_netdev_class = {
>  dpif_netdev_meter_set,
>  dpif_netdev_meter_get,
>  dpif_netdev_meter_del,
> +NULL,   /* meter_revalidate */
>  dpif_netdev_bond_add,
>  dpif_netdev_bond_del,
>  dpif_netdev_bond_stats_get,
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index a620a6ec52dd..fac3535e1748 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -25,6 +25,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -61,6 +62,7 @@
>  #include "packets.h"
>  #include "random.h"
>  #include "sset.h"
> +#include "tc.h"
>  #include "timeval.h"
>  #include "unaligned.h"
>  #include "util.h"
> @@ -4161,6 +4163,191 @@ dpif_netlink_meter_get_features(const struct dpif 
> *dpif_,
>  ofpbuf_delete(msg);
>  }
>
> +static bool dpif_netlink_meter_should_revalidate(struct id_pool *meter_ids,
> + uint32_t meter_id)
> +{
> +return !id_pool_id_exist(meter_ids, meter_id);
> +}
> +
> +static void
> +dpif_tc_meter_revalidate(struct dpif *dpif_ OVS_UNUSED,
> + struct id_pool *meter_ids, struct ofpbuf *reply)
> +{
> +static struct nl_policy actions_orders_policy[ACT_MAX_NUM + 1] = {};
> +struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)];
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,20);
> +static const struct nl_policy tca_root_policy[] = {
> +[TCA_ACT_TAB] = { .type = NL_A_NESTED, .optional = false },
> +[TCA_ROOT_COUNT] = { .type = NL_A_U32, .optional = false },
> +};
> +struct nlattr *action_root_attrs[ARRAY_SIZE(tca_root_policy)];
> +static const struct nl_policy police_policy[] = {
> +[TCA_POLICE_TBF] = { .type = NL_A_UNSPEC,
> + .min_len = sizeof(struct tc_police),
> + .optional = false},
> +};
> +struct nlattr *action_police_tab[ARRAY_SIZE(police_policy)];
> +static const struct nl_policy act_policy[] = {
> +[TCA_ACT_KIND] = { .type = NL_A_STRING, .optional = false, },
> +[TCA_ACT_COOKIE] = { .type = NL_A_UNSPEC, .optional = true, },
> +[TCA_ACT_OPTIONS] = { .type = NL_A_NESTED, .optional = true, },
> +[TCA_ACT_STATS] = { .type = NL_A_NESTED, .optional = false, },
> +};
> +struct nlattr *action_police_attrs[ARRAY_SIZE(act_policy)];
> +const int max_size = ARRAY_SIZE(actions_orders_policy);
> +const struct tc_police *tc_police = NULL;
> +ofproto_meter_id meter_id;
> +size_t revalidate_num;
> +size_t act_count;
> +uint32_t index;
> +int i;
> +
> +if (!reply) {
> +VLOG_ERR_RL(, "Null reply message during meter revalidation");
> +return;
> +}
> +
> +if (reply->size <= NLMSG_ALIGNTO + NLMSG_HDRLEN) {
> +VLOG_DBG_RL(, "No meters present in tc during meter "
> +"revalidation");
> +return;
> +}
> +
> +if 

Re: [ovs-dev] [PATCH branch-2.14 0/2] Release patches for v2.14.7.

[Flavio Leitner]

On Fri, Oct 07, 2022 at 01:16:41PM +0200, Ilya Maximets wrote:
> 
> Ilya Maximets (2):
>   Set release date for 2.14.7.
>   Prepare for 2.14.8.
> 
>  NEWS | 6 +-
>  configure.ac | 2 +-
>  debian/changelog | 8 +++-
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> -- 
> 2.37.3
> 

To the set:
Acked-by: Flavio Leitner 

Thanks Ilya,
fbl

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

Re: [ovs-dev] [PATCH branch-2.13 0/2] Release patches for v2.13.9.

[Flavio Leitner]

On Fri, Oct 07, 2022 at 01:16:29PM +0200, Ilya Maximets wrote:
> 
> Ilya Maximets (2):
>   Set release date for 2.13.9.
>   Prepare for 2.13.10.
> 
>  NEWS | 6 +-
>  configure.ac | 2 +-
>  debian/changelog | 8 +++-
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> -- 
> 2.37.3
> 

-- 
fbl

To the set
Acked-by: Flavio Leitner 

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

Re: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats

[Marcelo Leitner]

(+TC folks and netdev@)

On Fri, Oct 07, 2022 at 02:42:56PM +0200, Ilya Maximets wrote:
> On 10/7/22 13:37, Eelco Chaudron wrote:
> >
> >
> > On 15 Sep 2022, at 14:03, Ilya Maximets wrote:
> >
> >> On 9/15/22 13:56, Ilya Maximets wrote:
> >>> On 9/15/22 13:38, Tianyu Yuan wrote:
> 
>  On 9/15/22 19:28,  Ilya Maximets wrote:
> > On 9/14/22 16:19, Simon Horman wrote:
> >> From: Tianyu Yuan 
> >>
> >> Since dd9881ed55e6 ("tc: Fix stats dump when using same meter table")
> >> rule stats update will ignore meter police. Correspondingly, the
> >> reference stats of the test should also be modified to ensure the test
> >> could pass correctly.
> >>
> >> Fixes: dd9881ed55e6 ("tc: Fix stats dump when using same meter table")
> >> Signed-off-by: Tianyu Yuan 
> >> Signed-off-by: Simon Horman 
> >> ---
> >>  tests/system-offloads-traffic.at | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/tests/system-offloads-traffic.at
> >> b/tests/system-offloads-traffic.at
> >> index d9b815a5ddf4..24e49d42f589 100644
> >> --- a/tests/system-offloads-traffic.at
> >> +++ b/tests/system-offloads-traffic.at
> >> @@ -264,7 +264,7 @@ NS_CHECK_EXEC([at_ns0], [echo "mark" | nc
> >> $NC_EOF_OPT -u 10.1.1.2 5678 -p 6789])  done
> >>
> >>  AT_CHECK([ovs-appctl dpctl/dump-flows | grep "meter" |
> >> DUMP_CLEAN_SORTED], [0], [dnl
> >> -in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no),
> >> packets:10, bytes:330, used:0.001s, actions:meter(0),3
> >> +in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no),
> >> +packets:1, bytes:33, used:0.001s, actions:meter(0),3
> >
> > This looks very strange to me.  The test does send 10 packets.
> > Why the flow should report only one?
> >
>  Thanks for your review Ilya.
>  The test does send 10 packets but 9 of them are dropped by
>  meter action. As we descript in commit (dd9881ed55e6), the
>  flow stats should not report the police stats.
> >>
> >> "In previous patch, after parsing police action, the flower stats will
> >>  be updated by dumped meter table stats"
> >>
> >> I suppose, that is the root cause.  We should not mix these two
> >> completely different types of statistics.  I didn't look at the
> >> code though to say why this was implemented in a way it is or
> >> how to fix that.
> >
> > Tianyu I might have missed this, but is there a follow on fix for this? 
> > Asking as the test in the current master is broken.
> >
> > Ilya/Simon should we undo “dd9881ed5 tc: Fix stats dump when using same 
> > meter table” and get a proper fix, fixing these counter issues?
>
> As far as I understand, kernel act_police is just not suitable
> to represent an OpenFlow meter.  The main problem is that with
> OVS we need two separate entities - actual meter and the meter
> action that is using some actual meter.  And these two entities
> should have separate sets of statistics.  Actions should count
> how many packets went through these actions (ideally, TC chain
> as a whole should count the number of successful matches, but that
> is anther inconsistency between TC and OVS) and the actual meter
> should count how many packets went through/dropped/etc.
> Multiple meter actions may reference the same actual meter.
> Each of these actions should have statistics separate from the
> statistics of the actual meter.  Stats of actions should be
> reported as flow stats, stats of the meter should be reported
> as meter stats.
>
> But with the act_police the actual meter and the action are the
> same entity, so the only statistics we have is the statistics
> of the actual meter.  So, there is no way to get the accurate
> flow statistics if the meter is the first action.  This is just
> a broken API by design.

Thanks Ilya for the detailed explanation.

>
> Saying that, commit dd9881ed5 seems to be correct, because we
> should not use statistics of the actual meter as flow stats.
> At the same time we have no way to get flow stats.  They are
> broken either way but differently.
>
> For the time being we may "fix" the test by adding some action
> before the meter in OpenFlow rules, so we can get accurate
> stats from it.
>
> For the real solution - I don't see any that doesn't involve
> kernel changes.  We either need to:
>
> - separate act_police from the actual policer in the kernel,
>   so we can query stats for them independently.

I don't see how we could achieve this without breaking much of the
user experience.

>
> - or create something like act_count - a dummy action that only
>   counts packets, and put it in every datapath action from OVS.

This seems the easiest and best way forward IMHO. It's actually the
3rd option below but "on demand", considering that tc will already use
the stats of the first action as the flow stats (in
tcf_exts_dump_stats()), then we can patch ovs to add such action if a
meter is 

Re: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats

[Ilya Maximets]

On 10/7/22 13:37, Eelco Chaudron wrote:
> 
> 
> On 15 Sep 2022, at 14:03, Ilya Maximets wrote:
> 
>> On 9/15/22 13:56, Ilya Maximets wrote:
>>> On 9/15/22 13:38, Tianyu Yuan wrote:

 On 9/15/22 19:28,  Ilya Maximets wrote:
> On 9/14/22 16:19, Simon Horman wrote:
>> From: Tianyu Yuan 
>>
>> Since dd9881ed55e6 ("tc: Fix stats dump when using same meter table")
>> rule stats update will ignore meter police. Correspondingly, the
>> reference stats of the test should also be modified to ensure the test
>> could pass correctly.
>>
>> Fixes: dd9881ed55e6 ("tc: Fix stats dump when using same meter table")
>> Signed-off-by: Tianyu Yuan 
>> Signed-off-by: Simon Horman 
>> ---
>>  tests/system-offloads-traffic.at | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/system-offloads-traffic.at
>> b/tests/system-offloads-traffic.at
>> index d9b815a5ddf4..24e49d42f589 100644
>> --- a/tests/system-offloads-traffic.at
>> +++ b/tests/system-offloads-traffic.at
>> @@ -264,7 +264,7 @@ NS_CHECK_EXEC([at_ns0], [echo "mark" | nc
>> $NC_EOF_OPT -u 10.1.1.2 5678 -p 6789])  done
>>
>>  AT_CHECK([ovs-appctl dpctl/dump-flows | grep "meter" |
>> DUMP_CLEAN_SORTED], [0], [dnl
>> -in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no),
>> packets:10, bytes:330, used:0.001s, actions:meter(0),3
>> +in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no),
>> +packets:1, bytes:33, used:0.001s, actions:meter(0),3
>
> This looks very strange to me.  The test does send 10 packets.
> Why the flow should report only one?
>
 Thanks for your review Ilya.
 The test does send 10 packets but 9 of them are dropped by
 meter action. As we descript in commit (dd9881ed55e6), the
 flow stats should not report the police stats.
>>
>> "In previous patch, after parsing police action, the flower stats will
>>  be updated by dumped meter table stats"
>>
>> I suppose, that is the root cause.  We should not mix these two
>> completely different types of statistics.  I didn't look at the
>> code though to say why this was implemented in a way it is or
>> how to fix that.
> 
> Tianyu I might have missed this, but is there a follow on fix for this? 
> Asking as the test in the current master is broken.
> 
> Ilya/Simon should we undo “dd9881ed5 tc: Fix stats dump when using same meter 
> table” and get a proper fix, fixing these counter issues?

As far as I understand, kernel act_police is just not suitable
to represent an OpenFlow meter.  The main problem is that with
OVS we need two separate entities - actual meter and the meter
action that is using some actual meter.  And these two entities
should have separate sets of statistics.  Actions should count
how many packets went through these actions (ideally, TC chain
as a whole should count the number of successful matches, but that
is anther inconsistency between TC and OVS) and the actual meter
should count how many packets went through/dropped/etc.
Multiple meter actions may reference the same actual meter.
Each of these actions should have statistics separate from the
statistics of the actual meter.  Stats of actions should be
reported as flow stats, stats of the meter should be reported
as meter stats.

But with the act_police the actual meter and the action are the
same entity, so the only statistics we have is the statistics
of the actual meter.  So, there is no way to get the accurate
flow statistics if the meter is the first action.  This is just
a broken API by design.

Saying that, commit dd9881ed5 seems to be correct, because we
should not use statistics of the actual meter as flow stats.
At the same time we have no way to get flow stats.  They are
broken either way but differently.

For the time being we may "fix" the test by adding some action
before the meter in OpenFlow rules, so we can get accurate
stats from it.

For the real solution - I don't see any that doesn't involve
kernel changes.  We either need to:

- separate act_police from the actual policer in the kernel,
  so we can query stats for them independently.

- or create something like act_count - a dummy action that only
  counts packets, and put it in every datapath action from OVS.

- or make flower chains count packets that successfully matched
  and use that information as flows stats.

In any case, we need to document somehow that flow stats with
meters are not correct.

Any thoughts?

Best regards, Ilya Maximets.

> 
 In this case, only
 one packet passes the meter action and be used to update
 flow stats.
>>>
>>> The flow statistics counts how many packets hit the flow by
>>> the match criteria, not how many of them survived after the
>>> action execution.
>>>
>>> See the same test with offloads disabled (next to it in the file).
>>> It correctly counts all the 10 packets.
>>>
>>> If we will not count packets, OVS will 

Re: [ovs-dev] [PATCH] tests: fix reference output for meter offload stats

[Eelco Chaudron]

On 15 Sep 2022, at 14:03, Ilya Maximets wrote:

> On 9/15/22 13:56, Ilya Maximets wrote:
>> On 9/15/22 13:38, Tianyu Yuan wrote:
>>>
>>> On 9/15/22 19:28,  Ilya Maximets wrote:
 On 9/14/22 16:19, Simon Horman wrote:
> From: Tianyu Yuan 
>
> Since dd9881ed55e6 ("tc: Fix stats dump when using same meter table")
> rule stats update will ignore meter police. Correspondingly, the
> reference stats of the test should also be modified to ensure the test
> could pass correctly.
>
> Fixes: dd9881ed55e6 ("tc: Fix stats dump when using same meter table")
> Signed-off-by: Tianyu Yuan 
> Signed-off-by: Simon Horman 
> ---
>  tests/system-offloads-traffic.at | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/system-offloads-traffic.at
> b/tests/system-offloads-traffic.at
> index d9b815a5ddf4..24e49d42f589 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -264,7 +264,7 @@ NS_CHECK_EXEC([at_ns0], [echo "mark" | nc
> $NC_EOF_OPT -u 10.1.1.2 5678 -p 6789])  done
>
>  AT_CHECK([ovs-appctl dpctl/dump-flows | grep "meter" |
> DUMP_CLEAN_SORTED], [0], [dnl
> -in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no),
> packets:10, bytes:330, used:0.001s, actions:meter(0),3
> +in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no),
> +packets:1, bytes:33, used:0.001s, actions:meter(0),3

 This looks very strange to me.  The test does send 10 packets.
 Why the flow should report only one?

>>> Thanks for your review Ilya.
>>> The test does send 10 packets but 9 of them are dropped by
>>> meter action. As we descript in commit (dd9881ed55e6), the
>>> flow stats should not report the police stats.
>
> "In previous patch, after parsing police action, the flower stats will
>  be updated by dumped meter table stats"
>
> I suppose, that is the root cause.  We should not mix these two
> completely different types of statistics.  I didn't look at the
> code though to say why this was implemented in a way it is or
> how to fix that.

Tianyu I might have missed this, but is there a follow on fix for this? Asking 
as the test in the current master is broken.

Ilya/Simon should we undo “dd9881ed5 tc: Fix stats dump when using same meter 
table” and get a proper fix, fixing these counter issues?

>>> In this case, only
>>> one packet passes the meter action and be used to update
>>> flow stats.
>>
>> The flow statistics counts how many packets hit the flow by
>> the match criteria, not how many of them survived after the
>> action execution.
>>
>> See the same test with offloads disabled (next to it in the file).
>> It correctly counts all the 10 packets.
>>
>> If we will not count packets, OVS will eventually remove the
>> flow from the datapath causing removal from TC and hardware
>> and triggering upcalls on the next packet.  And OpenFlow
>> statistics will also be incorrect.
>>
>> Best regards, Ilya Maximets.
>>
>  ])
>
>  AT_CHECK([ovs-ofctl -O OpenFlow13 meter-stats br0 | sed -e
> 's/duration:[[0-9]].[[0-9]]*s/duration:0.001s/'], [0], [dnl

 These meter stats are correctly reporting 11 packets, so the datapath flow
 should report 10 (+1 on the upcall), AFAIU.

 Best regards, Ilya Maximets.
>>>
>>> Attach the tc filter information when running this test:
>>> filter protocol ip pref 3 flower chain 0
>>> filter protocol ip pref 3 flower chain 0 handle 0x1
>>>   dst_mac f0:00:00:01:01:02
>>>   src_mac f0:00:00:01:01:01
>>>   eth_type ipv4
>>>   ip_proto udp
>>>   ip_flags nofrag
>>>   not_in_hw
>>> action order 1:  police 0x1000 rate 0bit burst 0b mtu 64Kb 
>>> pkts_rate 1 pkts_burst 1 action drop/pipe overhead 0b linklayer unspec
>>> ref 2 bind 1  installed 2 sec used 0 sec firstused 0 sec
>>> Action statistics:
>>> Sent 330 bytes 10 pkt (dropped 9, overlimits 9 requeues 0)
>>> backlog 0b 0p requeues 0
>>>
>>> action order 2: mirred (Egress Redirect to device ovs-p1) stolen
>>> index 8 ref 1 bind 1 installed 1 sec used 0 sec firstused 0 sec
>>> Action statistics:
>>> Sent 33 bytes 1 pkt (dropped 0, overlimits 0 requeues 0)
>>> backlog 0b 0p requeues 0
>>> cookie 8455fe4dcd4e3677d0ca43b42684d1a6
>>> no_percpu
>>>
>>>
>>> Best regards,
>>> Tianyu Yuan
>>

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

Re: [ovs-dev] [OVN v10] OVN - Add Support for Remote Port Mirroring

[0-day Robot]

Bleep bloop.  Greetings Abhiram R N, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

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


checkpatch:
WARNING: Line lacks whitespace around operator
#2148 FILE: utilities/ovn-nbctl.c:275:
  mirror-add NAME TYPE INDEX FILTER IP\n\

WARNING: Line lacks whitespace around operator
#2157 FILE: utilities/ovn-nbctl.c:284:
  mirror-del [NAME] remove mirrors\n\

WARNING: Line lacks whitespace around operator
#2158 FILE: utilities/ovn-nbctl.c:285:
  mirror-list   print mirrors\n\

WARNING: Line lacks whitespace around operator
#2167 FILE: utilities/ovn-nbctl.c:327:
  lsp-attach-mirror PORT MIRROR   attach source PORT to MIRROR\n\

WARNING: Line lacks whitespace around operator
#2168 FILE: utilities/ovn-nbctl.c:328:
  lsp-detach-mirror PORT MIRROR   detach source PORT from MIRROR\n\

Lines checked: 2567, Warnings: 5, Errors: 0


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

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

[ovs-dev] [PATCH branch-2.17 2/2] Prepare for 2.17.4.

[Ilya Maximets]

Signed-off-by: Ilya Maximets 
---
 NEWS | 3 +++
 configure.ac | 2 +-
 debian/changelog | 6 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index 7858b957f..b04b7b4c9 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,6 @@
+v2.17.4 - xx xxx 
+-
+
 v2.17.3 - 07 Oct 2022
 -
- Bug fixes
diff --git a/configure.ac b/configure.ac
index 5cc3f4801..733068bfd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -13,7 +13,7 @@
 # limitations under the License.
 
 AC_PREREQ(2.63)
-AC_INIT(openvswitch, 2.17.3, b...@openvswitch.org)
+AC_INIT(openvswitch, 2.17.4, b...@openvswitch.org)
 AC_CONFIG_SRCDIR([datapath/datapath.c])
 AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_AUX_DIR([build-aux])
diff --git a/debian/changelog b/debian/changelog
index 10868d266..1a1f0984b 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+openvswitch (2.17.4-1) unstable; urgency=low
+   [ Open vSwitch team ]
+   * New upstream version
+
+ -- Open vSwitch team   Fri, 07 Oct 2022 13:12:51 +0200
+
 openvswitch (2.17.3-1) unstable; urgency=low
[ Open vSwitch team ]
* New upstream version
-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.17 0/2] Release patches for v2.17.3.

[Ilya Maximets]

Ilya Maximets (2):
  Set release date for 2.17.3.
  Prepare for 2.17.4.

 NEWS | 6 +-
 configure.ac | 2 +-
 debian/changelog | 8 +++-
 3 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.17 1/2] Set release date for 2.17.3.

[Ilya Maximets]

Signed-off-by: Ilya Maximets 
---
 NEWS | 3 ++-
 debian/changelog | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index 36fcbb874..7858b957f 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,6 @@
-v2.17.3 - xx xxx 
+v2.17.3 - 07 Oct 2022
 -
+   - Bug fixes
- OVSDB:
  * New Local_Config schema added to support Connections (--remote)
configuration in a clustered databse independently for each server.
diff --git a/debian/changelog b/debian/changelog
index 5ddd655d6..10868d266 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -2,7 +2,7 @@ openvswitch (2.17.3-1) unstable; urgency=low
[ Open vSwitch team ]
* New upstream version
 
- -- Open vSwitch team   Wed, 15 Jun 2022 12:04:07 +0200
+ -- Open vSwitch team   Fri, 07 Oct 2022 13:12:51 +0200
 
 openvswitch (2.17.2-1) unstable; urgency=low
[ Open vSwitch team ]
-- 
2.37.3

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

[ovs-dev] [PATCH branch-3.0 2/2] Prepare for 3.0.2.

[Ilya Maximets]

Signed-off-by: Ilya Maximets 
---
 NEWS | 3 +++
 configure.ac | 2 +-
 debian/changelog | 6 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index a7f367b7c..eee77189b 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,6 @@
+v3.0.2 - xx xxx 
+
+
 v3.0.1 - 07 Oct 2022
 
- Bug fixes
diff --git a/configure.ac b/configure.ac
index a7846f45e..049264f95 100644
--- a/configure.ac
+++ b/configure.ac
@@ -13,7 +13,7 @@
 # limitations under the License.
 
 AC_PREREQ(2.63)
-AC_INIT(openvswitch, 3.0.1, b...@openvswitch.org)
+AC_INIT(openvswitch, 3.0.2, b...@openvswitch.org)
 AC_CONFIG_SRCDIR([vswitchd/ovs-vswitchd.c])
 AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_AUX_DIR([build-aux])
diff --git a/debian/changelog b/debian/changelog
index e1f8974f5..157e7ad0b 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+openvswitch (3.0.2-1) unstable; urgency=low
+   [ Open vSwitch team ]
+   * New upstream version
+
+ -- Open vSwitch team   Fri, 07 Oct 2022 13:13:11 +0200
+
 openvswitch (3.0.1-1) unstable; urgency=low
[ Open vSwitch team ]
* New upstream version
-- 
2.37.3

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

[ovs-dev] [PATCH branch-3.0 1/2] Set release date for 3.0.1.

[Ilya Maximets]

Signed-off-by: Ilya Maximets 
---
 NEWS | 3 ++-
 debian/changelog | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index ad5ba021d..a7f367b7c 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,6 @@
-v3.0.1 - xx xxx 
+v3.0.1 - 07 Oct 2022
 
+   - Bug fixes
- DPDK:
  * OVS validated with DPDK 21.11.2.
DPDK 21.11.2 contains fixes for the following CVEs:
diff --git a/debian/changelog b/debian/changelog
index 670b704bb..e1f8974f5 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -2,7 +2,7 @@ openvswitch (3.0.1-1) unstable; urgency=low
[ Open vSwitch team ]
* New upstream version
 
- -- Open vSwitch team   Mon, 15 Aug 2022 17:43:58 +0200
+ -- Open vSwitch team   Fri, 07 Oct 2022 13:13:11 +0200
 
 openvswitch (3.0.0-1) unstable; urgency=low
 
-- 
2.37.3

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

[ovs-dev] [PATCH branch-3.0 0/2] Release patches for v3.0.1.

[Ilya Maximets]

Ilya Maximets (2):
  Set release date for 3.0.1.
  Prepare for 3.0.2.

 NEWS | 6 +-
 configure.ac | 2 +-
 debian/changelog | 8 +++-
 3 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.16 2/2] Prepare for 2.16.6.

[Ilya Maximets]

Signed-off-by: Ilya Maximets 
---
 NEWS | 3 +++
 configure.ac | 2 +-
 debian/changelog | 6 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index 9693a17db..331af302b 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,6 @@
+v2.16.6 - xx xxx 
+-
+
 v2.16.5 - 07 Oct 2022
 -
- Bug fixes
diff --git a/configure.ac b/configure.ac
index 406df116e..2a957c1e0 100644
--- a/configure.ac
+++ b/configure.ac
@@ -13,7 +13,7 @@
 # limitations under the License.
 
 AC_PREREQ(2.63)
-AC_INIT(openvswitch, 2.16.5, b...@openvswitch.org)
+AC_INIT(openvswitch, 2.16.6, b...@openvswitch.org)
 AC_CONFIG_SRCDIR([datapath/datapath.c])
 AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_AUX_DIR([build-aux])
diff --git a/debian/changelog b/debian/changelog
index 1378b5466..e749ad5f2 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+openvswitch (2.16.6-1) unstable; urgency=low
+   [ Open vSwitch team ]
+   * New upstream version
+
+ -- Open vSwitch team   Fri, 07 Oct 2022 13:12:46 +0200
+
 openvswitch (2.16.5-1) unstable; urgency=low
[ Open vSwitch team ]
* New upstream version
-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.16 1/2] Set release date for 2.16.5.

[Ilya Maximets]

Signed-off-by: Ilya Maximets 
---
 NEWS | 3 ++-
 debian/changelog | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index 578b845dd..9693a17db 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,6 @@
-v2.16.5 - xx xxx 
+v2.16.5 - 07 Oct 2022
 -
+   - Bug fixes
- DPDK:
  * OVS validated with DPDK 20.11.6.
DPDK 20.11.6 requires a meson version of 0.48.1 or higher.
diff --git a/debian/changelog b/debian/changelog
index 522e10b0e..1378b5466 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -2,7 +2,7 @@ openvswitch (2.16.5-1) unstable; urgency=low
[ Open vSwitch team ]
* New upstream version
 
- -- Open vSwitch team   Wed, 15 Jun 2022 12:03:55 +0200
+ -- Open vSwitch team   Fri, 07 Oct 2022 13:12:46 +0200
 
 openvswitch (2.16.4-1) unstable; urgency=low
[ Open vSwitch team ]
-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.16 0/2] Release patches for v2.16.5.

[Ilya Maximets]

Ilya Maximets (2):
  Set release date for 2.16.5.
  Prepare for 2.16.6.

 NEWS | 6 +-
 configure.ac | 2 +-
 debian/changelog | 8 +++-
 3 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.15 0/2] Release patches for v2.15.6.

[Ilya Maximets]

Ilya Maximets (2):
  Set release date for 2.15.6.
  Prepare for 2.15.7.

 NEWS | 6 +-
 configure.ac | 2 +-
 debian/changelog | 8 +++-
 3 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.15 1/2] Set release date for 2.15.6.

[Ilya Maximets]

Signed-off-by: Ilya Maximets 
---
 NEWS | 3 ++-
 debian/changelog | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index 4c264c090..775f36bb9 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,6 @@
-v2.15.6 - xx xxx 
+v2.15.6 - 07 Oct 2022
 -
+   - Bug fixes
- DPDK:
  * OVS validated with DPDK 20.11.6.
DPDK 20.11.6 requires a meson version of 0.48.1 or higher.
diff --git a/debian/changelog b/debian/changelog
index 8ffbbd311..18ca5ef2a 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -2,7 +2,7 @@ openvswitch (2.15.6-1) unstable; urgency=low
[ Open vSwitch team ]
* New upstream version
 
- -- Open vSwitch team   Wed, 15 Jun 2022 12:03:31 +0200
+ -- Open vSwitch team   Fri, 07 Oct 2022 13:12:36 +0200
 
 openvswitch (2.15.5-1) unstable; urgency=low
[ Open vSwitch team ]
-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.15 2/2] Prepare for 2.15.7.

[Ilya Maximets]

Signed-off-by: Ilya Maximets 
---
 NEWS | 3 +++
 configure.ac | 2 +-
 debian/changelog | 6 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index 775f36bb9..6510dfc96 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,6 @@
+v2.15.7 - xx xxx 
+-
+
 v2.15.6 - 07 Oct 2022
 -
- Bug fixes
diff --git a/configure.ac b/configure.ac
index 0ef4d1c07..72f676d72 100644
--- a/configure.ac
+++ b/configure.ac
@@ -13,7 +13,7 @@
 # limitations under the License.
 
 AC_PREREQ(2.63)
-AC_INIT(openvswitch, 2.15.6, b...@openvswitch.org)
+AC_INIT(openvswitch, 2.15.7, b...@openvswitch.org)
 AC_CONFIG_SRCDIR([datapath/datapath.c])
 AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_AUX_DIR([build-aux])
diff --git a/debian/changelog b/debian/changelog
index 18ca5ef2a..e503d30d9 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+openvswitch (2.15.7-1) unstable; urgency=low
+   [ Open vSwitch team ]
+   * New upstream version
+
+ -- Open vSwitch team   Fri, 07 Oct 2022 13:12:36 +0200
+
 openvswitch (2.15.6-1) unstable; urgency=low
[ Open vSwitch team ]
* New upstream version
-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.14 2/2] Prepare for 2.14.8.

[Ilya Maximets]

Signed-off-by: Ilya Maximets 
---
 NEWS | 3 +++
 configure.ac | 2 +-
 debian/changelog | 6 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index 57b703503..817249593 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,6 @@
+v2.14.8 - xx xxx 
+-
+
 v2.14.7 - 07 Oct 2022
 -
- Bug fixes
diff --git a/configure.ac b/configure.ac
index 4b1e1b9e8..322d46810 100644
--- a/configure.ac
+++ b/configure.ac
@@ -13,7 +13,7 @@
 # limitations under the License.
 
 AC_PREREQ(2.63)
-AC_INIT(openvswitch, 2.14.7, b...@openvswitch.org)
+AC_INIT(openvswitch, 2.14.8, b...@openvswitch.org)
 AC_CONFIG_SRCDIR([datapath/datapath.c])
 AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_AUX_DIR([build-aux])
diff --git a/debian/changelog b/debian/changelog
index 69f4108c2..a3f016d0c 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+openvswitch (2.14.8-1) unstable; urgency=low
+   [ Open vSwitch team ]
+   * New upstream version
+
+ -- Open vSwitch team   Fri, 07 Oct 2022 13:12:32 +0200
+
 openvswitch (2.14.7-1) unstable; urgency=low
[ Open vSwitch team ]
* New upstream version
-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.14 0/2] Release patches for v2.14.7.

[Ilya Maximets]

Ilya Maximets (2):
  Set release date for 2.14.7.
  Prepare for 2.14.8.

 NEWS | 6 +-
 configure.ac | 2 +-
 debian/changelog | 8 +++-
 3 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.14 1/2] Set release date for 2.14.7.

[Ilya Maximets]

Signed-off-by: Ilya Maximets 
---
 NEWS | 3 ++-
 debian/changelog | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index 67690e519..57b703503 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,6 @@
-v2.14.7 - xx xxx 
+v2.14.7 - 07 Oct 2022
 -
+   - Bug fixes
- DPDK:
  * OVS validated with DPDK 19.11.13.
DPDK 19.11.13 contains fixes for the following CVEs:
diff --git a/debian/changelog b/debian/changelog
index 739bfff9c..69f4108c2 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -2,7 +2,7 @@ openvswitch (2.14.7-1) unstable; urgency=low
[ Open vSwitch team ]
* New upstream version
 
- -- Open vSwitch team   Wed, 15 Jun 2022 12:03:27 +0200
+ -- Open vSwitch team   Fri, 07 Oct 2022 13:12:32 +0200
 
 openvswitch (2.14.6-1) unstable; urgency=low
[ Open vSwitch team ]
-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.13 2/2] Prepare for 2.13.10.

[Ilya Maximets]

Signed-off-by: Ilya Maximets 
---
 NEWS | 3 +++
 configure.ac | 2 +-
 debian/changelog | 6 ++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/NEWS b/NEWS
index 3b7ece1d3..fe5743bb8 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,6 @@
+v2.13.10 - xx xxx 
+--
+
 v2.13.9 - 07 Oct 2022
 -
- Bug fixes
diff --git a/configure.ac b/configure.ac
index 80af0bf05..dc69fd768 100644
--- a/configure.ac
+++ b/configure.ac
@@ -13,7 +13,7 @@
 # limitations under the License.
 
 AC_PREREQ(2.63)
-AC_INIT(openvswitch, 2.13.9, b...@openvswitch.org)
+AC_INIT(openvswitch, 2.13.10, b...@openvswitch.org)
 AC_CONFIG_SRCDIR([datapath/datapath.c])
 AC_CONFIG_MACRO_DIR([m4])
 AC_CONFIG_AUX_DIR([build-aux])
diff --git a/debian/changelog b/debian/changelog
index 2a72c170a..87231dcc6 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,9 @@
+openvswitch (2.13.10-1) unstable; urgency=low
+   [ Open vSwitch team ]
+   * New upstream version
+
+ -- Open vSwitch team   Fri, 07 Oct 2022 13:11:43 +0200
+
 openvswitch (2.13.9-1) unstable; urgency=low
[ Open vSwitch team ]
* New upstream version
-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.13 0/2] Release patches for v2.13.9.

[Ilya Maximets]

Ilya Maximets (2):
  Set release date for 2.13.9.
  Prepare for 2.13.10.

 NEWS | 6 +-
 configure.ac | 2 +-
 debian/changelog | 8 +++-
 3 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.37.3

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

[ovs-dev] [PATCH branch-2.13 1/2] Set release date for 2.13.9.

[Ilya Maximets]

Signed-off-by: Ilya Maximets 
---
 NEWS | 3 ++-
 debian/changelog | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index 8e2553901..3b7ece1d3 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,6 @@
-v2.13.9 - xx xxx 
+v2.13.9 - 07 Oct 2022
 -
+   - Bug fixes
- DPDK:
  * OVS validated with DPDK 19.11.13.
DPDK 19.11.13 contains fixes for the following CVEs:
diff --git a/debian/changelog b/debian/changelog
index f0ada427e..2a72c170a 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -2,7 +2,7 @@ openvswitch (2.13.9-1) unstable; urgency=low
[ Open vSwitch team ]
* New upstream version
 
- -- Open vSwitch team   Wed, 15 Jun 2022 11:56:46 +0200
+ -- Open vSwitch team   Fri, 07 Oct 2022 13:11:43 +0200
 
 openvswitch (2.13.8-1) unstable; urgency=low
[ Open vSwitch team ]
-- 
2.37.3

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

[ovs-dev] [RFC dpdk-latest v2 3/3] netdev-dpdk: Add per virtqueue statistics.

[David Marchand]

Request per virtqueue statistics from the vhost library and expose them
as per port OVS custom stats.

Note:
- the vhost stats API is experimental at the moment, this patch is
  sent as a demonstrator,
- we may drop maintaining rx stats in OVS itself and instead aggregate
  the per vq stats, this is something to investigate,
- a unit test is missing,

Signed-off-by: David Marchand 
---
 lib/netdev-dpdk.c | 203 --
 1 file changed, 194 insertions(+), 9 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 132ebb2843..3db5944977 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -532,11 +532,20 @@ struct netdev_dpdk {
 );
 
 PADDED_MEMBERS(CACHE_LINE_SIZE,
-/* Names of all XSTATS counters */
-struct rte_eth_xstat_name *rte_xstats_names;
-int rte_xstats_names_size;
-int rte_xstats_ids_size;
-uint64_t *rte_xstats_ids;
+union {
+struct {
+/* Names of all XSTATS counters */
+struct rte_eth_xstat_name *rte_xstats_names;
+int rte_xstats_names_size;
+int rte_xstats_ids_size;
+uint64_t *rte_xstats_ids;
+};
+struct {
+/* Names of all vhost stats */
+struct rte_vhost_stat_name *vhost_stat_names;
+int vhost_stat_size;
+};
+};
 );
 };
 
@@ -552,6 +561,7 @@ static int netdev_dpdk_get_sw_custom_stats(const struct 
netdev *,
struct netdev_custom_stats *);
 static void netdev_dpdk_configure_xstats(struct netdev_dpdk *dev);
 static void netdev_dpdk_clear_xstats(struct netdev_dpdk *dev);
+static void netdev_dpdk_vhost_clear_stats(struct netdev_dpdk *dev);
 
 int netdev_dpdk_get_vid(const struct netdev_dpdk *dev);
 
@@ -1586,6 +1596,7 @@ netdev_dpdk_vhost_destruct(struct netdev *netdev)
 dev->vhost_id = NULL;
 rte_free(dev->vhost_rxq_enabled);
 
+netdev_dpdk_vhost_clear_stats(dev);
 common_destruct(dev);
 
 ovs_mutex_unlock(_mutex);
@@ -3039,6 +3050,80 @@ netdev_dpdk_vhost_get_stats(const struct netdev *netdev,
 return 0;
 }
 
+static int
+netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev,
+   struct netdev_custom_stats *custom_stats)
+{
+netdev_dpdk_get_sw_custom_stats(netdev, custom_stats);
+
+#ifdef ALLOW_EXPERIMENTAL_API
+struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
+
+ovs_mutex_lock(>mutex);
+
+int vid = netdev_dpdk_get_vid(dev);
+
+if (vid >= 0 && dev->vhost_stat_size > 0) {
+struct rte_vhost_stat *vhost_stats;
+int stat_offset;
+int sw_stats_size;
+
+vhost_stats = xcalloc(dev->vhost_stat_size, sizeof *vhost_stats);
+
+stat_offset = 0;
+
+for (int q = 0; q < dev->up.n_rxq; q++) {
+int qid = q * VIRTIO_QNUM + VIRTIO_TXQ;
+int err;
+
+err = rte_vhost_vring_stats_get(vid, qid,
+_stats[stat_offset],
+dev->vhost_stat_size
+- stat_offset);
+if (err < 0 || stat_offset + err > dev->vhost_stat_size) {
+goto fail;
+}
+stat_offset += err;
+}
+
+for (int q = 0; q < dev->up.n_txq; q++) {
+int qid = q * VIRTIO_QNUM;
+int err;
+
+err = rte_vhost_vring_stats_get(vid, qid,
+_stats[stat_offset],
+dev->vhost_stat_size
+- stat_offset);
+if (err < 0 || stat_offset + err > dev->vhost_stat_size) {
+goto fail;
+}
+stat_offset += err;
+}
+
+sw_stats_size = custom_stats->size;
+custom_stats->size += dev->vhost_stat_size;
+custom_stats->counters = xrealloc(custom_stats->counters,
+  custom_stats->size *
+  sizeof *custom_stats->counters);
+
+for (int i = 0; i < stat_offset; i++) {
+ovs_strlcpy(custom_stats->counters[sw_stats_size + i].name,
+dev->vhost_stat_names[i].name,
+NETDEV_CUSTOM_STATS_NAME_SIZE);
+custom_stats->counters[sw_stats_size + i].value =
+vhost_stats[i].value;
+}
+
+fail:
+free(vhost_stats);
+}
+
+ ovs_mutex_unlock(>mutex);
+#endif
+
+return 0;
+}
+
 static void
 netdev_dpdk_convert_xstats(struct netdev_stats *stats,
const struct rte_eth_xstat *xstats,
@@ -5014,12 +5099,107 @@ out:
 return err;
 }
 
+static void
+netdev_dpdk_vhost_clear_stats(struct netdev_dpdk *dev)
+OVS_REQUIRES(dev->mutex)
+{
+

[ovs-dev] [PATCH dpdk-latest v2 2/3] netdev-dpdk: Drop reference to Rx header split.

[David Marchand]

The Rx header split feature was implemented by no driver in DPDK.
This feature is being reworked, and one associated field has been removed
from the Rx port configuration (see [1]).

OVS was not using it, simply drop reference to this field.

1: https://git.dpdk.org/dpdk/commit/?id=8d54b1ec4a8b

Signed-off-by: David Marchand 
---
 lib/netdev-dpdk.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index fb4b3282dc..132ebb2843 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -160,7 +160,6 @@ typedef uint16_t dpdk_port_t;
 
 static const struct rte_eth_conf port_conf = {
 .rxmode = {
-.split_hdr_size = 0,
 .offloads = 0,
 },
 .rx_adv_conf = {
-- 
2.37.3

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

[ovs-dev] [PATCH dpdk-latest v2 0/3] API updates for DPDK 22.11

[David Marchand]

Hello,

This series has two parts.

First, it aligns OVS for some small updates on DPDK API side:
- make use of new API following hiding of DPDK internals,
- remove reference to some unused Rx feature in port configuration,

Then, a patch adds per vhost virtqueue statistics.
This last patch is not for merging atm, this is a RFC but it was sent
along the 22.11 fixes so that CI can run.


-- 
David Marchand

David Marchand (3):
  netdev-dpdk: Report device bus specific information.
  netdev-dpdk: Drop reference to Rx header split.
  netdev-dpdk: Add per virtqueue statistics.

 lib/netdev-dpdk.c | 227 +-
 1 file changed, 202 insertions(+), 25 deletions(-)

-- 
2.37.3

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

[ovs-dev] [PATCH dpdk-latest v2 1/3] netdev-dpdk: Report device bus specific information.

[David Marchand]

22.11 dropped direct access to bus specific structures.
Instead, a new API has been added.

Report bus name and device bus specific information.

The difference looks like:
-driver_name=mlx5_pci, if_descr="DPDK 21.11.0 mlx5_pci"
+driver_name=mlx5_pci, if_descr="DPDK 22.11.0-rc0 mlx5_pci"

-pci-device_id="0x1019"
-pci-vendor_id="0x15b3"
+bus_info="bus_name=pci, vendor_id=15b3, device_id=1019"

Signed-off-by: David Marchand 
Acked-by: Sunil Pai G 
---
 lib/netdev-dpdk.c | 23 ---
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 0dd655507b..fb4b3282dc 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -26,9 +26,10 @@
 #include 
 #include 
 
-#include 
+#include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -3639,6 +3640,7 @@ netdev_dpdk_get_status(const struct netdev *netdev, 
struct smap *args)
 {
 struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
 struct rte_eth_dev_info dev_info;
+const char *bus_info;
 uint32_t link_speed;
 uint32_t dev_flags;
 
@@ -3651,19 +3653,8 @@ netdev_dpdk_get_status(const struct netdev *netdev, 
struct smap *args)
 rte_eth_dev_info_get(dev->port_id, _info);
 link_speed = dev->link.link_speed;
 dev_flags = *dev_info.dev_flags;
+bus_info = rte_dev_bus_info(dev_info.device);
 ovs_mutex_unlock(>mutex);
-const struct rte_bus *bus;
-const struct rte_pci_device *pci_dev;
-uint16_t vendor_id = RTE_PCI_ANY_ID;
-uint16_t device_id = RTE_PCI_ANY_ID;
-bus = rte_bus_find_by_device(dev_info.device);
-if (bus && !strcmp(bus->name, "pci")) {
-pci_dev = RTE_DEV_TO_PCI(dev_info.device);
-if (pci_dev) {
-vendor_id = pci_dev->id.vendor_id;
-device_id = pci_dev->id.device_id;
-}
-}
 ovs_mutex_unlock(_mutex);
 
 smap_add_format(args, "port_no", DPDK_PORT_ID_FMT, dev->port_id);
@@ -3687,8 +3678,10 @@ netdev_dpdk_get_status(const struct netdev *netdev, 
struct smap *args)
 smap_add_format(args, "if_type", "%"PRIu32, IF_TYPE_ETHERNETCSMACD);
 smap_add_format(args, "if_descr", "%s %s", rte_version(),
dev_info.driver_name);
-smap_add_format(args, "pci-vendor_id", "0x%x", vendor_id);
-smap_add_format(args, "pci-device_id", "0x%x", device_id);
+smap_add_format(args, "bus_info", "bus_name=%s%s%s",
+rte_bus_name(rte_dev_bus(dev_info.device)),
+bus_info != NULL ? ", " : "",
+bus_info != NULL ? bus_info : "");
 
 /* Not all link speeds are defined in the OpenFlow specs e.g. 25 Gbps.
  * In that case the speed will not be reported as part of the usual
-- 
2.37.3

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

[ovs-dev] [PATCH ovn] controller: improve buffered packets management

[Lorenzo Bianconi]

Improve buffered packet management in ovn-controller avoid useless loop
in run_buffered_binding routine and using datapath key and output port
key as buffered_packets_map hashmap hash. Add new selftest for buffered
packets.

Signed-off-by: Lorenzo Bianconi 
---
 controller/pinctrl.c | 119 -
 tests/system-ovn.at  | 124 +++
 2 files changed, 206 insertions(+), 37 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index 8859cb080..e5fe44daa 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -182,6 +182,7 @@ static void destroy_buffered_packets_map(void);
 static void
 run_buffered_binding(struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
  struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
+ struct ovsdb_idl_index *sbrec_port_binding_by_name,
  const struct hmap *local_datapaths)
 OVS_REQUIRES(pinctrl_mutex);
 
@@ -1430,6 +1431,9 @@ struct buffered_packets {
 struct in6_addr ip;
 struct eth_addr ea;
 
+uint64_t dp_key;
+uint64_t port_key;
+
 long long int timestamp;
 
 struct buffer_info data[BUFFER_QUEUE_DEPTH];
@@ -1556,13 +1560,12 @@ buffered_packets_map_gc(void)
 }
 
 static struct buffered_packets *
-pinctrl_find_buffered_packets(const struct in6_addr *ip, uint32_t hash)
+pinctrl_find_buffered_packets(uint64_t dp_key, uint64_t port_key,
+  uint32_t hash)
 {
 struct buffered_packets *qp;
-
-HMAP_FOR_EACH_WITH_HASH (qp, hmap_node, hash,
- _packets_map) {
-if (IN6_ARE_ADDR_EQUAL(>ip, ip)) {
+HMAP_FOR_EACH_WITH_HASH (qp, hmap_node, hash, _packets_map) {
+if (qp->dp_key == dp_key && qp->port_key == port_key) {
 return qp;
 }
 }
@@ -1575,19 +1578,13 @@ pinctrl_handle_buffered_packets(struct dp_packet 
*pkt_in,
 const struct match *md, bool is_arp)
 OVS_REQUIRES(pinctrl_mutex)
 {
-struct buffered_packets *bp;
-struct dp_packet *clone;
-struct in6_addr addr;
-
-if (is_arp) {
-addr = in6_addr_mapped_ipv4(htonl(md->flow.regs[0]));
-} else {
-ovs_be128 ip6 = hton128(flow_get_xxreg(>flow, 0));
-memcpy(, , sizeof addr);
-}
-
-uint32_t hash = hash_bytes(, sizeof addr, 0);
-bp = pinctrl_find_buffered_packets(, hash);
+uint64_t dp_key = ntohll(md->flow.metadata);
+uint64_t oport_key = md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0];
+uint32_t hash = hash_add(
+hash_bytes(_key, sizeof oport_key, 0),
+dp_key);
+struct buffered_packets *bp
+= pinctrl_find_buffered_packets(dp_key, oport_key, hash);
 if (!bp) {
 if (hmap_count(_packets_map) >= 1000) {
 COVERAGE_INC(pinctrl_drop_buffered_packets_map);
@@ -1597,12 +1594,20 @@ pinctrl_handle_buffered_packets(struct dp_packet 
*pkt_in,
 bp = xmalloc(sizeof *bp);
 hmap_insert(_packets_map, >hmap_node, hash);
 bp->head = bp->tail = 0;
-bp->ip = addr;
+if (is_arp) {
+bp->ip = in6_addr_mapped_ipv4(htonl(md->flow.regs[0]));
+} else {
+ovs_be128 ip6 = hton128(flow_get_xxreg(>flow, 0));
+memcpy(>ip, , sizeof bp->ip);
+}
+bp->dp_key = dp_key;
+bp->port_key = oport_key;
 }
 bp->timestamp = time_msec();
 /* clone the packet to send it later with correct L2 address */
-clone = dp_packet_clone_data(dp_packet_data(pkt_in),
- dp_packet_size(pkt_in));
+struct dp_packet *clone
+= dp_packet_clone_data(dp_packet_data(pkt_in),
+   dp_packet_size(pkt_in));
 buffered_push_packet(bp, clone, md);
 
 return 0;
@@ -3586,6 +3591,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
   sbrec_ip_multicast_opts);
 run_buffered_binding(sbrec_mac_binding_by_lport_ip,
  sbrec_port_binding_by_datapath,
+ sbrec_port_binding_by_name,
  local_datapaths);
 sync_svc_monitors(ovnsb_idl_txn, svc_mon_table, sbrec_port_binding_by_name,
   chassis);
@@ -4354,20 +4360,34 @@ run_put_mac_bindings(struct ovsdb_idl_txn 
*ovnsb_idl_txn,
 }
 }
 
+static struct buffered_packets *
+pinctrl_get_buffered_packets(uint64_t dp_key, uint64_t port_key)
+{
+uint32_t hash = hash_add(
+hash_bytes(_key, sizeof port_key, 0),
+dp_key);
+return pinctrl_find_buffered_packets(dp_key, port_key, hash);
+}
+
 static void
 run_buffered_binding(struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
  struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
+ struct ovsdb_idl_index *sbrec_port_binding_by_name,
  const struct hmap *local_datapaths)
 

Re: [ovs-dev] [PATCH v3 3/3] netdev-offload-dpdk: Enhance the support of tunnel pop action

[0-day Robot]

References:  <20221007103923.228716-4-simon.hor...@corigine.com>
 

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

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


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Simon Horman 
Lines checked: 55, Warnings: 1, Errors: 0


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

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

[ovs-dev] [OVN v10] OVN - Add Support for Remote Port Mirroring

[Abhiram R N]

Added changes in ovn-nbctl, ovn-sbctl, northd and in ovn-controller.
While Mirror creation just creates the mirror, the lsp-attach-mirror
triggers the sequence to create Mirror in OVS DB on compute node.
OVS already supports Port Mirroring.

Note: This is targeted to mirror to destinations anywhere outside the
cluster where the analyser resides and it need not be an OVN node.

Example commands are as below:

Mirror creation
ovn-nbctl mirror-add mirror1 gre 0 from-lport 10.10.10.2

Attach a logical port to the mirror.
ovn-nbctl lsp-attach-mirror sw0-port1 mirror1

Detach a source from Mirror
ovn-nbctl lsp-detach-mirror sw0-port1 mirror1

Mirror deletion
ovn-nbctl mirror-del mirror1

Co-authored-by: Veda Barrenkala 
Signed-off-by: Veda Barrenkala 
Signed-off-by: Abhiram R N 
---
V9 --> V10: Reverted typecasting in mirror.c as its needed.

V8 --> V9: Updated NEWS file which I missed earlier.
   Reverted the typcasting in ovn-nbctl.c as its needed.

V7 --> V8: Addressed review comments from V7 by Ihar
   i) Added new test case for Mirror bulk updates in ovn.at
  ii) Addressed the suggestions provided in mirror.c
 iii) Minor spacing correction in ovn-controller.c
  iv) Removed an un-required typcasting ovn-nbctl.c

Files modified (V7 --> v8):
Code --> mirror.c, ovn-controller.c, ovn-nbctl.c
Test --> ovn.at

Ihar,
Regarding 1 specific comment in v7(mirror.c) regarding if-else-if
to be merged with another if check in that function above.
This cant be changed as the the 2 checks are different.
In one we are checking the previous value in OVS to take decision if a
change is needed or not.
And the other we are checking new value from SB and taking action

 NEWS|   1 +
 controller/automake.mk  |   4 +-
 controller/mirror.c | 527 
 controller/mirror.h |  53 
 controller/ovn-controller.c | 266 --
 northd/en-northd.c  |   4 +
 northd/inc-proc-northd.c|   4 +
 northd/northd.c | 172 
 northd/northd.h |   2 +
 ovn-nb.ovsschema|  31 ++-
 ovn-nb.xml  |  63 +
 ovn-sb.ovsschema|  26 +-
 ovn-sb.xml  |  50 
 tests/ovn-nbctl.at  | 116 
 tests/ovn-northd.at | 102 +++
 tests/ovn.at| 347 
 utilities/ovn-nbctl.c   | 357 
 utilities/ovn-sbctl.c   |   4 +
 18 files changed, 2101 insertions(+), 28 deletions(-)
 create mode 100644 controller/mirror.c
 create mode 100644 controller/mirror.h

diff --git a/NEWS b/NEWS
index 224a7b83e..aa7172eb0 100644
--- a/NEWS
+++ b/NEWS
@@ -25,6 +25,7 @@ OVN v22.09.0 - 16 Sep 2022
 any of LR's LRP IP, there is no need to create SNAT entry.  Now such
 traffic destined to LRP IP is not dropped.
   - Bump python version required for building OVN to 3.6.
+  - Added Support for Remote Port Mirroring
 
 OVN v22.06.0 - 03 Jun 2022
 --
diff --git a/controller/automake.mk b/controller/automake.mk
index c2ab1bbe6..334672b4d 100644
--- a/controller/automake.mk
+++ b/controller/automake.mk
@@ -41,7 +41,9 @@ controller_ovn_controller_SOURCES = \
controller/ovsport.h \
controller/ovsport.c \
controller/vif-plug.h \
-   controller/vif-plug.c
+   controller/vif-plug.c \
+   controller/mirror.h \
+   controller/mirror.c
 
 controller_ovn_controller_LDADD = lib/libovn.la $(OVS_LIBDIR)/libopenvswitch.la
 man_MANS += controller/ovn-controller.8
diff --git a/controller/mirror.c b/controller/mirror.c
new file mode 100644
index 0..a539c8ddc
--- /dev/null
+++ b/controller/mirror.c
@@ -0,0 +1,527 @@
+/* Copyright (c) 2022 Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+
+/* library headers */
+#include "lib/sset.h"
+#include "lib/util.h"
+
+/* OVS includes. */
+#include "lib/vswitch-idl.h"
+#include "openvswitch/vlog.h"
+
+/* OVN includes. */
+#include "binding.h"
+#include "lib/ovn-sb-idl.h"
+#include "mirror.h"
+
+VLOG_DEFINE_THIS_MODULE(port_mirror);
+
+/* Static function declarations */
+
+static const struct ovsrec_port *
+get_port_for_iface(const struct ovsrec_interface *iface,
+  const struct ovsrec_bridge *br_int)
+{
+for (size_t i = 0; i < br_int->n_ports; i++) {
+const struct ovsrec_port *p = 

Re: [ovs-dev] [PATCH 0/2] Remove deprecated OpenSSL functions on openssl 3.0

[Ilya Maximets]

On 9/22/22 15:40, Timothy Redaelli wrote:
> Currently, it's not possible to build OVS using OpenSSL 3.0 with
> --enable-Werror since OpenSSL 3.0 deprecated some functions.
> Moreover, it's not possible to generate dhparams.c anymore on
> OpenSSL 3.0 since -C option was removed from openssl dhparam tool.
> 
> With this series, it's possible to generate lib/dhparams.c using OpenSSL 3.0
> and to replace the deprecated function by using the new one.
> 
> OpenSSL team also suggests using SSL_CTX_set_dh_auto to set DH keys instead of
> use build-time hardcoded keys (it's also a good idea for FIPS compliance).
> 
> Timothy Redaelli (2):
>   dhparams: Fix .c file generation with OpenSSL >= 3.0
>   Add support for openssl 3.0 functions
> 
>  build-aux/generate-dhparams-c | 81 +++
>  lib/dhparams.c|  2 +
>  lib/stream-ssl.c  | 12 ++
>  3 files changed, 87 insertions(+), 8 deletions(-)
> 

Thanks!  Applied and backported down to 2.17 to avoid build
issues on LTS branch.

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

Re: [ovs-dev] [PATCH v3 2/3] netdev-offload-dpdk: Support offload of set IPv6 DSCP action

[0-day Robot]

References:  <20221007103923.228716-3-simon.hor...@corigine.com>
 

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

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


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Simon Horman 
Lines checked: 64, Warnings: 1, Errors: 0


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

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

Re: [ovs-dev] [PATCH v3 1/3] netdev-offload-dpdk: Support offload of set IPv4 DSCP action

[0-day Robot]

References:  <20221007103923.228716-2-simon.hor...@corigine.com>
 

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

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


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors 
or committers: Simon Horman 
Lines checked: 85, Warnings: 1, Errors: 0


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

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

Re: [ovs-dev] [ovs-dev v3 3/4] ofproto-dpif-upcall: new ukey needs to take the old ukey's dump seq

[Eelco Chaudron]

Hi hepeng,

I tried your changes, but I also do get a lot of zero packet failures on 
test 50. Not sure what happened, maybe it was related to syncing to the 
latest master.


However I focused on test 49, and I can replicate it every x test, but 
within 40 tries:


## -- ##
## openvswitch 3.0.90 test suite. ##
## -- ##
 49: datapath - truncate and output to gre tunnel by simulated packets 
FAILED (ovs-macros.at:247)


...
...
system-traffic.at:1663: waiting until ovs-ofctl dump-flows br-underlay | 
grep "in_port=LOCAL" | sed -n 's/.*\(n\_bytes=[0-9]*\).*/\1/p' | grep 
"n_bytes=138"...

system-traffic.at:1663: wait failed after 30 seconds
n_bytes=18446744073707612670
./ovs-macros.at:247: hard failure


I’ll try to dig into this later once I get some time...

//Eelco


On 2 Oct 2022, at 8:40, Peng He wrote:


Hi,

I have tried test #50 for 200 times, the overflow as well as stats of 
0 did

not show up.

the command line is:
for i in {1..100}; do echo $i; make check-offloads TESTSUITEFLAGS="-v 
50"

|| break; done

I am using Debian 10, and my kernel version is
root@ubuntu:~/ovs# uname -r
5.4.0-121-generic

I doubt the overflow might be related to kernel datapath.
IIRC, the kernel datapath does not support some IPv6 ND/NA match.

In the logs, there are some warning saying that "failed to get/del/put 
some

datapath flow", which is all
related the ICMPv6. It seems to prove the missing of the support of 
NA/ND

match in datapath.

since the ICMPv6 is not related to the test itself. I did some 
modification

to the test to drop all IPv6 traffic.

Below is my complete diff to the #50:

diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index dfbc30e47..b759c4bb9 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -1762,7 +1762,7 @@ on_exit 'rm -f payload200.bin'

 AT_CHECK([ovs-ofctl del-flows br0])
 AT_DATA([flows.txt], [dnl
-priority=99,in_port=1,actions=output(port=2,max_len=100),output(port=3,max_len=100)
+priority=99,in_port=1,ip,actions=output(port=2,max_len=100),output(port=3,max_len=100)
 priority=99,in_port=2,udp,actions=output(port=1,max_len=100)
 priority=1,in_port=4,ip,actions=drop
 priority=1,actions=drop
@@ -1771,6 +1771,8 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt])

 AT_CHECK([ovs-ofctl del-flows br-underlay])
 AT_DATA([flows-underlay.txt], [dnl
+priority=99,arp,in_port=1,actions=LOCAL
+priority=99,arp,in_port=LOCAL,actions=1
 priority=99,dl_type=0x0800,nw_proto=47,in_port=1,actions=LOCAL
 priority=99,dl_type=0x0800,nw_proto=47,in_port=LOCAL,ip_dst=
172.31.1.1/24,actions=1
 priority=1,actions=drop
@@ -1787,7 +1789,7 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep 
"in_port=2"

| sed -n 's/.*\(n\_bytes=[
 n_bytes=242
 ])
 dnl After truncation = outer ETH(14) + outer IP(20) + GRE(4) + 100 = 
138B
-AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | 
sed -n

's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
+AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "ip,in_port=LOCAL" 
| sed

-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=138
 ])

@@ -1822,7 +1824,7 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep 
"in_port=2"

| sed -n 's/.*\(n\_bytes=[
 n_bytes=242
 ])
 dnl After truncation = outer ETH(14) + outer IP(20) + GRE(4) + 100 = 
138B
-AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "in_port=LOCAL" | 
sed -n

's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
+AT_CHECK([ovs-ofctl dump-flows br-underlay | grep "ip,in_port=LOCAL" 
| sed

-n 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl
 n_bytes=138
 ])

@@ -1834,7 +1836,11 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep
"in_port=4" | ofctl_strip], [0], [dnl
  n_packets=1, n_bytes=100, priority=1,ip,in_port=4 actions=drop
 ])

-OVS_TRAFFIC_VSWITCHD_STOP
+OVS_TRAFFIC_VSWITCHD_STOP(["dnl
+/.*lost packet on port.*/d
+/.failed to flow_get.*/d
+/.failed to flow_del.*/d
+/.*Failed to acquire udpif_key corresponding to unexpected 
flow.*/d"])

 AT_CLEANUP


It seems that the overflow here is not related to the race in 
revalidators,

but it is related to the design of
the testsuite and also the missing of the support of NA/ND in kernel
datapath.



Eelco Chaudron  于2022年9月30日周五 
23:47写道：



On 30 Sep 2022, at 17:41, Peng He wrote:


It's so easy to reproduce ?

Thanks! then I should have to dig it again.


It’s been on my to-do for a while but did not get to it. Sometimes 
it

reproduced easily, and sometimes it takes 100+ runs :(

If you apply the following series:

https://patchwork.ozlabs.org/project/openvswitch/list/?series=316861

And then the diff below you can run something like:

  sudo bash -c 'for i in {1..100}; do make check-offloads
TESTSUITEFLAGS="49 50"

 49: datapath - truncate and output to gre tunnel by simulated 
packets ok

 50: datapath - truncate and output to gre tunnelok

And after a while, it will fail with the error (sometimes also 0 
bytes, I

hope it’s the same issue :):

system-traffic.at:1663: wait failed after 30 

[ovs-dev] [PATCH v3 2/3] netdev-offload-dpdk: Support offload of set IPv6 DSCP action

[Simon Horman]

From: Chaoyong He 

Add the support of offload of set IPv6 DSCP action.

Signed-off-by: Chaoyong He 
Reviewed-by: Louis Peens 
Signed-off-by: Simon Horman 
---
 lib/netdev-offload-dpdk.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 9cbd472fa81d..2819ebd1aa69 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -771,7 +771,8 @@ dump_flow_action(struct ds *s, struct ds *s_extra,
   IP_ARGS(set_ipv4->ipv4_addr));
 }
 ds_put_cstr(s, "/ ");
-} else if (actions->type == RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP) {
+} else if (actions->type == RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP ||
+   actions->type == RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP) {
 const struct rte_flow_action_set_dscp *set_dscp = actions->conf;
 
 ds_put_cstr(s, "set_dscp ");
@@ -1840,6 +1841,13 @@ add_set_flow_action__(struct flow_actions *actions,
 *(uint8_t *) mask == RTE_IPV4_HDR_ECN_MASK)
 goto out;
 break;
+case RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP:
+if (*(uint8_t *) mask == RTE_IPV6_HDR_DSCP_MASK >>
+RTE_IPV6_HDR_TC_SHIFT ||
+*(uint8_t *) mask == RTE_IPV6_HDR_ECN_MASK >>
+RTE_IPV6_HDR_TC_SHIFT)
+goto out;
+break;
 default:
 break;
 }
@@ -1876,6 +1884,8 @@ BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_ipv6) 
==
   MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_src));
 BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_ipv6) ==
   MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_dst));
+BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_dscp) ==
+  MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_tclass));
 BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_ttl) ==
   MEMBER_SIZEOF(struct ovs_key_ipv6, ipv6_hlimit));
 BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_tp) ==
@@ -1934,6 +1944,8 @@ parse_set_actions(struct flow_actions *actions,
 
 add_set_flow_action(ipv6_src, RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC);
 add_set_flow_action(ipv6_dst, RTE_FLOW_ACTION_TYPE_SET_IPV6_DST);
+add_set_flow_action(ipv6_tclass,
+RTE_FLOW_ACTION_TYPE_SET_IPV6_DSCP);
 add_set_flow_action(ipv6_hlimit, RTE_FLOW_ACTION_TYPE_SET_TTL);
 
 if (mask && !is_all_zeros(mask, sizeof *mask)) {
-- 
2.30.2

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

[ovs-dev] [PATCH v3 1/3] netdev-offload-dpdk: Support offload of set IPv4 DSCP action

[Simon Horman]

From: Chaoyong He 

Both the 'mod_nw_tos' and 'mod_nw_enc' command of ovs will cause
the 'RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP' be sent to PMD, and they
share one mask 'ipv4_tos' of size 1 byte.

According the original logic, both of them won't pass the mask
check because the check is based on size of byte. And the same
situation of shared mask will also won't pass this check in the
furture.

Add the support of offload of set IPv4 DSCP action.

Signed-off-by: Chaoyong He 
Reviewed-by: Louis Peens 
Signed-off-by: Simon Horman 
---
 lib/netdev-offload-dpdk.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 80a64a6cc06a..9cbd472fa81d 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -771,6 +771,14 @@ dump_flow_action(struct ds *s, struct ds *s_extra,
   IP_ARGS(set_ipv4->ipv4_addr));
 }
 ds_put_cstr(s, "/ ");
+} else if (actions->type == RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP) {
+const struct rte_flow_action_set_dscp *set_dscp = actions->conf;
+
+ds_put_cstr(s, "set_dscp ");
+if (set_dscp) {
+ds_put_format(s, "dscp_value %d ", set_dscp->dscp);
+}
+ds_put_cstr(s, "/ ");
 } else if (actions->type == RTE_FLOW_ACTION_TYPE_SET_TTL) {
 const struct rte_flow_action_set_ttl *set_ttl = actions->conf;
 
@@ -1826,11 +1834,21 @@ add_set_flow_action__(struct flow_actions *actions,
 return 0;
 }
 if (!is_all_ones(mask, size)) {
+switch (attr) {
+case RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP:
+if (*(uint8_t *) mask == RTE_IPV4_HDR_DSCP_MASK ||
+*(uint8_t *) mask == RTE_IPV4_HDR_ECN_MASK)
+goto out;
+break;
+default:
+break;
+}
 VLOG_DBG_RL(, "Partial mask is not supported");
 return -1;
 }
 }
 
+out:
 spec = xzalloc(size);
 memcpy(spec, value, size);
 add_flow_action(actions, attr, spec);
@@ -1850,6 +1868,8 @@ BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_ipv4) 
==
   MEMBER_SIZEOF(struct ovs_key_ipv4, ipv4_src));
 BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_ipv4) ==
   MEMBER_SIZEOF(struct ovs_key_ipv4, ipv4_dst));
+BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_dscp) ==
+  MEMBER_SIZEOF(struct ovs_key_ipv4, ipv4_tos));
 BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_ttl) ==
   MEMBER_SIZEOF(struct ovs_key_ipv4, ipv4_ttl));
 BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_ipv6) ==
@@ -1901,6 +1921,7 @@ parse_set_actions(struct flow_actions *actions,
 
 add_set_flow_action(ipv4_src, RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC);
 add_set_flow_action(ipv4_dst, RTE_FLOW_ACTION_TYPE_SET_IPV4_DST);
+add_set_flow_action(ipv4_tos, RTE_FLOW_ACTION_TYPE_SET_IPV4_DSCP);
 add_set_flow_action(ipv4_ttl, RTE_FLOW_ACTION_TYPE_SET_TTL);
 
 if (mask && !is_all_zeros(mask, sizeof *mask)) {
-- 
2.30.2

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

[ovs-dev] [PATCH v3 3/3] netdev-offload-dpdk: Enhance the support of tunnel pop action

[Simon Horman]

From: Chaoyong He 

We should try our best to fill all the fields of 'struct rte_flow_tunnel',
rather than just give PMD what it need for now.

The number and logic of PMD are increase everyday, and they can use any
field in the API I think.

For nfp PMD, to implement the offload of tunnel pop action, use the
value of 'is_ipv6' field of 'struct rte_flow_tunnel' is natural and simple.
Of course it can scan and parse the pattern array to get this information,
but that will increase complex logic.

Fixes: be56e063d028 ("netdev-offload-dpdk: Support tunnel pop action.")
Signed-off-by: Chaoyong He 
Reviewed-by: Louis Peens 
Signed-off-by: Simon Horman 
---
 lib/netdev-offload-dpdk.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 2819ebd1aa69..7c8d2e536f6d 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -1108,12 +1108,18 @@ vport_to_rte_tunnel(struct netdev *vport,
 const struct netdev_tunnel_config *tnl_cfg;
 
 memset(tunnel, 0, sizeof *tunnel);
+
+tnl_cfg = netdev_get_tunnel_config(vport);
+if (!tnl_cfg) {
+return -1;
+}
+
+if (!IN6_IS_ADDR_V4MAPPED(_cfg->ipv6_dst)) {
+tunnel->is_ipv6 = true;
+}
+
 if (!strcmp(netdev_get_type(vport), "vxlan")) {
 tunnel->type = RTE_FLOW_ITEM_TYPE_VXLAN;
-tnl_cfg = netdev_get_tunnel_config(vport);
-if (!tnl_cfg) {
-return -1;
-}
 tunnel->tp_dst = tnl_cfg->dst_port;
 if (!VLOG_DROP_DBG()) {
 ds_put_format(s_tnl, "flow tunnel create %d type vxlan; ",
-- 
2.30.2

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

[ovs-dev] [PATCH v3 0/3] add functions about rte_flow to ovs-dpdk

[Simon Horman]

This patch series add some logics about rte_flow to ovs-dpdk, includes:

* Support offload of set IPv4/IPv6 DSCP action
* Enhanced the support of tunnel pop action

Changes since v2
* Revise commit messages
* Add the fix tag to patch 3/3
* Revise dscp masking to check for correct partial mask
* Drop geneve vport patch, differed as future work

Changes since v1
* Address checkpatch warnings
* Drop the geneve decap patch, planning to along with the option support in the 
future

Chaoyong He (3):
  netdev-offload-dpdk: Support offload of set IPv4 DSCP action
  netdev-offload-dpdk: Support offload of set IPv6 DSCP action
  netdev-offload-dpdk: Enhance the support of tunnel pop action

 lib/netdev-offload-dpdk.c | 47 +++
 1 file changed, 43 insertions(+), 4 deletions(-)

-- 
2.30.2

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

Re: [ovs-dev] [PATCH v3] daemon-unix: Fix file descriptor leak when monitor restarts child

[Ilya Maximets]

On 9/30/22 03:09, Fengqi Li wrote:
> When segmentation fault occured in ovn-northd, monitor will try to
> restart the ovn-northd daemon process every 10s.
> Assume the following scenarios: There is a segmentation fault and
> the ovn-northd daemon process doen not restart properly everytime.
> New fds are created each time the ovn-northd daemon process is
> restarted by the monitor process, but old fds(fd[1]) ownered by
> the monitor process was not closed properly. One pipe leak for
> each restart of the ovn-northd daemon process. After a long time
> the OS's pipe was exhausted.
> Fixes: e2ed6fbeb18c ("fatal-signal: Catch SIGSEGV and print backtrace.")
> 
> Signed-off-by: Fengqi Li 
> ---
>  lib/daemon-unix.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
> index 52f3d4bc6..1a7ba427d 100644
> --- a/lib/daemon-unix.c
> +++ b/lib/daemon-unix.c
> @@ -396,6 +396,8 @@ monitor_daemon(pid_t daemon_pid)
>  }
>  
>  log_received_backtrace(daemonize_fd);
> +close(daemonize_fd);
> +daemonize_fd = -1;
>  
>  /* Throttle restarts to no more than once every 10 seconds. 
> */
>  if (time(NULL) < last_restart + 10) {

Thanks!  Applied and backported down to 2.13.

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

Re: [ovs-dev] [PATCH] vconn: Allow ECONNREFUSED in refuse connection test

[Ilya Maximets]

On 9/28/22 11:00, Eelco Chaudron wrote:
> 
> 
> On 27 Sep 2022, at 18:04, Mike Pattrick wrote:
> 
>> The "tcp vconn - refuse connection" test may fail due to a Connection
>> Refused error. The network stack returns ECONNREFUSED on a reset
>> connection in SYN_SENT state and EPIPE or ECONNRESET in all other
>> cases.
>>
>>   2022-09-19T17:45:48Z|1|socket_util|INFO|0:127.0.0.1: listening on
>> port 34189
>>   2022-09-19T17:45:48Z|2|poll_loop|DBG|wakeup due to [POLLOUT][
>> POLLERR][POLLHUP] on fd 4 (127.0.0.1:47140<->) at ../lib/stream-fd.
>> c:153
>>   test-vconn: unexpected vconn_connect() return value 111 (Connection
>> refused)
>>   ../../tests/vconn.at:21: exit code was 1, expected 0
>>   530. vconn.at:21: 530. tcp vconn - refuse connection (vconn.at:21):
>> FAILED (vconn.at:21)
>>
>> This was observed from a CI system, and isn't a common case.
>>
>> Signed-off-by: Mike Pattrick 
> 
> Changes look fine to me. Ran the basic check, no problems were found.
> 
> Acked-by: Eelco Chaudron 

Thanks!  Applied and backported down to 2.13.

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

Re: [ovs-dev] [PATCH v2] sparse: Add a guard for netinet/ip6.h header on FreeBSD.

[Ilya Maximets]

On 10/6/22 17:53, Mike Pattrick wrote:
> On Mon, Sep 26, 2022 at 5:19 PM Ilya Maximets  wrote:
>>
>> Same as arpa/inet.h, the netinet/ip6.h on FreeBSD requires
>> netinet/in.h to be included first.  So, adding a similar guard.
>>
>> Also fixing one instance where this is not respected at the moment.
>>
>> We do have FreeBSD CI these days, but it is still nice to have
>> a more clear error message.
>>
>> Fixes: b2befd5bb2db ("sparse: Add guards to prevent FreeBSD-incompatible 
>> #include order.")
>> Signed-off-by: Ilya Maximets 
>> ---
>>
>> Version 2:
>>   - Switched the guard from sys/types.h to netinet/in.h since
>> struct ip6_addr is defined there.  Fixed one instance where
>> this is not respected at the moment.
>>
> 
> Looks reasonable to me.
> 
> Acked-by: Mike Pattrick 
> 

Applied.  Thanks!

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

Re: [ovs-dev] [PATCH ovn 5/8] controller: Add support for templated actions and matches.

[Dumitru Ceara]

On 10/7/22 04:03, Han Zhou wrote:
> On Fri, Sep 30, 2022 at 7:02 AM Dumitru Ceara  wrote:
>>
>> Expand SB.Template_Var records in two stages:
>> 1. first expand them to local values in match/action strings
>> 2. then reparse the expanded strings
>>
>> For the case when a lflow references a Template_Var also track
>> references (similar to the ones maintained for multicast groups, address
>> sets, port_groups, port bindings).
>>
>> Signed-off-by: Dumitru Ceara 
> 
> Hi Dumitru,
> 

Hi Han,

> In addition to the two-stage parsing concerns we are discussing in the

I'm doing some more targetted performance testing.  I'll update the
other thread when I have more data to share.

> other thread, I have some minor comments below. The major one is whether we
> should allow matching hostname or not.
> 
>> ---
>>  controller/lflow.c  |   59 +++-
>>  controller/lflow.h  |1
>>  controller/lport.c  |3
>>  controller/ofctrl.c |9 +
>>  controller/ofctrl.h |3
>>  controller/ovn-controller.c |  317
> +++
>>  include/ovn/expr.h  |4 -
>>  include/ovn/lex.h   |   14 +-
>>  lib/actions.c   |9 +
>>  lib/expr.c  |   18 ++
>>  lib/lex.c   |   55 +++
>>  lib/objdep.c|1
>>  lib/objdep.h|1
>>  lib/ovn-util.c  |7 +
>>  lib/ovn-util.h  |3
>>  tests/ovn.at|2
>>  tests/test-ovn.c|   16 ++
>>  utilities/ovn-trace.c   |   26 +++-
>>  18 files changed, 512 insertions(+), 36 deletions(-)
>>
>> diff --git a/controller/lflow.c b/controller/lflow.c
>> index f9f2e6286..ad58fcd3c 100644
>> --- a/controller/lflow.c
>> +++ b/controller/lflow.c
>> @@ -81,6 +81,8 @@ convert_match_to_expr(const struct sbrec_logical_flow *,
>>const struct local_datapath *ldp,
>>struct expr **prereqs, const struct shash
> *addr_sets,
>>const struct shash *port_groups,
>> +  const struct smap *template_vars,
>> +  struct sset *template_vars_ref,
>>struct objdep_mgr *, bool *pg_addr_set_ref);
>>  static void
>>  add_matches_to_flow_table(const struct sbrec_logical_flow *,
>> @@ -356,10 +358,15 @@ consider_lflow_for_added_as_ips__(
>>  .n_tables = LOG_PIPELINE_LEN,
>>  .cur_ltable = lflow->table_id,
>>  };
>> +struct sset template_vars_ref = SSET_INITIALIZER(_vars_ref);
>>  struct expr *prereqs = NULL;
>>  char *error;
>>
>> -error = ovnacts_parse_string(lflow->actions, , ,
> );
>> +char *actions_s =
> lexer_parse_template_string(xstrdup(lflow->actions),
> 
> It may be better to xstrdup inside the function when necessary, and use
> const char* as parameter.
> 

Definitely, I'll change it in v2.

>> +
>  l_ctx_in->template_vars,
>> +  _vars_ref);
>> +error = ovnacts_parse_string(actions_s, , , );
> 
> The two functions can be wrapped to a single function, because they will be
> called together again in consider_logical_flow__().
> 

I can do that.  I didn't do it initially because there's already quite
some overlap between consider_lflow_for_added_as_ips__() and
consider_logical_flow__().  But that's something for a different patch I
guess.

>> +free(actions_s);
>>  if (error) {
>>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>>  VLOG_WARN_RL(, "error parsing actions \"%s\": %s",
>> @@ -367,6 +374,7 @@ consider_lflow_for_added_as_ips__(
>>  free(error);
>>  ovnacts_free(ovnacts.data, ovnacts.size);
>>  ofpbuf_uninit();
>> +sset_destroy(_vars_ref);
>>  return true;
>>  }
>>
>> @@ -430,6 +438,8 @@ consider_lflow_for_added_as_ips__(
>>  struct expr *expr = convert_match_to_expr(lflow, ldp, ,
>>l_ctx_in->addr_sets,
>>l_ctx_in->port_groups,
>> +  l_ctx_in->template_vars,
>> +  _vars_ref,
>>l_ctx_out->lflow_deps_mgr,
> NULL);
>>  shash_replace((struct shash *)l_ctx_in->addr_sets, as_name, real_as);
>>  if (new_fake_as) {
>> @@ -501,6 +511,14 @@ done:
>>  ofpbuf_uninit();
>>  expr_destroy(expr);
>>  expr_matches_destroy();
>> +
>> +const char *tv_name;
>> +SSET_FOR_EACH (tv_name, _vars_ref) {
>> +objdep_mgr_add(l_ctx_out->lflow_deps_mgr, OBJDEP_TYPE_TEMPLATE,
>> +   tv_name, >header_.uuid);
>> +}
>> +sset_destroy(_vars_ref);
>> +
>>  return handled;
>>  }
>>
>> @@ -911,6 +929,8 @@ convert_match_to_expr(const struct sbrec_logical_flow
> *lflow,
>>struct expr **prereqs,
>>

Re: [ovs-dev] [OVN v9] OVN - Add Support for Remote Port Mirroring

[0-day Robot]

Bleep bloop.  Greetings Abhiram R N, I am a robot and I have tried out your 
patch.
Thanks for your contribution.

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


checkpatch:
WARNING: Line lacks whitespace around operator
#2148 FILE: utilities/ovn-nbctl.c:275:
  mirror-add NAME TYPE INDEX FILTER IP\n\

WARNING: Line lacks whitespace around operator
#2157 FILE: utilities/ovn-nbctl.c:284:
  mirror-del [NAME] remove mirrors\n\

WARNING: Line lacks whitespace around operator
#2158 FILE: utilities/ovn-nbctl.c:285:
  mirror-list   print mirrors\n\

WARNING: Line lacks whitespace around operator
#2167 FILE: utilities/ovn-nbctl.c:327:
  lsp-attach-mirror PORT MIRROR   attach source PORT to MIRROR\n\

WARNING: Line lacks whitespace around operator
#2168 FILE: utilities/ovn-nbctl.c:328:
  lsp-detach-mirror PORT MIRROR   detach source PORT from MIRROR\n\

Lines checked: 2567, Warnings: 5, Errors: 0


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

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

[ovs-dev] [OVN v9] OVN - Add Support for Remote Port Mirroring

[Abhiram R N]

Added changes in ovn-nbctl, ovn-sbctl, northd and in ovn-controller.
While Mirror creation just creates the mirror, the lsp-attach-mirror
triggers the sequence to create Mirror in OVS DB on compute node.
OVS already supports Port Mirroring.

Note: This is targeted to mirror to destinations anywhere outside the
cluster where the analyser resides and it need not be an OVN node.

Example commands are as below:

Mirror creation
ovn-nbctl mirror-add mirror1 gre 0 from-lport 10.10.10.2

Attach a logical port to the mirror.
ovn-nbctl lsp-attach-mirror sw0-port1 mirror1

Detach a source from Mirror
ovn-nbctl lsp-detach-mirror sw0-port1 mirror1

Mirror deletion
ovn-nbctl mirror-del mirror1

Co-authored-by: Veda Barrenkala 
Signed-off-by: Veda Barrenkala 
Signed-off-by: Abhiram R N 
---
V8 --> V9: Updated NEWS file which I missed earlier.
   Reverted the typcasting in ovn-nbctl.c as its needed.

 NEWS|   1 +
 controller/automake.mk  |   4 +-
 controller/mirror.c | 527 
 controller/mirror.h |  53 
 controller/ovn-controller.c | 266 --
 northd/en-northd.c  |   4 +
 northd/inc-proc-northd.c|   4 +
 northd/northd.c | 172 
 northd/northd.h |   2 +
 ovn-nb.ovsschema|  31 ++-
 ovn-nb.xml  |  63 +
 ovn-sb.ovsschema|  26 +-
 ovn-sb.xml  |  50 
 tests/ovn-nbctl.at  | 116 
 tests/ovn-northd.at | 102 +++
 tests/ovn.at| 347 
 utilities/ovn-nbctl.c   | 357 
 utilities/ovn-sbctl.c   |   4 +
 18 files changed, 2101 insertions(+), 28 deletions(-)
 create mode 100644 controller/mirror.c
 create mode 100644 controller/mirror.h

diff --git a/NEWS b/NEWS
index 224a7b83e..aa7172eb0 100644
--- a/NEWS
+++ b/NEWS
@@ -25,6 +25,7 @@ OVN v22.09.0 - 16 Sep 2022
 any of LR's LRP IP, there is no need to create SNAT entry.  Now such
 traffic destined to LRP IP is not dropped.
   - Bump python version required for building OVN to 3.6.
+  - Added Support for Remote Port Mirroring
 
 OVN v22.06.0 - 03 Jun 2022
 --
diff --git a/controller/automake.mk b/controller/automake.mk
index c2ab1bbe6..334672b4d 100644
--- a/controller/automake.mk
+++ b/controller/automake.mk
@@ -41,7 +41,9 @@ controller_ovn_controller_SOURCES = \
controller/ovsport.h \
controller/ovsport.c \
controller/vif-plug.h \
-   controller/vif-plug.c
+   controller/vif-plug.c \
+   controller/mirror.h \
+   controller/mirror.c
 
 controller_ovn_controller_LDADD = lib/libovn.la $(OVS_LIBDIR)/libopenvswitch.la
 man_MANS += controller/ovn-controller.8
diff --git a/controller/mirror.c b/controller/mirror.c
new file mode 100644
index 0..89f71acd0
--- /dev/null
+++ b/controller/mirror.c
@@ -0,0 +1,527 @@
+/* Copyright (c) 2022 Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include 
+#include 
+
+/* library headers */
+#include "lib/sset.h"
+#include "lib/util.h"
+
+/* OVS includes. */
+#include "lib/vswitch-idl.h"
+#include "openvswitch/vlog.h"
+
+/* OVN includes. */
+#include "binding.h"
+#include "lib/ovn-sb-idl.h"
+#include "mirror.h"
+
+VLOG_DEFINE_THIS_MODULE(port_mirror);
+
+/* Static function declarations */
+
+static const struct ovsrec_port *
+get_port_for_iface(const struct ovsrec_interface *iface,
+  const struct ovsrec_bridge *br_int)
+{
+for (size_t i = 0; i < br_int->n_ports; i++) {
+const struct ovsrec_port *p = br_int->ports[i];
+for (size_t j = 0; j < p->n_interfaces; j++) {
+if (!strcmp(iface->name, p->interfaces[j]->name)) {
+return p;
+}
+}
+}
+return NULL;
+}
+
+static bool
+mirror_create(const struct sbrec_port_binding *pb,
+  struct port_mirror_ctx *pm_ctx)
+{
+const struct ovsrec_mirror *mirror = NULL;
+
+if (pb->n_up && !pb->up[0]) {
+return true;
+}
+
+if (pb->chassis != pm_ctx->chassis_rec) {
+return true;
+}
+
+if (!pm_ctx->ovs_idl_txn) {
+return false;
+}
+
+
+VLOG_INFO("Mirror rule(s) present for %s ", pb->logical_port);
+/* Loop through the mirror rules */
+for (size_t i =0; i < pb->n_mirror_rules; i++) {
+/* check if the mirror already exists in OVS DB */
+