Re: [ovs-dev] [PATCH v2] doc: Fix description of max_len for controller action.

[Ilya Maximets]

On 8/26/23 00:51, Antonin Bas wrote:
> Hi Ilya,
> 
> Thanks for applying the patch.
> I just finished testing / investigating output(port=controller,max_len=X), 
> and there is no need to patch the documentation, for the following reasons:
> 
> The output(port=controller,max_len=X) action is rejected by OVS:
> root@ovs-test-node-1:/home/vagrant# ovs-ofctl mod-flows br-int 
> 'table=0,priority=100,in_port=veth1,ip 
> actions=output(port=controller,max_len=128)'
> ovs-ofctl: output to unsupported truncate port: controller
> root@ovs-test-node-1:/home/vagrant#
> root@ovs-test-node-1:/home/vagrant#
> root@ovs-test-node-1:/home/vagrant# ovs-ofctl mod-flows br-int 
> 'table=0,priority=100,in_port=veth1,ip actions=output(port=1,max_len=128)'
> root@ovs-test-node-1:/home/vagrant#
> 
> This behavior is expected and already documented:
> /The port may also be one of the following additional OpenFlow ports, unless 
> max_len is specified:/
> /.../
> /controller/
> /.../
> 
> When using max_len with the output action, the truncation is the 
> responsibility of the datapath, and is not available for controller packets.
> When using max_lengh with the controller action, the truncation was meant to 
> be the responsibility of the userspace, but buffering is no longer available 
> in ovs-vswitchd.

OK, nice.  Thanks for checking!

Best regards, Ilya Maximets.

> 
> Thanks,
> 
> Antonin
> 
> Le ven. 25 août 2023 à 14:58, Ilya Maximets  > a écrit :
> 
> On 8/25/23 18:38, Antonin Bas wrote:
> > Hi Ilya,
> >
> > Do you think we can merge this patch first as it is specific to the 
> controller action?
> > I will check the behavior of output(port=controller,max_len=X) and once 
> I have confirmation, I can submit a new patch.
> 
> OK.  I applied the change now.
> Also backported down to 2.17.
> 
> Thanks!
> 
> Best regards, Ilya Maximets.
> 
> >
> > Thanks,
> >
> > Antonin
> >
> > Le ven. 25 août 2023 à 07:10, Ilya Maximets    >> a écrit :
> >
> >     On 8/17/23 02:30, Antonin Bas wrote:
> >     > From: Antonin Bas    >>
> >     >
> >     > From: Antonin Bas mailto:a...@vmware.com> 
> >>
> >     >
> >     > Since Open vSwitch 2.7, the max_len option has no effect, and the 
> full
> >     > packet is always sent to controllers. This was confirmed with 
> both the
> >     > kernel and netdev datapaths.
> >
> >     Hi, Antonin.  Thanks for the patch!  And sorry for delay.
> >
> >     IIUC, this also affects the output(port=controller,max_len=X) 
> actions, right?
> >     In this case, we need to update the docs for the 'output' action as 
> well.
> >
> >     Best regards, Ilya Maximets.
> >
> >     >
> >     > Reported-by: Antonin Bas   >>
> >     > Reported-at: https://github.com/openvswitch/ovs-issues/issues/295 
>  
>  >
> >     > Signed-off-by: Antonin Bas   >>
> >     > ---
> >     > v2: Fix typos, set author to VMware email address
> >     > ---
> >     >  Documentation/ref/ovs-actions.7.rst | 9 -
> >     >  1 file changed, 8 insertions(+), 1 deletion(-)
> >     >
> >     > diff --git a/Documentation/ref/ovs-actions.7.rst 
> b/Documentation/ref/ovs-actions.7.rst
> >     > index d13895655..36adcc5db 100644
> >     > --- a/Documentation/ref/ovs-actions.7.rst
> >     > +++ b/Documentation/ref/ovs-actions.7.rst
> >     > @@ -694,7 +694,8 @@ encapsulated in an OpenFlow ``packet-in`` 
> message.  The supported options are:
> >     >      Limit to *max_len* the number of bytes of the packet to send 
> in the
> >     >      ``packet-in.``  A *max_len* of 0 prevents any of the packet 
> from being
> >     >      sent (thus, only metadata is included).  By default, the 
> entire packet is
> >     > -    sent, equivalent to a *max_len* of 65535.
> >     > +    sent, equivalent to a *max_len* of 65535.  This option has 
> no effect in
> >     > +    Open vSwith 2.7 and later: the entire packet will always be 
> sent.
> >     > 
> >     >    ``reason=``\ *reason*
> >     >      Specify *reason* as the reason for sending the message in the
> >     > @@ -733,6 +734,12 @@ encapsulated in an OpenFlow ``packet-in`` 
> message.  The supported options are:
> >     >    options require the Open vSwitch ``NXAST_CONTROLLER`` 
> extension action added
> >     > 

Re: [ovs-dev] [PATCH v2] doc: Fix description of max_len for controller action.

[Antonin Bas]

Hi Ilya,

Thanks for applying the patch.
I just finished testing / investigating output(port=controller,max_len=X),
and there is no need to patch the documentation, for the following reasons:

The output(port=controller,max_len=X) action is rejected by OVS:
root@ovs-test-node-1:/home/vagrant# ovs-ofctl mod-flows br-int
'table=0,priority=100,in_port=veth1,ip
actions=output(port=controller,max_len=128)'
ovs-ofctl: output to unsupported truncate port: controller
root@ovs-test-node-1:/home/vagrant#
root@ovs-test-node-1:/home/vagrant#
root@ovs-test-node-1:/home/vagrant# ovs-ofctl mod-flows br-int
'table=0,priority=100,in_port=veth1,ip actions=output(port=1,max_len=128)'
root@ovs-test-node-1:/home/vagrant#

This behavior is expected and already documented:
*The port may also be one of the following additional OpenFlow ports,
unless max_len is specified:*
*...*
*controller*
*...*

When using max_len with the output action, the truncation is the
responsibility of the datapath, and is not available for controller packets.
When using max_lengh with the controller action, the truncation was meant
to be the responsibility of the userspace, but buffering is no longer
available in ovs-vswitchd.

Thanks,

Antonin

Le ven. 25 août 2023 à 14:58, Ilya Maximets  a écrit :

> On 8/25/23 18:38, Antonin Bas wrote:
> > Hi Ilya,
> >
> > Do you think we can merge this patch first as it is specific to the
> controller action?
> > I will check the behavior of output(port=controller,max_len=X) and once
> I have confirmation, I can submit a new patch.
>
> OK.  I applied the change now.
> Also backported down to 2.17.
>
> Thanks!
>
> Best regards, Ilya Maximets.
>
> >
> > Thanks,
> >
> > Antonin
> >
> > Le ven. 25 août 2023 à 07:10, Ilya Maximets  i.maxim...@ovn.org>> a écrit :
> >
> > On 8/17/23 02:30, Antonin Bas wrote:
> > > From: Antonin Bas  antonin@gmail.com>>
> > >
> > > From: Antonin Bas mailto:a...@vmware.com>>
> > >
> > > Since Open vSwitch 2.7, the max_len option has no effect, and the
> full
> > > packet is always sent to controllers. This was confirmed with both
> the
> > > kernel and netdev datapaths.
> >
> > Hi, Antonin.  Thanks for the patch!  And sorry for delay.
> >
> > IIUC, this also affects the output(port=controller,max_len=X)
> actions, right?
> > In this case, we need to update the docs for the 'output' action as
> well.
> >
> > Best regards, Ilya Maximets.
> >
> > >
> > > Reported-by: Antonin Bas mailto:a...@vmware.com
> >>
> > > Reported-at: https://github.com/openvswitch/ovs-issues/issues/295
> 
> > > Signed-off-by: Antonin Bas  a...@vmware.com>>
> > > ---
> > > v2: Fix typos, set author to VMware email address
> > > ---
> > >  Documentation/ref/ovs-actions.7.rst | 9 -
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/ref/ovs-actions.7.rst
> b/Documentation/ref/ovs-actions.7.rst
> > > index d13895655..36adcc5db 100644
> > > --- a/Documentation/ref/ovs-actions.7.rst
> > > +++ b/Documentation/ref/ovs-actions.7.rst
> > > @@ -694,7 +694,8 @@ encapsulated in an OpenFlow ``packet-in``
> message.  The supported options are:
> > >  Limit to *max_len* the number of bytes of the packet to send
> in the
> > >  ``packet-in.``  A *max_len* of 0 prevents any of the packet
> from being
> > >  sent (thus, only metadata is included).  By default, the
> entire packet is
> > > -sent, equivalent to a *max_len* of 65535.
> > > +sent, equivalent to a *max_len* of 65535.  This option has no
> effect in
> > > +Open vSwith 2.7 and later: the entire packet will always be
> sent.
> > >
> > >``reason=``\ *reason*
> > >  Specify *reason* as the reason for sending the message in the
> > > @@ -733,6 +734,12 @@ encapsulated in an OpenFlow ``packet-in``
> message.  The supported options are:
> > >options require the Open vSwitch ``NXAST_CONTROLLER`` extension
> action added
> > >in Open vSwitch 1.6.
> > >
> > > +  Open vSwitch 2.7 and later is configured to not buffer packets
> for the
> > > +  packet-in event.  As a result, the full packet is always sent to
> > > +  controllers.  This means that the ``max_len`` option has no
> effect on the
> > > +  ``controller`` action, and all values (even 0) are equivalent
> to the default
> > > +  value of 65535.
> > > +
> > >
> > >  The ``enqueue`` action
> > >  --
> >
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 0/3] Python ovsdb bindings distribution improvements

[Ilya Maximets]

On 8/23/23 16:28, Robin Jarry wrote:
> v2:
> 
> * Keep code in the python dir but rename it so that it does not clash
>   with `python -m build`.
> * Remove duplication in makefiles.
> 
> Robin Jarry (3):
>   python: Rename build related code to ovs_build_helpers.
>   python: Use twine to upload sdist package to pypi.org.
>   python: Use build to generate PEP517 compatible archives.
> 
>  Makefile.am   |  2 +-
>  build-aux/extract-ofp-fields  | 12 -
>  build-aux/gen_ofp_field_decoders  |  4 +--
>  build-aux/sodepends.py|  3 ++-
>  build-aux/soexpand.py |  3 ++-
>  build-aux/xml2nroff   | 10 
>  ovsdb/ovsdb-doc   |  2 +-
>  python/automake.mk| 25 +++
>  .../{build => ovs_build_helpers}/__init__.py  |  0
>  .../extract_ofp_fields.py |  0
>  python/{build => ovs_build_helpers}/nroff.py  |  0
>  python/{build => ovs_build_helpers}/soutil.py |  0
>  12 files changed, 33 insertions(+), 28 deletions(-)
>  rename python/{build => ovs_build_helpers}/__init__.py (100%)
>  rename python/{build => ovs_build_helpers}/extract_ofp_fields.py (100%)
>  rename python/{build => ovs_build_helpers}/nroff.py (100%)
>  rename python/{build => ovs_build_helpers}/soutil.py (100%)
> 

Thanks, Robin!  Applied.

Also backported down to 2.17, since we may need to release
python packages on older branches as well.

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

Re: [ovs-dev] [PATCH] dpif-netdev: Fix length calculation of netdet_flow_key.

[Ilya Maximets]

On 8/24/23 17:12, Eelco Chaudron wrote:
> 
> 
> On 13 Aug 2023, at 11:08, Zhiqi Chen via dev wrote:
> 
>> The 'len' of a netdev_flow_key initialized by netdev_flow_key_init()
>> is always zero, which may cause errors when cloning a netdev_flow_key
>> by netdev_flow_key_clone().
>>
>> Currently the 'len' member of a netdev_flow_key initialized by
>> netdev_flow_key_init() is not used, so this error will not cause any
>> bad behavior for now.
>>
>> Fixes: c82f496c3b69 ("dpif-netdev: Use unmasked key when adding datapath 
>> flows.")
>> Signed-off-by: Zhiqi Chen 
> 
> 
> Thanks for fixing this issue, the changes look good to me!
> 
> Acked-by: Eelco Chaudron 


Thanks, Zhiqi and Eelco!  Applied and backported down to 2.17.

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

Re: [ovs-dev] [PATCH v2] doc: Fix description of max_len for controller action.

[Ilya Maximets]

On 8/25/23 18:38, Antonin Bas wrote:
> Hi Ilya,
> 
> Do you think we can merge this patch first as it is specific to the 
> controller action?
> I will check the behavior of output(port=controller,max_len=X) and once I 
> have confirmation, I can submit a new patch.

OK.  I applied the change now.
Also backported down to 2.17.

Thanks!

Best regards, Ilya Maximets.

> 
> Thanks,
> 
> Antonin
> 
> Le ven. 25 août 2023 à 07:10, Ilya Maximets  > a écrit :
> 
> On 8/17/23 02:30, Antonin Bas wrote:
> > From: Antonin Bas mailto:antonin@gmail.com>>
> >
> > From: Antonin Bas mailto:a...@vmware.com>>
> >
> > Since Open vSwitch 2.7, the max_len option has no effect, and the full
> > packet is always sent to controllers. This was confirmed with both the
> > kernel and netdev datapaths.
> 
> Hi, Antonin.  Thanks for the patch!  And sorry for delay.
> 
> IIUC, this also affects the output(port=controller,max_len=X) actions, 
> right?
> In this case, we need to update the docs for the 'output' action as well.
> 
> Best regards, Ilya Maximets.
> 
> >
> > Reported-by: Antonin Bas mailto:a...@vmware.com>>
> > Reported-at: https://github.com/openvswitch/ovs-issues/issues/295 
> 
> > Signed-off-by: Antonin Bas mailto:a...@vmware.com>>
> > ---
> > v2: Fix typos, set author to VMware email address
> > ---
> >  Documentation/ref/ovs-actions.7.rst | 9 -
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ref/ovs-actions.7.rst 
> b/Documentation/ref/ovs-actions.7.rst
> > index d13895655..36adcc5db 100644
> > --- a/Documentation/ref/ovs-actions.7.rst
> > +++ b/Documentation/ref/ovs-actions.7.rst
> > @@ -694,7 +694,8 @@ encapsulated in an OpenFlow ``packet-in`` message.  
> The supported options are:
> >      Limit to *max_len* the number of bytes of the packet to send in the
> >      ``packet-in.``  A *max_len* of 0 prevents any of the packet from 
> being
> >      sent (thus, only metadata is included).  By default, the entire 
> packet is
> > -    sent, equivalent to a *max_len* of 65535.
> > +    sent, equivalent to a *max_len* of 65535.  This option has no 
> effect in
> > +    Open vSwith 2.7 and later: the entire packet will always be sent.
> > 
> >    ``reason=``\ *reason*
> >      Specify *reason* as the reason for sending the message in the
> > @@ -733,6 +734,12 @@ encapsulated in an OpenFlow ``packet-in`` message. 
>  The supported options are:
> >    options require the Open vSwitch ``NXAST_CONTROLLER`` extension 
> action added
> >    in Open vSwitch 1.6.
> > 
> > +  Open vSwitch 2.7 and later is configured to not buffer packets for 
> the
> > +  packet-in event.  As a result, the full packet is always sent to
> > +  controllers.  This means that the ``max_len`` option has no effect 
> on the
> > +  ``controller`` action, and all values (even 0) are equivalent to the 
> default
> > +  value of 65535.
> > +
> > 
> >  The ``enqueue`` action
> >  --
> 

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

Re: [ovs-dev] [PATCH v13 3/4] lib, ovsdb, vtep: Add various null pointer checks

[Aaron Conole]

James Raphael Tiovalen  writes:

> This commit adds various null pointer checks to some files in the `lib`
> and the `ovsdb` directories to fix several Coverity defects. These
> changes are grouped together as they perform similar checks, returning
> early or skipping some action if a null pointer is encountered.
>
> Signed-off-by: James Raphael Tiovalen 
> Reviewed-by: Simon Horman 
> ---
>  lib/dp-packet.c|  8 
>  lib/dpctl.c| 12 
>  lib/shash.c|  4 
>  lib/sset.c |  5 +
>  ovsdb/jsonrpc-server.c |  2 +-
>  ovsdb/monitor.c| 11 +--
>  ovsdb/ovsdb-client.c   |  7 ++-
>  ovsdb/row.c|  5 -
>  vtep/vtep-ctl.c|  5 +
>  9 files changed, 50 insertions(+), 9 deletions(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 6e3a8297a..a9cf4bfc1 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -357,7 +357,7 @@ void *
>  dp_packet_put_zeros(struct dp_packet *b, size_t size)
>  {
>  void *dst = dp_packet_put_uninit(b, size);
> -memset(dst, 0, size);
> +nullable_memset(dst, 0, size);
>  return dst;
>  }
>  
> @@ -368,7 +368,7 @@ void *
>  dp_packet_put(struct dp_packet *b, const void *p, size_t size)
>  {
>  void *dst = dp_packet_put_uninit(b, size);
> -memcpy(dst, p, size);
> +nullable_memcpy(dst, p, size);
>  return dst;
>  }
>  
> @@ -440,7 +440,7 @@ void *
>  dp_packet_push_zeros(struct dp_packet *b, size_t size)
>  {
>  void *dst = dp_packet_push_uninit(b, size);
> -memset(dst, 0, size);
> +nullable_memset(dst, 0, size);
>  return dst;
>  }
>  
> @@ -451,7 +451,7 @@ void *
>  dp_packet_push(struct dp_packet *b, const void *p, size_t size)
>  {
>  void *dst = dp_packet_push_uninit(b, size);
> -memcpy(dst, p, size);
> +nullable_memcpy(dst, p, size);
>  return dst;
>  }
>  
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 4394653ab..013919572 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> @@ -336,6 +336,12 @@ dpctl_add_if(int argc OVS_UNUSED, const char *argv[],
>  value = "";
>  }
>  
> +if (!key) {
> +dpctl_error(dpctl_p, 0, "key is NULL");
> +error = EINVAL;
> +goto next;
> +}
> +
>  if (!strcmp(key, "type")) {
>  type = value;
>  } else if (!strcmp(key, "port_no")) {
> @@ -454,6 +460,12 @@ dpctl_set_if(int argc, const char *argv[], struct 
> dpctl_params *dpctl_p)
>  value = "";
>  }
>  
> +if (!key) {
> +dpctl_error(dpctl_p, 0, "key is NULL");
> +error = EINVAL;
> +goto next_destroy_args;
> +}
> +
>  if (!strcmp(key, "type")) {
>  if (strcmp(value, type)) {
>  dpctl_error(dpctl_p, 0,
> diff --git a/lib/shash.c b/lib/shash.c
> index 2bfc8eb50..b62b586fc 100644
> --- a/lib/shash.c
> +++ b/lib/shash.c
> @@ -205,6 +205,10 @@ shash_delete(struct shash *sh, struct shash_node *node)
>  char *
>  shash_steal(struct shash *sh, struct shash_node *node)
>  {
> +if (!node) {
> +return NULL;
> +}
> +
>  char *name = node->name;

We typically like to declare variables at the head of the function, even
if it is allowed by the compiler.  That said, I guess this probably
isn't too big of a deal - it gets a bit mixed all over the code base.

>  hmap_remove(>map, >node);
> diff --git a/lib/sset.c b/lib/sset.c
> index aa1790020..fda268129 100644
> --- a/lib/sset.c
> +++ b/lib/sset.c
> @@ -261,6 +261,11 @@ char *
>  sset_pop(struct sset *set)
>  {
>  const char *name = SSET_FIRST(set);
> +
> +if (!name) {
> +return NULL;
> +}
> +
>  char *copy = xstrdup(name);
>  sset_delete(set, SSET_NODE_FROM_NAME(name));
>  return copy;
> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> index 17868f5b7..5361b3c76 100644
> --- a/ovsdb/jsonrpc-server.c
> +++ b/ovsdb/jsonrpc-server.c
> @@ -1038,7 +1038,7 @@ ovsdb_jsonrpc_session_got_request(struct 
> ovsdb_jsonrpc_session *s,
>   request->id);
>  } else if (!strcmp(request->method, "get_schema")) {
>  struct ovsdb *db = ovsdb_jsonrpc_lookup_db(s, request, );
> -if (!reply) {
> +if (db && !reply) {

I guess this is just to shut up coverity, since we check the reply
value.

>  reply = jsonrpc_create_reply(ovsdb_schema_to_json(db->schema),
>   request->id);
>  }
> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
> index 01091fabe..af88b96e3 100644
> --- a/ovsdb/monitor.c
> +++ b/ovsdb/monitor.c
> @@ -483,6 +483,9 @@ ovsdb_monitor_add_column(struct ovsdb_monitor *dbmon,
>  struct ovsdb_monitor_column *c;
>  
>  mt = shash_find_data(>tables, table->schema->name);
> +if (!mt) {
> +return NULL;
> +}
>  

Re: [ovs-dev] [PATCH v13 2/4] lib, ovs-vsctl: Add zero-initializations

[Aaron Conole]

James Raphael Tiovalen  writes:

> This commit adds zero-initializations by changing `SFL_ALLOC` from
> `malloc` to `xzalloc`, adding a `memset` call to `sflAlloc`,
> initializing a `pollfd` struct variable with zeroes, and changing some
> calls to `xmalloc` to `xzalloc`. This is to prevent potential data leaks
> or undefined behavior from potentially uninitialized variables.
>
> Some variables would always be initialized by either the code flow or
> the compiler. Thus, some of the associated Coverity reports might be
> false positives. That said, it is still considered best practice to
> zero-initialize variables upfront just in case to ensure the overall
> resilience and security of OVS, as long as they do not impact
> performance-critical code. As a bonus, it would also make static
> analyzer tools, such as Coverity, happy.
>
> Signed-off-by: James Raphael Tiovalen 
> ---
>  lib/latch-unix.c  |  2 +-
>  lib/sflow_agent.c | 12 ++--
>  lib/sflow_api.h   |  2 +-
>  utilities/ovs-vsctl.c |  5 ++---
>  4 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/lib/latch-unix.c b/lib/latch-unix.c
> index f4d10c39a..c62bb024b 100644
> --- a/lib/latch-unix.c
> +++ b/lib/latch-unix.c
> @@ -71,7 +71,7 @@ latch_set(struct latch *latch)
>  bool
>  latch_is_set(const struct latch *latch)
>  {
> -struct pollfd pfd;
> +struct pollfd pfd = {0};
>  int retval;
>  
>  pfd.fd = latch->fds[0];
> diff --git a/lib/sflow_agent.c b/lib/sflow_agent.c
> index c95f654a5..743774a27 100644
> --- a/lib/sflow_agent.c
> +++ b/lib/sflow_agent.c
> @@ -510,8 +510,16 @@ void sfl_agent_sysError(SFLAgent *agent, char *modName, 
> char *msg)
>  
>  static void * sflAlloc(SFLAgent *agent, size_t bytes)
>  {
> -if(agent->allocFn) return (*agent->allocFn)(agent->magic, agent, bytes);
> -else return SFL_ALLOC(bytes);
> +void *alloc;
> +
> +if (agent->allocFn) {
> +alloc = (*agent->allocFn)(agent->magic, agent, bytes);
> +ovs_assert(alloc);
> +memset(alloc, 0, bytes);
> +} else {
> +alloc = SFL_ALLOC(bytes);
> +}
> +return alloc;
>  }
>  
>  static void sflFree(SFLAgent *agent, void *obj)
> diff --git a/lib/sflow_api.h b/lib/sflow_api.h
> index a0530b37a..eb23e2acd 100644
> --- a/lib/sflow_api.h
> +++ b/lib/sflow_api.h
> @@ -337,7 +337,7 @@ void sfl_agent_sysError(SFLAgent *agent, char *modName, 
> char *msg);
>  
>  u_int32_t sfl_receiver_samplePacketsSent(SFLReceiver *receiver);
>  
> -#define SFL_ALLOC malloc
> +#define SFL_ALLOC xzalloc
>  #define SFL_FREE free
>  
>  #endif /* SFLOW_API_H */
> diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> index 62b512302..f55c2965a 100644
> --- a/utilities/ovs-vsctl.c
> +++ b/utilities/ovs-vsctl.c
> @@ -575,7 +575,7 @@ add_bridge_to_cache(struct vsctl_context *vsctl_ctx,
>  struct ovsrec_bridge *br_cfg, const char *name,
>  struct vsctl_bridge *parent, int vlan)
>  {
> -struct vsctl_bridge *br = xmalloc(sizeof *br);
> +struct vsctl_bridge *br = xzalloc(sizeof *br);
>  br->br_cfg = br_cfg;
>  br->name = xstrdup(name);
>  ovs_list_init(>ports);
> @@ -659,7 +659,7 @@ static struct vsctl_port *
>  add_port_to_cache(struct vsctl_context *vsctl_ctx, struct vsctl_bridge 
> *parent,
>struct ovsrec_port *port_cfg)
>  {
> -struct vsctl_port *port;
> +struct vsctl_port *port = xzalloc(sizeof *port);

Not sure why we moved the assignment to here?  I would prefer it be left
in-place and just change the xmalloc to xzalloc.

That can probably be changed on apply if you agree, and the maintainer
applying it agrees as well.

Otherwise,

Acked-by: Aaron Conole 

>  if (port_cfg->tag
>  && *port_cfg->tag >= 0 && *port_cfg->tag <= 4095) {
> @@ -671,7 +671,6 @@ add_port_to_cache(struct vsctl_context *vsctl_ctx, struct 
> vsctl_bridge *parent,
>  }
>  }
>  
> -port = xmalloc(sizeof *port);
>  ovs_list_push_back(>ports, >ports_node);
>  ovs_list_init(>ifaces);
>  port->port_cfg = port_cfg;

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

Re: [ovs-dev] [PATCH v13 1/4] lib: Add non-null assertions to some return values of `dp_packet_data`

[Aaron Conole]

James Raphael Tiovalen  writes:

> This commit adds some `ovs_assert()` checks to some return values of
> `dp_packet_data()` to ensure that they are not NULL and to prevent
> null-pointer dereferences, which might lead to unwanted crashes. We use
> assertions since it should be impossible for these calls to
> `dp_packet_data()` to return NULL.
>
> Signed-off-by: James Raphael Tiovalen 
> Reviewed-by: Simon Horman 
> ---

Acked-by: Aaron Conole 

>  lib/dp-packet.c | 15 ++-
>  lib/netdev-native-tnl.c |  6 +-
>  lib/pcap-file.c |  4 +++-
>  3 files changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 27114a9a9..6e3a8297a 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -187,9 +187,12 @@ dp_packet_clone_with_headroom(const struct dp_packet 
> *buffer, size_t headroom)
>  struct dp_packet *new_buffer;
>  uint32_t mark;
>  
> -new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer),
> - dp_packet_size(buffer),
> - headroom);
> +const void *data_dp = dp_packet_data(buffer);
> +ovs_assert(data_dp);
> +
> +new_buffer = dp_packet_clone_data_with_headroom(data_dp,
> +dp_packet_size(buffer),
> +headroom);
>  /* Copy the following fields into the returned buffer: l2_pad_size,
>   * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
>  memcpy(_buffer->l2_pad_size, >l2_pad_size,
> @@ -326,8 +329,10 @@ dp_packet_shift(struct dp_packet *b, int delta)
> : true);
>  
>  if (delta != 0) {
> -char *dst = (char *) dp_packet_data(b) + delta;
> -memmove(dst, dp_packet_data(b), dp_packet_size(b));
> +const void *data_dp = dp_packet_data(b);
> +ovs_assert(data_dp);
> +char *dst = (char *) data_dp + delta;
> +memmove(dst, data_dp, dp_packet_size(b));
>  dp_packet_set_data(b, dst);
>  }
>  }
> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c
> index 715bbab2b..311ba6895 100644
> --- a/lib/netdev-native-tnl.c
> +++ b/lib/netdev-native-tnl.c
> @@ -43,6 +43,7 @@
>  #include "seq.h"
>  #include "unaligned.h"
>  #include "unixctl.h"
> +#include "util.h"
>  #include "openvswitch/vlog.h"
>  
>  VLOG_DEFINE_THIS_MODULE(native_tnl);
> @@ -419,7 +420,10 @@ netdev_gre_pop_header(struct dp_packet *packet)
>  struct flow_tnl *tnl = >tunnel;
>  int hlen = sizeof(struct eth_header) + 4;
>  
> -hlen += netdev_tnl_is_header_ipv6(dp_packet_data(packet)) ?
> +const void *data_dp = dp_packet_data(packet);
> +ovs_assert(data_dp);
> +
> +hlen += netdev_tnl_is_header_ipv6(data_dp) ?
>  IPV6_HEADER_LEN : IP_HEADER_LEN;
>  
>  pkt_metadata_init_tnl(md);
> diff --git a/lib/pcap-file.c b/lib/pcap-file.c
> index 3ed7ea488..9f4e2e1e2 100644
> --- a/lib/pcap-file.c
> +++ b/lib/pcap-file.c
> @@ -284,6 +284,8 @@ ovs_pcap_write(struct pcap_file *p_file, struct dp_packet 
> *buf)
>  struct timeval tv;
>  
>  ovs_assert(dp_packet_is_eth(buf));
> +const void *data_dp = dp_packet_data(buf);
> +ovs_assert(data_dp);
>  
>  xgettimeofday();
>  prh.ts_sec = tv.tv_sec;
> @@ -291,7 +293,7 @@ ovs_pcap_write(struct pcap_file *p_file, struct dp_packet 
> *buf)
>  prh.incl_len = dp_packet_size(buf);
>  prh.orig_len = dp_packet_size(buf);
>  ignore(fwrite(, sizeof prh, 1, p_file->file));
> -ignore(fwrite(dp_packet_data(buf), dp_packet_size(buf), 1, 
> p_file->file));
> +ignore(fwrite(data_dp, dp_packet_size(buf), 1, p_file->file));
>  fflush(p_file->file);
>  }

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

Re: [ovs-dev] [PATCH ovn] northd: Fix recompute of referenced chassis in HA chassis groups.

[Ilya Maximets]

On 8/25/23 18:55, Ilya Maximets wrote:
> On 8/25/23 18:52, Mark Michelson wrote:
>> Hi Ilya,
>>
>> I merged this change into main and branch-23.06. The conflicts were easy 
>> to resolve in branch-23.06, but when attempting to merge to 23.03, I was 
>> not as comfortable trying to resolve the conflicts. Could you please 
>> post a version of the patch that applies to 23.03 please?
> 
> Sure, I'll do that.  If not today, then on Monday.

Posted a backport here:
  
https://patchwork.ozlabs.org/project/ovn/patch/20230825180336.2011920-1-i.maxim...@ovn.org/

It should be applicable to all the branches down to 22.03.
There were no significant changes during backport, mostly
a struct northd_input change that caused conflicts.

> 
> Best regards, Ilya Maximets.
> 
>>
>> Thanks,
>> Mark Michelson
>>
>> On 8/24/23 15:02, Mark Michelson wrote:
>>> Thanks Ilya,
>>>
>>> Acked-by: Mark Michelson 
>>>
>>> The topic of backporting this patch was brought up in the OVN 
>>> development IRC meeting today, and it was universally agreed there that 
>>> this performance issue is so bad that we should backport this fix when 
>>> merging.
>>>
>>> On 8/23/23 17:41, Ilya Maximets wrote:
 Current implementation of 'ref_chassis' recompute is horribly slow.
 For each port binding it finds LR group, then it adds a chassis of the
 current port binding to each HA chassis group of this LR group.

 The number of ports is much greater than number of chassis.  And the
 number of distinct LR groups is even smaller than a number of chassis
 in a typical setup.  Chances are, the setup only has one LR group.
 So, northd today is trying to add the same chassis to the same
 HA chassis group over and over again.

 In an OpenStack setup with ~20 chassis, 40K ports and 5K routers we
 end up with 33+ million calls to add_to_ha_ref_chassis_info() that
 also performs a linear search for duplicates leading to about 270
 million operations, while we only have 5K Sb HA chassis groups with
 15 ref_chassis in each.  I.e. only 5000 * 15 = 75.000 operations out
 of 270 million are actually useful.

 In practice that leads to 80 second recompute:

    inc_proc_eng|INFO|node: sync_from_sb, recompute (forced) took 80074ms

 Fix that by accumulating unique chassis per unique LR groups first
 and then updating ref_chassis with these unique values avoiding any
 unnecessary calls.  Result is ~1000x performance improvement:

    inc_proc_eng|DBG|node: sync_from_sb, recompute (forced) took 75ms

 Considering this as a performance bug, because it makes OVN practically
 unusable in certain (default?) OpenStack configurations at scale.

 Fixes: b82d351976a1 ("ovn: Add generic HA chassis group")
 Reported-at: 
 https://mail.openvswitch.org/pipermail/ovs-discuss/2023-August/052614.html
 Reported-by: Roberto Bartzen Acosta 
 Signed-off-by: Ilya Maximets 
 ---
   northd/northd.c | 118 ++--
   1 file changed, 75 insertions(+), 43 deletions(-)

 diff --git a/northd/northd.c b/northd/northd.c
 index 0a749931e..f5fb5e83a 100644
 --- a/northd/northd.c
 +++ b/northd/northd.c
 @@ -799,6 +799,8 @@ struct lrouter_group {
   int n_router_dps;
   /* Set of ha_chassis_groups which are associated with the router 
 dps. */
   struct sset ha_chassis_groups;
 +    /* Temporary storage for chassis references while computing HA 
 groups. */
 +    struct hmapx tmp_ha_chassis;
   };
   static struct ovn_datapath *
 @@ -8708,6 +8710,7 @@ build_lrouter_groups(struct hmap *lr_ports, 
 struct ovs_list *lr_list)
   od->lr_group->router_dps[0] = od;
   od->lr_group->n_router_dps = 1;
   sset_init(>lr_group->ha_chassis_groups);
 +    hmapx_init(>lr_group->tmp_ha_chassis);
   build_lrouter_groups__(lr_ports, od);
   }
   }
 @@ -17399,6 +17402,7 @@ destroy_datapaths_and_ports(struct 
 ovn_datapaths *ls_datapaths,
   free(lr_group->router_dps);
   sset_destroy(_group->ha_chassis_groups);
 +    hmapx_destroy(_group->tmp_ha_chassis);
   free(lr_group);
   }
   }
 @@ -17671,38 +17675,27 @@ ovnnb_db_run(struct northd_input *input_data,
   smap_destroy();
   }
 -/* Stores the list of chassis which references an ha_chassis_group.
 +/* Stores the set of chassis which references an ha_chassis_group.
    */
   struct ha_ref_chassis_info {
   const struct sbrec_ha_chassis_group *ha_chassis_group;
 -    struct sbrec_chassis **ref_chassis;
 -    size_t n_ref_chassis;
 -    size_t free_slots;
 +    struct hmapx ref_chassis;
   };
   static void
   add_to_ha_ref_chassis_info(struct 

[ovs-dev] [PATCH ovn branch-23.03] northd: Fix recompute of referenced chassis in HA chassis groups.

[Ilya Maximets]

Current implementation of 'ref_chassis' recompute is horribly slow.
For each port binding it finds LR group, then it adds a chassis of the
current port binding to each HA chassis group of this LR group.

The number of ports is much greater than number of chassis.  And the
number of distinct LR groups is even smaller than a number of chassis
in a typical setup.  Chances are, the setup only has one LR group.
So, northd today is trying to add the same chassis to the same
HA chassis group over and over again.

In an OpenStack setup with ~20 chassis, 40K ports and 5K routers we
end up with 33+ million calls to add_to_ha_ref_chassis_info() that
also performs a linear search for duplicates leading to about 270
million operations, while we only have 5K Sb HA chassis groups with
15 ref_chassis in each.  I.e. only 5000 * 15 = 75.000 operations out
of 270 million are actually useful.

In practice that leads to 80 second recompute:

  inc_proc_eng|INFO|node: sync_from_sb, recompute (forced) took 80074ms

Fix that by accumulating unique chassis per unique LR groups first
and then updating ref_chassis with these unique values avoiding any
unnecessary calls.  Result is ~1000x performance improvement:

  inc_proc_eng|DBG|node: sync_from_sb, recompute (forced) took 75ms

Considering this as a performance bug, because it makes OVN practically
unusable in certain (default?) OpenStack configurations at scale.

Fixes: b82d351976a1 ("ovn: Add generic HA chassis group")
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2023-August/052614.html
Reported-by: Roberto Bartzen Acosta 
Acked-by: Mark Michelson 
Signed-off-by: Ilya Maximets 
---

cherry-picked from commit 4023d6a5fa573021a6218ac49796f3f7688227d7

This version is also applicable all the way down to 22.03.

 northd/northd.c | 119 +++-
 1 file changed, 76 insertions(+), 43 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 4d3646d10..7b91a94cb 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -857,6 +857,8 @@ struct lrouter_group {
 int n_router_dps;
 /* Set of ha_chassis_groups which are associated with the router dps. */
 struct sset ha_chassis_groups;
+/* Temporary storage for chassis references while computing HA groups. */
+struct hmapx tmp_ha_chassis;
 };
 
 static struct ovn_datapath *
@@ -7874,6 +7876,7 @@ build_lrouter_groups(struct hmap *ports, struct ovs_list 
*lr_list)
 od->lr_group->router_dps[0] = od;
 od->lr_group->n_router_dps = 1;
 sset_init(>lr_group->ha_chassis_groups);
+hmapx_init(>lr_group->tmp_ha_chassis);
 build_lrouter_groups__(ports, od);
 }
 }
@@ -15783,6 +15786,7 @@ destroy_datapaths_and_ports(struct hmap *datapaths, 
struct hmap *ports,
 
 free(lr_group->router_dps);
 sset_destroy(_group->ha_chassis_groups);
+hmapx_destroy(_group->tmp_ha_chassis);
 free(lr_group);
 }
 }
@@ -16319,38 +16323,27 @@ ovnnb_db_run(struct northd_input *input_data,
 smap_destroy();
 }
 
-/* Stores the list of chassis which references an ha_chassis_group.
+/* Stores the set of chassis which references an ha_chassis_group.
  */
 struct ha_ref_chassis_info {
 const struct sbrec_ha_chassis_group *ha_chassis_group;
-struct sbrec_chassis **ref_chassis;
-size_t n_ref_chassis;
-size_t free_slots;
+struct hmapx ref_chassis;
 };
 
 static void
 add_to_ha_ref_chassis_info(struct ha_ref_chassis_info *ref_ch_info,
-   const struct sbrec_chassis *chassis)
+   const struct hmapx *chassis)
 {
-for (size_t j = 0; j < ref_ch_info->n_ref_chassis; j++) {
-if (ref_ch_info->ref_chassis[j] == chassis) {
-   return;
-}
-}
+if (!hmapx_count(_ch_info->ref_chassis)) {
+hmapx_destroy(_ch_info->ref_chassis);
+hmapx_clone(_ch_info->ref_chassis, chassis);
+} else {
+struct hmapx_node *node;
 
-/* Allocate space for 3 chassis at a time. */
-if (!ref_ch_info->free_slots) {
-ref_ch_info->ref_chassis =
-xrealloc(ref_ch_info->ref_chassis,
- sizeof *ref_ch_info->ref_chassis *
- (ref_ch_info->n_ref_chassis + 3));
-ref_ch_info->free_slots = 3;
+HMAPX_FOR_EACH (node, chassis) {
+hmapx_add(_ch_info->ref_chassis, node->data);
+}
 }
-
-ref_ch_info->ref_chassis[ref_ch_info->n_ref_chassis] =
-CONST_CAST(struct sbrec_chassis *, chassis);
-ref_ch_info->n_ref_chassis++;
-ref_ch_info->free_slots--;
 }
 
 struct ha_chassis_group_node {
@@ -16379,9 +16372,20 @@ update_sb_ha_group_ref_chassis(struct northd_input 
*input_data,
 struct shash_node *node;
 SHASH_FOR_EACH_SAFE (node, ha_ref_chassis_map) {
 struct ha_ref_chassis_info *ha_ref_info = node->data;
+size_t n = hmapx_count(_ref_info->ref_chassis);
+

Re: [ovs-dev] [PATCH ovn v4] northd: Allow delay of northd engine runs

[Mark Michelson]

Thanks for the changes Ales.

Acked-by: Mark Michelson 

On 8/24/23 01:29, Ales Musil wrote:

Add config option called "northd-backoff-interval-ms" that allows
to delay northd engine runs capped by the config option.
When the config option is set to 0 or unspecified, the engine
will run without any restrictions. If the value >0 we will delay
northd engine run by the previous run time capped by the
config option.

The reason to delay northd is to prevent it from consuming 100%
CPU all the time, secondary to that is the batch of NB updates
that could be processed in single northd run. With the recent
changes to I-P the engine processing updates faster, but not
quite fast enough for processing to be faster than NB changes,
which in turn can result into northd never going to sleep. With
the backoff period enabled northd can sleep and process the DB
updates in bigger batch.

In addition to process the updates as fast as possible wake the
northd immediately if there are changes accumulated over period
of 500 ms or bigger.

The results are very noticeable during scale testing.
Run without any backoff period:
northd aggregate CPU 9810% avg / 12765% max
northd was spinning at 100% CPU the entire second half of the test.

Run with 200 ms max backoff period:
northd aggregate CPU 6066% avg / 7689% max
northd was around 60% for the second half of the test.

One thing to note is that the overall latency was slightly
increased from P99 4s to P99 4.1s.

Signed-off-by: Ales Musil 
---
v4: Skip the northd engine delay if there is more changes accumulated in the 
IDL loop.
---
  NEWS |  2 ++
  northd/inc-proc-northd.c | 27 +++---
  northd/inc-proc-northd.h | 13 ++-
  northd/ovn-northd.c  | 48 
  ovn-nb.xml   |  9 
  5 files changed, 76 insertions(+), 23 deletions(-)

diff --git a/NEWS b/NEWS
index 93d4bedcd..0667e7f94 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,8 @@ Post v23.06.0
  Existing sessions might get re-hashed to a different ECMP path when
  OVN detects the algorithm support in the datapath during an upgrade
  or restart of ovn-controller.
+  - Add "northd-backoff-interval-ms" config option to delay northd engine
+runs capped at the set value.
  
  OVN v23.06.0 - 01 Jun 2023

  --
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index d328deb22..0b759ae1d 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -295,16 +295,18 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
  /* Returns true if the incremental processing ended up updating nodes. */
  bool inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn,
   struct ovsdb_idl_txn *ovnsb_txn,
- bool recompute) {
+ struct northd_engine_context *ctx) {
  ovs_assert(ovnnb_txn && ovnsb_txn);
+
+int64_t start = time_msec();
  engine_init_run();
  
  /* Force a full recompute if instructed to, for example, after a NB/SB

   * reconnect event.  However, make sure we don't overwrite an existing
   * force-recompute request if 'recompute' is false.
   */
-if (recompute) {
-engine_set_force_recompute(recompute);
+if (ctx->recompute) {
+engine_set_force_recompute(ctx->recompute);
  }
  
  struct engine_context eng_ctx = {

@@ -330,6 +332,12 @@ bool inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn,
  } else {
  engine_set_force_recompute(false);
  }
+
+int64_t now = time_msec();
+/* Postpone the next run by length of current run with maximum capped
+ * by "northd-backoff-interval-ms" interval. */
+ctx->next_run_ms = now + MIN(now - start, ctx->backoff_ms);
+
  return engine_has_updated();
  }
  
@@ -339,6 +347,19 @@ void inc_proc_northd_cleanup(void)

  engine_set_context(NULL);
  }
  
+bool

+inc_proc_northd_can_run(struct northd_engine_context *ctx)
+{
+if (ctx->recompute || time_msec() >= ctx->next_run_ms ||
+ctx->nb_idl_duration_ms >= IDL_LOOP_MAX_DURATION_MS ||
+ctx->sb_idl_duration_ms >= IDL_LOOP_MAX_DURATION_MS) {
+return true;
+}
+
+poll_timer_wait_until(ctx->next_run_ms);
+return false;
+}
+
  static void
  chassis_features_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
const char *argv[] OVS_UNUSED, void *features_)
diff --git a/northd/inc-proc-northd.h b/northd/inc-proc-northd.h
index 9b81c7ee0..a2b9b7fdb 100644
--- a/northd/inc-proc-northd.h
+++ b/northd/inc-proc-northd.h
@@ -6,11 +6,22 @@
  #include "northd.h"
  #include "ovsdb-idl.h"
  
+#define IDL_LOOP_MAX_DURATION_MS 500

+
+struct northd_engine_context {
+int64_t next_run_ms;
+uint64_t nb_idl_duration_ms;
+uint64_t sb_idl_duration_ms;
+uint32_t backoff_ms;
+bool recompute;
+};
+
  void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
struct 

Re: [ovs-dev] [PATCH ovn] northd: Fix incorrect memory allocation for router group datapaths.

[Mark Michelson]

I merged this change to main and all branches back to 22.03.

Thanks Ilya!

On 8/24/23 15:02, Mark Michelson wrote:

Thanks Ilya,

Acked-by: Mark Michelson 

On 8/23/23 17:57, Ilya Maximets wrote:

We need to allocate memory for 'n_router_dps' pointers, but the
current code allocates space for 'n_router_dps' datapaths, which
is 762 byte structure.  Having 5000 routers in the OpenStack
setup, we allocate 762 * 5000 * 5000 ~= 17 GB of memory instead
of 190 MB:

  98.17% (19,962,746,247B) (heap allocation functions)
  ->90.67% (18,436,818,400B) xcalloc__ (util.c:124)
   ->90.67% (18,436,818,400B) xcalloc (util.c:161)
    ->90.67% (18,436,818,400B) build_lrouter_groups (northd.c:8707)
 ->90.67% (18,436,818,400B) ovnnb_db_run (northd.c:17611)
  ->90.67% (18,436,818,400B) en_northd_run (en-northd.c:137)
   ->90.67% (18,436,818,400B) engine_recompute (inc-proc-eng.c:415)
    ->90.67% (18,436,818,400B) engine_run_node (inc-proc-eng.c:477)
 ->90.67% (18,436,818,400B) engine_run (inc-proc-eng.c:528)
  ->90.67% (18,436,818,400B) inc_proc_northd_run 
(inc-proc-northd.c:316)

   ->90.67% (18,436,818,400B) main (ovn-northd.c:937)

Also, even though this doesn't affect the allocation size, the order
of arguments is incorrect.

Fix that by using correct pointers and order.  That saves a huge
amount of memory as well as time on allocating and zeroing it out.
In my testing northd node saved 10 seconds on these allocations.

Fixes: b82d351976a1 ("ovn: Add generic HA chassis group")
Signed-off-by: Ilya Maximets 
---
  northd/northd.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/northd/northd.c b/northd/northd.c
index 0a749931e..c701d929e 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -8704,7 +8704,8 @@ build_lrouter_groups(struct hmap *lr_ports, 
struct ovs_list *lr_list)

  od->lr_group = xzalloc(sizeof *od->lr_group);
  /* Each logical router group can have max
   * 'n_router_dps'. So allocate enough memory. */
-    od->lr_group->router_dps = xcalloc(sizeof *od, 
n_router_dps);

+    od->lr_group->router_dps =
+    xcalloc(n_router_dps, sizeof *od->lr_group->router_dps);
  od->lr_group->router_dps[0] = od;
  od->lr_group->n_router_dps = 1;
  sset_init(>lr_group->ha_chassis_groups);




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

Re: [ovs-dev] [PATCH ovn] northd: Fix recompute of referenced chassis in HA chassis groups.

[Ilya Maximets]

On 8/25/23 18:52, Mark Michelson wrote:
> Hi Ilya,
> 
> I merged this change into main and branch-23.06. The conflicts were easy 
> to resolve in branch-23.06, but when attempting to merge to 23.03, I was 
> not as comfortable trying to resolve the conflicts. Could you please 
> post a version of the patch that applies to 23.03 please?

Sure, I'll do that.  If not today, then on Monday.

Best regards, Ilya Maximets.

> 
> Thanks,
> Mark Michelson
> 
> On 8/24/23 15:02, Mark Michelson wrote:
>> Thanks Ilya,
>>
>> Acked-by: Mark Michelson 
>>
>> The topic of backporting this patch was brought up in the OVN 
>> development IRC meeting today, and it was universally agreed there that 
>> this performance issue is so bad that we should backport this fix when 
>> merging.
>>
>> On 8/23/23 17:41, Ilya Maximets wrote:
>>> Current implementation of 'ref_chassis' recompute is horribly slow.
>>> For each port binding it finds LR group, then it adds a chassis of the
>>> current port binding to each HA chassis group of this LR group.
>>>
>>> The number of ports is much greater than number of chassis.  And the
>>> number of distinct LR groups is even smaller than a number of chassis
>>> in a typical setup.  Chances are, the setup only has one LR group.
>>> So, northd today is trying to add the same chassis to the same
>>> HA chassis group over and over again.
>>>
>>> In an OpenStack setup with ~20 chassis, 40K ports and 5K routers we
>>> end up with 33+ million calls to add_to_ha_ref_chassis_info() that
>>> also performs a linear search for duplicates leading to about 270
>>> million operations, while we only have 5K Sb HA chassis groups with
>>> 15 ref_chassis in each.  I.e. only 5000 * 15 = 75.000 operations out
>>> of 270 million are actually useful.
>>>
>>> In practice that leads to 80 second recompute:
>>>
>>>    inc_proc_eng|INFO|node: sync_from_sb, recompute (forced) took 80074ms
>>>
>>> Fix that by accumulating unique chassis per unique LR groups first
>>> and then updating ref_chassis with these unique values avoiding any
>>> unnecessary calls.  Result is ~1000x performance improvement:
>>>
>>>    inc_proc_eng|DBG|node: sync_from_sb, recompute (forced) took 75ms
>>>
>>> Considering this as a performance bug, because it makes OVN practically
>>> unusable in certain (default?) OpenStack configurations at scale.
>>>
>>> Fixes: b82d351976a1 ("ovn: Add generic HA chassis group")
>>> Reported-at: 
>>> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-August/052614.html
>>> Reported-by: Roberto Bartzen Acosta 
>>> Signed-off-by: Ilya Maximets 
>>> ---
>>>   northd/northd.c | 118 ++--
>>>   1 file changed, 75 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index 0a749931e..f5fb5e83a 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -799,6 +799,8 @@ struct lrouter_group {
>>>   int n_router_dps;
>>>   /* Set of ha_chassis_groups which are associated with the router 
>>> dps. */
>>>   struct sset ha_chassis_groups;
>>> +    /* Temporary storage for chassis references while computing HA 
>>> groups. */
>>> +    struct hmapx tmp_ha_chassis;
>>>   };
>>>   static struct ovn_datapath *
>>> @@ -8708,6 +8710,7 @@ build_lrouter_groups(struct hmap *lr_ports, 
>>> struct ovs_list *lr_list)
>>>   od->lr_group->router_dps[0] = od;
>>>   od->lr_group->n_router_dps = 1;
>>>   sset_init(>lr_group->ha_chassis_groups);
>>> +    hmapx_init(>lr_group->tmp_ha_chassis);
>>>   build_lrouter_groups__(lr_ports, od);
>>>   }
>>>   }
>>> @@ -17399,6 +17402,7 @@ destroy_datapaths_and_ports(struct 
>>> ovn_datapaths *ls_datapaths,
>>>   free(lr_group->router_dps);
>>>   sset_destroy(_group->ha_chassis_groups);
>>> +    hmapx_destroy(_group->tmp_ha_chassis);
>>>   free(lr_group);
>>>   }
>>>   }
>>> @@ -17671,38 +17675,27 @@ ovnnb_db_run(struct northd_input *input_data,
>>>   smap_destroy();
>>>   }
>>> -/* Stores the list of chassis which references an ha_chassis_group.
>>> +/* Stores the set of chassis which references an ha_chassis_group.
>>>    */
>>>   struct ha_ref_chassis_info {
>>>   const struct sbrec_ha_chassis_group *ha_chassis_group;
>>> -    struct sbrec_chassis **ref_chassis;
>>> -    size_t n_ref_chassis;
>>> -    size_t free_slots;
>>> +    struct hmapx ref_chassis;
>>>   };
>>>   static void
>>>   add_to_ha_ref_chassis_info(struct ha_ref_chassis_info *ref_ch_info,
>>> -   const struct sbrec_chassis *chassis)
>>> +   const struct hmapx *chassis)
>>>   {
>>> -    for (size_t j = 0; j < ref_ch_info->n_ref_chassis; j++) {
>>> -    if (ref_ch_info->ref_chassis[j] == chassis) {
>>> -   return;
>>> -    }
>>> -    }
>>> +    if (!hmapx_count(_ch_info->ref_chassis)) {
>>> +    hmapx_destroy(_ch_info->ref_chassis);
>>> +   

Re: [ovs-dev] [PATCH ovn] northd: Fix recompute of referenced chassis in HA chassis groups.

[Mark Michelson]

Hi Ilya,

I merged this change into main and branch-23.06. The conflicts were easy 
to resolve in branch-23.06, but when attempting to merge to 23.03, I was 
not as comfortable trying to resolve the conflicts. Could you please 
post a version of the patch that applies to 23.03 please?


Thanks,
Mark Michelson

On 8/24/23 15:02, Mark Michelson wrote:

Thanks Ilya,

Acked-by: Mark Michelson 

The topic of backporting this patch was brought up in the OVN 
development IRC meeting today, and it was universally agreed there that 
this performance issue is so bad that we should backport this fix when 
merging.


On 8/23/23 17:41, Ilya Maximets wrote:

Current implementation of 'ref_chassis' recompute is horribly slow.
For each port binding it finds LR group, then it adds a chassis of the
current port binding to each HA chassis group of this LR group.

The number of ports is much greater than number of chassis.  And the
number of distinct LR groups is even smaller than a number of chassis
in a typical setup.  Chances are, the setup only has one LR group.
So, northd today is trying to add the same chassis to the same
HA chassis group over and over again.

In an OpenStack setup with ~20 chassis, 40K ports and 5K routers we
end up with 33+ million calls to add_to_ha_ref_chassis_info() that
also performs a linear search for duplicates leading to about 270
million operations, while we only have 5K Sb HA chassis groups with
15 ref_chassis in each.  I.e. only 5000 * 15 = 75.000 operations out
of 270 million are actually useful.

In practice that leads to 80 second recompute:

   inc_proc_eng|INFO|node: sync_from_sb, recompute (forced) took 80074ms

Fix that by accumulating unique chassis per unique LR groups first
and then updating ref_chassis with these unique values avoiding any
unnecessary calls.  Result is ~1000x performance improvement:

   inc_proc_eng|DBG|node: sync_from_sb, recompute (forced) took 75ms

Considering this as a performance bug, because it makes OVN practically
unusable in certain (default?) OpenStack configurations at scale.

Fixes: b82d351976a1 ("ovn: Add generic HA chassis group")
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2023-August/052614.html

Reported-by: Roberto Bartzen Acosta 
Signed-off-by: Ilya Maximets 
---
  northd/northd.c | 118 ++--
  1 file changed, 75 insertions(+), 43 deletions(-)

diff --git a/northd/northd.c b/northd/northd.c
index 0a749931e..f5fb5e83a 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -799,6 +799,8 @@ struct lrouter_group {
  int n_router_dps;
  /* Set of ha_chassis_groups which are associated with the router 
dps. */

  struct sset ha_chassis_groups;
+    /* Temporary storage for chassis references while computing HA 
groups. */

+    struct hmapx tmp_ha_chassis;
  };
  static struct ovn_datapath *
@@ -8708,6 +8710,7 @@ build_lrouter_groups(struct hmap *lr_ports, 
struct ovs_list *lr_list)

  od->lr_group->router_dps[0] = od;
  od->lr_group->n_router_dps = 1;
  sset_init(>lr_group->ha_chassis_groups);
+    hmapx_init(>lr_group->tmp_ha_chassis);
  build_lrouter_groups__(lr_ports, od);
  }
  }
@@ -17399,6 +17402,7 @@ destroy_datapaths_and_ports(struct 
ovn_datapaths *ls_datapaths,

  free(lr_group->router_dps);
  sset_destroy(_group->ha_chassis_groups);
+    hmapx_destroy(_group->tmp_ha_chassis);
  free(lr_group);
  }
  }
@@ -17671,38 +17675,27 @@ ovnnb_db_run(struct northd_input *input_data,
  smap_destroy();
  }
-/* Stores the list of chassis which references an ha_chassis_group.
+/* Stores the set of chassis which references an ha_chassis_group.
   */
  struct ha_ref_chassis_info {
  const struct sbrec_ha_chassis_group *ha_chassis_group;
-    struct sbrec_chassis **ref_chassis;
-    size_t n_ref_chassis;
-    size_t free_slots;
+    struct hmapx ref_chassis;
  };
  static void
  add_to_ha_ref_chassis_info(struct ha_ref_chassis_info *ref_ch_info,
-   const struct sbrec_chassis *chassis)
+   const struct hmapx *chassis)
  {
-    for (size_t j = 0; j < ref_ch_info->n_ref_chassis; j++) {
-    if (ref_ch_info->ref_chassis[j] == chassis) {
-   return;
-    }
-    }
+    if (!hmapx_count(_ch_info->ref_chassis)) {
+    hmapx_destroy(_ch_info->ref_chassis);
+    hmapx_clone(_ch_info->ref_chassis, chassis);
+    } else {
+    struct hmapx_node *node;
-    /* Allocate space for 3 chassis at a time. */
-    if (!ref_ch_info->free_slots) {
-    ref_ch_info->ref_chassis =
-    xrealloc(ref_ch_info->ref_chassis,
- sizeof *ref_ch_info->ref_chassis *
- (ref_ch_info->n_ref_chassis + 3));
-    ref_ch_info->free_slots = 3;
+    HMAPX_FOR_EACH (node, chassis) {
+    hmapx_add(_ch_info->ref_chassis, node->data);
+    }
  

[ovs-dev] [PATCH ovn] tests: offload scapy transformations to a separate daemon

[Ihar Hrachyshka]

The daemon life cycle spans over the whole test case life time, which
significantly speeds up test cases that rely on fmt_pkt for packet byte
representations.

The speed-up comes from the fact that we no longer start a python
environment with all scapy modules imported on any fmt_pkt invocation;
but only on the first call to fmt_pkt.

The daemon is not started for test cases that don't trigger fmt_pkt.
(The server is started lazily as part of fmt_pkt call.)

For example, without the daemon, all tests that use fmt_pkt took the
following on my vagrant box:

real15m27.117s
user23m55.023s
sys 4m35.469s

With the daemon, the same set of tests run in:

real4m34.092s
user3m20.231s
sys 1m10.886s

We may want to make the daemon global, so that it's invoked once per
test suite run. But I haven't figured out, yet, how to run a trap to
clean up the deamon and its socket and pid files on suite exit (and not
on test case exit.)

Signed-off-by: Ihar Hrachyshka 
---
 tests/automake.mk |   1 +
 tests/ovn-macros.at   |  27 ++--
 tests/scapy_server.py | 100 ++
 3 files changed, 124 insertions(+), 4 deletions(-)
 create mode 100755 tests/scapy_server.py

diff --git a/tests/automake.mk b/tests/automake.mk
index eea0d00f4..0f7f1bd9a 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -309,6 +309,7 @@ tests_ovstest_LDADD = $(OVS_LIBDIR)/daemon.lo \
 CHECK_PYFILES = \
tests/test-l7.py \
tests/uuidfilt.py \
+   tests/scapy_server.py \
tests/test-tcp-rst.py \
tests/check_acl_log.py
 
diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
index 13d5dc3d4..8b12f8e55 100644
--- a/tests/ovn-macros.at
+++ b/tests/ovn-macros.at
@@ -816,6 +816,15 @@ ovn_trace_client() {
 ovs-appctl -t $target trace "$@" | tee trace | sed '/^# /d'
 }
 
+wait_unix_socket() {
+local socketfile=$1
+for i in `seq 100`; do
+nc -zU "$socketfile" && return 0
+sleep 0.1
+done
+return 1
+}
+
 # Receives a string with scapy python code that represents a packet.
 # Returns a hex-string that contains bytes that reflect the packet symbolic
 # description.
@@ -833,10 +842,20 @@ ovn_trace_client() {
 # ovs-appctl netdev-dummy/receive $vif $packet
 #
 fmt_pkt() {
-echo "from scapy.all import *; \
-  import binascii; \
-  out = binascii.hexlify(raw($1)); \
-  print(out.decode())" | $PYTHON3
+sockfile=$ovs_base/scapy.sock
+if [[ ! -e $sockfile ]]; then
+start_scapy_server > /dev/null
+wait_unix_socket $sockfile
+fi
+echo "$(echo $1 | nc -U $sockfile)"
+}
+
+start_scapy_server() {
+pidfile=$ovs_base/scapy.pid
+sockfile=$ovs_base/scapy.sock
+"$top_srcdir"/tests/scapy_server.py --pidfile=$pidfile --sockfile=$sockfile
+on_exit "test -e \"$pidfile\" && kill \`cat \"$pidfile\"\`"
+on_exit "test -e \"$sockfile\" && rm \"$sockfile\""
 }
 
 sleep_sb() {
diff --git a/tests/scapy_server.py b/tests/scapy_server.py
new file mode 100755
index 0..a417d42c9
--- /dev/null
+++ b/tests/scapy_server.py
@@ -0,0 +1,100 @@
+#!/usr/bin/env python3
+
+import argparse
+import atexit
+import os
+import socket
+import sys
+import threading
+
+import binascii
+from scapy.all import *  # noqa: F401,F403
+from scapy.all import raw
+
+
+_MAX_PATTERN_LEN = 1024 * 32
+
+
+def write_pidfile(pidfile):
+pid = str(os.getpid())
+with open(pidfile, 'w') as f:
+f.write(pid)
+
+
+def remove_pidfile(pidfile):
+os.remove(pidfile)
+
+
+def check_pidfile(pidfile):
+if os.path.exists(pidfile):
+with open(pidfile, 'r') as f:
+pid = f.read().strip()
+try:
+pid = int(pid)
+if os.path.exists(f"/proc/{pid}"):
+print("Scapy server is already running:", pid)
+sys.exit(1)
+except ValueError:
+sys.exit(1)
+
+
+def fork():
+try:
+pid = os.fork()
+if pid > 0:
+sys.exit(0)
+except OSError as e:
+print("Fork failed:", e)
+sys.exit(1)
+
+
+def daemonize(pidfile):
+fork()
+check_pidfile(pidfile)
+write_pidfile(pidfile)
+atexit.register(remove_pidfile, pidfile)
+
+
+def process(data):
+try:
+return binascii.hexlify(raw(eval(data)))
+except Exception:
+return ""
+
+
+def handle_client(client_socket):
+data = client_socket.recv(_MAX_PATTERN_LEN)
+data = data.strip()
+if data:
+client_socket.send(process(data))
+client_socket.close()
+
+
+def main(pidfile, socket_path):
+daemonize(pidfile)
+
+if os.path.exists(socket_path):
+os.remove(socket_path)
+
+server_socket = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+server_socket.bind(socket_path)
+server_socket.listen()
+
+while True:
+client_socket, _ = server_socket.accept()
+handler = threading.Thread(target=handle_client, 

Re: [ovs-dev] [PATCH v2] doc: Fix description of max_len for controller action.

[Antonin Bas]

Hi Ilya,

Do you think we can merge this patch first as it is specific to the
controller action?
I will check the behavior of output(port=controller,max_len=X) and once I
have confirmation, I can submit a new patch.

Thanks,

Antonin

Le ven. 25 août 2023 à 07:10, Ilya Maximets  a écrit :

> On 8/17/23 02:30, Antonin Bas wrote:
> > From: Antonin Bas 
> >
> > From: Antonin Bas 
> >
> > Since Open vSwitch 2.7, the max_len option has no effect, and the full
> > packet is always sent to controllers. This was confirmed with both the
> > kernel and netdev datapaths.
>
> Hi, Antonin.  Thanks for the patch!  And sorry for delay.
>
> IIUC, this also affects the output(port=controller,max_len=X) actions,
> right?
> In this case, we need to update the docs for the 'output' action as well.
>
> Best regards, Ilya Maximets.
>
> >
> > Reported-by: Antonin Bas 
> > Reported-at: https://github.com/openvswitch/ovs-issues/issues/295
> > Signed-off-by: Antonin Bas 
> > ---
> > v2: Fix typos, set author to VMware email address
> > ---
> >  Documentation/ref/ovs-actions.7.rst | 9 -
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ref/ovs-actions.7.rst
> b/Documentation/ref/ovs-actions.7.rst
> > index d13895655..36adcc5db 100644
> > --- a/Documentation/ref/ovs-actions.7.rst
> > +++ b/Documentation/ref/ovs-actions.7.rst
> > @@ -694,7 +694,8 @@ encapsulated in an OpenFlow ``packet-in`` message.
> The supported options are:
> >  Limit to *max_len* the number of bytes of the packet to send in the
> >  ``packet-in.``  A *max_len* of 0 prevents any of the packet from
> being
> >  sent (thus, only metadata is included).  By default, the entire
> packet is
> > -sent, equivalent to a *max_len* of 65535.
> > +sent, equivalent to a *max_len* of 65535.  This option has no
> effect in
> > +Open vSwith 2.7 and later: the entire packet will always be sent.
> >
> >``reason=``\ *reason*
> >  Specify *reason* as the reason for sending the message in the
> > @@ -733,6 +734,12 @@ encapsulated in an OpenFlow ``packet-in`` message.
> The supported options are:
> >options require the Open vSwitch ``NXAST_CONTROLLER`` extension
> action added
> >in Open vSwitch 1.6.
> >
> > +  Open vSwitch 2.7 and later is configured to not buffer packets for the
> > +  packet-in event.  As a result, the full packet is always sent to
> > +  controllers.  This means that the ``max_len`` option has no effect on
> the
> > +  ``controller`` action, and all values (even 0) are equivalent to the
> default
> > +  value of 65535.
> > +
> >
> >  The ``enqueue`` action
> >  --
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3 ovn] northd: support binding remote ports in ovn-northd

[Lorenzo Bianconi]

ovn supports creating remote logical ports. An user
can set requested-chassis option for a logical switch port
to the remote chassis and ovn-northd can bind it to that chassis.
This is required for OVN IC in ovn-k8s. Right now ovn-k8s
ovnkube-controller after creating a remote logical port, sets the
chassis column of the corresponding port binding in SB DB to the
remote chassis. This process can be implemented in ovn-northd.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2217930
Signed-off-by: Lorenzo Bianconi 
---
Changes since v2:
- address missing logica cases
- add more unit tests

Changes since v1:
- add NEWS entry
- do not remove ovn-ic code to bind sb to gw chassis
- simplify codebase
---
 NEWS|  2 ++
 ic/ovn-ic.c |  5 
 northd/northd.c | 28 +++
 tests/ovn-ic.at |  2 ++
 tests/ovn-northd.at | 67 +
 tests/ovn.at| 27 ++
 6 files changed, 131 insertions(+)

diff --git a/NEWS b/NEWS
index 93d4bedcd..d7580d9bd 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,8 @@ Post v23.06.0
 Existing sessions might get re-hashed to a different ECMP path when
 OVN detects the algorithm support in the datapath during an upgrade
 or restart of ovn-controller.
+  - Introduce support for binding remote ports in ovn-northd if the CMS sets
+requested-chassis option for a remote logical switch port.
 
 OVN v23.06.0 - 01 Jun 2023
 --
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index db7e86bc1..2a8383523 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -651,6 +651,11 @@ sync_remote_port(struct ic_context *ctx,
 /* Sync tunnel key from ISB to NB */
 sync_lsp_tnl_key(lsp, isb_pb->tunnel_key);
 
+/* Skip port binding if it is already requested by the CMS. */
+if (smap_get(>options, "requested-chassis")) {
+return;
+}
+
 /* Sync gateway from ISB to SB */
 if (isb_pb->gateway[0]) {
 if (!sb_pb->chassis || strcmp(sb_pb->chassis->name, isb_pb->gateway)) {
diff --git a/northd/northd.c b/northd/northd.c
index a01f59efb..e6b37e66a 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3579,6 +3579,28 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
 }
 }
 
+if (lsp_is_remote(op->nbsp)) {
+/* ovn-northd is supposed to set port_binding for remote ports
+ * if requested chassis is marked as remote. */
+if (op->sb->requested_chassis &&
+smap_get_bool(>sb->requested_chassis->other_config,
+  "is-remote", false)) {
+sbrec_port_binding_set_chassis(op->sb,
+   op->sb->requested_chassis);
+smap_add(, "is-remote-nb-binded", "true");
+} else if (smap_get_bool(>sb->options,
+ "is-remote-nb-binded", false)) {
+sbrec_port_binding_set_chassis(op->sb, NULL);
+smap_add(, "is-remote-nb-binded", "false");
+}
+} else if (op->sb->chassis &&
+   smap_get_bool(>sb->chassis->other_config,
+ "is-remote", false)) {
+/* Its not a remote port but if the chassis is set and if its a
+ * remote chassis then clear it. */
+sbrec_port_binding_set_chassis(op->sb, NULL);
+}
+
 sbrec_port_binding_set_options(op->sb, );
 smap_destroy();
 if (ovn_is_known_nb_lsp_type(op->nbsp->type)) {
@@ -3616,6 +3638,12 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn *ovnsb_txn,
 sbrec_port_binding_set_ha_chassis_group(op->sb, NULL);
 }
 } else {
+if (op->sb->chassis &&
+smap_get_bool(>sb->chassis->other_config,
+  "is-remote", false)) {
+sbrec_port_binding_set_chassis(op->sb, NULL);
+}
+
 const char *chassis = NULL;
 if (op->peer && op->peer->od && op->peer->od->nbr) {
 chassis = smap_get(>peer->od->nbr->options, "chassis");
diff --git a/tests/ovn-ic.at b/tests/ovn-ic.at
index 09eac860c..66368e7c9 100644
--- a/tests/ovn-ic.at
+++ b/tests/ovn-ic.at
@@ -336,6 +336,7 @@ ovn-nbctl lsp-set-addresses lsp-ts1-lr1 router
 ovn-nbctl lsp-set-type lsp-ts1-lr1 router
 ovn-nbctl --wait=hv lsp-set-options lsp-ts1-lr1 router-port=lrp-lr1-ts1
 
+ovn_as az2 ovn-nbctl lsp-set-options lsp-ts1-lr1 requested-chassis=gw1
 AT_CHECK([ovn_as az2 ovn-nbctl show | uuidfilt], [0], [dnl
 switch <0> (ts1)
 port lsp-ts1-lr1
@@ -350,6 +351,7 @@ lsp-ts1-lr1,remote
 ovn-nbctl lrp-set-gateway-chassis lrp-lr1-ts1 gw1
 OVS_WAIT_UNTIL([ovn_as az2 ovn-sbctl show | grep lsp-ts1-lr1])
 
+ovn_as az2 ovn-nbctl lsp-set-options lsp-ts1-lr1 requested-chassis=""
 ovn-nbctl 

Re: [ovs-dev] [PATCH] lib/fatal-signal: Drop logging of failed dummy backtrace.

[Ilya Maximets]

On 8/22/23 10:54, Ales Musil wrote:
> On Tue, Aug 22, 2023 at 10:40 AM Frode Nordahl 
> wrote:
> 
>> Some systems may provide backtrace() in libc but for some reason
>> not provide any frames when attempting to use it.
>>
>> On those systems the fatal_signal_init() function currently logs
>> this debug message: "Capturing of dummy backtrace has failed."
>>
>> A consequence of this logging may be false negative test results.
>>
>> Logging the fact that backtrace() does not work has limited value
>> on a production system and I propose we drop it.
>>
>> Fixes: 759a29dc2d97 ("backtrace: Extend the backtrace functionality.")
>> Reported-at: https://launchpad.net/bugs/2032623
>> Signed-off-by: Frode Nordahl 
>> ---
>>  lib/fatal-signal.c | 4 
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c
>> index 77f0c87dd..953150074 100644
>> --- a/lib/fatal-signal.c
>> +++ b/lib/fatal-signal.c
>> @@ -138,10 +138,6 @@ fatal_signal_init(void)
>>
>>  backtrace_capture(_bt);
>>
>> -if (!dummy_bt.n_frames) {
>> -VLOG_DBG("Capturing of dummy backtrace has failed.");
>> -}
>> -
>>  fatal_signal_create_wakeup_events();
>>
>>  #ifdef _WIN32
>> --
>> 2.40.1
>>
>>
> Hi Frode,
> 
> I wonder why the test fails on the debug message?

It's a separate binary for which we do not filter output.  We could have
add some checks to this particular test, but it seems like the log is
indeed not very useful

> Nevertheless I don't mind dropping it whatsoever.
> 
> Acked-by: Ales Musil 

Thanks, Frode and Ales!  Applied and backported to 3.2.

Best regards, Ilya Maximets.

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

Re: [ovs-dev] [PATCH v3] docs: Fix rendering of VLAN Comparison Chart

[Ilya Maximets]

On 8/24/23 10:31, Frode Nordahl wrote:
> From: Colin Watson 
> 
> tbl defaults to expecting table entries to be separated by tab
> characters.  However, commit 5a0e4aec1af5cf7741c490bce704577e51e536b9
> converted these to spaces and inadvertently broke the rendering.  Use
> semicolons as separators instead; these are less prone to being broken
> by tree-wide changes, and match the style used by
> build-aux/extract-ofp-fields.
> 
> Reported-by: Lucas Nussbaum 
> Reported-at: https://bugs.debian.org/1042358
> Signed-off-by: Colin Watson 
> Signed-off-by: Frode Nordahl 
> ---
>  lib/meta-flow.xml | 30 +++---
>  1 file changed, 15 insertions(+), 15 deletions(-)

Thanks, Colin and Frode!  Applied and backported down to 2.17.

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

Re: [ovs-dev] [PATCH v2 7/7] system-dpdk: Disable some datapath tests.

[Aaron Conole]

David Marchand  writes:

> On Fri, Aug 25, 2023 at 1:40 PM Eelco Chaudron  wrote:
>> On 23 Aug 2023, at 17:34, David Marchand wrote:
>>
>> > As reported by Ales, net/tap has broken support for checksum offloading.
>> > Fixes have been sent to the DPDK side, but waiting for the fixes, simply
>> > disable all conntrack related tests and some IPv6 tunnel tests.
>> >
>> > Signed-off-by: David Marchand 
>> I think we should make this series dependent on “netdev-dpdk:
>> Disable net/tap Tx L4 checksum offloads.”, so this patch can be
>> removed from the series.

+1

>
> I don't think CI handles such dependencies, a easier way is to send a
> series with everything in it.

That is the only way currently.  We previously discussed having a
mechanism like depends-on / series_*, but there wasn't much interest in
supporting it.  The drawback of depends-on is that it pollutes the git
commit log, and the drawback of developing on top of in-flight series is
when a new version needs to be posted.

> Would that be ok for you?

That's probably the best approach assuming Ales is on board with folding
it all into a single series.

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

Re: [ovs-dev] [PATCH v2 2/2] netdev-tc-offload: Add IPIP/GRE protocols to offload in ip rewrite

[Aaron Conole]

Faicker Mo   writes:

> The warning message is
> |1|tc(handler4)|WARN|can't offload rewrite of IP/IPV6 with ip_proto: X.
>
> IPIP and GRE only need the checksum recalculation of the IP header if the
> IP header is rewritten.
>
> Fixes: d6118e628988 ("netdev-tc-offloads: Verify csum flags on dump from tc")
> Signed-off-by: Faicker Mo 
> ---

This doesn't eliminate the warning completely, so I think the commit
message isn't entirely accurate.  We should instead mention that it is
possible to rewrite the IP/IPV6 headers in the case of these protocols
and therefore we add the support for them.

>  lib/tc.c |  4 +++-
>  tests/system-offloads-traffic.at | 34 
>  2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/lib/tc.c b/lib/tc.c
> index 52a74d9d0..a8cb86371 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -2973,7 +2973,9 @@ csum_update_flag(struct tc_flower *flower,
>  } else if (flower->key.ip_proto == IPPROTO_UDP) {
>  flower->needs_full_ip_proto_mask = true;
>  flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_UDP;
> -} else if (flower->key.ip_proto == IPPROTO_ICMP) {
> +} else if (flower->key.ip_proto == IPPROTO_ICMP ||
> +   flower->key.ip_proto == IPPROTO_IPIP ||
> +   flower->key.ip_proto == IPPROTO_GRE) {
>  flower->needs_full_ip_proto_mask = true;
>  } else if (flower->key.ip_proto == IPPROTO_ICMPV6) {
>  flower->needs_full_ip_proto_mask = true;
> diff --git a/tests/system-offloads-traffic.at 
> b/tests/system-offloads-traffic.at
> index 7215e36e2..2ff74480d 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -855,3 +855,37 @@ AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded 
> | grep "eth_type(0x0800)
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>
> +AT_SETUP([offloads - Add IPIP/GRE protocols to offload in ip rewrite - 
> offloads enabled])
> +OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . 
> other_config:hw-offload=true])
> +
> +AT_CHECK([ovs-ofctl add-flow br0 "priority=0 actions=normal"])
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +
> +dnl Set up the ip field modify flow.
> +AT_CHECK([ovs-ofctl add-flow br0 "priority=100 
> in_port=ovs-p0,ip,nw_dst=10.1.1.2 actions=dec_ttl,output:ovs-p1"])
> +AT_CHECK([ovs-ofctl add-flow br0 "priority=100 
> in_port=ovs-p1,ip,nw_dst=10.1.1.1 actions=dec_ttl,output:ovs-p0"])
> +
> +dnl Set up ipip tunnel in NS.
> +NS_CHECK_EXEC([at_ns0], [ip tunnel add ipip0 remote 10.1.1.2 2>/dev/null], 
> [0])
> +NS_CHECK_EXEC([at_ns0], [ip link set dev ipip0 up 2>/dev/null], [0])
> +NS_CHECK_EXEC([at_ns0], [ip addr add dev ipip0 192.168.1.1/30 2>/dev/null], 
> [0])
> +NS_CHECK_EXEC([at_ns1], [ip tunnel add ipip0 remote 10.1.1.1 2>/dev/null], 
> [0])
> +NS_CHECK_EXEC([at_ns1], [ip link set dev ipip0 up 2>/dev/null], [0])
> +NS_CHECK_EXEC([at_ns1], [ip addr add dev ipip0 192.168.1.2/30 2>/dev/null], 
> [0])
> +
> +dnl Check the tunnel.
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 192.168.1.2 | 
> FORMAT_PING], [0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +
> +dnl Check the offloaded flow.
> +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep 
> "eth_type(0x0800)" | wc -l], [0], [dnl
> +2
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> --
> 2.39.3

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

Re: [ovs-dev] [PATCH v2 1/2] netdev-tc-offload: Add csum offload of protocols IGMP/UDPLITE/SCTP

[Aaron Conole]

Faicker Mo   writes:

> Add tc csum offload support of protocols IGMP/UDPLITE/SCTP
>
> Signed-off-by: Faicker Mo 
> ---

Acked-by: Aaron Conole 

>  lib/tc.c | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/lib/tc.c b/lib/tc.c
> index f49048cda..52a74d9d0 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -2978,6 +2978,15 @@ csum_update_flag(struct tc_flower *flower,
>  } else if (flower->key.ip_proto == IPPROTO_ICMPV6) {
>  flower->needs_full_ip_proto_mask = true;
>  flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_ICMP;
> +} else if (flower->key.ip_proto == IPPROTO_IGMP) {
> +flower->needs_full_ip_proto_mask = true;
> +flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_IGMP;
> +} else if (flower->key.ip_proto == IPPROTO_UDPLITE) {
> +flower->needs_full_ip_proto_mask = true;
> +flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_UDPLITE;
> +} else if (flower->key.ip_proto == IPPROTO_SCTP) {
> +flower->needs_full_ip_proto_mask = true;
> +flower->csum_update_flags |= TCA_CSUM_UPDATE_FLAG_SCTP;
>  } else {
>  VLOG_WARN_RL(_rl,
>   "can't offload rewrite of IP/IPV6 with ip_proto: 
> %d",
> --
> 2.39.3

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

Re: [ovs-dev] [PATCH v2 7/7] system-dpdk: Disable some datapath tests.

[David Marchand]

On Fri, Aug 25, 2023 at 1:40 PM Eelco Chaudron  wrote:
> On 23 Aug 2023, at 17:34, David Marchand wrote:
>
> > As reported by Ales, net/tap has broken support for checksum offloading.
> > Fixes have been sent to the DPDK side, but waiting for the fixes, simply
> > disable all conntrack related tests and some IPv6 tunnel tests.
> >
> > Signed-off-by: David Marchand 
> I think we should make this series dependent on “netdev-dpdk: Disable net/tap 
> Tx L4 checksum offloads.”, so this patch can be removed from the series.

I don't think CI handles such dependencies, a easier way is to send a
series with everything in it.
Would that be ok for you?


-- 
David Marchand

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

Re: [ovs-dev] [PATCH] netdev-dpdk: Disable net/tap Tx L4 checksum offloads.

[Eelco Chaudron]

On 25 Aug 2023, at 15:46, Ilya Maximets wrote:

> On 8/24/23 17:19, David Marchand wrote:
>> As reported by Ales when doing some OVN integration tests with OVS 3.2,
>> net/tap has broken L4 checksum offloads.
>>
>> Fixes are pending on DPDK side.
>> Until they get in a LTS release used by OVS, disable those Tx offloads.
>>
>> Signed-off-by: David Marchand 
>> ---
>>  lib/netdev-dpdk.c | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 8f1361e21f..fc7225cba1 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -1312,6 +1312,14 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>  dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER;
>>  }
>>
>> +if (!strcmp(info.driver_name, "net_tap")) {
>> +VLOG_WARN("%s: disabled Tx L4 checksum offloads for a net/tap 
>> port.",
>> +  netdev_get_name(>up));
>
> Maybe convert this one to INFO?  I'm not sure we need to warn users every
> time the tap interface is reconfigured.  It's not a high performance port
> anyway.
>
> Also, would be nice to have an XXX/FIXME comment here explaining the
> situation, so we do not forget to remove this hack in the future.
>
>> +info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_UDP_CKSUM;
>> +info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_TCP_CKSUM;
>> +info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_TCP_TSO;
>
> Did someone test this with userspace-tso enabled?  I mean, do we need to
> backport this to 3.1 as well?  Or even maybe 2.17 ?

I did not test this with TSO other than the make check’s. Mike can you verify, 
have any comments on this change?

//Eelco

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

Re: [ovs-dev] [PATCH v2 6/7] system-dpdk: Run traffic tests.

[David Marchand]

On Wed, Aug 23, 2023 at 5:35 PM David Marchand
 wrote:
> diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
> index a72133fae1..de126946a0 100644
> --- a/tests/system-dpdk-macros.at
> +++ b/tests/system-dpdk-macros.at
> @@ -87,7 +87,13 @@ $1";/does not exist. The Open vSwitch kernel module is 
> probably not loaded./d
>  /EAL: No \(available\|free\) .*hugepages reported/d
>  /Failed to enable flow control/d
>  /Rx checksum offload is not supported on/d
> -/TELEMETRY: No legacy callbacks, legacy socket not created/d"])
> +/TELEMETRY: No legacy callbacks, legacy socket not created/d
> +/tap_nl_dump_ext_ack(): Specified qdisc kind is unknown/d
> +/qdisc_create_multiq(): Could not add multiq qdisc (2): No such file or 
> directory/d
> +/eth_dev_tap_create(): .*: failed to create multiq qdisc./d
> +/eth_dev_tap_create():  Disabling rte flow support: No such file or 
> directory/d
> +/tap_mp_req_on_rxtx(): Failed to send start req to secondary/d

Since those logs come from the traffic tests (especially the net/tap
logs), I think it is more appropriate to move those to
OVS_TRAFFIC_VSWITCHD_STOP.
Opinion?


> +/does not exist. The Open vSwitch kernel module is probably not loaded./d"])
>])
>
>


-- 
David Marchand

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

Re: [ovs-dev] [PATCH v2 6/7] system-dpdk: Run traffic tests.

[David Marchand]

On Thu, Aug 24, 2023 at 7:52 PM Ilya Maximets  wrote:
>
> On 8/24/23 17:22, Aaron Conole wrote:
> > David Marchand  writes:
> >
> >> On Wed, Aug 23, 2023 at 5:35 PM David Marchand
> >>  wrote:
> >>>
> >>> Integrate system-traffic.at tests as part of check-dpdk.
> >>
> >> CI (thinking of Intel robot) other than GHA might not be too happy
> >> about this change.
> >> It is hard to tell what fails in the report I received.
> >>
> >> A safer approach is probably to define a new check-XXX target instead
> >> of expanding check-dpdk.
> >
> > I'm not sure - at least for Intel, we can ask them to investigate.
> >
> > I noticed that we didn't get any new results from intel after this
> > patch, so I wonder if this change broke something.
> >
> > I also noticed that without the conntrack tests (7/7) this passed, but
> > with them, we got a failure report.  Is it possible that the tests are
> > taking too long (in which case, we need to investigate a way of
> > optimizing this).
>
> I believe, all the tests failed because of this:
>
> > 2023-08-23T22:23:10.040Z|00088|dpdk|ERR|tap_dev_configure(): net_taptap1: 
> > number of rx queues 2 must be equal to number of tx queues 3
> > 2023-08-23T22:23:10.040Z|00089|dpdk|ERR|Port0 dev_configure = -1
> > 2023-08-23T22:23:10.040Z|00090|netdev_dpdk|WARN|Interface ovs-tap1 eth_dev 
> > setup error Operation not permitted

Yes, it seems to be the reason though I did not understand why those
rxq/txq count were used.


> > 2023-08-23T22:23:10.040Z|00091|netdev_dpdk|ERR|Interface ovs-tap1(rxq:2 
> > txq:3 lsc interrupt mode:false) configure error: Operation not permitted
> > 2023-08-23T22:23:10.040Z|00092|netdev_dpdk|INFO|ovs-tap1: rx-steering: 
> > default rss
> > 2023-08-23T22:23:10.040Z|00093|dpif_netdev|ERR|Failed to set interface 
> > ovs-tap1 new configuration
> > 2023-08-23T22:23:10.040Z|00094|dpif_netdev|INFO|Performing pmd to rx queue 
> > assignment using cycles algorithm.
> > 2023-08-23T22:23:10.040Z|00095|dpif_netdev|INFO|Core 21 on numa node 0 
> > assigned port 'dpdkvhostuser0' rx queue 0 (measured processing cycles 0).
> > 2023-08-23T22:23:10.040Z|00096|dpif|WARN|netdev at ovs-netdev: failed to 
> > add ovs-tap1 as port: Invalid argument
> > 2023-08-23T22:23:10.040Z|00097|bridge|WARN|could not add network device 
> > ovs-tap1 to ofproto (Invalid argument)
>
> And failing tests typically take much longer, because we wait
> for some traffic or some flows that never appear.
>
> Configuration for net/tap DPDK ports should be managed more carefully
> in the tests.  We will likely have to use dummy NUMA even.

Oh that's the catch, numa... I reproduced in a vm and I have something
that seems to work, testing a bit more.


>
> The Lab seems to work fine.  Error reporting could be better,
> but it's enough to identify the issue in this case.
>
> We should also keep in mind that these tests should pass in the debian
> autopkgtest environment:
>   
> https://patchwork.ozlabs.org/project/openvswitch/patch/20230627101104.72417-2-frode.nord...@canonical.com/
> Even if we don't have this merged, this is what Ubuntu/Debian folks
> are running internally.

Ok.
For now, I don't see what could be blocking, do you have any specific
problem in mind?


-- 
David Marchand

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

Re: [ovs-dev] [PATCH v2 5/7] system-dpdk: Refactor OVS daemons helpers.

[David Marchand]

On Fri, Aug 25, 2023 at 2:06 PM Eelco Chaudron  wrote:
> > What I think instead is that having DPDK probes all devices by default
> > is not desirable, though it has been working like this since OVS
> > integrated DPDK.
> > I have a patch in store for a long time, I just never found an
> > explicit need for it and it had some drawback in one usecase (multiple
> > ports for a single pci device), maybe I could send a rfc.
>
> See above, as it’s a false alarm and we do not need to make this work ;)

Ah.. too bad, for next time :-).


-- 
David Marchand

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

Re: [ovs-dev] [PATCH v2] ofproto-dpif-mirror: Add support for pre-selection filter

[Adrian Moreno]

On 7/18/23 21:38, Mike Pattrick wrote:

Currently a bridge mirror will collect all packets and tools like
ovs-tcpdump can apply additional filters after they have already been
duplicated by vswitchd. This can result in inefficient collection.

This patch adds support to apply pre-selection to bridge mirrors, which
can limit which packets are mirrored based on flow metadata. This
significantly improves overall vswitchd performance during mirroring if
only a subset of traffic is required.

I benchmarked this with two setups. A netlink based test with two veth
pairs connected to a single bridge, and a netdev based test involving a
mix of DPDK nics, and netdev-linux interfaces. Both tests involved
saturating the link with iperf3 and then sending an icmp ping every
second. I then measured the throughput on the link with no mirroring,
icmp pre-selected mirroring, and full mirroring. The results, below,
indicate a significant reduction to impact of mirroring when only a
subset of the traffic on a port is selected for collection.

  Test No Mirror | No Filter |   Filter  | No Filter |  Filter  |
 +---+---+---+---+--+
netlink | 39.0 Gbps | 36.1 Gbps | 38.2 Gbps | 7%|2%|
netdev  | 7.39 Gbps | 4.95 Gbps | 6.24 Gbps |33%|   15%|

The ratios above are the percent reduction in total throughput when
mirroring is used either with or without a filter.



Looks really promising!


Signed-off-by: Mike Pattrick 
---
  Documentation/ref/ovs-tcpdump.8.rst |  4 ++
  NEWS|  2 +
  lib/flow.h  | 11 ++
  ofproto/ofproto-dpif-mirror.c   | 60 +++--
  ofproto/ofproto-dpif-mirror.h   |  9 -
  ofproto/ofproto-dpif-xlate.c|  6 ++-
  ofproto/ofproto-dpif.c  |  2 +-
  ofproto/ofproto.h   |  3 ++
  tests/ofproto-dpif.at   | 43 +
  utilities/ovs-tcpdump.in| 13 ++-
  vswitchd/bridge.c   |  8 
  vswitchd/vswitch.ovsschema  |  4 +-
  vswitchd/vswitch.xml|  6 +++
  13 files changed, 160 insertions(+), 11 deletions(-)

diff --git a/Documentation/ref/ovs-tcpdump.8.rst 
b/Documentation/ref/ovs-tcpdump.8.rst
index b9f8cdf6f..9163c8a5e 100644
--- a/Documentation/ref/ovs-tcpdump.8.rst
+++ b/Documentation/ref/ovs-tcpdump.8.rst
@@ -61,6 +61,10 @@ Options
  
If specified, mirror all ports (optional).
  
+* ``--filter ``

+
+  If specified, only mirror flows that match the provided filter.
+
  See Also
  
  
diff --git a/NEWS b/NEWS

index 7a852427e..26797ca56 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,7 @@
  Post-v3.2.0
  
+   - ovs-tcpdump:
+ * Added new --filter parameter to apply pre-selection on mirrored flows.
  


Maybe also comment on the new column in the Mirror table of the OVSDB?

  
  v3.2.0 - xx xxx 

diff --git a/lib/flow.h b/lib/flow.h
index a9d026e1c..c2e67dfc5 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -939,6 +939,17 @@ flow_union_with_miniflow(struct flow *dst, const struct 
miniflow *src)
  flow_union_with_miniflow_subset(dst, src, src->map);
  }
  
+/* Perform a bitwise OR of minimask 'src' mask data with the equivalent

+ * fields in 'dst', storing the result in 'dst'. */
+static inline void
+flow_wildcards_union_with_minimask(struct flow_wildcards *dst,
+   const struct minimask *src)
+{
+flow_union_with_miniflow_subset(>masks,
+>masks,
+src->masks.map);
+}
+
  static inline bool is_ct_valid(const struct flow *flow,
 const struct flow_wildcards *mask,
 struct flow_wildcards *wc)
diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
index 343b75f0e..e4174a564 100644
--- a/ofproto/ofproto-dpif-mirror.c
+++ b/ofproto/ofproto-dpif-mirror.c
@@ -21,6 +21,7 @@
  #include "cmap.h"
  #include "hmapx.h"
  #include "ofproto.h"
+#include "ofproto-dpif-trace.h"
  #include "vlan-bitmap.h"
  #include "openvswitch/vlog.h"
  
@@ -57,6 +58,10 @@ struct mirror {

  struct hmapx srcs;  /* Contains "struct mbundle*"s. */
  struct hmapx dsts;  /* Contains "struct mbundle*"s. */
  
+/* Filter criteria. */

+struct miniflow *filter;
+struct minimask *mask;
+
  /* This is accessed by handler threads assuming RCU protection (see
   * mirror_get()), but can be manipulated by mirror_set() without any
   * explicit synchronization. */
@@ -212,7 +217,9 @@ mirror_set(struct mbridge *mbridge, void *aux, const char 
*name,
 struct ofbundle **dsts, size_t n_dsts,
 unsigned long *src_vlans, struct ofbundle *out_bundle,
 uint16_t snaplen,
-   uint16_t out_vlan)
+   uint16_t out_vlan,
+   const char *filter,
+   const struct 

Re: [ovs-dev] [PATCH v2] doc: Fix description of max_len for controller action.

[Ilya Maximets]

On 8/17/23 02:30, Antonin Bas wrote:
> From: Antonin Bas 
> 
> From: Antonin Bas 
> 
> Since Open vSwitch 2.7, the max_len option has no effect, and the full
> packet is always sent to controllers. This was confirmed with both the
> kernel and netdev datapaths.

Hi, Antonin.  Thanks for the patch!  And sorry for delay.

IIUC, this also affects the output(port=controller,max_len=X) actions, right?
In this case, we need to update the docs for the 'output' action as well.

Best regards, Ilya Maximets.

> 
> Reported-by: Antonin Bas 
> Reported-at: https://github.com/openvswitch/ovs-issues/issues/295
> Signed-off-by: Antonin Bas 
> ---
> v2: Fix typos, set author to VMware email address
> ---
>  Documentation/ref/ovs-actions.7.rst | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ref/ovs-actions.7.rst 
> b/Documentation/ref/ovs-actions.7.rst
> index d13895655..36adcc5db 100644
> --- a/Documentation/ref/ovs-actions.7.rst
> +++ b/Documentation/ref/ovs-actions.7.rst
> @@ -694,7 +694,8 @@ encapsulated in an OpenFlow ``packet-in`` message.  The 
> supported options are:
>  Limit to *max_len* the number of bytes of the packet to send in the
>  ``packet-in.``  A *max_len* of 0 prevents any of the packet from being
>  sent (thus, only metadata is included).  By default, the entire packet is
> -sent, equivalent to a *max_len* of 65535.
> +sent, equivalent to a *max_len* of 65535.  This option has no effect in
> +Open vSwith 2.7 and later: the entire packet will always be sent.
>  
>``reason=``\ *reason*
>  Specify *reason* as the reason for sending the message in the
> @@ -733,6 +734,12 @@ encapsulated in an OpenFlow ``packet-in`` message.  The 
> supported options are:
>options require the Open vSwitch ``NXAST_CONTROLLER`` extension action 
> added
>in Open vSwitch 1.6.
>  
> +  Open vSwitch 2.7 and later is configured to not buffer packets for the
> +  packet-in event.  As a result, the full packet is always sent to
> +  controllers.  This means that the ``max_len`` option has no effect on the
> +  ``controller`` action, and all values (even 0) are equivalent to the 
> default
> +  value of 65535.
> +
>  
>  The ``enqueue`` action
>  --

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

Re: [ovs-dev] [PATCH ovn v2 1/5] tests: Check proper DP and port key

[Mark Michelson]

Hi Ales, I applied this series to main and all branches back to 22.09.

On 8/15/23 07:03, Ales Musil wrote:

The test was assuming that the DP key and Port keys are
always fixed. Make sure that we use proper values for the
flow check.

Signed-off-by: Ales Musil 
Acked-by: Mark Michelson 
---
v2: Add ack from Mark.
---
  tests/ovn.at | 67 +++-
  1 file changed, 35 insertions(+), 32 deletions(-)

diff --git a/tests/ovn.at b/tests/ovn.at
index 94f04d011..9bb4b7287 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -21871,8 +21871,8 @@ eth_dst=ff01
  ip_src=$(ip_to_hex 10 0 0 10)
  ip_dst=$(ip_to_hex 172 168 0 101)
  send_icmp_packet 1 1 $eth_src $eth_dst $ip_src $ip_dst c4c9 
00
-AT_CHECK([as hv1 ovs-ofctl dump-flows br-int metadata=0x$lr0_dp_key | awk '/table=28, 
n_packets=1, n_bytes=45/{print $7" "$8}'],[0],[dnl
-priority=80,ip,reg15=0x3,metadata=0x3,nw_src=10.0.0.10 actions=drop
+AT_CHECK_UNQUOTED([as hv1 ovs-ofctl dump-flows br-int metadata=0x$lr0_dp_key | awk 
'/table=28, n_packets=1, n_bytes=45/{print $7" "$8}'],[0],[dnl
+priority=80,ip,reg15=0x$lr0_public_dp_key,metadata=0x$lr0_dp_key,nw_src=10.0.0.10
 actions=drop
  ])
  
  # hv1 should remove the flow for the ACL with is_chassis_redirect check for sw0-vir.

@@ -31432,6 +31432,9 @@ sw0_dpkey=$(fetch_column datapath_binding tunnel_key 
external_ids:name=sw0)
  sw0p1_dpkey=$(fetch_column port_binding tunnel_key logical_port=sw0-p1)
  sw0p3_dpkey=$(fetch_column port_binding tunnel_key logical_port=sw0-p3)
  
+dp_key=$(printf "%x" $sw0_dpkey)

+port_key=$(printf "%x" $sw0p1_dpkey)
+
  check_column '50:54:00:00:00:13' fdb mac
  check_column $sw0_dpkey fdb dp_key
  check_column $sw0p1_dpkey fdb port_key
@@ -31443,12 +31446,12 @@ as hv2 ovs-ofctl dump-flows br-int table=71 > 
hv2_offlows_table71.txt
  
  AT_CAPTURE_FILE([hv1_offlows_table71.txt])

  AT_CAPTURE_FILE([hv2_offlows_table71.txt])
-AT_CHECK([cat hv1_offlows_table71.txt | grep -v NXST | cut -d ' ' -f8- | 
sort], [0], [dnl
-priority=100,metadata=0x1,dl_dst=50:54:00:00:00:13 
actions=load:0x1->NXM_NX_REG15[[]]
+AT_CHECK_UNQUOTED([cat hv1_offlows_table71.txt | grep -v NXST | cut -d ' ' 
-f8- | sort], [0], [dnl
+priority=100,metadata=0x$dp_key,dl_dst=50:54:00:00:00:13 
actions=load:0x$port_key->NXM_NX_REG15[[]]
  ])
  
-AT_CHECK([cat hv2_offlows_table71.txt | grep -v NXST | cut -d ' ' -f8- | sort], [0], [dnl

-priority=100,metadata=0x1,dl_dst=50:54:00:00:00:13 
actions=load:0x1->NXM_NX_REG15[[]]
+AT_CHECK_UNQUOTED([cat hv2_offlows_table71.txt | grep -v NXST | cut -d ' ' 
-f8- | sort], [0], [dnl
+priority=100,metadata=0x$dp_key,dl_dst=50:54:00:00:00:13 
actions=load:0x$port_key->NXM_NX_REG15[[]]
  ])
  
  as hv1 ovs-ofctl dump-flows br-int table=72 > hv1_offlows_table72.txt

@@ -31456,12 +31459,12 @@ as hv2 ovs-ofctl dump-flows br-int table=72 > 
hv2_offlows_table72.txt
  
  AT_CAPTURE_FILE([hv1_offlows_table72.txt])

  AT_CAPTURE_FILE([hv2_offlows_table72.txt])
-AT_CHECK([cat hv1_offlows_table72.txt | grep -v NXST | cut -d ' ' -f8- | 
sort], [0], [dnl
-priority=100,reg14=0x1,metadata=0x1,dl_src=50:54:00:00:00:13 
actions=load:0x1->NXM_NX_REG10[[8]]
+AT_CHECK_UNQUOTED([cat hv1_offlows_table72.txt | grep -v NXST | cut -d ' ' 
-f8- | sort], [0], [dnl
+priority=100,reg14=0x$port_key,metadata=0x$dp_key,dl_src=50:54:00:00:00:13 
actions=load:0x1->NXM_NX_REG10[[8]]
  ])
  
-AT_CHECK([cat hv2_offlows_table72.txt | grep -v NXST | cut -d ' ' -f8- | sort], [0], [dnl

-priority=100,reg14=0x1,metadata=0x1,dl_src=50:54:00:00:00:13 
actions=load:0x1->NXM_NX_REG10[[8]]
+AT_CHECK_UNQUOTED([cat hv2_offlows_table72.txt | grep -v NXST | cut -d ' ' 
-f8- | sort], [0], [dnl
+priority=100,reg14=0x$port_key,metadata=0x$dp_key,dl_src=50:54:00:00:00:13 
actions=load:0x1->NXM_NX_REG10[[8]]
  ])
  
  # Create the logical port sw0-p4 and this should be claimed by

@@ -31480,12 +31483,12 @@ as hv3 ovs-ofctl dump-flows br-int table=72 > 
hv3_offlows_table72.txt
  AT_CAPTURE_FILE([hv3_offlows_table71.txt])
  AT_CAPTURE_FILE([hv3_offlows_table72.txt])
  
-AT_CHECK([cat hv3_offlows_table71.txt | grep -v NXST | cut -d ' ' -f8- | sort], [0], [dnl

-priority=100,metadata=0x1,dl_dst=50:54:00:00:00:13 
actions=load:0x1->NXM_NX_REG15[[]]
+AT_CHECK_UNQUOTED([cat hv3_offlows_table71.txt | grep -v NXST | cut -d ' ' 
-f8- | sort], [0], [dnl
+priority=100,metadata=0x$dp_key,dl_dst=50:54:00:00:00:13 
actions=load:0x$port_key->NXM_NX_REG15[[]]
  ])
  
-AT_CHECK([cat hv3_offlows_table72.txt | grep -v NXST | cut -d ' ' -f8- | sort], [0], [dnl

-priority=100,reg14=0x1,metadata=0x1,dl_src=50:54:00:00:00:13 
actions=load:0x1->NXM_NX_REG10[[8]]
+AT_CHECK_UNQUOTED([cat hv3_offlows_table72.txt | grep -v NXST | cut -d ' ' 
-f8- | sort], [0], [dnl
+priority=100,reg14=0x$port_key,metadata=0x$dp_key,dl_src=50:54:00:00:00:13 
actions=load:0x1->NXM_NX_REG10[[8]]
  ])
  
  # Use the src mac 50:54:00:00:00:14 instead of 50:54:00:00:00:03

@@ -31512,19 +31515,19 @@ as hv3 ovs-ofctl 

Re: [ovs-dev] [PATCH] netdev-dpdk: Disable net/tap Tx L4 checksum offloads.

[Ilya Maximets]

On 8/24/23 17:19, David Marchand wrote:
> As reported by Ales when doing some OVN integration tests with OVS 3.2,
> net/tap has broken L4 checksum offloads.
> 
> Fixes are pending on DPDK side.
> Until they get in a LTS release used by OVS, disable those Tx offloads.
> 
> Signed-off-by: David Marchand 
> ---
>  lib/netdev-dpdk.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 8f1361e21f..fc7225cba1 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1312,6 +1312,14 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>  dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER;
>  }
>  
> +if (!strcmp(info.driver_name, "net_tap")) {
> +VLOG_WARN("%s: disabled Tx L4 checksum offloads for a net/tap port.",
> +  netdev_get_name(>up));

Maybe convert this one to INFO?  I'm not sure we need to warn users every
time the tap interface is reconfigured.  It's not a high performance port
anyway.

Also, would be nice to have an XXX/FIXME comment here explaining the
situation, so we do not forget to remove this hack in the future.

> +info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_UDP_CKSUM;
> +info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_TCP_CKSUM;
> +info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_TCP_TSO;

Did someone test this with userspace-tso enabled?  I mean, do we need to
backport this to 3.1 as well?  Or even maybe 2.17 ?

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

Re: [ovs-dev] [PATCH ovn] northd: Fix order of calloc arguments.

[Ilya Maximets]

On 8/24/23 21:02, Mark Michelson wrote:
> Thanks for the change, Ilya.
> 
> I'm not so sure about this. If there were other related changes in this 
> region of code, then also swapping the arguments would make sense. But 
> on its own, this doesn't fix anything, and it doesn't add any 
> improvements. The only thing I think this might do is cause a headache 
> when trying to apply backports to changes that touch this area of code.

It will not be a problem if this fix is backported. :)

> 
> I can be convinced otherwise, but currently, I don't see a good reason 
> to merge this.

The only concern I have is that issues like this tend to spread if
left in the code.  But I'm fine with dropping this patch, if you
don't think it's necessary.

Best regards, Ilya Maximets.

> 
> On 8/23/23 18:03, Ilya Maximets wrote:
>> It doesn't affect the allocation size, but the number of elements
>> should go before the size of these elements.
>>
>> Fixes: 74daa0607c7f ("ovn-northd: Introduce parallel lflow build")
>> Signed-off-by: Ilya Maximets 
>> ---
>>   northd/northd.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/northd/northd.c b/northd/northd.c
>> index 0a749931e..ea95f6a5c 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -15832,7 +15832,7 @@ build_lswitch_and_lrouter_flows(const struct 
>> ovn_datapaths *ls_datapaths,
>>   struct lswitch_flow_build_info *lsiv;
>>   int index;
>>   
>> -lsiv = xcalloc(sizeof(*lsiv), build_lflows_pool->size);
>> +lsiv = xcalloc(build_lflows_pool->size, sizeof *lsiv);
>>   
>>   /* Set up "work chunks" for each thread to work on. */
>>   
> 

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

Re: [ovs-dev] [PATCH v2 5/7] system-dpdk: Refactor OVS daemons helpers.

[Eelco Chaudron]

On 25 Aug 2023, at 13:34, David Marchand wrote:

> On Fri, Aug 25, 2023 at 12:57 PM Eelco Chaudron  wrote:
>>>
>>> +m4_define([OVS_DPDK_STOP_VSWITCHD],
>>> +  [OVS_VSWITCHD_STOP([dnl
>>> +$1";/does not exist. The Open vSwitch kernel module is probably not 
>>> loaded./d
>>> +/does not support MTU configuration,/d
>>> +/EAL: No \(available\|free\) .*hugepages reported/d
>>> +/Failed to enable flow control/d
>>> +/Rx checksum offload is not supported on/d
>>> +/TELEMETRY: No legacy callbacks, legacy socket not created/d"])
>>
>> On my VMs (with all patches applied) all the existing dpdk tests and new 
>> ones are failing due to the following error messages:
>
> As you mention, the issue predates this current patch and it points at
> a different problem than the logs matching.

I was wrong, it are only the new system tests that are failing, the existing 
ones have the --no-pci option.

So I guess you should add the same for the system traffic one, i.e. the 
following seems to work:

--- a/tests/system-dpdk-macros.at
+++ b/tests/system-dpdk-macros.at
@@ -147,7 +147,7 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
   [
OVS_DPDK_PRE_CHECK()
OVS_WAIT_WHILE([ip link show ovs-netdev])
-   OVS_DPDK_START([], [--disable-system --no-pci], [$3])
+   OVS_DPDK_START([--no-pci], [--disable-system], [$3])
dnl Add bridges, ports, etc.
OVS_WAIT_WHILE([ip link show br0])
AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| 
uuidfilt])], [0], [$2])



>>
>> +2023-08-25T10:09:42.252Z|00016|dpdk|ERR|eth_virtio_pci_init(): Failed to 
>> init PCI device
>> +2023-08-25T10:09:42.252Z|00017|dpdk|ERR|EAL: Requested device :00:05.0 
>> cannot be used
>> +2023-08-25T10:09:42Z|00016|dpdk|ERR|eth_virtio_pci_init(): Failed to init 
>> PCI device
>> +2023-08-25T10:09:42Z|00017|dpdk|ERR|EAL: Requested device :00:05.0 
>> cannot be used
>>
>> Maybe we could exclude them here (this is what I did) or add some --no-pci 
>> option (and make sure the ones that do need them have it).
>>
>> @@ -93,6 +93,8 @@ $1";/does not exist. The Open vSwitch kernel module is 
>> probably not loaded./d
>>  /eth_dev_tap_create(): .*: failed to create multiq qdisc./d
>>  /eth_dev_tap_create():  Disabling rte flow support: No such file or 
>> directory/d
>>  /tap_mp_req_on_rxtx(): Failed to send start req to secondary/d
>> +/eth_virtio_pci_init(): Failed to init PCI device/d
>> +c
>>  /does not exist. The Open vSwitch kernel module is probably not loaded./d"])
>>])
>>
>
> Those warnings are due to the fact that OVS lets DPDK try to grab all
> devices by default.
> And here, we are affected by a port that you don't seem to care about.
>
>
> Besides, about log matching, in the context of the phy port tests, we
> do want to catch errors like "EAL: Requested device .* cannot be
> used".
> You could also imagine someone passing the PCI device of a virtio-net
> for those tests to run so I would not filter the virtio errors.
> So I don't think we should ignore those specific logs in the common macro.
>
>
> Passing --no-pci in the tests that wants to probe one pci device is not 
> doable.
>
>
> What I think instead is that having DPDK probes all devices by default
> is not desirable, though it has been working like this since OVS
> integrated DPDK.
> I have a patch in store for a long time, I just never found an
> explicit need for it and it had some drawback in one usecase (multiple
> ports for a single pci device), maybe I could send a rfc.

See above, as it’s a false alarm and we do not need to make this work ;)

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

Re: [ovs-dev] [PATCH v2 7/7] system-dpdk: Disable some datapath tests.

[Eelco Chaudron]

On 23 Aug 2023, at 17:34, David Marchand wrote:

> As reported by Ales, net/tap has broken support for checksum offloading.
> Fixes have been sent to the DPDK side, but waiting for the fixes, simply
> disable all conntrack related tests and some IPv6 tunnel tests.
>
> Signed-off-by: David Marchand 
I think we should make this series dependent on “netdev-dpdk: Disable net/tap 
Tx L4 checksum offloads.”, so this patch can be removed from the series.

Tested this series with the patch applied and all tests are passing even with 
ASAN, UBSAN enabled.

Cheers,

Eelco




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

Re: [ovs-dev] [PATCH] netdev-dpdk: Disable net/tap Tx L4 checksum offloads.

[Eelco Chaudron]

On 24 Aug 2023, at 17:19, David Marchand wrote:

> As reported by Ales when doing some OVN integration tests with OVS 3.2,
> net/tap has broken L4 checksum offloads.
>
> Fixes are pending on DPDK side.
> Until they get in a LTS release used by OVS, disable those Tx offloads.
>
> Signed-off-by: David Marchand 


Thanks for the patch, this one looks good to me, and tested it in combination 
with the ‘dpdk test patch series’.

Acked-by: Eelco Chaudron 


> ---
>  lib/netdev-dpdk.c | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 8f1361e21f..fc7225cba1 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1312,6 +1312,14 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>  dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER;
>  }
>
> +if (!strcmp(info.driver_name, "net_tap")) {
> +VLOG_WARN("%s: disabled Tx L4 checksum offloads for a net/tap port.",
> +  netdev_get_name(>up));
> +info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_UDP_CKSUM;
> +info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_TCP_CKSUM;
> +info.tx_offload_capa &= ~RTE_ETH_TX_OFFLOAD_TCP_TSO;
> +}
> +
>  if (info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPV4_CKSUM) {
>  dev->hw_ol_features |= NETDEV_TX_IPV4_CKSUM_OFFLOAD;
>  } else {
> -- 
> 2.41.0

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

Re: [ovs-dev] [PATCH v2 5/7] system-dpdk: Refactor OVS daemons helpers.

[David Marchand]

On Fri, Aug 25, 2023 at 12:57 PM Eelco Chaudron  wrote:
> >
> > +m4_define([OVS_DPDK_STOP_VSWITCHD],
> > +  [OVS_VSWITCHD_STOP([dnl
> > +$1";/does not exist. The Open vSwitch kernel module is probably not 
> > loaded./d
> > +/does not support MTU configuration,/d
> > +/EAL: No \(available\|free\) .*hugepages reported/d
> > +/Failed to enable flow control/d
> > +/Rx checksum offload is not supported on/d
> > +/TELEMETRY: No legacy callbacks, legacy socket not created/d"])
>
> On my VMs (with all patches applied) all the existing dpdk tests and new ones 
> are failing due to the following error messages:

As you mention, the issue predates this current patch and it points at
a different problem than the logs matching.

>
> +2023-08-25T10:09:42.252Z|00016|dpdk|ERR|eth_virtio_pci_init(): Failed to 
> init PCI device
> +2023-08-25T10:09:42.252Z|00017|dpdk|ERR|EAL: Requested device :00:05.0 
> cannot be used
> +2023-08-25T10:09:42Z|00016|dpdk|ERR|eth_virtio_pci_init(): Failed to init 
> PCI device
> +2023-08-25T10:09:42Z|00017|dpdk|ERR|EAL: Requested device :00:05.0 
> cannot be used
>
> Maybe we could exclude them here (this is what I did) or add some --no-pci 
> option (and make sure the ones that do need them have it).
>
> @@ -93,6 +93,8 @@ $1";/does not exist. The Open vSwitch kernel module is 
> probably not loaded./d
>  /eth_dev_tap_create(): .*: failed to create multiq qdisc./d
>  /eth_dev_tap_create():  Disabling rte flow support: No such file or 
> directory/d
>  /tap_mp_req_on_rxtx(): Failed to send start req to secondary/d
> +/eth_virtio_pci_init(): Failed to init PCI device/d
> +c
>  /does not exist. The Open vSwitch kernel module is probably not loaded./d"])
>])
>

Those warnings are due to the fact that OVS lets DPDK try to grab all
devices by default.
And here, we are affected by a port that you don't seem to care about.


Besides, about log matching, in the context of the phy port tests, we
do want to catch errors like "EAL: Requested device .* cannot be
used".
You could also imagine someone passing the PCI device of a virtio-net
for those tests to run so I would not filter the virtio errors.
So I don't think we should ignore those specific logs in the common macro.


Passing --no-pci in the tests that wants to probe one pci device is not doable.


What I think instead is that having DPDK probes all devices by default
is not desirable, though it has been working like this since OVS
integrated DPDK.
I have a patch in store for a long time, I just never found an
explicit need for it and it had some drawback in one usecase (multiple
ports for a single pci device), maybe I could send a rfc.



-- 
David Marchand

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

Re: [ovs-dev] [PATCH v2 6/7] system-dpdk: Run traffic tests.

[Eelco Chaudron]

On 23 Aug 2023, at 17:34, David Marchand wrote:

> Integrate system-traffic.at tests as part of check-dpdk.
>
> DPDK net/tap specific logs are added to the log exclusions.
>
> Some tests that can't work with the userspace datapath are skipped by
> overriding some OVS_CHECK_* macros.
>
> ADD_VETH is implemented with a tap interface in the kernel, directly
> polled by OVS.
>
> Signed-off-by: David Marchand 

I think the patch by itself is fine, and I ran it on my Ubuntu 22.04 test VM 
with the “netdev-dpdk: Disable net/tap Tx L4 checksum offloads” applied and it 
was working fine.

Anyhow I’ll include my Acked-by flag, assuming it will pass the autopkgtest 
mentioned by Ilya (guess you need another revision anyway :)


Acked-by: Eelco Chaudron 

> ---
>  tests/system-dpdk-macros.at| 85 +-
>  tests/system-dpdk-testsuite.at |  2 +
>  tests/system-dpdk.at   |  3 --
>  3 files changed, 86 insertions(+), 4 deletions(-)
>
> diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
> index a72133fae1..de126946a0 100644
> --- a/tests/system-dpdk-macros.at
> +++ b/tests/system-dpdk-macros.at
> @@ -87,7 +87,13 @@ $1";/does not exist. The Open vSwitch kernel module is 
> probably not loaded./d
>  /EAL: No \(available\|free\) .*hugepages reported/d
>  /Failed to enable flow control/d
>  /Rx checksum offload is not supported on/d
> -/TELEMETRY: No legacy callbacks, legacy socket not created/d"])
> +/TELEMETRY: No legacy callbacks, legacy socket not created/d
> +/tap_nl_dump_ext_ack(): Specified qdisc kind is unknown/d
> +/qdisc_create_multiq(): Could not add multiq qdisc (2): No such file or 
> directory/d
> +/eth_dev_tap_create(): .*: failed to create multiq qdisc./d
> +/eth_dev_tap_create():  Disabling rte flow support: No such file or 
> directory/d
> +/tap_mp_req_on_rxtx(): Failed to send start req to secondary/d
> +/does not exist. The Open vSwitch kernel module is probably not loaded./d"])
>])
>
>
> @@ -124,3 +130,80 @@ m4_define([OVS_DPDK_STOP_TESTPMD],
>[AT_CHECK([kill `cat testpmd.pid`])
> OVS_WAIT([kill -0 `cat testpmd.pid`], [kill -9 `cat testpmd.pid`])
>])
> +
> +
> +# OVS_TRAFFIC_VSWITCHD_START([vsctl-args], [vsctl-output], [dbinit-aux-args])
> +#
> +# Creates a database and starts ovsdb-server, starts ovs-vswitchd
> +# connected to that database, calls ovs-vsctl to create a bridge named
> +# br0 with predictable settings, passing 'vsctl-args' as additional
> +# commands to ovs-vsctl.  If 'vsctl-args' causes ovs-vsctl to provide
> +# output (e.g. because it includes "create" commands) then 'vsctl-output'
> +# specifies the expected output after filtering through uuidfilt.
> +# 'dbinit-aux-args' are passed as additional commands to 'ovs-vsctl init'
> +# before starting ovs-vswitchd.
> +m4_define([OVS_TRAFFIC_VSWITCHD_START],
> +  [
> +   OVS_DPDK_PRE_CHECK()
> +   OVS_WAIT_WHILE([ip link show ovs-netdev])
> +   OVS_DPDK_START([], [--disable-system], [$3])
> +   dnl Add bridges, ports, etc.
> +   OVS_WAIT_WHILE([ip link show br0])
> +   AT_CHECK([ovs-vsctl -- _ADD_BR([br0]) -- $1 m4_if([$2], [], [], [| 
> uuidfilt])], [0], [$2])
> +])
> +
> +
> +# OVS_TRAFFIC_VSWITCHD_STOP([ALLOWLIST], [extra_cmds])
> +#
> +# Gracefully stops ovs-vswitchd and ovsdb-server, checking their log files
> +# for messages with severity WARN or higher and signaling an error if any
> +# is present.  The optional ALLOWLIST may contain shell-quoted "sed"
> +# commands to delete any warnings that are actually expected, e.g.:
> +#
> +#   OVS_TRAFFIC_VSWITCHD_STOP(["/expected error/d"])
> +#
> +# 'extra_cmds' are shell commands to be executed after OVS_VSWITCHD_STOP() is
> +# invoked. They can be used to perform additional cleanups such as name space
> +# removal.
> +m4_define([OVS_TRAFFIC_VSWITCHD_STOP],
> +  [OVS_DPDK_STOP_VSWITCHD([$1])
> +   AT_CHECK([:; $2])
> +  ])
> +
> +
> +m4_define([OVS_CHECK_8021AD],
> +[AT_SKIP_IF([:])])
> +
> +
> +m4_define([OVS_CHECK_TC_QDISC],
> +[AT_SKIP_IF([:])])
> +
> +
> +m4_define([OVS_CHECK_TCPDUMP],
> +[AT_SKIP_IF([:])])
> +
> +
> +# Fake a veth by creating a tap on kernel side and plug it in OVS using the
> +# net/tap DPDK driver.
> +m4_define([ADD_VETH],
> +[ AT_CHECK([ovs-vsctl add-port $3 ovs-$1 -- \
> +set interface ovs-$1 external-ids:iface-id="$1" -- \
> +set interface ovs-$1 type=dpdk -- \
> +set interface ovs-$1 options:n_rxq=2 -- \
> +set interface ovs-$1 
> options:dpdk-devargs=net_tap$1,iface=$1])
> +  OVS_WAIT_UNTIL([ip link show dev $1 | grep -qw LOWER_UP])
> +  AT_CHECK([ip link set $1 netns $2])
> +  NS_CHECK_EXEC([$2], [ip addr add $4 dev $1 $7])
> +  NS_CHECK_EXEC([$2], [ip link set dev $1 up])
> +  if test -n "$5"; then
> +NS_CHECK_EXEC([$2], [ip link set dev $1 address $5])
> +  fi
> +  if test -n "$6"; then
> +NS_CHECK_EXEC([$2], [ip route add default via $6])
> +  fi
> +]
> +)

Re: [ovs-dev] [PATCH v2 5/7] system-dpdk: Refactor OVS daemons helpers.

[Eelco Chaudron]

On 23 Aug 2023, at 17:34, David Marchand wrote:

> Align system-dpdk existing helpers to other common OVS helpers so they
> can accept some optional arguments.
>
> Introduce a OVS_DPDK_STOP_VSWITCHD wrapper around OVS_VSWITCHD_STOP to
> catch dpdk related logs in a centralised fashion.
>
> Signed-off-by: David Marchand 

For this one I do have a comments :)

//Eelco

> ---
>  tests/system-dpdk-macros.at |  21 -
>  tests/system-dpdk.at| 159 +++-
>  2 files changed, 83 insertions(+), 97 deletions(-)
>
> diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
> index 108280f70d..a72133fae1 100644
> --- a/tests/system-dpdk-macros.at
> +++ b/tests/system-dpdk-macros.at
> @@ -36,12 +36,13 @@ m4_define([OVS_DPDK_PRE_PHY_SKIP],
>  #
>  m4_define([OVS_DPDK_START],
>[dnl start ovs dpdk
> -   OVS_DPDK_START_OVSDB()
> +   OVS_DPDK_START_OVSDB($3)
> dnl Enable DPDK functionality
> AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch . 
> other_config:dpdk-init=true])
> -   OVS_DPDK_START_VSWITCHD($1)
> +   OVS_DPDK_START_VSWITCHD([$1], [$2])
>  ])
>
> +
>  # OVS_DPDK_START_OVSDB()
>  #
>  # Create an empty database and start ovsdb-server.
> @@ -60,9 +61,10 @@ m4_define([OVS_DPDK_START_OVSDB],
> AT_CAPTURE_FILE([ovsdb-server.log])
>
> dnl Initialize database.
> -   AT_CHECK([ovs-vsctl --no-wait init])
> +   AT_CHECK([ovs-vsctl --no-wait init $1])
>  ])
>
> +
>  # OVS_DPDK_START_VSWITCHD()
>  #
>  # Add special configuration for dpdk-init. Start ovs-vswitchd.
> @@ -72,12 +74,23 @@ m4_define([OVS_DPDK_START_VSWITCHD],
> AT_CHECK([ovs-vsctl --no-wait set Open_vSwitch . 
> other_config:dpdk-extra="--log-level=pmd.*:error $1"])
>
> dnl Start ovs-vswitchd.
> -   AT_CHECK([ovs-vswitchd --detach --no-chdir --pidfile --log-file -vvconn 
> -vofproto_dpif -vunixctl], [0], [stdout], [stderr])
> +   AT_CHECK([ovs-vswitchd $2 --detach --no-chdir --pidfile --log-file 
> -vvconn -vofproto_dpif -vunixctl], [0], [stdout], [stderr])
> AT_CAPTURE_FILE([ovs-vswitchd.log])
> on_exit "kill_ovs_vswitchd `cat ovs-vswitchd.pid`"
>  ])
>
>
> +m4_define([OVS_DPDK_STOP_VSWITCHD],
> +  [OVS_VSWITCHD_STOP([dnl
> +$1";/does not exist. The Open vSwitch kernel module is probably not loaded./d
> +/does not support MTU configuration,/d
> +/EAL: No \(available\|free\) .*hugepages reported/d
> +/Failed to enable flow control/d
> +/Rx checksum offload is not supported on/d
> +/TELEMETRY: No legacy callbacks, legacy socket not created/d"])

On my VMs (with all patches applied) all the existing dpdk tests and new ones 
are failing due to the following error messages:

+2023-08-25T10:09:42.252Z|00016|dpdk|ERR|eth_virtio_pci_init(): Failed to init 
PCI device
+2023-08-25T10:09:42.252Z|00017|dpdk|ERR|EAL: Requested device :00:05.0 
cannot be used
+2023-08-25T10:09:42Z|00016|dpdk|ERR|eth_virtio_pci_init(): Failed to init PCI 
device
+2023-08-25T10:09:42Z|00017|dpdk|ERR|EAL: Requested device :00:05.0 cannot 
be used

Maybe we could exclude them here (this is what I did) or add some --no-pci 
option (and make sure the ones that do need them have it).

@@ -93,6 +93,8 @@ $1";/does not exist. The Open vSwitch kernel module is 
probably not loaded./d
 /eth_dev_tap_create(): .*: failed to create multiq qdisc./d
 /eth_dev_tap_create():  Disabling rte flow support: No such file or directory/d
 /tap_mp_req_on_rxtx(): Failed to send start req to secondary/d
+/eth_virtio_pci_init(): Failed to init PCI device/d
+/EAL: Requested device .* cannot be used/d
 /does not exist. The Open vSwitch kernel module is probably not loaded./d"])
   ])

> +
> +
>  # OVS_DPDK_CHECK_TESTPMD()
>  #
>  # Check dpdk-testpmd availability.
> diff --git a/tests/system-dpdk.at b/tests/system-dpdk.at
> index 270587e2c0..e8a04d1d86 100644
> --- a/tests/system-dpdk.at
> +++ b/tests/system-dpdk.at
> @@ -3,15 +3,6 @@ m4_define([CONFIGURE_VETH_OFFLOADS],
>
>  AT_BANNER([OVS-DPDK unit tests])
>
> -m4_define([SYSTEM_DPDK_ALLOWED_LOGS],[
> -\@does not exist. The Open vSwitch kernel module is probably not loaded.@d
> -\@does not support MTU configuration,@d
> -\@EAL: No \(available\|free\) .*hugepages reported@d
> -\@Failed to enable flow control@d
> -\@Rx checksum offload is not supported on@d
> -\@TELEMETRY: No legacy callbacks, legacy socket not created@d
> -])
> -
>  dnl CHECK_MEMPOOL_PARAM([mtu], [numa], [+line])
>  dnl
>  dnl Waits for logs to indicate that the user has configured a mempool
> @@ -36,7 +27,7 @@ OVS_DPDK_START([--no-pci])
>  AT_CHECK([grep "DPDK Enabled - initializing..." ovs-vswitchd.log], [], 
> [stdout])
>  AT_CHECK([grep "EAL" ovs-vswitchd.log], [], [stdout])
>  AT_CHECK([grep "DPDK Enabled - initialized" ovs-vswitchd.log], [], [stdout])
> -OVS_VSWITCHD_STOP("[SYSTEM_DPDK_ALLOWED_LOGS]")
> +OVS_DPDK_STOP_VSWITCHD
>  AT_CLEANUP
>  dnl 
> --
>
> @@ -58,7 +49,7 @@ sleep 2
>
>  dnl Clean up
>  AT_CHECK([ovs-vsctl 

Re: [ovs-dev] [PATCH v2 4/7] tests: Define a macro to skip tc/tcpdump relying tests.

[Eelco Chaudron]

On 23 Aug 2023, at 17:34, David Marchand wrote:

> Some unit tests expect that a OVS port has an associated netdevice on
> which they can hook tc or tcpdump.
> This will not be possible when testing the userspace datapath with DPDK.
> Introduce two helpers (which will be overriden in system-dpdk tests) and
> use them in the existing tests.
>
> Signed-off-by: David Marchand 

These changes also look good to me!

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v2 3/7] ci: Run DPDK tests in GitHub Actions.

[Eelco Chaudron]

On 23 Aug 2023, at 17:34, David Marchand wrote:

> Let's enhance our coverage in the CI and run DPDK system tests.
>
> A few DPDK drivers are enabled in DPDK compilation.
>
> Put DPDK build in $PATH for dpdk-testpmd to be available.
> sudo drops PATH= updates and -E alone does not seem to preserve this
> variable.
> Pass PATH=$PATH when running the tests, as a workaround.
> Since those tests are run as root, the collection of logs is updated
> accordingly.
>
> In GHA, only two cores are available but some test rely on testpmd using
> three lcores.
> Add a DPDK_EAL_OPTIONS environment variable and use it to map all
> testpmd lcores to core 1 (and leave core 0 alone for OVS main and PMD
> threads).
>
> Signed-off-by: David Marchand 
> Acked-by: Aaron Conole 

Changes look good to me!

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v2 2/7] system-dpdk: Don't require hugetlbfs.

[Eelco Chaudron]

On 23 Aug 2023, at 17:34, David Marchand wrote:

> dpdk-testpmd does not need hugetlbfs backing as we don't require
> multiprocess support in OVS unit tests.
> Plus, there is no need for explicitly reserving more memory than what
> testpmd would actually need at runtime.
>
> Switch to --in-memory, use dynamic allocations and remove the then
> unneeded check on hugetlbfs presence.
>
> Signed-off-by: David Marchand 
> Acked-by: Aaron Conole 

Thanks for the patch, this one looks good to me.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v2 1/7] system-dpdk: Introduce helpers for testpmd.

[Eelco Chaudron]

On 23 Aug 2023, at 17:34, David Marchand wrote:

> Rather than copy/paste everywhere, introduce helpers to control
> testpmd runs.
> Rely on --stats-period (which outputs port stats every n seconds) so that
> testpmd keeps running without expecting any user input.
>
> Signed-off-by: David Marchand 
> Acked-by: Aaron Conole 
> ---
> Changes since v1:
> - fixed OVS_DPDK_START_TESTPMD passed arguments evaluation:: $@ -> $1,

Thanks for sending out this series! This patch looks good to me.

Acked-by: Eelco Chaudron 

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

Re: [ovs-dev] [PATCH v5] ofproto-dpif-xlate: Don't reinstall removed XC_LEARN rule

[Mike Pattrick]

On Thu, Aug 24, 2023 at 6:01 PM Ilya Maximets  wrote:
>
> On 8/22/23 22:45, Mike Pattrick wrote:
> > When the a revalidator thread is updating statistics for an XC_LEARN
> > xcache entry in xlate_push_stats_entry it uses ofproto_flow_mod_learn.
> > The revalidator will update stats for rules even if they are in a
> > removed state or marked as invisible. However, ofproto_flow_mod_learn
> > will detect if a flow has been removed and re-add it in that case. This
> > can result in an old learn action replacing the new learn action that
> > had replaced it in the first place.
> >
> > This change adds a new stats_used parameter to ofproto_flow_mod_learn
> > allowing the caller to provide a timestamp that will be fed into the
> > learned rule's modified time. The provided timestamp should be the time
> > of the last packet activity. If stats_used is not set then the current
> > time is used, as is the current behaviour.
> >
> > This change also adds a check when replacing a learned rule to favour the
> > newest rule.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2213892
> > Signed-off-by: Mike Pattrick 
> > ---
> >  v2: Added additional checks if rule is removed
> >  v3: v2 patch was corrupted in transit
> >  v4: Added check against dpif flow stats
> >  v5: Fixed typos, updated commit message
> >  Changed timestamps to use datapath timestamp more consistently
> > ---
>
> I'll quote my previous review comment:
>
> It should be possible to add a test for this issue using time
> warping.  Please, add one.

I tried to make a test with netdev-dummy/receive and time/warp,
however, it didn't reproduce the behaviour that's observed with a full
test. Do you have any suggestions?


Thank you,
Mike

>
>
>
> >  ofproto/ofproto-dpif-xlate-cache.c |  2 +-
> >  ofproto/ofproto-dpif-xlate.c   |  8 +++-
> >  ofproto/ofproto-dpif.c |  2 +-
> >  ofproto/ofproto-provider.h |  6 ++-
> >  ofproto/ofproto.c  | 65 +-
> >  5 files changed, 69 insertions(+), 14 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate-cache.c 
> > b/ofproto/ofproto-dpif-xlate-cache.c
> > index 9224ee2e6..2e1fcb3a6 100644
> > --- a/ofproto/ofproto-dpif-xlate-cache.c
> > +++ b/ofproto/ofproto-dpif-xlate-cache.c
> > @@ -125,7 +125,7 @@ xlate_push_stats_entry(struct xc_entry *entry,
> >  case XC_LEARN: {
> >  enum ofperr error;
> >  error = ofproto_flow_mod_learn(entry->learn.ofm, true,
> > -   entry->learn.limit, NULL);
> > +   entry->learn.limit, NULL, 
> > stats->used);
> >  if (error) {
> >  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> >  VLOG_WARN_RL(, "xcache LEARN action execution failed.");
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 47ea0f47e..bcacb2f0a 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -5700,8 +5700,14 @@ xlate_learn_action(struct xlate_ctx *ctx, const 
> > struct ofpact_learn *learn)
> >  if (!error) {
> >  bool success = true;
> >  if (ctx->xin->allow_side_effects) {
> > +long long int stats_used = 0;
> > +
> > +if (ctx->xin->resubmit_stats) {
> > +stats_used = ctx->xin->resubmit_stats->used;
> > +}
> >  error = ofproto_flow_mod_learn(ofm, ctx->xin->xcache != 
> > NULL,
> > -   learn->limit, );
> > +   learn->limit, ,
> > +   stats_used);
> >  } else if (learn->limit) {
> >  if (!ofm->temp_rule
> >  || ofm->temp_rule->state != RULE_INSERTED) {
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index e22ca757a..1efd9fc91 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -4880,7 +4880,7 @@ packet_xlate(struct ofproto *ofproto_, struct 
> > ofproto_packet_out *opo)
> >  if (entry->type == XC_LEARN) {
> >  struct ofproto_flow_mod *ofm = entry->learn.ofm;
> >
> > -error = ofproto_flow_mod_learn_refresh(ofm);
> > +error = ofproto_flow_mod_learn_refresh(ofm, 0);
> >  if (error) {
> >  goto error_out;
> >  }
> > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> > index 143ded690..dc5c1a60d 100644
> > --- a/ofproto/ofproto-provider.h
> > +++ b/ofproto/ofproto-provider.h
> > @@ -2027,9 +2027,11 @@ enum ofperr ofproto_flow_mod_init_for_learn(struct 
> > ofproto *,
> >  struct ofproto_flow_mod *)
> >  OVS_EXCLUDED(ofproto_mutex);
> >  enum ofperr ofproto_flow_mod_learn(struct ofproto_flow_mod *, bool 
> >