Re: [ovs-dev] [PATCH ovn v5] ovn-sbctl.c Add logical flows count numbers

[Alexey Roytman]

OK, will make it a separate command.

On Mon, May 24, 2021 at 8:44 PM Ben Pfaff  wrote:

> On Sat, May 22, 2021 at 10:56:13PM +0300, Alexey Roytman wrote:
> > From: Alexey Roytman 
> >
> > For big scale deployments, when number of logical flows can be 2M+,
> > sometimes users just need to know the total number of logical flows
> > and numbers of logical flows per table/per datapath.
> >
> > New command output
> > Datapath: "sw1" (4b1e53d8-9f0f-4768-b4a6-6cbc58a4bfda) pipeline: ingress
> >   table=0 (ls_in_port_sec_l2  ) lflows=2
> >   table=1 (ls_in_port_sec_ip  ) lflows=1
>
> If adding  "--count" to "dump-flows" makes it no longer list flows, I
> think it would be better to make it a "count-flows" command.  We have
> precedent for adding statistics to a command (rather than dropping the
> full output) by adding -s or --stats.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] ovsdb-tool: add db-set-election-timer command for clustered databases

[Ben Pfaff]

On Mon, May 24, 2021 at 02:59:58PM -0500, Dan Williams wrote:
> On Mon, 2021-05-24 at 10:49 -0700, Ben Pfaff wrote:
> > On Fri, May 21, 2021 at 02:25:52PM -0500, Dan Williams wrote:
> > > After creating the new cluster database write a raft entry that
> > > sets the desired election timer. This allows CMSes to set the
> > > election timer at cluster start and avoid an error-prone
> > > election timer modification process after the cluster is up.
> > > 
> > > Reported-at: https://bugzilla.redhat.com/1831778
> > > 
> > > Signed-off-by: Dan Williams 
> > > ---
> > > v2:
> > > - Address Ben's comments; add --help and manpage docs
> > > - Write raft record directly instead of using private raft.c
> > > functions
> > 
> > Hmm, I don't think this change was what I asked for.  I asked for a
> > new
> > option, like "--election-timer=TIMEOUT create-cluster DB CONTENTS
> > LOCAL".  Then, if we add some other new option later, we can type
> > "--foobar-option=QUUXWAG create-cluster DB CONTENTS LOCAL", and so
> > on.
> > This adds a new command, which is quite different.
> 
> Aha, then I completely misunderstood you. Will do v3.

Sorry about that!  Thanks for humoring me.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] ovsdb-tool: add db-set-election-timer command for clustered databases

[Dan Williams]

On Mon, 2021-05-24 at 10:49 -0700, Ben Pfaff wrote:
> On Fri, May 21, 2021 at 02:25:52PM -0500, Dan Williams wrote:
> > After creating the new cluster database write a raft entry that
> > sets the desired election timer. This allows CMSes to set the
> > election timer at cluster start and avoid an error-prone
> > election timer modification process after the cluster is up.
> > 
> > Reported-at: https://bugzilla.redhat.com/1831778
> > 
> > Signed-off-by: Dan Williams 
> > ---
> > v2:
> > - Address Ben's comments; add --help and manpage docs
> > - Write raft record directly instead of using private raft.c
> > functions
> 
> Hmm, I don't think this change was what I asked for.  I asked for a
> new
> option, like "--election-timer=TIMEOUT create-cluster DB CONTENTS
> LOCAL".  Then, if we add some other new option later, we can type
> "--foobar-option=QUUXWAG create-cluster DB CONTENTS LOCAL", and so
> on.
> This adds a new command, which is quite different.

Aha, then I completely misunderstood you. Will do v3.

Thanks,
Dan

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

Re: [ovs-dev] [PATCH 5/5] dpif-netlink: Fix send of uninitialized memory in ct limit requests.

[Ilya Maximets]

On 5/21/21 9:23 AM, Mark Gray wrote:
> On 20/05/2021 19:24, Ilya Maximets wrote:
>> On 5/20/21 7:46 PM, Ilya Maximets wrote:
>>> On 5/20/21 6:55 PM, Mark Gray wrote:
 On 04/04/2021 18:31, Ilya Maximets wrote:
> ct limit requests never initializes the whole 'struct ovs_zone_limit'
> sending uninitialized stack memory to kernel:
>
>  Syscall param sendmsg(msg.msg_iov[0]) points to uninitialised byte(s)
> at 0x5E23867: sendmsg (in /usr/lib64/libpthread-2.28.so)
> by 0x54F761: nl_sock_transact_multiple__ (netlink-socket.c:858)
> by 0x54FB6E: nl_sock_transact_multiple.part.9 (netlink-socket.c:1079)
> by 0x54FCC0: nl_sock_transact_multiple (netlink-socket.c:1044)
> by 0x54FCC0: nl_sock_transact (netlink-socket.c:1108)
> by 0x550B6F: nl_transact (netlink-socket.c:1804)
> by 0x53BEA2: dpif_netlink_ct_get_limits (dpif-netlink.c:3052)
> by 0x588B57: dpctl_ct_get_limits (dpctl.c:2178)
> by 0x586FF2: dpctl_unixctl_handler (dpctl.c:2870)
> by 0x52C241: process_command (unixctl.c:310)
> by 0x52C241: run_connection (unixctl.c:344)
> by 0x52C241: unixctl_server_run (unixctl.c:395)
> by 0x407526: main (ovs-vswitchd.c:128)
>   Address 0x10b87480 is 32 bytes inside a block of size 4,096 alloc'd
> at 0x4C30F0B: malloc (vg_replace_malloc.c:307)
> by 0x52CDE4: xmalloc (util.c:138)
> by 0x4F7E07: ofpbuf_init (ofpbuf.c:123)
> by 0x4F7E07: ofpbuf_new (ofpbuf.c:151)
> by 0x53BDE3: dpif_netlink_ct_get_limits (dpif-netlink.c:3025)
> by 0x588B57: dpctl_ct_get_limits (dpctl.c:2178)
> by 0x586FF2: dpctl_unixctl_handler (dpctl.c:2870)
> by 0x52C241: process_command (unixctl.c:310)
> by 0x52C241: run_connection (unixctl.c:344)
> by 0x52C241: unixctl_server_run (unixctl.c:395)
> by 0x407526: main (ovs-vswitchd.c:128)
>   Uninitialised value was created by a stack allocation
> at 0x46AAA0: ct_dpif_get_limits (ct-dpif.c:197)
>
> Fix that by using designated initializers that will clear all the
> non-specified fields.
>
> CC: Yi-Hung Wei 
> Fixes: 906ff9d229ee ("dpif-netlink: Implement conntrack zone limit")
> Signed-off-by: Ilya Maximets 
> ---
>  lib/dpif-netlink.c | 24 ++--
>  1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index ceb56c685..ee96f4011 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -2923,8 +2923,6 @@ dpif_netlink_ct_set_limits(struct dpif *dpif 
> OVS_UNUSED,
> const uint32_t *default_limits,
> const struct ovs_list *zone_limits)
>  {
> -struct ovs_zone_limit req_zone_limit;
> -
>  if (ovs_ct_limit_family < 0) {
>  return EOPNOTSUPP;
>  }
> @@ -2941,8 +2939,10 @@ dpif_netlink_ct_set_limits(struct dpif *dpif 
> OVS_UNUSED,
>  size_t opt_offset;
>  opt_offset = nl_msg_start_nested(request, 
> OVS_CT_LIMIT_ATTR_ZONE_LIMIT);
>  if (default_limits) {
> -req_zone_limit.zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE;
> -req_zone_limit.limit = *default_limits;
> +struct ovs_zone_limit req_zone_limit = {
> +.zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE,
> +.limit   = *default_limits,
> +};
>  nl_msg_put(request, _zone_limit, sizeof req_zone_limit);
>  }
>  
> @@ -2950,8 +2950,10 @@ dpif_netlink_ct_set_limits(struct dpif *dpif 
> OVS_UNUSED,
>  struct ct_dpif_zone_limit *zone_limit;
>  
>  LIST_FOR_EACH (zone_limit, node, zone_limits) {
> -req_zone_limit.zone_id = zone_limit->zone;
> -req_zone_limit.limit = zone_limit->limit;
> +struct ovs_zone_limit req_zone_limit = {
> +.zone_id = zone_limit->zone,
> +.limit   = zone_limit->limit,
> +};
>  nl_msg_put(request, _zone_limit, sizeof req_zone_limit);
>  }
>  }
> @@ -3035,8 +3037,9 @@ dpif_netlink_ct_get_limits(struct dpif *dpif 
> OVS_UNUSED,
>  size_t opt_offset = nl_msg_start_nested(request,
>  
> OVS_CT_LIMIT_ATTR_ZONE_LIMIT);
>  
> -struct ovs_zone_limit req_zone_limit;
> -req_zone_limit.zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE;
> +struct ovs_zone_limit req_zone_limit = {
> +.zone_id = OVS_ZONE_LIMIT_DEFAULT_ZONE,
> +};
>  nl_msg_put(request, _zone_limit, sizeof req_zone_limit);
>  
>  struct ct_dpif_zone_limit *zone_limit;
> @@ -3086,8 +3089,9 @@ dpif_netlink_ct_del_limits(struct dpif *dpif 
> OVS_UNUSED,
>  
>  

Re: [ovs-dev] [PATCH 4/5] ofproto-dpif: Fix use of uninitialized attributes of timeout policy.

[Ilya Maximets]

On 5/20/21 6:49 PM, Mark Gray wrote:
> On 04/04/2021 18:31, Ilya Maximets wrote:
>> 'cdtp' is allocated on a stack and it has uninitialized 'present'
>> field that specifies which attributes are actually set.  This
>> causes use of uninitialized attributes.
>>
>>  Conditional jump or move depends on uninitialised value(s)
>> at 0x539651: dpif_netlink_get_nl_tp_udp_attrs (dpif-netlink.c:3206)
>> by 0x539651: dpif_netlink_get_nl_tp_attrs (dpif-netlink.c:3234)
>> by 0x539651: dpif_netlink_ct_set_timeout_policy (dpif-netlink.c:3370)
>> by 0x42B615: ct_add_timeout_policy_to_dpif (ofproto-dpif.c:5421)
>> by 0x42B615: ct_set_zone_timeout_policy (ofproto-dpif.c:5525)
>> by 0x40BD98: ct_zones_reconfigure (bridge.c:756)
>> by 0x40BD98: datapath_reconfigure (bridge.c:804)
>> by 0x40D737: bridge_reconfigure (bridge.c:903)
>> by 0x411720: bridge_run (bridge.c:3331)
>> by 0x40751C: main (ovs-vswitchd.c:127)
>>
>> Clearing the whole structure to avoid this kind of problems.
>>
>> CC: Yi-Hung Wei 
>> Fixes: 993cae678bca ("ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy 
>> tables")
>> Signed-off-by: Ilya Maximets 
>> ---
>>  ofproto/ofproto-dpif.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index fd0b2fdea..47db9bb57 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -5413,6 +5413,8 @@ ct_add_timeout_policy_to_dpif(struct dpif *dpif,
>>  struct ct_dpif_timeout_policy cdtp;
>>  struct simap_node *node;
>>  
>> +memset(, 0, sizeof cdtp);
>> +
>>  cdtp.id = ct_tp->tp_id;
>>  SIMAP_FOR_EACH (node, _tp->tp) {
>>  ct_dpif_set_timeout_policy_attr_by_name(, node->name, 
>> node->data);
>>
> LGTM
> 
> Acked-by: Mark D. Gray 
> 

Thanks!  Applied to master 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 1/5] netdev-linux: Fix use of uninitialized LAG master name.

[Ilya Maximets]

On 5/20/21 6:43 PM, Mark Gray wrote:
> On 04/04/2021 18:31, Ilya Maximets wrote:
>> 'if_indextoname' may fail leaving the 'master_name' uninitialized:
>>
>>  Conditional jump or move depends on uninitialised value(s)
>> at 0x4C34329: strlen (vg_replace_strmem.c:459)
>> by 0x51C638: hash_string (hash.h:342)
>> by 0x51C638: hash_name (shash.c:28)
>> by 0x51CC51: shash_find (shash.c:231)
>> by 0x51CD38: shash_find_data (shash.c:245)
>> by 0x4A797F: netdev_from_name (netdev.c:2013)
>> by 0x544148: netdev_linux_update_lag (netdev-linux.c:676)
>> by 0x544148: netdev_linux_run (netdev-linux.c:769)
>> by 0x4A5997: netdev_run (netdev.c:186)
>> by 0x40752B: main (ovs-vswitchd.c:129)
>>   Uninitialised value was created by a stack allocation
>> at 0x543AFA: netdev_linux_run (netdev-linux.c:722)
>>
>> CC: John Hurley 
>> Fixes: d22f8927c3c9 ("netdev-linux: monitor and offload LAG slaves to TC")
>> Signed-off-by: Ilya Maximets 
>> ---
>>  lib/netdev-linux.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
>> index 15b25084b..2b8283e94 100644
>> --- a/lib/netdev-linux.c
>> +++ b/lib/netdev-linux.c
>> @@ -672,7 +672,9 @@ netdev_linux_update_lag(struct rtnetlink_change *change)
>>  uint32_t block_id;
>>  int error = 0;
>>  
>> -if_indextoname(change->master_ifindex, master_name);
>> +if (!if_indextoname(change->master_ifindex, master_name)) {
>> +return;
>> +}
>>  master_netdev = netdev_from_name(master_name);
>>  if (!master_netdev) {
>>  return;
>>
> Interesting that strlen() doesn't check for NULL. LGTM
> 
> Acked-by: Mark D. Gray 
> 

Thanks!  Applied to master and backported down to 2.12.

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

Re: [ovs-dev] [PATCH net-next v2] net: openvswitch: IPv6: Add IPv6 extension header support

[Ilya Maximets]

On 5/24/21 8:57 PM, Cpp Code wrote:
> Yes, these changes only works together with changes in userspace. I
> believe in any solution there should be corresponding changes in
> userspace. If we would be able to easily run old version of userspace
> with these changes in kernel without userspace complaining about
> struct size, we could get in to a situation with hard to find bugs.

You can't modify existing binaries and we can't expect that everyone
will get new version of OVS along with the kernel update.  Existing
binaries should work fine with any version of underlying kernel.
With this change applied, kernel will not be able to parse
OVS_KEY_ATTR_IPV6 sent from userspace by the older OVS and this OVS
will not be able to correctly parse netlink messages sent by the
kernel.

> 
> I don't agree with the solution of a new struct key as semantically
> ipv6 extension headers are integral part of every ipv6 packet thus
> expected to be in the struct along with label, for example. Correct if
> I am missing something.

Even though ipv6 extensions are part of ipv6, they never was part of
the user interface here.  I agree that original design of this structure
was not perfect, but breaking of the user interface, i.e. breaking all
the existing OVS binaries, is just not acceptable.

> 
> On Wed, May 19, 2021 at 2:52 AM Ilya Maximets  wrote:
>>
>> On 5/17/21 5:20 PM, Toms Atteka wrote:
>>> IPv6 extension headers carry optional internet layer information
>>> and are placed between the fixed header and the upper-layer
>>> protocol header.
>>>
>>> This change adds a new OpenFlow field OFPXMT_OFB_IPV6_EXTHDR and
>>> packets can be filtered using ipv6_ext flag.
>>>
>>> Tested-at: https://github.com/TomCodeLV/ovs/actions/runs/504185214
>>> Signed-off-by: Toms Atteka 
>>> ---
>>>  include/uapi/linux/openvswitch.h |   1 +
>>>  net/openvswitch/flow.c   | 141 +++
>>>  net/openvswitch/flow.h   |  14 +++
>>>  net/openvswitch/flow_netlink.c   |   5 +-
>>>  4 files changed, 160 insertions(+), 1 deletion(-)
>>>
>>>
>>> base-commit: 5d869070569a23aa909c6e7e9d010fc438a492ef
>>>
>>> diff --git a/include/uapi/linux/openvswitch.h 
>>> b/include/uapi/linux/openvswitch.h
>>> index 8d16744edc31..a19812b6631a 100644
>>> --- a/include/uapi/linux/openvswitch.h
>>> +++ b/include/uapi/linux/openvswitch.h
>>> @@ -420,6 +420,7 @@ struct ovs_key_ipv6 {
>>>   __u8   ipv6_tclass;
>>>   __u8   ipv6_hlimit;
>>>   __u8   ipv6_frag;   /* One of OVS_FRAG_TYPE_*. */
>>> + __u16  ipv6_exthdr;
>>>  };
>>
>> Wouldn't this break existing userspace?  Curent OVS expects netlink
>> message with attribute size equal to the old version of 'struct ovs_key_ipv6'
>> and it will discard OVS_KEY_ATTR_IPV6 as malformed.
>>
>> This should likely be a completely new structure and a completely new
>> OVS_KEY_ATTR.
>>
>> Best regards, Ilya Maximets.

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

Re: [ovs-dev] [PATCH v2 ovn 4/4] NEWS: Add CoPP support.

[0-day Robot]

Bleep bloop.  Greetings Lorenzo Bianconi, 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: Lorenzo Bianconi 
Lines checked: 26, 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 v2 ovn 2/4] ovn-northd: Add support for CoPP.

[0-day Robot]

Bleep bloop.  Greetings Lorenzo Bianconi, 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
#862 FILE: utilities/ovn-nbctl.c:430:
  ls-copp-add SWITCH PROTO METER\n\

WARNING: Line lacks whitespace around operator
#865 FILE: utilities/ovn-nbctl.c:433:
  ls-copp-del SWITCH [PROTO]\n\

WARNING: Line lacks whitespace around operator
#869 FILE: utilities/ovn-nbctl.c:437:
  ls-copp-list SWITCH\n\

WARNING: Line lacks whitespace around operator
#872 FILE: utilities/ovn-nbctl.c:440:
  lr-copp-add ROUTER PROTO METER\n\

WARNING: Line lacks whitespace around operator
#875 FILE: utilities/ovn-nbctl.c:443:
  lr-copp-del ROUTER [PROTO]\n\

WARNING: Line lacks whitespace around operator
#879 FILE: utilities/ovn-nbctl.c:447:
  lr-copp-list ROUTER\n\

Lines checked: 1057, Warnings: 6, 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 net-next v2] net: openvswitch: IPv6: Add IPv6 extension header support

[Cpp Code]

Yes, these changes only works together with changes in userspace. I
believe in any solution there should be corresponding changes in
userspace. If we would be able to easily run old version of userspace
with these changes in kernel without userspace complaining about
struct size, we could get in to a situation with hard to find bugs.

I don't agree with the solution of a new struct key as semantically
ipv6 extension headers are integral part of every ipv6 packet thus
expected to be in the struct along with label, for example. Correct if
I am missing something.

On Wed, May 19, 2021 at 2:52 AM Ilya Maximets  wrote:
>
> On 5/17/21 5:20 PM, Toms Atteka wrote:
> > IPv6 extension headers carry optional internet layer information
> > and are placed between the fixed header and the upper-layer
> > protocol header.
> >
> > This change adds a new OpenFlow field OFPXMT_OFB_IPV6_EXTHDR and
> > packets can be filtered using ipv6_ext flag.
> >
> > Tested-at: https://github.com/TomCodeLV/ovs/actions/runs/504185214
> > Signed-off-by: Toms Atteka 
> > ---
> >  include/uapi/linux/openvswitch.h |   1 +
> >  net/openvswitch/flow.c   | 141 +++
> >  net/openvswitch/flow.h   |  14 +++
> >  net/openvswitch/flow_netlink.c   |   5 +-
> >  4 files changed, 160 insertions(+), 1 deletion(-)
> >
> >
> > base-commit: 5d869070569a23aa909c6e7e9d010fc438a492ef
> >
> > diff --git a/include/uapi/linux/openvswitch.h 
> > b/include/uapi/linux/openvswitch.h
> > index 8d16744edc31..a19812b6631a 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -420,6 +420,7 @@ struct ovs_key_ipv6 {
> >   __u8   ipv6_tclass;
> >   __u8   ipv6_hlimit;
> >   __u8   ipv6_frag;   /* One of OVS_FRAG_TYPE_*. */
> > + __u16  ipv6_exthdr;
> >  };
>
> Wouldn't this break existing userspace?  Curent OVS expects netlink
> message with attribute size equal to the old version of 'struct ovs_key_ipv6'
> and it will discard OVS_KEY_ATTR_IPV6 as malformed.
>
> This should likely be a completely new structure and a completely new
> OVS_KEY_ATTR.
>
> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2 ovn 3/4] ovn-northd: Add CoPP policies for flows that punt packets to ovn-controller.

[Lorenzo Bianconi]

From: Dumitru Ceara 

Change the ovn-northd implementation to set the new 'controller_meter'
field for flows that need to punt packets to ovn-controller.

Protocol packets for which CoPP is enforced when sending packets to
ovn-controller (if configured):
- ARP
- ND_NS
- ND_NA
- ND_RA
- DNS
- IGMP
- packets that require ARP resolution before forwarding
- packets that require ND_NS before forwarding
- packets that need to be replied to with ICMP Errors
- packets that need to be replied to with TCP RST
- packets that need to be replied to with DHCP_OPTS
- packets that trigger a SCTP abort action
- contoller_events
- BFD

Co-authored-by: Lorenzo Bianconi 
Signed-off-by: Lorenzo Bianconi 
Signed-off-by: Dumitru Ceara 
---
 include/ovn/actions.h |   1 -
 lib/actions.c |  50 +---
 lib/copp.c|   1 +
 lib/copp.h|   1 +
 northd/ovn-northd.c   | 493 --
 ovn-nb.xml|   3 +
 tests/atlocal.in  |   3 +
 tests/ovn.at  |   4 +-
 tests/system-ovn.at   | 135 +++
 utilities/ovn-nbctl.8.xml |   3 +
 10 files changed, 469 insertions(+), 225 deletions(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index ab03df12c..b2f2f57c6 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -393,7 +393,6 @@ struct ovnact_controller_event {
 int event_type;   /* controller event type */
 struct ovnact_gen_option *options;
 size_t n_options;
-char *meter;
 };
 
 /* OVNACT_BIND_VPORT. */
diff --git a/lib/actions.c b/lib/actions.c
index 155b4a45a..f0291afef 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -1613,9 +1613,6 @@ format_TRIGGER_EVENT(const struct ovnact_controller_event 
*event,
 {
 ds_put_format(s, "trigger_event(event = \"%s\"",
   event_to_string(event->event_type));
-if (event->meter) {
-ds_put_format(s, ", meter = \"%s\"", event->meter);
-}
 for (const struct ovnact_gen_option *o = event->options;
  o < >options[event->n_options]; o++) {
 ds_put_cstr(s, ", ");
@@ -1790,24 +1787,11 @@ encode_event_empty_lb_backends_opts(struct ofpbuf 
*ofpacts,
 
 static void
 encode_TRIGGER_EVENT(const struct ovnact_controller_event *event,
- const struct ovnact_encode_params *ep OVS_UNUSED,
+ const struct ovnact_encode_params *ep,
  struct ofpbuf *ofpacts)
 {
-uint32_t meter_id = NX_CTLR_NO_METER;
-size_t oc_offset;
-
-if (event->meter) {
-meter_id = ovn_extend_table_assign_id(ep->meter_table, event->meter,
-  ep->lflow_uuid);
-if (meter_id == EXT_TABLE_ID_INVALID) {
-VLOG_WARN("Unable to assign id for trigger meter: %s",
-  event->meter);
-return;
-}
-}
-
-oc_offset = encode_start_controller_op(ACTION_OPCODE_EVENT, false,
-   meter_id, ofpacts);
+size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_EVENT, false,
+  ep->ctrl_meter_id, ofpacts);
 ovs_be32 ofs = htonl(event->event_type);
 ofpbuf_put(ofpacts, , sizeof ofs);
 
@@ -2341,27 +2325,12 @@ parse_trigger_event(struct action_context *ctx,
  sizeof *event->options);
 }
 
-if (lexer_match_id(ctx->lexer, "meter")) {
-if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
-return;
-}
-/* If multiple meters are given, use the most recent. */
-if (ctx->lexer->token.type == LEX_T_STRING &&
-strlen(ctx->lexer->token.s)) {
-free(event->meter);
-event->meter = xstrdup(ctx->lexer->token.s);
-} else if (ctx->lexer->token.type != LEX_T_STRING) {
-lexer_syntax_error(ctx->lexer, "expecting string");
-return;
-}
-lexer_get(ctx->lexer);
-} else {
-struct ovnact_gen_option *o = >options[event->n_options++];
-memset(o, 0, sizeof *o);
-parse_gen_opt(ctx, o,
->pp->controller_event_opts->event_opts[event_type],
-event_to_string(event_type));
-}
+struct ovnact_gen_option *o = >options[event->n_options++];
+memset(o, 0, sizeof *o);
+parse_gen_opt(ctx, o,
+  >pp->controller_event_opts->event_opts[event_type],
+  event_to_string(event_type));
+
 if (ctx->lexer->error) {
 return;
 }
@@ -2382,7 +2351,6 @@ static void
 ovnact_controller_event_free(struct ovnact_controller_event *event)
 {
 free_gen_options(event->options, event->n_options);
-free(event->meter);
 }
 
 static void
diff --git a/lib/copp.c b/lib/copp.c
index e3d14938a..bbe66924b 100644
--- a/lib/copp.c
+++ 

[ovs-dev] [PATCH v2 ovn 2/4] ovn-northd: Add support for CoPP.

[Lorenzo Bianconi]

From: Dumitru Ceara 

Add new 'Copp' (Control plane protection) table to OVN Northbound DB:
- this stores mappings between control plane protocol names and meters
  that should be used to rate limit controller-destined traffic for
  those protocols.

Add new 'copp' columns to the following OVN Northbound DB tables:
- Logical_Switch
- Logical_Router

For now, no control plane protection policy is installed for any of
the existing flows that punt packets to ovn-controller. This will be
added in follow-up patches.

Add CLI commands in 'ovn-nbctl' to allow the user to manage Control
Plane Protection Policies at different levels (logical switch,
logical router).

Co-authored-by: Lorenzo Bianconi 
Signed-off-by: Lorenzo Bianconi 
Signed-off-by: Dumitru Ceara 
---
 lib/automake.mk   |   2 +
 lib/copp.c| 142 ++
 lib/copp.h|  58 +
 northd/ovn-northd.c   |  44 +++---
 ovn-nb.ovsschema  |  16 +++-
 ovn-nb.xml|  78 +
 tests/ovn-controller.at   |  48 ++
 tests/ovn-northd.at   |  81 +
 utilities/ovn-nbctl.8.xml | 116 +
 utilities/ovn-nbctl.c | 178 ++
 10 files changed, 751 insertions(+), 12 deletions(-)
 create mode 100644 lib/copp.c
 create mode 100644 lib/copp.h

diff --git a/lib/automake.mk b/lib/automake.mk
index 781be2109..20e296fff 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -9,6 +9,8 @@ lib_libovn_la_SOURCES = \
lib/actions.c \
lib/chassis-index.c \
lib/chassis-index.h \
+   lib/copp.c \
+   lib/copp.h \
lib/ovn-dirs.h \
lib/expr.c \
lib/extend-table.h \
diff --git a/lib/copp.c b/lib/copp.c
new file mode 100644
index 0..e3d14938a
--- /dev/null
+++ b/lib/copp.c
@@ -0,0 +1,142 @@
+/* Copyright (c) 2021, 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 
+
+#include "openvswitch/shash.h"
+#include "db-ctl-base.h"
+#include "smap.h"
+#include "lib/ovn-nb-idl.h"
+#include "lib/copp.h"
+
+static char *copp_proto_names[COPP_PROTO_MAX] = {
+[COPP_ARP]   = "arp",
+[COPP_ARP_RESOLVE]   = "arp-resolve",
+[COPP_DHCPV4_OPTS]   = "dhcpv4-opts",
+[COPP_DHCPV6_OPTS]   = "dhcpv6-opts",
+[COPP_DNS]   = "dns",
+[COPP_EVENT_ELB] = "event-elb",
+[COPP_ICMP4_ERR] = "icmp4-error",
+[COPP_ICMP6_ERR] = "icmp6-error",
+[COPP_IGMP]  = "igmp",
+[COPP_ND_NA] = "nd-na",
+[COPP_ND_NS] = "nd-ns",
+[COPP_ND_NS_RESOLVE] = "nd-ns-resolve",
+[COPP_ND_RA_OPTS]= "nd-ra-opts",
+[COPP_TCP_RESET] = "tcp-reset",
+[COPP_BFD]   = "bfd",
+};
+
+static const char *
+copp_proto_get_name(enum copp_proto proto)
+{
+if (proto >= COPP_PROTO_MAX) {
+return "";
+}
+return copp_proto_names[proto];
+}
+
+const char *
+copp_meter_get(enum copp_proto proto, const struct nbrec_copp *copp,
+   const struct shash *meter_groups)
+{
+if (!copp || proto >= COPP_PROTO_MAX) {
+return NULL;
+}
+
+const char *meter = smap_get(>meters, copp_proto_names[proto]);
+
+if (meter && shash_find(meter_groups, meter)) {
+return meter;
+}
+
+return NULL;
+}
+
+void
+copp_meter_list(struct ctl_context *ctx, const struct nbrec_copp *copp)
+{
+if (!copp) {
+return;
+}
+
+struct smap_node *node;
+
+SMAP_FOR_EACH (node, >meters) {
+ds_put_format(>output, "%s: %s\n", node->key, node->value);
+}
+}
+
+const struct nbrec_copp *
+copp_meter_add(struct ctl_context *ctx, const struct nbrec_copp *copp,
+   const char *proto_name, const char *meter)
+{
+if (!copp) {
+copp = nbrec_copp_insert(ctx->txn);
+}
+
+struct smap meters;
+smap_init();
+smap_clone(, >meters);
+smap_replace(, proto_name, meter);
+nbrec_copp_set_meters(copp, );
+smap_destroy();
+
+return copp;
+}
+
+void
+copp_meter_del(const struct nbrec_copp *copp, const char *proto_name)
+{
+if (!copp) {
+return;
+}
+
+if (proto_name) {
+if (smap_get(>meters, proto_name)) {
+struct smap meters;
+smap_init();
+smap_clone(, >meters);
+smap_remove(, proto_name);
+nbrec_copp_set_meters(copp, );
+smap_destroy();
+}
+ 

[ovs-dev] [PATCH v2 ovn 1/4] ovn-controller: Add support for Logical_Flow control meters

[Lorenzo Bianconi]

From: Dumitru Ceara 

Add a new 'controller_meter' column to OVN Southbound Logical_Flow
table. This stores an optional string which should correspond to
the Meter that must be used for rate limiting controller actions
generated by packets hitting the flow.

Add a new 'ofctrl_add_flow_metred' function to create a new 'ovn_flow'
with an attached controller meter.

Change ofctrl_check_and_add_flow to allow specifying a meter ID for
packets that are punted to controller.

Change consider_logical_flow to parse controller_meter from the logical
flow and use it when building openflow entries.

Add a new 'ctrl_meter_id' field to 'struct ovnact_encode_params' to be
used when encoding controller actions from logical flow actions.

Co-authored-by: Lorenzo Bianconi 
Signed-off-by: Lorenzo Bianconi 
Signed-off-by: Dumitru Ceara 
---
 controller/lflow.c| 40 +++---
 controller/ofctrl.c   | 55 +---
 controller/ofctrl.h   | 21 ++
 controller/physical.c |  7 +++--
 include/ovn/actions.h |  2 ++
 lib/actions.c | 66 +++
 ovn-sb.ovsschema  |  6 ++--
 ovn-sb.xml|  6 
 8 files changed, 140 insertions(+), 63 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index 680b8cca1..ab7f3d286 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -557,6 +557,27 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
 return false;
 }
 
+static void
+lflow_parse_ctrl_meter(const struct sbrec_logical_flow *lflow,
+   struct ovn_extend_table *meter_table,
+   uint32_t *meter_id)
+{
+ovs_assert(meter_id);
+*meter_id = NX_CTLR_NO_METER;
+
+if (lflow->controller_meter) {
+*meter_id = ovn_extend_table_assign_id(meter_table,
+   lflow->controller_meter,
+   lflow->header_.uuid);
+if (*meter_id == EXT_TABLE_ID_INVALID) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
+VLOG_WARN_RL(, "Unable to assign id for meter: %s",
+ lflow->controller_meter);
+return;
+}
+}
+}
+
 static void
 add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
   const struct sbrec_datapath_binding *dp,
@@ -572,6 +593,13 @@ add_matches_to_flow_table(const struct sbrec_logical_flow 
*lflow,
 .dp = dp,
 };
 
+/* Parse any meter to be used if this flow should punt packets to
+ * controller.
+ */
+uint32_t ctrl_meter_id = NX_CTLR_NO_METER;
+lflow_parse_ctrl_meter(lflow, l_ctx_out->meter_table,
+   _meter_id);
+
 /* Encode OVN logical actions into OpenFlow. */
 uint64_t ofpacts_stub[1024 / 8];
 struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
@@ -595,6 +623,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow 
*lflow,
 .ct_snat_vip_ptable = OFTABLE_CT_SNAT_FOR_VIP,
 .fdb_ptable = OFTABLE_GET_FDB,
 .fdb_lookup_ptable = OFTABLE_LOOKUP_FDB,
+.ctrl_meter_id = ctrl_meter_id,
 };
 ovnacts_encode(ovnacts->data, ovnacts->size, , );
 
@@ -621,9 +650,11 @@ add_matches_to_flow_table(const struct sbrec_logical_flow 
*lflow,
 }
 }
 if (!m->n) {
-ofctrl_add_flow(l_ctx_out->flow_table, ptable, lflow->priority,
-lflow->header_.uuid.parts[0], >match, ,
->header_.uuid);
+ofctrl_add_flow_metered(l_ctx_out->flow_table, ptable,
+lflow->priority,
+lflow->header_.uuid.parts[0], >match,
+, >header_.uuid,
+ctrl_meter_id);
 } else {
 uint64_t conj_stubs[64 / 8];
 struct ofpbuf conj;
@@ -641,7 +672,8 @@ add_matches_to_flow_table(const struct sbrec_logical_flow 
*lflow,
 
 ofctrl_add_or_append_flow(l_ctx_out->flow_table, ptable,
   lflow->priority, 0,
-  >match, , >header_.uuid);
+  >match, , >header_.uuid,
+  ctrl_meter_id);
 ofpbuf_uninit();
 }
 }
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index c29c3d180..3293aa6b6 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -66,6 +66,7 @@ struct ovn_flow {
 struct ofpact *ofpacts;
 size_t ofpacts_len;
 uint64_t cookie;
+uint32_t ctrl_meter_id; /* Meter to be used for controller actions. */
 };
 
 /* A desired flow, in struct ovn_desired_flow_table, calculated by the
@@ -220,7 +221,8 @@ static struct desired_flow *desired_flow_alloc(
 uint16_t priority,
 uint64_t cookie,
 const struct 

[ovs-dev] [PATCH v2 ovn 4/4] NEWS: Add CoPP support.

[Lorenzo Bianconi]

From: Dumitru Ceara 

Signed-off-by: Dumitru Ceara 
Signed-off-by: Lorenzo Bianconi 
---
 NEWS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/NEWS b/NEWS
index f8aa81e56..37d016d87 100644
--- a/NEWS
+++ b/NEWS
@@ -23,6 +23,7 @@ Post-v21.03.0
   - Utilities:
 * ovn-nbctl daemon mode is no longer considered experimental.
 * ovn-sbctl now also supports daemon mode.
+  - Added Control Plane Protection support (control plane traffic metering).
 
 OVN v21.03.0 - 12 Mar 2021
 -
-- 
2.31.1

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

[ovs-dev] [PATCH v2 ovn 0/4] respin CoPP series

[Lorenzo Bianconi]

This series respin CoPP support introduced here [0] by Dumitru rebasing on top
of ovn master branch and adding some missing meters (e.g. bfd or acl reject).
The main goal of this series is to continue the discussion about the proposed
approach and to align on CMS APIs.
For the moment no ddlog support has been added.
Related bz:
https://bugzilla.redhat.com/show_bug.cgi?id=1947913
https://bugzilla.redhat.com/show_bug.cgi?id=1946610

Changes since v1:
- merge patch 3/5 and 4/5
- cosmetics
- improve naming conventions
- add more unit-tests/system-tests
- remove duplicated flow
- remove some leftover entries in ovn-nbctl.8.xml
- add metering for sctp abort packets

Changes since RFC:
- drop per-port metering
- add unit/system tests
- add reject action metering

[0] https://patchwork.ozlabs.org/project/openvswitch/list/?series=140778=*

Dumitru Ceara (4):
  ovn-controller: Add support for Logical_Flow control meters
  ovn-northd: Add support for CoPP.
  ovn-northd: Add CoPP policies for flows that punt packets to
ovn-controller.
  NEWS: Add CoPP support.

 NEWS  |   1 +
 controller/lflow.c|  40 ++-
 controller/ofctrl.c   |  55 ++--
 controller/ofctrl.h   |  21 +-
 controller/physical.c |   7 +-
 include/ovn/actions.h |   3 +-
 lib/actions.c | 116 -
 lib/automake.mk   |   2 +
 lib/copp.c| 143 ++
 lib/copp.h|  59 +
 northd/ovn-northd.c   | 535 --
 ovn-nb.ovsschema  |  16 +-
 ovn-nb.xml|  81 ++
 ovn-sb.ovsschema  |   6 +-
 ovn-sb.xml|   6 +
 tests/atlocal.in  |   3 +
 tests/ovn-controller.at   |  48 
 tests/ovn-northd.at   |  81 ++
 tests/ovn.at  |   4 +-
 tests/system-ovn.at   | 135 ++
 utilities/ovn-nbctl.8.xml | 119 +
 utilities/ovn-nbctl.c | 178 +
 22 files changed, 1360 insertions(+), 299 deletions(-)
 create mode 100644 lib/copp.c
 create mode 100644 lib/copp.h

-- 
2.31.1

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

Re: [ovs-dev] [PATCH ovn 0/5] respin CoPP series

[Lorenzo Bianconi]

> On 29/04/2021 18:04, Lorenzo Bianconi wrote:
> > This series respin CoPP support introduced here [0] by Dumitru rebasing on 
> > top
> > of ovn master branch and adding some missing meters (e.g. bfd or acl 
> > reject).
> > The main goal of this series is to continue the discussion about the 
> > proposed
> > approach and to align on CMS APIs.
> > For the moment no ddlog support has been added.
> > Related bz:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1947913
> > https://bugzilla.redhat.com/show_bug.cgi?id=1946610
> > 
> 
> In a previous revision, I suggested that metering was not the right
> approach and perhaps metering in conjunction with scheduling would be
> more appropriate. I still believe this to be the case. I also suggested
> that we modify the schema to potentially allow for that in the future.
> However, I have been unable to think of a way to provide a generic
> enough schema to achieve this. Therefore, I suggest we proceed as you
> have defined here unless anyone else can come up with a better one. This
> may mean a change in the future .. but maybe not!

ack, v2 on the way ;)

Regards,
Lorenzo

> 
> I do think, at least, it would be good to provide a tutorial describing
> a problem and showing how this could be used to fix it.
> 
> > Changes since RFC:
> > - drop per-port metering
> > - add unit/system tests
> > - add reject action metering
> > 
> > [0] 
> > https://patchwork.ozlabs.org/project/openvswitch/list/?series=140778=*
> > 
> > Dumitru Ceara (5):
> >   ovn-controller: Add support for Logical_Flow control meters
> >   ovn-northd: Add support for CoPP.
> >   ovn-northd: Add CoPP policies for flows that punt packets to
> > ovn-controller.
> >   ovn-northd: Extend metering to Controller-Events
> >   NEWS: Add CoPP support.
> > 
> >  NEWS  |   1 +
> >  controller/lflow.c|  40 ++-
> >  controller/ofctrl.c   |  54 ++--
> >  controller/ofctrl.h   |  21 +-
> >  controller/physical.c |   7 +-
> >  include/ovn/actions.h |   3 +-
> >  lib/actions.c | 116 -
> >  lib/automake.mk   |   2 +
> >  lib/copp.c| 145 +++
> >  lib/copp.h|  61 +
> >  northd/ovn-northd.c   | 511 --
> 
> ddlog needs to be added.
> 
> >  ovn-nb.ovsschema  |  18 +-
> >  ovn-nb.xml|  81 ++
> >  ovn-sb.ovsschema  |   6 +-
> >  ovn-sb.xml|   6 +
> >  tests/atlocal.in  |   3 +
> >  tests/ovn-northd.at   |  49 
> >  tests/ovn.at  |   4 +-
> >  tests/system-ovn.at   | 119 +
> >  utilities/ovn-nbctl.8.xml | 117 +
> >  utilities/ovn-nbctl.c | 167 +
> >  21 files changed, 1241 insertions(+), 290 deletions(-)
> >  create mode 100644 lib/copp.c
> >  create mode 100644 lib/copp.h
> > 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn 3/5] ovn-northd: Add CoPP policies for flows that punt packets to ovn-controller.

[Lorenzo Bianconi]

> On 29/04/2021 18:04, Lorenzo Bianconi wrote:
> > From: Dumitru Ceara 
> > 
> > Change the ovn-northd implementation to set the new 'controller_meter'
> > field for flows that need to punt packets to ovn-controller.
> > 
> > Protocol packets for which CoPP is enforced when sending packets to
> > ovn-controller (if configured):
> > - ARP
> > - ND_NS
> > - ND_NA
> > - ND_RA
> > - DNS
> > - IGMP
> > - packets that require ARP resolution before forwarding
> > - packets that require ND_NS before forwarding
> > - packets that need to be replied to with ICMP Errors
> > - packets that need to be replied to with TCP RST
> > - packets that need to be replied to with DHCP_OPTS
> > - BFD
> > 
> Add reject?
> 
> Do you need to add event-elb?

ack, I will fix it in v2

> 
> > Co-authored-by: Lorenzo Bianconi 
> > Signed-off-by: Lorenzo Bianconi 
> > Signed-off-by: Dumitru Ceara 
> > ---
> >  lib/copp.c|   1 +
> >  lib/copp.h|   1 +
> >  northd/ovn-northd.c   | 451 --
> >  ovn-nb.xml|   3 +
> >  tests/atlocal.in  |   3 +
> >  tests/system-ovn.at   | 119 ++
> >  utilities/ovn-nbctl.8.xml |   1 +
> >  7 files changed, 417 insertions(+), 162 deletions(-)
> > 
> > diff --git a/lib/copp.c b/lib/copp.c
> > index ac53a1094..7713046e5 100644
> > --- a/lib/copp.c
> > +++ b/lib/copp.c
> > @@ -37,6 +37,7 @@ static char *copp_proto_names[COPP_PROTO_MAX] = {
> >  [COPP_ND_NS_RESOLVE] = "nd-ns-resolve",
> >  [COPP_ND_RA_OPTS]= "nd-ra-opts",
> >  [COPP_TCP_RESET] = "tcp-reset",
> > +[COPP_REJECT]= "reject",
> >  [COPP_BFD]   = "bfd",
> >  };
> >  
[...]
> 
> Why do you add the same flow twice but one with a meter instead of
> modifying the flow above?

it is a bug :) thx for spotting it. I will fix in v2

> 
> >  const char *dns_action = "eth.dst <-> eth.src; ip4.src <-> 
> > ip4.dst; "
> >"udp.dst = udp.src; udp.src = 53; outport = inport; "
> >"flags.loopback = 1; output;";
> > @@ -7244,7 +7280,8 @@ build_lswitch_external_port(struct ovn_port *op,
> >  static void
> >  build_lswitch_destination_lookup_bmcast(struct ovn_datapath *od,
> >  struct hmap *lflows,
> > -struct ds *actions)
> > +struct ds *actions,
> > +struct shash *meter_groups)
> >  {
> >  if (od->nbs) {
> >  
> > @@ -7265,12 +7302,16 @@ build_lswitch_destination_lookup_bmcast(struct 
> > ovn_datapath *od,
> >  }
> >  ds_put_cstr(actions, "igmp;");
> >  /* Punt IGMP traffic to controller. */
> > -ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> > - "ip4 && ip.proto == 2", ds_cstr(actions));
> > +ovn_lflow_add_unique__(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> > +   "ip4 && ip.proto == 2", 
> > ds_cstr(actions),
> > +   copp_meter_get(COPP_IGMP, od->nbs->copp,
> > +  meter_groups));
> >  
> >  /* Punt MLD traffic to controller. */
> > -ovn_lflow_add_unique(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> > - "mldv1 || mldv2", ds_cstr(actions));
> > +ovn_lflow_add_unique__(lflows, od, S_SWITCH_IN_L2_LKUP, 100,
> > +   "mldv1 || mldv2", ds_cstr(actions),
> > +   copp_meter_get(COPP_IGMP, od->nbs->copp,
> > +  meter_groups));
> >  
> >  /* Flood all IP multicast traffic destined to 224.0.0.X to all
> >   * ports - RFC 4541, section 2.1.2, item 2.
> > @@ -8650,23 +8691,29 @@ add_router_lb_flow(struct hmap *lflows, struct 
> > ovn_datapath *od,
> > struct ds *match, struct ds *actions, int priority,
> > enum lb_snat_type snat_type, struct ovn_lb_vip *lb_vip,
> > const char *proto, struct nbrec_load_balancer *lb,
> > -   struct shash *meter_groups, struct sset *nat_entries)
> > +   struct shash *meter_groups, struct sset *nat_entries,
> > +   bool reject)
> >  {
> >  build_empty_lb_event_flow(od, lflows, lb_vip, lb, S_ROUTER_IN_DNAT,
> >meter_groups);
> >  
> > +const char *meter = NULL;
> > +if (reject) {
> > +meter = copp_meter_get(COPP_REJECT, od->nbr->copp, meter_groups);
> > +}
> >  /* A match and actions for new connections. */
> >  char *new_match = xasprintf("ct.new && %s", ds_cstr(match));
> >  if (snat_type == FORCE_SNAT || snat_type == SKIP_SNAT) {
> >  char *new_actions = xasprintf("flags.%s_snat_for_lb = 1; %s",
> >   

Re: [ovs-dev] [PATCH v2] ovsdb-tool: add db-set-election-timer command for clustered databases

[Ben Pfaff]

On Fri, May 21, 2021 at 02:25:52PM -0500, Dan Williams wrote:
> After creating the new cluster database write a raft entry that
> sets the desired election timer. This allows CMSes to set the
> election timer at cluster start and avoid an error-prone
> election timer modification process after the cluster is up.
> 
> Reported-at: https://bugzilla.redhat.com/1831778
> 
> Signed-off-by: Dan Williams 
> ---
> v2:
> - Address Ben's comments; add --help and manpage docs
> - Write raft record directly instead of using private raft.c functions

Hmm, I don't think this change was what I asked for.  I asked for a new
option, like "--election-timer=TIMEOUT create-cluster DB CONTENTS
LOCAL".  Then, if we add some other new option later, we can type
"--foobar-option=QUUXWAG create-cluster DB CONTENTS LOCAL", and so on.
This adds a new command, which is quite different.

Thanks,

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

Re: [ovs-dev] [PATCH ovn v5] ovn-sbctl.c Add logical flows count numbers

[Ben Pfaff]

On Sat, May 22, 2021 at 10:56:13PM +0300, Alexey Roytman wrote:
> From: Alexey Roytman 
> 
> For big scale deployments, when number of logical flows can be 2M+,
> sometimes users just need to know the total number of logical flows
> and numbers of logical flows per table/per datapath.
> 
> New command output
> Datapath: "sw1" (4b1e53d8-9f0f-4768-b4a6-6cbc58a4bfda) pipeline: ingress
>   table=0 (ls_in_port_sec_l2  ) lflows=2
>   table=1 (ls_in_port_sec_ip  ) lflows=1

If adding  "--count" to "dump-flows" makes it no longer list flows, I
think it would be better to make it a "count-flows" command.  We have
precedent for adding statistics to a command (rather than dropping the
full output) by adding -s or --stats.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2 3/3] configure: Also find and verify version of ovsdb2ddlog.

[Ben Pfaff]

On Mon, May 24, 2021 at 12:14:31PM -0400, Mark Michelson wrote:
> On 5/21/21 11:02 PM, Ben Pfaff wrote:
> > This tool is also needed and also varies from one version of DDlog to
> > another, so we should find it and check its version in the same way.
> > 
> > Signed-off-by: Ben Pfaff 
> > ---
> >   acinclude.m4   | 24 
> >   northd/automake.mk |  4 ++--
> >   2 files changed, 18 insertions(+), 10 deletions(-)
> > 
> > diff --git a/acinclude.m4 b/acinclude.m4
> > index 7009f1dd39c2..20c5ed02309e 100644
> > --- a/acinclude.m4
> > +++ b/acinclude.m4
> > @@ -85,14 +85,22 @@ AC_DEFUN([OVS_CHECK_DDLOG], [
> >   AC_MSG_ERROR([ddlog is required to build with DDlog])
> >   fi
> > -AC_MSG_CHECKING([$DDLOG version])
> > -$DDLOG --version >_MESSAGE_LOG_FD 2>&1
> > -ddlog_version=$($DDLOG --version | sed -n 's/^DDlog v\([[^ 
> > ]]*\).*/\1/p')
> > -AC_MSG_RESULT([$ddlog_version])
> > -m4_if([$1], [], [], [
> > -AS_CASE([$ddlog_version],
> > -[$1 | $1.*], [],
> > -[*], [AC_MSG_ERROR([DDlog version $1.x is required, but 
> > $ddlog_version is installed])])])
> > +AC_ARG_VAR([OVSDB2DDLOG], [path to ovsdb2ddlog binary])
> > +AC_PATH_PROGS([OVSDB2DDLOG], [ovsdb2ddlog], [none], [$DDLOG_PATH])
> > +if test X"$OVSDB2DDLOG" = X"none"; then
> > +AC_MSG_ERROR([ovsdb2ddlog is required to build with DDlog])
> > +fi
> > +
> > +for tool in "$DDLOG" "$OVSDB2DDLOG"; do
> > +  AC_MSG_CHECKING([$tool version])
> > +  $tool --version >_MESSAGE_LOG_FD 2>&1
> > +  tool_version=$($tool --version | sed -n 's/^.* v\([[^ ]]*\).*/\1/p')
> 
> (I should make this comment on patch 1, but I'm making it here so I can just
> send one email response for this patch series)
> 
> There is a small danger of this regex matching on something unexpected in
> case a DDLog tool ever has a word starting with a lowercase v in it. For
> example, a hypothetical "DDLog virtualization suite" would give us the
> version string "irtualization". It may be worthwhile to at least ensure the
> character directly after "v" is a number.
> 
> Maybe I'm just being overly nitpicky here though.
> 
> > +  AC_MSG_RESULT([$tool_version])
> > +  m4_if([$1], [], [], [
> > +  AS_CASE([$tool_version],
> > +  [$1 | $1.*], [],
> > +  [*], [AC_MSG_ERROR([DDlog version $1.x is required, but 
> > $tool is version $ddlog_version])])])
> 
> This error message should print $tool_version instead of $ddlog_version.
> $ddlog_version is no longer set, so this just always prints an empty string
> for the version if you have the wrong one installed.

Thanks for the review.  I fixed both of these and posted v3.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH ovn v3 3/3] configure: Also find and verify version of ovsdb2ddlog.

[Ben Pfaff]

This tool is also needed and also varies from one version of DDlog to
another, so we should find it and check its version in the same way.

Signed-off-by: Ben Pfaff 
---
 acinclude.m4   | 24 
 northd/automake.mk |  4 ++--
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 2fbcab8463c3..e7f829520029 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -85,14 +85,22 @@ AC_DEFUN([OVS_CHECK_DDLOG], [
 AC_MSG_ERROR([ddlog is required to build with DDlog])
 fi
 
-AC_MSG_CHECKING([$DDLOG version])
-$DDLOG --version >_MESSAGE_LOG_FD 2>&1
-ddlog_version=$($DDLOG --version | sed -n 's/^DDlog v\([[0-9]][[^ 
]]*\).*/\1/p')
-AC_MSG_RESULT([$ddlog_version])
-m4_if([$1], [], [], [
-AS_CASE([$ddlog_version],
-[$1 | $1.*], [],
-[*], [AC_MSG_ERROR([DDlog version $1.x is required, but 
$ddlog_version is installed])])])
+AC_ARG_VAR([OVSDB2DDLOG], [path to ovsdb2ddlog binary])
+AC_PATH_PROGS([OVSDB2DDLOG], [ovsdb2ddlog], [none], [$DDLOG_PATH])
+if test X"$OVSDB2DDLOG" = X"none"; then
+AC_MSG_ERROR([ovsdb2ddlog is required to build with DDlog])
+fi
+
+for tool in "$DDLOG" "$OVSDB2DDLOG"; do
+  AC_MSG_CHECKING([$tool version])
+  $tool --version >_MESSAGE_LOG_FD 2>&1
+  tool_version=$($tool --version | sed -n 's/^.* v\([[0-9]][[^ 
]]*\).*/\1/p')
+  AC_MSG_RESULT([$tool_version])
+  m4_if([$1], [], [], [
+  AS_CASE([$tool_version],
+  [$1 | $1.*], [],
+  [*], [AC_MSG_ERROR([DDlog version $1.x is required, but $tool is 
version $tool_version])])])
+done
 
 AC_ARG_VAR([CARGO])
 AC_CHECK_PROGS([CARGO], [cargo], [none])
diff --git a/northd/automake.mk b/northd/automake.mk
index aaea7e1b1336..4fc81c17bfa3 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -50,13 +50,13 @@ northd_ovn_northd_ddlog_LDADD = \
 
 nb_opts = $$(cat $(srcdir)/northd/ovn-nb.dlopts)
 northd/OVN_Northbound.dl: ovn-nb.ovsschema northd/ovn-nb.dlopts
-   $(AM_V_GEN)ovsdb2ddlog -f $< --output-file $@ $(nb_opts)
+   $(AM_V_GEN)$(OVSDB2DDLOG) -f $< --output-file $@ $(nb_opts)
 northd/ovn-northd-ddlog-nb.inc: ovn-nb.ovsschema northd/ovn-nb.dlopts 
northd/ovsdb2ddlog2c
$(AM_V_GEN)$(run_python) $(srcdir)/northd/ovsdb2ddlog2c -p nb_ -f $< 
--output-file $@ $(nb_opts)
 
 sb_opts = $$(cat $(srcdir)/northd/ovn-sb.dlopts)
 northd/OVN_Southbound.dl: ovn-sb.ovsschema northd/ovn-sb.dlopts
-   $(AM_V_GEN)ovsdb2ddlog -f $< --output-file $@ $(sb_opts)
+   $(AM_V_GEN)$(OVSDB2DDLOG) -f $< --output-file $@ $(sb_opts)
 northd/ovn-northd-ddlog-sb.inc: ovn-sb.ovsschema northd/ovn-sb.dlopts 
northd/ovsdb2ddlog2c
$(AM_V_GEN)$(run_python) $(srcdir)/northd/ovsdb2ddlog2c -p sb_ -f $< 
--output-file $@ $(sb_opts)
 
-- 
2.31.1

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

[ovs-dev] [PATCH ovn v3 0/3] Check ddlog version and improve configure process

[Ben Pfaff]

v1->v2: Change leading tabs to spaces.
v2->v3:
  - Use stricter regex for recognizing version numbers in "configure:
Check for correct ddlog version.".
  - Print correct version number in error message in "configure: Also
find and verify version of ovsdb2ddlog."

Ben Pfaff (3):
  configure: Check for correct ddlog version.
  configure: Improve how to find ddlog binaries and libraries.
  configure: Also find and verify version of ovsdb2ddlog.

 Documentation/intro/install/general.rst | 37 +++---
 acinclude.m4| 66 +++--
 configure.ac|  2 +-
 northd/automake.mk  |  4 +-
 4 files changed, 84 insertions(+), 25 deletions(-)

-- 
2.31.1

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

[ovs-dev] [PATCH ovn v3 1/3] configure: Check for correct ddlog version.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
Suggested-by: Mark Michelson 
---
 acinclude.m4 | 20 +---
 configure.ac |  2 +-
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 1f8bf8224a9a..93177dfe71d4 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -42,9 +42,14 @@ AC_DEFUN([OVS_ENABLE_WERROR],
fi
AC_SUBST([SPARSE_WERROR])])
 
-dnl OVS_CHECK_DDLOG
+dnl OVS_CHECK_DDLOG([VERSION])
 dnl
-dnl Configure ddlog source tree
+dnl Configure ddlog source tree, checking for the given DDlog VERSION.
+dnl VERSION should be a major and minor, e.g. 0.36, which will accept
+dnl 0.36.0, 0.36.1, and so on.  Omit VERSION to accept any version of
+dnl ddlog (which is probably only useful for developers who are trying
+dnl different versions, since OVN is currently bound to a particular
+dnl DDlog version).
 AC_DEFUN([OVS_CHECK_DDLOG], [
   AC_ARG_WITH([ddlog],
   [AC_HELP_STRING([--with-ddlog=.../differential-datalog/lib],
@@ -52,6 +57,7 @@ AC_DEFUN([OVS_CHECK_DDLOG], [
   [DDLOGLIBDIR=$withval], [DDLOGLIBDIR=no])
 
   AC_MSG_CHECKING([for DDlog library directory])
+  AC_MSG_RESULT([$DDLOGLIBDIR])
   if test "$DDLOGLIBDIR" != no; then
 if test ! -d "$DDLOGLIBDIR"; then
   AC_MSG_ERROR([ddlog library dir "$DDLOGLIBDIR" doesn't exist])
@@ -65,6 +71,15 @@ AC_DEFUN([OVS_CHECK_DDLOG], [
 AC_MSG_ERROR([ddlog is required to build with DDlog])
 fi
 
+AC_MSG_CHECKING([$DDLOG version])
+$DDLOG --version >_MESSAGE_LOG_FD 2>&1
+ddlog_version=$($DDLOG --version | sed -n 's/^DDlog v\([[0-9]][[^ 
]]*\).*/\1/p')
+AC_MSG_RESULT([$ddlog_version])
+m4_if([$1], [], [], [
+AS_CASE([$ddlog_version],
+[$1 | $1.*], [],
+[*], [AC_MSG_ERROR([DDlog version $1.x is required, but 
$ddlog_version is installed])])])
+
 AC_ARG_VAR([CARGO])
 AC_CHECK_PROGS([CARGO], [cargo], [none])
 if test X"$CARGO" = X"none"; then
@@ -80,7 +95,6 @@ AC_DEFUN([OVS_CHECK_DDLOG], [
 AC_SUBST([DDLOGLIBDIR])
 AC_DEFINE([DDLOG], [1], [Build OVN daemons with ddlog.])
   fi
-  AC_MSG_RESULT([$DDLOGLIBDIR])
 
   AM_CONDITIONAL([DDLOG], [test "$DDLOGLIBDIR" != no])
 ])
diff --git a/configure.ac b/configure.ac
index ec5ee31df0da..1c57c4d10482 100644
--- a/configure.ac
+++ b/configure.ac
@@ -169,7 +169,7 @@ OVS_CONDITIONAL_CC_OPTION([-Wno-unused-parameter], 
[HAVE_WNO_UNUSED_PARAMETER])
 OVS_ENABLE_WERROR
 OVS_ENABLE_SPARSE
 
-OVS_CHECK_DDLOG
+OVS_CHECK_DDLOG([0.38])
 OVS_CHECK_PRAGMA_MESSAGE
 OVN_CHECK_OVS
 OVS_CTAGS_IDENTIFIERS
-- 
2.31.1

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

[ovs-dev] [PATCH ovn v3 2/3] configure: Improve how to find ddlog binaries and libraries.

[Ben Pfaff]

There was no easy way to use a ddlog installation from one of the
tarballs provided by the ddlog developers, because these put the
binaries in a subdirectory of an installation directory instead of in
a system- or user-level bin directory.  This commit makes that easier:
just do "--with-ddlog=$INSTALLDIR".

Signed-off-by: Ben Pfaff 
---
 Documentation/intro/install/general.rst | 37 +++-
 acinclude.m4| 38 +
 2 files changed, 56 insertions(+), 19 deletions(-)

diff --git a/Documentation/intro/install/general.rst 
b/Documentation/intro/install/general.rst
index ee48272422fe..589518846233 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -213,13 +213,36 @@ the default database directory, add options as shown 
here::
   ``yum install`` or ``rpm -ivh``) and .deb (e.g. via
   ``apt-get install`` or ``dpkg -i``) use the above configure options.
 
-To build with DDlog support, add ``--with-ddlog=/lib``
-to the ``configure`` command line.  Building with DDLog adds a few
-minutes to the build because the Rust compiler is slow.  To speed this
-up by about 2x, also add ``--enable-ddlog-fast-build``.  This disables
-some Rust compiler optimizations, making a much slower
-``ovn-northd-ddlog`` executable, so it should not be used for
-production builds or for profiling.
+Use ``--with-ddlog`` to build with DDlog support.  To build with
+DDlog, the build system needs to be able to find the ``ddlog`` and
+``ovsdb2ddlog`` binaries and the DDlog library directory (the
+directory that contains ``ddlog_std.dl``).  This option supports a
+few ways to do that:
+
+  * If binaries are in $PATH, use the library directory as argument,
+e.g. ``--with-ddlog=$HOME/differential-datalog/lib``.  This is
+suitable if DDlog was installed from source via ``stack install`` or
+from (hypothetical) distribution packaging.
+
+The DDlog documentation recommends pointing $DDLOG_HOME to the
+DDlog source directory.  If you did this, so that $DDLOG_HOME/lib
+is the library directory, you may use ``--with-ddlog`` without an
+argument.
+
+  * If the binaries and libraries are in the ``bin`` and ``lib``
+subdirectories of an installation directory, use the installation
+directory as the argument.  This is suitable if DDlog was
+installed from one of the binary tarballs published by the DDlog
+developers.
+
+.. note::
+
+   Building with DDLog adds a few minutes to the build because the
+   Rust compiler is slow.  Add ``--enable-ddlog-fast-build`` to make
+   this about 2x faster.  This disables some Rust compiler
+   optimizations, making a much slower ``ovn-northd-ddlog``
+   executable, so it should not be used for production builds or for
+   profiling.
 
 By default, static libraries are built and linked against. If you want to use
 shared libraries instead::
diff --git a/acinclude.m4 b/acinclude.m4
index 93177dfe71d4..2fbcab8463c3 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -51,22 +51,36 @@ dnl ddlog (which is probably only useful for developers who 
are trying
 dnl different versions, since OVN is currently bound to a particular
 dnl DDlog version).
 AC_DEFUN([OVS_CHECK_DDLOG], [
-  AC_ARG_WITH([ddlog],
-  [AC_HELP_STRING([--with-ddlog=.../differential-datalog/lib],
-  [Enables DDlog by pointing to its library dir])],
-  [DDLOGLIBDIR=$withval], [DDLOGLIBDIR=no])
+  AC_ARG_VAR([DDLOG_HOME], [Root of the DDlog installation])
+  AC_ARG_WITH(
+[ddlog],
+[AC_HELP_STRING([--with-ddlog[[=INSTALLDIR|LIBDIR]]], [Enables DDlog])],
+[DDLOG_PATH=$PATH
+ if test "$withval" = yes; then
+   # --with-ddlog: $DDLOG_HOME must be set
+   if test -z "$DDLOG_HOME"; then
+ AC_MSG_ERROR([To build with DDlog, specify the DDlog install or 
library directory on --with-ddlog or in \$DDLOG_HOME])
+   fi
+   DDLOGLIBDIR=$DDLOG_HOME/lib
+   test -d "$DDLOG_HOME/bin" && DDLOG_PATH=$DDLOG_HOME/bin
+ elif test -f "$withval/lib/ddlog_std.dl"; then
+   # --with-ddlog=INSTALLDIR
+   DDLOGLIBDIR=$withval/lib
+   test -d "$withval/bin" && DDLOG_PATH=$withval/bin
+ elif test -f "$withval/ddlog_std.dl"; then
+   # --with-ddlog=LIBDIR
+   DDLOGLIBDIR=$withval/lib
+ else
+   AC_MSG_ERROR([$withval does not contain ddlog_std.dl or 
lib/ddlog_std.dl])
+ fi],
+[DDLOGLIBDIR=no
+ DDLOG_PATH=no])
 
   AC_MSG_CHECKING([for DDlog library directory])
   AC_MSG_RESULT([$DDLOGLIBDIR])
   if test "$DDLOGLIBDIR" != no; then
-if test ! -d "$DDLOGLIBDIR"; then
-  AC_MSG_ERROR([ddlog library dir "$DDLOGLIBDIR" doesn't exist])
-elif test ! -f "$DDLOGLIBDIR"/ddlog_std.dl; then
-  AC_MSG_ERROR([ddlog library dir "$DDLOGLIBDIR" lacks ddlog_std.dl])
-fi
-
-AC_ARG_VAR([DDLOG])
-AC_CHECK_PROGS([DDLOG], [ddlog], [none])
+AC_ARG_VAR([DDLOG], [path to ddlog binary])
+

Re: [ovs-dev] [PATCH ovn 4/5] ovn-northd: Extend metering to Controller-Events

[Lorenzo Bianconi]

> On 29/04/2021 18:04, Lorenzo Bianconi wrote:
> > From: Dumitru Ceara 
> > 
> > Co-authored-by: Lorenzo Bianconi 
> > Signed-off-by: Lorenzo Bianconi 
> > Signed-off-by: Dumitru Ceara 
> > ---
> >  include/ovn/actions.h |  1 -
> >  lib/actions.c | 50 ---
> >  northd/ovn-northd.c   | 18 
> >  tests/ovn.at  |  4 ++--
> >  4 files changed, 20 insertions(+), 53 deletions(-)
> > 
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index ab03df12c..b2f2f57c6 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -393,7 +393,6 @@ struct ovnact_controller_event {
> >  int event_type;   /* controller event type */
> >  struct ovnact_gen_option *options;
> >  size_t n_options;
> > -char *meter;
> >  };
> >  
> >  /* OVNACT_BIND_VPORT. */
> > diff --git a/lib/actions.c b/lib/actions.c
> > index 155b4a45a..f0291afef 100644
> > --- a/lib/actions.c
> > +++ b/lib/actions.c
> > @@ -1613,9 +1613,6 @@ format_TRIGGER_EVENT(const struct 
> > ovnact_controller_event *event,
> >  {
> >  ds_put_format(s, "trigger_event(event = \"%s\"",
> >event_to_string(event->event_type));
> > -if (event->meter) {
> > -ds_put_format(s, ", meter = \"%s\"", event->meter);
> > -}
> >  for (const struct ovnact_gen_option *o = event->options;
> >   o < >options[event->n_options]; o++) {
> >  ds_put_cstr(s, ", ");
> > @@ -1790,24 +1787,11 @@ encode_event_empty_lb_backends_opts(struct ofpbuf 
> > *ofpacts,
> >  
> >  static void
> >  encode_TRIGGER_EVENT(const struct ovnact_controller_event *event,
> > - const struct ovnact_encode_params *ep OVS_UNUSED,
> > + const struct ovnact_encode_params *ep,
> >   struct ofpbuf *ofpacts)
> >  {
> > -uint32_t meter_id = NX_CTLR_NO_METER;
> > -size_t oc_offset;
> > -
> > -if (event->meter) {
> > -meter_id = ovn_extend_table_assign_id(ep->meter_table, 
> > event->meter,
> > -  ep->lflow_uuid);
> > -if (meter_id == EXT_TABLE_ID_INVALID) {
> > -VLOG_WARN("Unable to assign id for trigger meter: %s",
> > -  event->meter);
> > -return;
> > -}
> > -}
> > -
> > -oc_offset = encode_start_controller_op(ACTION_OPCODE_EVENT, false,
> > -   meter_id, ofpacts);
> > +size_t oc_offset = encode_start_controller_op(ACTION_OPCODE_EVENT, 
> > false,
> > +  ep->ctrl_meter_id, 
> > ofpacts);
> >  ovs_be32 ofs = htonl(event->event_type);
> >  ofpbuf_put(ofpacts, , sizeof ofs);
> >  
> > @@ -2341,27 +2325,12 @@ parse_trigger_event(struct action_context *ctx,
> >   sizeof *event->options);
> >  }
> >  
> > -if (lexer_match_id(ctx->lexer, "meter")) {
> > -if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> > -return;
> > -}
> > -/* If multiple meters are given, use the most recent. */
> > -if (ctx->lexer->token.type == LEX_T_STRING &&
> > -strlen(ctx->lexer->token.s)) {
> > -free(event->meter);
> > -event->meter = xstrdup(ctx->lexer->token.s);
> > -} else if (ctx->lexer->token.type != LEX_T_STRING) {
> > -lexer_syntax_error(ctx->lexer, "expecting string");
> > -return;
> > -}
> > -lexer_get(ctx->lexer);
> > -} else {
> > -struct ovnact_gen_option *o = 
> > >options[event->n_options++];
> > -memset(o, 0, sizeof *o);
> > -parse_gen_opt(ctx, o,
> > -
> > >pp->controller_event_opts->event_opts[event_type],
> > -event_to_string(event_type));
> > -}
> > +struct ovnact_gen_option *o = >options[event->n_options++];
> > +memset(o, 0, sizeof *o);
> > +parse_gen_opt(ctx, o,
> > +  
> > >pp->controller_event_opts->event_opts[event_type],
> > +  event_to_string(event_type));
> > +
> >  if (ctx->lexer->error) {
> >  return;
> >  }
> > @@ -2382,7 +2351,6 @@ static void
> >  ovnact_controller_event_free(struct ovnact_controller_event *event)
> >  {
> >  free_gen_options(event->options, event->n_options);
> > -free(event->meter);
> >  }
> >  
> >  static void
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index 93b431f4c..a3eb8f646 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -5083,11 +5083,7 @@ build_empty_lb_event_flow(struct ovn_datapath *od, 
> > struct hmap *lflows,
> >  
> >  bool ipv4 = IN6_IS_ADDR_V4MAPPED(_vip->vip);
> >  struct ds match = DS_EMPTY_INITIALIZER;
> > -char *meter = "", *action;
> > -

Re: [ovs-dev] [PATCH ovn 2/5] ovn-northd: Add support for CoPP.

[Lorenzo Bianconi]

> On 29/04/2021 18:04, Lorenzo Bianconi wrote:
> > From: Dumitru Ceara 
> > 

[...]
> 
> Could this be changed to copp_proto_get_name()?

ack, I will fix it in v2

> 
> > +{
> > +if (proto >= COPP_PROTO_MAX) {
> > +return "";
> > +}
> > +return copp_proto_names[proto];
> > +}
> > +
> > +const char *
> > +copp_meter_get(enum copp_proto proto, const struct nbrec_copp *copp,
> > +   const struct shash *meter_groups)
> > +{
> > +if (!copp || proto >= COPP_PROTO_MAX) {
> > +return NULL;
> > +}
> > +
> > +const char *meter = smap_get(>meters, copp_proto_names[proto]);
> > +
> > +if (meter && shash_find(meter_groups, meter)) {
> > +return meter;
> > +}
> > +
> > +return NULL;
> > +}
> > +
> > +void
> > +copp_list(struct ctl_context *ctx, const struct nbrec_copp *copp)
> > +{
> > +if (!copp) {
> > +return;
> > +}
> > +
> > +struct smap_node *node;
> > +
> > +SMAP_FOR_EACH (node, >meters) {
> > +ds_put_format(>output, "%s: %s\n", node->key, node->value);
> > +}
> > +}
> > +
> > +const struct nbrec_copp *
> > +copp_add_meter(struct ctl_context *ctx, const struct nbrec_copp *copp,
> > +   const char *proto_name, const char *meter)
> > +{
> > +if (!copp) {
> > +copp = nbrec_copp_insert(ctx->txn);
> > +}
> > +
> > +struct smap meters;
> > +smap_init();
> > +smap_clone(, >meters);
> > +smap_replace(, proto_name, meter);
> > +nbrec_copp_set_meters(copp, );
> > +smap_destroy();
> > +
> > +return copp;
> > +}
> > +
> > +void
> > +copp_del_meter(const struct nbrec_copp *copp, const char *proto_name)
> > +{
> > +if (!copp) {
> > +return;
> > +}
> > +
> > +if (proto_name) {
> > +if (smap_get(>meters, proto_name)) {
> > +struct smap meters;
> > +smap_init();
> > +smap_clone(, >meters);
> > +smap_remove(, proto_name);
> > +nbrec_copp_set_meters(copp, );
> > +smap_destroy();
> > +}
> > +} else {
> > +nbrec_copp_delete(copp);
> > +}
> > +}
> > +
> > +char *
> > +copp_proto_validate(const char *proto_name)> +{
> > +for (size_t i = COPP_PROTO_FIRST; i < COPP_PROTO_MAX; i++) {
> > +if (!strcmp(proto_name, copp_proto_get(i))) {
> > +return NULL;
> > +}
> > +}
> > +
> > +struct ds usage = DS_EMPTY_INITIALIZER;
> > +
> > +ds_put_cstr(, "Invalid control protocol. Allowed values: ");
> > +for (size_t i = COPP_PROTO_FIRST; i < COPP_PROTO_MAX; i++) {
> > +ds_put_format(, "%s, ", copp_proto_get(i));
> > +}
> > +ds_chomp(, ' ');
> > +ds_chomp(, ',');
> > +ds_put_cstr(, ".");
> > +
> > +char *usage_str = xstrdup(ds_cstr());
> > +ds_destroy();
> > +return usage_str;
> > +}
> > diff --git a/lib/copp.h b/lib/copp.h
> > new file mode 100644
> > index 0..82581e7e4
> > --- /dev/null
> > +++ b/lib/copp.h
> > @@ -0,0 +1,60 @@
> > +/* Copyright (c) 2021, 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.
> > + */
> > +
> > +#ifndef OVN_COPP_H
> > +#define OVN_COPP_H 1
> > +
> > +/*
> > + * Control plane protection - metered actions.
> > + */
> > +enum copp_proto {
> > +COPP_PROTO_FIRST,
> > +COPP_ARP = COPP_PROTO_FIRST,
> > +COPP_ARP_RESOLVE,
> > +COPP_DHCPV4_OPTS,
> > +COPP_DHCPV6_OPTS,
> > +COPP_DNS,
> > +COPP_EVENT_ELB,
> > +COPP_ICMP4_ERR,
> > +COPP_ICMP6_ERR,
> > +COPP_IGMP,
> > +COPP_ND_NA,
> > +COPP_ND_NS,
> > +COPP_ND_NS_RESOLVE,
> > +COPP_ND_RA_OPTS,
> > +COPP_TCP_RESET,
> > +COPP_BFD,
> > +COPP_PROTO_MAX,
> > +COPP_PROTO_INVALID = COPP_PROTO_MAX,
> > +};
> > +
> > +struct nbrec_copp;
> > +struct ctl_context;
> > +
> > +const char *copp_proto_get(enum copp_proto);
> 
> I think this could a static and declared in copp.c as it is not used
> outside of there.

ack, I will fix it in v2
> 
> > +
> > +const char *copp_meter_get(enum copp_proto proto,
> > +   const struct nbrec_copp *copp,
> > +   const struct shash *meter_groups);
> > +
> > +void copp_list(struct ctl_context *ctx, const struct nbrec_copp *copp);
> > +const struct nbrec_copp *
> > +copp_add_meter(struct ctl_context *ctx, const struct nbrec_copp *copp,
> > +   const char *proto_name, const char *meter);
> > 

Re: [ovs-dev] [PATCH ovn 2/5] ovn-northd: Add support for CoPP.

[Lorenzo Bianconi]

> On 4/29/21 1:04 PM, Lorenzo Bianconi wrote:
> > From: Dumitru Ceara 
> > 

[...]

> > +
> > +ds_put_cstr(, "Invalid control protocol. Allowed values: ");
> > +for (size_t i = COPP_PROTO_FIRST; i < COPP_PROTO_MAX; i++) {
> > +ds_put_format(, "%s, ", copp_proto_get(i));
> > +}
> > +ds_chomp(, ' ');
> > +ds_chomp(, ',');
> > +ds_put_cstr(, ".");
> > +
> > +char *usage_str = xstrdup(ds_cstr());
> > +ds_destroy(); > +return usage_str;
> 
> You can replace the above 3 lines with:
> 
> return ds_steal_cstr();
> 
> Stealing the cstr means you do not need to destroy the dynamic string, and
> it saves you an extra allocation.

ack, I will fix it in v2

> 
[...]
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([ovn -- check CoPP config])
> 
> I think this test should also attempt trying to add a nonsense protocol
> name, and ensure that
> 
> 1) It doesn't cause OVN to blow up.
> 2) It doesn't end up somehow getting configured.
> 
> This should also attempt to ls-copp-add an acceptable protocol but with a
> non-existent meter for similar purposes.

ack, I will fix it in v2

> 
> > +AT_KEYWORDS([northd-CoPP])
> > +ovn_start
> > +
> > +check ovn-nbctl --wait=sb lr-add r0
> > +check ovn-nbctl --wait=sb lrp-add r0 r0-sw1 00:00:00:00:00:01 
> > 192.168.1.1/24
> > +check ovn-nbctl --wait=sb ls-add sw1
> > +check ovn-nbctl --wait=sb lsp-add sw1 sw1-r0
> > +check ovn-nbctl --wait=sb lsp-set-type sw1-r0 router
> > +check ovn-nbctl --wait=sb lsp-set-options sw1-r0 router-port=r0-sw1
> > +check ovn-nbctl --wait=sb lsp-set-addresses sw1-r0 00:00:00:00:00:01
> > +
> > +ovn-nbctl --wait=hv meter-add meter0 drop 100 pktps 10
> > +ovn-nbctl --wait=hv ls-copp-add sw1 event-elb meter0
> > +AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
> > +event-elb: meter0
> > +])
> > +
> > +ovn-nbctl --wait=hv meter-add meter1 drop 200 pktps 10
> > +ovn-nbctl --wait=hv ls-copp-add sw1 arp meter1
> > +AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
> > +arp: meter1
> > +event-elb: meter0
> > +])
> > +
> > +ovn-nbctl --wait=hv ls-copp-del sw1 arp
> > +AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
> > +event-elb: meter0
> > +])
> > +
> > +ovn-nbctl --wait=hv ls-copp-del sw1 event-elb
> > +AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
> > +])
> > +
> > +ovn-nbctl --wait=hv lr-copp-add r0 bfd meter0
> > +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
> > +bfd: meter0
> > +])
> > +
> > +ovn-nbctl --wait=hv lr-copp-add r0 igmp meter1
> > +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
> > +bfd: meter0
> > +igmp: meter1
> > +])
> > +
> > +AT_CLEANUP
> > +])
> > +
> >   AT_SETUP([ovn -- check LSP attached to multiple LS])
> >   ovn_start
> > diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> > index 03d47dba5..6c95e8104 100644
> > --- a/utilities/ovn-nbctl.8.xml
> > +++ b/utilities/ovn-nbctl.8.xml
> > @@ -1131,6 +1131,122 @@
> > 
> >   
> > + Control Plane Protection Policy commands
> > +
> > +  These commands manages meters contained in  table
> 
> "These commands manage meters configured in ..."

ack, I will fix it in v2

> 
> > +  linking them to logical datapaths through copp column in
> > +   or  
> > tables.
> > +  Protocol packets for which CoPP is enforced when sending packets to
> > +  ovn-controller (if configured):
> > +  
> > +  ARP
> > +  ND_NS
> > +  ND_NA
> > +  ND_RA
> > +  ND
> > +  DNS
> > +  IGMP
> > +  packets that require ARP resolution before forwarding
> > +  packets that require ND_NS before forwarding
> > +  packets that need to be replied to with ICMP Errors
> > +  packets that need to be replied to with TCP RST
> > +  packets that need to be replied to with DHCP_OPTS
> > +  BFD
> 
> Should there also be a list item here mentioning packets that need to be
> rplied to with SCTP ABORT?

ack, I will fix it in v2

Regards,
Lorenzo

> 
> > +  
> > +
> > +
> > +
> > +  ls-copp-add switch proto
> > +  meter
> > +  
> > +Adds the control proto to meter mapping
> > +to the switch control plane protection policy. If no
> > +policy exists yet, it creates one. If a mapping already existed for
> > +proto, this will overwrite it.
> > +  
> > +
> > +  ls-copp-del switch 
> > [proto]
> > +  
> > +Removes the control proto mapping from the
> > +switch control plane protection policy. If
> > +proto is not specified, the whole control plane
> > +protection policy is destroyed.
> > +  
> > +
> > +  ls-copp-list switch
> > +  
> > +Display the current control plane protection policy for
> > +switch.
> > +  
> > +
> > +  lsp-copp-add proto proto
> > +  meter
> > +  
> > +Adds the control proto to meter mapping
> > +to the port control plane protection policy. If no
> > +policy 

Re: [ovs-dev] [PATCH v3 2/2] dpctl: dpif: allow viewing and configuring dp cache sizes

[Paolo Valerio]

Hi Eelco,

Eelco Chaudron  writes:

> This patch adds a general way of viewing/configuring datapath
> cache sizes. With an implementation for the netlink interface.
>
> The ovs-dpctl/ovs-appctl show commands will display the
> current cache sizes configured:
>
> ovs-dpctl show
> system@ovs-system:
>   lookups: hit:25 missed:63 lost:0
>   flows: 0
>   masks: hit:282 total:0 hit/pkt:3.20
>   cache: hit:4 hit rate:4.5455%
>   caches:
> masks-cache: size: 256
>   port 0: ovs-system (internal)
>   port 1: br-int (internal)
>   port 2: genev_sys_6081 (geneve: packet_type=ptap)
>   port 3: br-ex (internal)
>   port 4: eth2
>   port 5: sw0p1 (internal)
>   port 6: sw0p3 (internal)
>
> A specific cache can be configured as follows:
>
> ovs-appctl dpctl/cache-set-size DP CACHE SIZE
> ovs-dpctl cache-set-size DP CACHE SIZE
>

there's an implication here based on the current dp code (see below)

> For example to disable the cache do:
>
> $ ovs-dpctl cache-set-size system@ovs-system masks-cache 0
> Setting cache size successful, new size 0.
>
> Signed-off-by: Eelco Chaudron 
> ---
> v2: - Changed cache naming to use a hyphen instead of spaces
> - Some error message grammar changes
> - Update dpctl man page
> - Add self tests to for the new set/get commands
> v3: - Rebase on the latest master branch
>
>  datapath/linux/compat/include/linux/openvswitch.h |1 
>  lib/dpctl.c   |  120 
> +
>  lib/dpctl.man |   14 ++
>  lib/dpif-netdev.c |4 +
>  lib/dpif-netlink.c|  113 
>  lib/dpif-provider.h   |   20 
>  lib/dpif.c|   32 ++
>  lib/dpif.h|7 +
>  tests/system-traffic.at   |   36 ++
>  utilities/ovs-dpctl.c |4 +
>  10 files changed, 351 insertions(+)
>

[...]

> +static int
> +dpif_netlink_cache_set_size(struct dpif *dpif_, uint32_t level, uint32_t 
> size)
> +{
> +int error;
> +struct ofpbuf *bufp;
> +struct dpif_netlink_dp request, reply;
> +struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
> +
> +size = ROUND_UP_POW2(size);
> +
> +if (level != 0) {
> +return EINVAL;
> +}
> +
> +dpif_netlink_dp_init();
> +request.cmd = OVS_DP_CMD_SET;
> +request.name = dpif_->base_name;
> +request.dp_ifindex = dpif->dp_ifindex;
> +request.cache_size = size;
> +

with this nl transaction, we lose the 'user_features' because it gets
blanked in the datapath that, ATM, always requires user_featues to be
present.

if we issue:

# ovs-appctl dpctl/cache-set-size system@ovs-system masks-cache 512

probing user_features value (previously set to 0x7):

ovs-vswitchd 44589 [010] 12160.797672: probe:ovs_dp_change_L30: 
(c168f700) user_features=0x0

Note that a new dp open will set back the correct value.
We have two potential solutions here:

the first one is in userspace, and requires, for newer userspace
versions, that OVS_DP_ATTR_USER_FEATURES becomes mandatory (this has to
be always included from userspace unless it's not supported).
In this case we should just add:

request.user_features = dpif->user_features;

The alternative involves changing the kernel's behavior.
I tested both approaches, and both do the job.
Based on the preference we could modify this patch, or update the
kernel's behavior.

Paolo

> +error = dpif_netlink_dp_transact(, , );
> +if (!error) {
> +ofpbuf_delete(bufp);
> +if (reply.cache_size != size) {
> +return EINVAL;
> +}
> +}
> +
> +return error;
> +}
> +
>  
>  const struct dpif_class dpif_netlink_class = {
>  "system",
> @@ -4026,6 +4121,10 @@ const struct dpif_class dpif_netlink_class = {
>  NULL,   /* bond_add */
>  NULL,   /* bond_del */
>  NULL,   /* bond_stats_get */
> +dpif_netlink_cache_get_supported_levels,
> +dpif_netlink_cache_get_name,
> +dpif_netlink_cache_get_size,
> +dpif_netlink_cache_set_size,
>  };
>  
>  static int
> @@ -4286,6 +4385,9 @@ dpif_netlink_dp_from_ofpbuf(struct dpif_netlink_dp *dp, 
> const struct ofpbuf *buf
>  [OVS_DP_ATTR_USER_FEATURES] = {
>  .type = NL_A_U32,
>  .optional = true },
> +[OVS_DP_ATTR_MASKS_CACHE_SIZE] = {
> +.type = NL_A_U32,
> +.optional = true },
>  };
>  
>  dpif_netlink_dp_init(dp);
> @@ -4318,6 +4420,12 @@ dpif_netlink_dp_from_ofpbuf(struct dpif_netlink_dp 
> *dp, const struct ofpbuf *buf
>  dp->user_features = nl_attr_get_u32(a[OVS_DP_ATTR_USER_FEATURES]);
>  }
>  
> +if (a[OVS_DP_ATTR_MASKS_CACHE_SIZE]) {
> +dp->cache_size = 

Re: [ovs-dev] [PATCH ovn v2 3/3] configure: Also find and verify version of ovsdb2ddlog.

[Mark Michelson]

On 5/21/21 11:02 PM, Ben Pfaff wrote:

This tool is also needed and also varies from one version of DDlog to
another, so we should find it and check its version in the same way.

Signed-off-by: Ben Pfaff 
---
  acinclude.m4   | 24 
  northd/automake.mk |  4 ++--
  2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 7009f1dd39c2..20c5ed02309e 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -85,14 +85,22 @@ AC_DEFUN([OVS_CHECK_DDLOG], [
  AC_MSG_ERROR([ddlog is required to build with DDlog])
  fi
  
-AC_MSG_CHECKING([$DDLOG version])

-$DDLOG --version >_MESSAGE_LOG_FD 2>&1
-ddlog_version=$($DDLOG --version | sed -n 's/^DDlog v\([[^ ]]*\).*/\1/p')
-AC_MSG_RESULT([$ddlog_version])
-m4_if([$1], [], [], [
-AS_CASE([$ddlog_version],
-[$1 | $1.*], [],
-[*], [AC_MSG_ERROR([DDlog version $1.x is required, but 
$ddlog_version is installed])])])
+AC_ARG_VAR([OVSDB2DDLOG], [path to ovsdb2ddlog binary])
+AC_PATH_PROGS([OVSDB2DDLOG], [ovsdb2ddlog], [none], [$DDLOG_PATH])
+if test X"$OVSDB2DDLOG" = X"none"; then
+AC_MSG_ERROR([ovsdb2ddlog is required to build with DDlog])
+fi
+
+for tool in "$DDLOG" "$OVSDB2DDLOG"; do
+  AC_MSG_CHECKING([$tool version])
+  $tool --version >_MESSAGE_LOG_FD 2>&1
+  tool_version=$($tool --version | sed -n 's/^.* v\([[^ ]]*\).*/\1/p')


(I should make this comment on patch 1, but I'm making it here so I can 
just send one email response for this patch series)


There is a small danger of this regex matching on something unexpected 
in case a DDLog tool ever has a word starting with a lowercase v in it. 
For example, a hypothetical "DDLog virtualization suite" would give us 
the version string "irtualization". It may be worthwhile to at least 
ensure the character directly after "v" is a number.


Maybe I'm just being overly nitpicky here though.


+  AC_MSG_RESULT([$tool_version])
+  m4_if([$1], [], [], [
+  AS_CASE([$tool_version],
+  [$1 | $1.*], [],
+  [*], [AC_MSG_ERROR([DDlog version $1.x is required, but $tool is 
version $ddlog_version])])])


This error message should print $tool_version instead of $ddlog_version. 
$ddlog_version is no longer set, so this just always prints an empty 
string for the version if you have the wrong one installed.



+done
  
  AC_ARG_VAR([CARGO])

  AC_CHECK_PROGS([CARGO], [cargo], [none])
diff --git a/northd/automake.mk b/northd/automake.mk
index aaea7e1b1336..4fc81c17bfa3 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -50,13 +50,13 @@ northd_ovn_northd_ddlog_LDADD = \
  
  nb_opts = $$(cat $(srcdir)/northd/ovn-nb.dlopts)

  northd/OVN_Northbound.dl: ovn-nb.ovsschema northd/ovn-nb.dlopts
-   $(AM_V_GEN)ovsdb2ddlog -f $< --output-file $@ $(nb_opts)
+   $(AM_V_GEN)$(OVSDB2DDLOG) -f $< --output-file $@ $(nb_opts)
  northd/ovn-northd-ddlog-nb.inc: ovn-nb.ovsschema northd/ovn-nb.dlopts 
northd/ovsdb2ddlog2c
$(AM_V_GEN)$(run_python) $(srcdir)/northd/ovsdb2ddlog2c -p nb_ -f $< 
--output-file $@ $(nb_opts)
  
  sb_opts = $$(cat $(srcdir)/northd/ovn-sb.dlopts)

  northd/OVN_Southbound.dl: ovn-sb.ovsschema northd/ovn-sb.dlopts
-   $(AM_V_GEN)ovsdb2ddlog -f $< --output-file $@ $(sb_opts)
+   $(AM_V_GEN)$(OVSDB2DDLOG) -f $< --output-file $@ $(sb_opts)
  northd/ovn-northd-ddlog-sb.inc: ovn-sb.ovsschema northd/ovn-sb.dlopts 
northd/ovsdb2ddlog2c
$(AM_V_GEN)$(run_python) $(srcdir)/northd/ovsdb2ddlog2c -p sb_ -f $< 
--output-file $@ $(sb_opts)
  



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

Re: [ovs-dev] [PATCH ovn 1/5] ovn-controller: Add support for Logical_Flow control meters

[Lorenzo Bianconi]

> On 20/05/2021 21:26, Mark Michelson wrote:
> > I think this patch could use some ovn-controller tests to ensure that 
> > meters configured in the SB end up being applied to the resulting 
> > controller() actions.
> > 
> > On 4/29/21 1:04 PM, Lorenzo Bianconi wrote:
> >> From: Dumitru Ceara 
> >>
> >> Add a new 'controller_meter' column to OVN Southbound Logical_Flow
> >> table. This stores an optional string which should correspond to
> >> the Meter that must be used for rate limiting controller actions
> >> generated by packets hitting the flow.
> >>
> >> Add a new 'ofctrl_add_flow_metred' function to create a new 'ovn_flow'
> > 
> > This spelling is odd to me. I think it should be 
> > "ofctrl_add_flow_metered". I was worried when making this suggestion I 
> > was being US-centric with my spelling suggestion, but I think other 
> > variants of English also use the "meter" spelling instead of "metre" 
> > when referring to a regulating tool. Therefore, I think "metered" is 
> > preferred over "metred".
> > 
> > (Someone from Canada or from east of the Atlantic can correct me if I'm 
> > mistaken here).
> 
> Coming from Ireland, I do believe you are wrong ..
> 
> .. however, we do generally use the U.S.Americanised (notice I did not
> use a 'z' there) version of 'meter' so I think you are right.

ack, will fix it :)

Regards,
Lorenzo

> > 
> >> with an attached controller meter.
> >>
> >> Change ofctrl_check_and_add_flow to allow specifying a meter ID for
> >> packets that are punted to controller.
> >>
> >> Change consider_logical_flow to parse controller_meter from the logical
> >> flow and use it when building openflow entries.
> >>
> >> Add a new 'ctrl_meter_id' field to 'struct ovnact_encode_params' to be
> >> used when encoding controller actions from logical flow actions.
> >>
> >> Co-authored-by: Lorenzo Bianconi 
> >> Signed-off-by: Lorenzo Bianconi 
> >> Signed-off-by: Dumitru Ceara 
> >> ---
> >>   controller/lflow.c| 40 +++---
> >>   controller/ofctrl.c   | 54 ---
> >>   controller/ofctrl.h   | 21 ++
> >>   controller/physical.c |  7 +++--
> >>   include/ovn/actions.h |  2 ++
> >>   lib/actions.c | 66 +++
> >>   ovn-sb.ovsschema  |  6 ++--
> >>   ovn-sb.xml|  6 
> >>   8 files changed, 139 insertions(+), 63 deletions(-)
> >>
> >> diff --git a/controller/lflow.c b/controller/lflow.c
> >> index b8424e1fb..f3f901c32 100644
> >> --- a/controller/lflow.c
> >> +++ b/controller/lflow.c
> >> @@ -557,6 +557,27 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t 
> >> n_conjs)
> >>   return false;
> >>   }
> >>   
> >> +static void
> >> +lflow_parse_ctrl_meter(const struct sbrec_logical_flow *lflow,
> >> +   struct ovn_extend_table *meter_table,
> >> +   uint32_t *meter_id)
> >> +{
> >> +ovs_assert(meter_id);
> >> +*meter_id = NX_CTLR_NO_METER;
> >> +
> >> +if (lflow->controller_meter) {
> >> +*meter_id = ovn_extend_table_assign_id(meter_table,
> >> +   lflow->controller_meter,
> >> +   lflow->header_.uuid);
> >> +if (*meter_id == EXT_TABLE_ID_INVALID) {
> >> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> >> +VLOG_WARN_RL(, "Unable to assign id for meter: %s",
> >> + lflow->controller_meter);
> >> +return;
> >> +}
> >> +}
> >> +}
> >> +
> >>   static void
> >>   add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
> >> const struct sbrec_datapath_binding *dp,
> >> @@ -572,6 +593,13 @@ add_matches_to_flow_table(const struct 
> >> sbrec_logical_flow *lflow,
> >>   .dp = dp,
> >>   };
> >>   
> >> +/* Parse any meter to be used if this flow should punt packets to
> >> + * controller.
> >> + */
> >> +uint32_t ctrl_meter_id = NX_CTLR_NO_METER;
> >> +lflow_parse_ctrl_meter(lflow, l_ctx_out->meter_table,
> >> +   _meter_id);
> >> +
> >>   /* Encode OVN logical actions into OpenFlow. */
> >>   uint64_t ofpacts_stub[1024 / 8];
> >>   struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
> >> @@ -595,6 +623,7 @@ add_matches_to_flow_table(const struct 
> >> sbrec_logical_flow *lflow,
> >>   .ct_snat_vip_ptable = OFTABLE_CT_SNAT_FOR_VIP,
> >>   .fdb_ptable = OFTABLE_GET_FDB,
> >>   .fdb_lookup_ptable = OFTABLE_LOOKUP_FDB,
> >> +.ctrl_meter_id = ctrl_meter_id,
> >>   };
> >>   ovnacts_encode(ovnacts->data, ovnacts->size, , );
> >>   
> >> @@ -621,9 +650,11 @@ add_matches_to_flow_table(const struct 
> >> sbrec_logical_flow *lflow,
> >>   }
> >>   }
> >>   if (!m->n) {
> >> -ofctrl_add_flow(l_ctx_out->flow_table, ptable, 
> >> 

Re: [ovs-dev] [PATCH ovn 1/5] ovn-controller: Add support for Logical_Flow control meters

[Lorenzo Bianconi]

> I think this patch could use some ovn-controller tests to ensure that meters
> configured in the SB end up being applied to the resulting controller()
> actions.
> 
> On 4/29/21 1:04 PM, Lorenzo Bianconi wrote:
> > From: Dumitru Ceara 
> > 
> > Add a new 'controller_meter' column to OVN Southbound Logical_Flow
> > table. This stores an optional string which should correspond to
> > the Meter that must be used for rate limiting controller actions
> > generated by packets hitting the flow.

ack, I will add them in subsequent patch in order to use ovn-nbctl utilities

> > 
> > Add a new 'ofctrl_add_flow_metred' function to create a new 'ovn_flow'
> 
> This spelling is odd to me. I think it should be "ofctrl_add_flow_metered".
> I was worried when making this suggestion I was being US-centric with my
> spelling suggestion, but I think other variants of English also use the
> "meter" spelling instead of "metre" when referring to a regulating tool.
> Therefore, I think "metered" is preferred over "metred".
> 
> (Someone from Canada or from east of the Atlantic can correct me if I'm
> mistaken here).

ack, I will fix it in v2.

Regards,
Lorenzo

> 
> > with an attached controller meter.
> > 
> > Change ofctrl_check_and_add_flow to allow specifying a meter ID for
> > packets that are punted to controller.
> > 
> > Change consider_logical_flow to parse controller_meter from the logical
> > flow and use it when building openflow entries.
> > 
> > Add a new 'ctrl_meter_id' field to 'struct ovnact_encode_params' to be
> > used when encoding controller actions from logical flow actions.
> > 
> > Co-authored-by: Lorenzo Bianconi 
> > Signed-off-by: Lorenzo Bianconi 
> > Signed-off-by: Dumitru Ceara 
> > ---
> >   controller/lflow.c| 40 +++---
> >   controller/ofctrl.c   | 54 ---
> >   controller/ofctrl.h   | 21 ++
> >   controller/physical.c |  7 +++--
> >   include/ovn/actions.h |  2 ++
> >   lib/actions.c | 66 +++
> >   ovn-sb.ovsschema  |  6 ++--
> >   ovn-sb.xml|  6 
> >   8 files changed, 139 insertions(+), 63 deletions(-)
> > 
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index b8424e1fb..f3f901c32 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -557,6 +557,27 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t 
> > n_conjs)
> >   return false;
> >   }
> > +static void
> > +lflow_parse_ctrl_meter(const struct sbrec_logical_flow *lflow,
> > +   struct ovn_extend_table *meter_table,
> > +   uint32_t *meter_id)
> > +{
> > +ovs_assert(meter_id);
> > +*meter_id = NX_CTLR_NO_METER;
> > +
> > +if (lflow->controller_meter) {
> > +*meter_id = ovn_extend_table_assign_id(meter_table,
> > +   lflow->controller_meter,
> > +   lflow->header_.uuid);
> > +if (*meter_id == EXT_TABLE_ID_INVALID) {
> > +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > +VLOG_WARN_RL(, "Unable to assign id for meter: %s",
> > + lflow->controller_meter);
> > +return;
> > +}
> > +}
> > +}
> > +
> >   static void
> >   add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
> > const struct sbrec_datapath_binding *dp,
> > @@ -572,6 +593,13 @@ add_matches_to_flow_table(const struct 
> > sbrec_logical_flow *lflow,
> >   .dp = dp,
> >   };
> > +/* Parse any meter to be used if this flow should punt packets to
> > + * controller.
> > + */
> > +uint32_t ctrl_meter_id = NX_CTLR_NO_METER;
> > +lflow_parse_ctrl_meter(lflow, l_ctx_out->meter_table,
> > +   _meter_id);
> > +
> >   /* Encode OVN logical actions into OpenFlow. */
> >   uint64_t ofpacts_stub[1024 / 8];
> >   struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(ofpacts_stub);
> > @@ -595,6 +623,7 @@ add_matches_to_flow_table(const struct 
> > sbrec_logical_flow *lflow,
> >   .ct_snat_vip_ptable = OFTABLE_CT_SNAT_FOR_VIP,
> >   .fdb_ptable = OFTABLE_GET_FDB,
> >   .fdb_lookup_ptable = OFTABLE_LOOKUP_FDB,
> > +.ctrl_meter_id = ctrl_meter_id,
> >   };
> >   ovnacts_encode(ovnacts->data, ovnacts->size, , );
> > @@ -621,9 +650,11 @@ add_matches_to_flow_table(const struct 
> > sbrec_logical_flow *lflow,
> >   }
> >   }
> >   if (!m->n) {
> > -ofctrl_add_flow(l_ctx_out->flow_table, ptable, lflow->priority,
> > -lflow->header_.uuid.parts[0], >match, 
> > ,
> > ->header_.uuid);
> > +ofctrl_add_flow_metred(l_ctx_out->flow_table, ptable,
> > +   lflow->priority,
> > +   

Re: [ovs-dev] [PATCH v2] ofproto-dpif-xlate: avoid successive ct_clear datapath actions

[Timothy Redaelli]

On Tue, 18 May 2021 06:17:48 -0400
Eelco Chaudron  wrote:

> Due to flow lookup optimizations, especially in the resubmit/clone cases,
> we might end up with multiple ct_clear actions, which are not necessary.
> 
> This patch only adds the ct_clear action to the datapath if any ct state
> is tracked.
> 
> Signed-off-by: Eelco Chaudron 
> ---
> v2: Insert ct_clear only when ct information is tracked vs tracking successive
> ct_clear actions.
> 
> ofproto/ofproto-dpif-xlate.c |4 +++-
>  tests/ofproto-dpif.at|   25 +
>  2 files changed, 28 insertions(+), 1 deletion(-)


Acked-By: Timothy Redaelli 

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

Re: [ovs-dev] [PATCH ovn v3] ovn-sbctl: Add logical flows count numbers

[Alexey Roytman]

Hi Mark,
Thanks for the comments.
With a delay, but I've fixed the patch.
Mistakenly, I used a slightly different email subject, so please see the
fixes in the [ovs-dev] [PATCH ovn v5] ovn-sbctl.c Add logical flows count
numbers 
thread.
Thanks Alexey.

On Mon, May 10, 2021 at 11:42 PM Mark Michelson  wrote:

> Hi Alexey,
>
> In general, the C side of things looks good. I recommend that you run
> ./utilities/checkpatch.py against the resulting patch, though.
> checkpatch.py recently had a bug fixed in it so that it correctly
> catches lines that are in excess of 80 characters now. You'll find that
> there are several violations in this patch.
>
> See below for comments about the added test.
>
> On 4/13/21 11:18 AM, Alexey Roytman wrote:
> > From: Alexey Roytman 
> >
> > For big scale deployments, when number of logical flows can be 2M+,
> > sometimes users just need to know the total number of logical flows
> > and numbers of logical flows per table/per datapath.
> >
> > New command output
> > Datapath: "sw1" (4b1e53d8-9f0f-4768-b4a6-6cbc58a4bfda)  Pipeline: ingress
> >table=0 (ls_in_port_sec_l2  ) lflows=2
> >table=1 (ls_in_port_sec_ip  ) lflows=1
> >table=2 (ls_in_port_sec_nd  ) lflows=1
> >table=3 (ls_in_lookup_fdb   ) lflows=1
> >table=4 (ls_in_put_fdb  ) lflows=1
> >table=5 (ls_in_pre_acl  ) lflows=2
> >table=6 (ls_in_pre_lb   ) lflows=3
> >table=7 (ls_in_pre_stateful ) lflows=2
> >table=8 (ls_in_acl_hint ) lflows=1
> >table=9 (ls_in_acl  ) lflows=2
> >table=10(ls_in_qos_mark ) lflows=1
> >table=11(ls_in_qos_meter) lflows=1
> >table=12(ls_in_lb   ) lflows=1
> >table=13(ls_in_stateful ) lflows=8
> >table=14(ls_in_pre_hairpin  ) lflows=1
> >table=15(ls_in_nat_hairpin  ) lflows=1
> >table=16(ls_in_hairpin  ) lflows=1
> >table=17(ls_in_arp_rsp  ) lflows=1
> >table=18(ls_in_dhcp_options ) lflows=1
> >table=19(ls_in_dhcp_response) lflows=1
> >table=20(ls_in_dns_lookup   ) lflows=1
> >table=21(ls_in_dns_response ) lflows=1
> >table=22(ls_in_external_port) lflows=1
> >table=23(ls_in_l2_lkup  ) lflows=3
> >table=24(ls_in_l2_unknown   ) lflows=2
> > Total number of logical flows in the datapath = 41
> >
> > Datapath: "sw1" (4b1e53d8-9f0f-4768-b4a6-6cbc58a4bfda)  Pipeline: egress
> >table=0 (ls_out_pre_lb  ) lflows=3
> >table=1 (ls_out_pre_acl ) lflows=2
> >table=2 (ls_out_pre_stateful) lflows=2
> >table=3 (ls_out_lb  ) lflows=1
> >table=4 (ls_out_acl_hint) lflows=1
> >table=5 (ls_out_acl ) lflows=2
> >table=6 (ls_out_qos_mark) lflows=1
> >table=7 (ls_out_qos_meter   ) lflows=1
> >table=8 (ls_out_stateful) lflows=3
> >table=9 (ls_out_port_sec_ip ) lflows=1
> >table=10(ls_out_port_sec_l2 ) lflows=1
> > Total number of logical flows in the datapath = 18
> >
> > Total number of logical flows =  59
> >
> > Signed-off-by: Alexey Roytman 
> > ---
> >   tests/ovn-sbctl.at   | 29 
> >   utilities/ovn-sbctl.8.in |  5 +++
> >   utilities/ovn-sbctl.c| 72 
> >   3 files changed, 99 insertions(+), 7 deletions(-)
> >
> > diff --git a/tests/ovn-sbctl.at b/tests/ovn-sbctl.at
> > index 2712cc154..b15b15964 100644
> > --- a/tests/ovn-sbctl.at
> > +++ b/tests/ovn-sbctl.at
> > @@ -148,3 +148,32 @@ inactivity_probe: 3
> >
> >   OVN_SBCTL_TEST_STOP
> >   AT_CLEANUP
> > +
> > +dnl
> -
> > +
> > +AT_SETUP([ovn-sbctl --count lflow-list/dump-flows])
> > +OVN_SBCTL_TEST_START
> > +
> > +AT_CHECK([ovn-nbctl ls-add count-test])
>
> You can use the "check" function here as a shortcut since you are not
> checking any output:
>
> check ovn-nbctl ls-add count-test
>
> > +AT_CHECK([ovn-sbctl --count lflow-list |  cat < > +import sys
> > +
> > +lflows=0
> > +dp_flows=0
> > +total_flows=0
> > +
> > +for line in sys.stdin:
> > +x = line.rsplit("=", 1)
> > +if "table=" in line:
> > +   lflows += int(x[1])
> > +elif "flows in the datapath" in line:
> > +   dp_flows += int(x[1])
> > +elif "Total number of logical flows =" in line:
> > +   total_flows = int(x[1])
> > +if lflows == dp_flows == total_flows:
> > +sys.exit(0)
> > +sys.exit(1)
> > +EOF], [0], [ignore], [ignore])
> > +
> > +OVN_SBCTL_TEST_STOP
> > +AT_CLEANUP
>
> The issue I have with this test is that it doesn't check the values
> against any expected result. It ensures that the values reported are
> internally consistent, but it does not ensure that the counts are correct.
>
> In an ideal world, you'd be able to write up some logical networks with
> known numbers of generated logical flows. Then you'd run the test
> command and ensure that the flows are what you expect. However, being
> able to accurately predict the 

Re: [ovs-dev] [PATCH] dpif-netdev.c : Fix indentation.

[Ilya Maximets]

On 5/19/21 5:48 PM, lin huang wrote:
> dpif-netdev.c : Fix indentation.
> Add extra space to fix indentation.
> 
> Signed-off-by: miter 
> ---
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 816945375..dc77fa2fa 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -7404,7 +7404,7 @@ dp_netdev_input__(struct dp_netdev_pmd_thread *pmd,
>  enum { PKT_ARRAY_SIZE = NETDEV_MAX_BURST };
>  #endif
>  OVS_ALIGNED_VAR(CACHE_LINE_SIZE)
> -struct netdev_flow_key keys[PKT_ARRAY_SIZE];
> +struct netdev_flow_key keys[PKT_ARRAY_SIZE];

Hi.  This is a continuation of a previous line. So, the
indentation is correct.

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

Re: [ovs-dev] [PATCH v2] dpif-netdev: Forwarding optimization for direct output flows.

[0-day Robot]

Bleep bloop.  Greetings Sriharsha Basavapatna, 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: Sriharsha Basavapatna 
Lines checked: 587, Warnings: 1, Errors: 0


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

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

[ovs-dev] [PATCH v2] dpif-netdev: Forwarding optimization for direct output flows.

[Sriharsha Basavapatna via dev]

From: Ilya Maximets 

There are some cases where users want to have simple forwarding or drop
rules for all packets received from particular port, i.e :

  "in_port=1,actions=2"
  "in_port=1,actions=IN_PORT"
  "in_port=1,actions=drop"

There are also cases where complex OF flows could be simplified down
to simple forwarding/drop datapath flows.

In theory, we don't need to parse packets at all to follow these flows.
"Direct output forwarding" optimization is intended to speed up above
cases.

Design:

Due to various implementation restrictions userspace datapath has
following flow fields always in exact match (i.e. it's required to
match at least these fields of a packet even if the OF rule doesn't
need that):

  - recirc_id
  - in_port
  - packet_type
  - dl_type
  - vlan_tci
  - nw_frag (for ip packets)

Not all of these fields are related to packet itself. We already
know the current 'recirc_id' and the 'in_port' before starting the
packet processing. It also seems safe to assume that we're working
with Ethernet packets. dpif-netdev sets exact match on 'vlan_tci'
to avoid issues with flow format conversion and we don't really need
to match with it until ofproto layer didn't ask us to.
So, for the simple forwarding OF rule we need to match only with
'dl_type' and 'nw_frag'.

'in_port', 'dl_type' and 'nw_frag' could be combined in a single
64bit integer that could be used as a hash in hash map.

New per-PMD flow table 'direct_output_table' introduced to store
direct output flows only. 'dp_netdev_flow_add' adds flow to the
usual 'flow_table' and to 'direct_output_table' if the flow meets
following constraints:

  - 'recirc_id' in flow match is 0.
  - 'packet_type' in flow match is Ethernet.
  - Flow wildcards originally had wildcarded 'vlan_tci'.
  - Flow has no actions (drop) or exactly one action equal to
OVS_ACTION_ATTR_OUTPUT.
  - Flow wildcards contains only minimal set of non-wildcarded fields
(listed above).

If the number of flows for current 'in_port' in regular 'flow_table'
equals number of flows for current 'in_port' in 'direct_output_table',
we may use direct output optimization, because all the flows we have
are direct output flows. This means that we only need to parse
'dl_type' and 'nw_frag' to perform packet matching.
Now we making the unique flow mark from the 'in_port', 'dl_type' and
'nw_frag' and looking for it in 'direct_output_table'.
On successful lookup we don't need to make full 'miniflow_extract()'.

Unsuccessful lookup technically means that we have no sufficient flow
in datapath and upcall will be required. We may optimize this path
in the future by bypassing the EMC, SMC and dpcls lookups in this case.

Performance improvement of this solution on a 'direct output' flows
should be comparable with partial HW offloading, because it parses same
packet fields and uses similar flow lookup scheme.
However, unlike partial HW offloading, it works for all port types
including virtual ones.

Signed-off-by: Ilya Maximets 
Signed-off-by: Sriharsha Basavapatna 

---
v1->v2:
Rebased to master branch.
Added a coverage counter.
---

 lib/dpif-netdev.c | 263 +++---
 lib/flow.c|  12 ++-
 lib/flow.h|   4 +-
 3 files changed, 259 insertions(+), 20 deletions(-)

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 650e67ab3..ec09c67cd 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -35,6 +35,7 @@
 
 #include "bitmap.h"
 #include "cmap.h"
+#include "ccmap.h"
 #include "conntrack.h"
 #include "conntrack-tp.h"
 #include "coverage.h"
@@ -114,6 +115,7 @@ COVERAGE_DEFINE(datapath_drop_invalid_port);
 COVERAGE_DEFINE(datapath_drop_invalid_bond);
 COVERAGE_DEFINE(datapath_drop_invalid_tnl_port);
 COVERAGE_DEFINE(datapath_drop_rx_invalid_packet);
+COVERAGE_DEFINE(datapath_direct_output_packet);
 
 /* Protects against changes to 'dp_netdevs'. */
 static struct ovs_mutex dp_netdev_mutex = OVS_MUTEX_INITIALIZER;
@@ -543,6 +545,8 @@ struct dp_netdev_flow {
 /* Hash table index by unmasked flow. */
 const struct cmap_node node; /* In owning dp_netdev_pmd_thread's */
  /* 'flow_table'. */
+const struct cmap_node direct_output_node; /* In dp_netdev_pmd_thread's
+ 'direct_output_table'. */
 const struct cmap_node mark_node; /* In owning flow_mark's mark_to_flow */
 const ovs_u128 ufid; /* Unique flow identifier. */
 const ovs_u128 mega_ufid;/* Unique mega flow identifier. */
@@ -556,7 +560,8 @@ struct dp_netdev_flow {
 struct ovs_refcount ref_cnt;
 
 bool dead;
-uint32_t mark;   /* Unique flow mark assigned to a flow */
+uint32_t mark;   /* Unique flow mark for netdev offloading. */
+uint64_t direct_output_mark; /* Unique flow mark for direct output. */
 
 /* Statistics. */
 struct dp_netdev_flow_stats stats;
@@ -690,12 +695,19 @@ struct dp_netdev_pmd_thread {
 
 

Re: [ovs-dev] [PATCH v2] Improve concurrency by adjust flow-limit

[taoyunupt]

Hi Ben,
Please help to review this patch ,which to optimize the flow-limit 
adjustment.


The v1 patch links to 
http://patchwork.ozlabs.org/project/openvswitch/patch/20210511075200.19037-1-taoyunxi...@cmss.chinamobile.com/




Thanks,
Yun


At 2021-05-17 15:22:48, "Tao YunXiang"  wrote:
>OVS uses a set of gentle mechanisms to control the number of flows in 
>datapath. However, it is difficult for the number of datapath flows to 
>reach a high level in a short time.
>
>Through some tests with Apache benchmark, I found the point is that
>the value of flow-limit increases too slowly, but decreases very quickly. 
>
>This change has two improvements. One is the flow-limit will adjust more 
>linearly. The other one is, when the duration is small, the flow-limit will
>increase faster.
>
>Through this change, the test result of Apache benchmark can reach 12k from 5k,
>the test command is as follows:
>
>ab -n 20 -c 500 -r http://1.1.1.2/
>
>v1->v2: Make flow-limit changes linearly. Add a limitation to make flow_limit
>won't overflow UINT_MAX.
>
>
>Signed-off-by: Tao YunXiang 
>Cc: Ben Pfaff 
>
>---
> ofproto/ofproto-dpif-upcall.c | 20 +---
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
>diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>index ccf97266c..805409165 100644
>--- a/ofproto/ofproto-dpif-upcall.c
>+++ b/ofproto/ofproto-dpif-upcall.c
>@@ -963,14 +963,20 @@ udpif_revalidator(void *arg)
> 
> duration = MAX(time_msec() - start_time, 1);
> udpif->dump_duration = duration;
>-if (duration > 2000) {
>-flow_limit /= duration / 1000;
>-} else if (duration > 1300) {
>-flow_limit = flow_limit * 3 / 4;
>-} else if (duration < 1000 &&
>-   flow_limit < n_flows * 1000 / duration) {
>-flow_limit += 1000;
>+
>+/* Use duration as a reference, adjust the value of flow_limit,
>+ * when the duration is short, increase the flow_limit, and vice
>+ * versa. The purpose of using multiple conversions between int
>+ * and float is to make the flow_limit change more linear. */ 
>+
>+if (duration >= 1000) {
>+flow_limit = (int) ((float) (flow_limit) / (duration / 
>1000.0));
>+} else if (flow_limit * (1000.0 / duration) >= UINT_MAX) {
>+flow_limit = ofproto_flow_limit;
>+} else {
>+flow_limit = (int) (flow_limit * (1000.0 / duration));
> }
>+
> flow_limit = MIN(ofproto_flow_limit, MAX(flow_limit, 1000));
> atomic_store_relaxed(>flow_limit, flow_limit);
> 
>-- 
>2.17.1
>
>
>
>___
>dev mailing list
>d...@openvswitch.org
>https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev