Re: [ovs-dev] [PATCH v4] ofproto: Add upcall/dump-ufid-rules command to map UFIDs to OpenFlow.

[Mike Pattrick]

On Thu, Jun 27, 2024 at 4:13 PM Ilya Maximets  wrote:
>
> On 6/18/24 21:41, Dumitru Ceara wrote:
> > It improves the debugging experience if we can easily get a list of
> > OpenFlow rules and groups that contribute to the creation of a datapath
> > flow.
> >
> > The suggested workflow is:
> > a. dump datapath flows (along with UUIDs), this also prints the core IDs
> > (PMD IDs) when applicable.
> >
> >   $ ovs-appctl dpctl/dump-flows -m
> >   flow-dump from pmd on cpu core: 7
> >   ufid:7460db8f..., recirc_id(0), 
> >
> > b. dump related OpenFlow rules and groups:
> >   $ ovs-appctl upcall/dump-ufid-rules ufid:7460db8f... pmd=7
> >   cookie=0x12345678, table=0 
> > priority=100,ip,in_port=2,nw_dst=10.0.0.2,actions=resubmit(,1)
> >   cookie=0x0, table=1 priority=200,actions=group:1
> >   group_id=1,bucket=bucket_id:0,actions=ct(commit,table=2,nat(dst=20.0.0.2))
> >   cookie=0x0, table=2 actions=output:1
>
> Hi, Dumitru.  Thanks for the patch!
>
> This is definitely an interesting debug interface.  See some comments inline.
>
>
> As an idea, should we call it ofproto/detrace ?
>
> >
> > The new command only shows rules and groups attached to ukeys that are
> > in states UKEY_VISIBLE or UKEY_OPERATIONAL.  That should be fine as all
> > other ukeys should not be relevant for the use case presented above.
> >
> > For ukeys that don't have an xcache populated yet, the command goes
> > ahead and populates one.  In theory this is creates a slight overhead as
> > those ukeys might not need an xcache until they see traffic (and get
> > revalidated) but in practice the overhead should be minimal.
>
> The flow dumped for the first time should always be revalidated,
> even if it didn't see any more traffic.  See the should_revalidate()
> function.  This means that there is only very narrow window between
> the flow installation and the first revalidation, so the cache should
> be availabel in all practical cases.
>
> Kepeing that in mind, I don't think that populating xcache from the
> debug appctl is a good idea.  We're holding the mutex for a long time
> while effectively revalidating the ukey from the appctl and more
> importantly we're revalidating it with side effects allowed.  This
> means we can learn some rules or update some other caches or even
> emit some packets.  We should not do that from the debug appctl.
>
> I suggest to avoid this and just return early if the cache is not
> yet available.  It should be available in most practical situations.
>
> >
> > This commit tries to mimic the output format of the ovs-ofctl
> > dump-flows/dump-groups commands.  For groups it actually uses
> > ofputil_group/_bucket functions for formatting.  For rules it uses
> > flow_stats_ds() (the original function was exported and renamed to
> > ofproto_rule_stats_ds()).
> >
> > Signed-off-by: Dumitru Ceara 
> > ---
> > V4:
> > - Addressed Mike's comment:
> >   - use predictable port IDs.
> > V3:
> > - Addressed Mike's comments:
> >   - use ds_put_char()
> >   - make the test less flaky
> > V2:
> > - Addressed Adrian's comments:
> >   - check return value of populate_xcache()
> >   - use flow_stats_ds() (renamed to ofproto_rule_stats_ds()) instead of
> > custom printing
> >   - move ukey->state check to caller
> >   - handle case when group bucket is not available
> >   - update test case to cover the point above
> > ---
> >  NEWS|   3 +
> >  include/openvswitch/ofp-group.h |   7 ++
> >  lib/ofp-group.c | 110 +++-
> >  ofproto/ofproto-dpif-upcall.c   |  85 
> >  ofproto/ofproto-dpif.c  |  24 +++
> >  ofproto/ofproto-dpif.h  |   2 +
> >  ofproto/ofproto-provider.h  |   1 +
> >  ofproto/ofproto.c   |  85 
> >  tests/ofproto-dpif.at   |  47 ++
> >  tests/ofproto-macros.at |   9 ++-
> >  10 files changed, 285 insertions(+), 88 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 5ae0108d552b..1bc085f97045 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -9,6 +9,9 @@ Post-v3.3.0
> >   https://github.com/openvswitch/ovs.git
> > - DPDK:
> >   * OVS validated with DPDK 23.11.1.
> > +   - ovs-appctl:
> > + * Added 'upcall/dump-ufid-rules' to output the set of OpenFlow rules 
> > and
> > +   groups that contributed to the creation of a specific datapath flow.
>
> I'd move this to the top, alpabetically.  I know that some entries
> here are not in order, but it's better to keep them at least in
> approximate order.
>
> >
> >
> >  v3.3.0 - 16 Feb 2024
> > diff --git a/include/openvswitch/ofp-group.h 
> > b/include/openvswitch/ofp-group.h
> > index cd7af0ebff9c..79fcb3a4c0d1 100644
> > --- a/include/openvswitch/ofp-group.h
> > +++ b/include/openvswitch/ofp-group.h
> > @@ -70,6 +70,11 @@ struct ofputil_bucket *ofputil_bucket_find(const struct 
> > ovs_list *,
> >  bool ofputil_bucket_check_duplicate_id(const struct ovs_list *);
> >  st

[ovs-dev] [PATCH v1 2/2] ovs-monitor-ipsec: LibreSwan v5 support.

[Mike Pattrick]

In version 5, LibreSwan made significant command line interface changes.
This includes changing the order or command line parameters and removing
the "ipsec auto" command.

To maintain compatibility with previous versions, the ipsec.d version
check is repurposed for this. Checking the version proved simpler than
removing use of auto.

There was also a change to ipsec status command that effected the tests.
However, this change was backwards compatible.

Reported-at: https://issues.redhat.com/browse/FDP-645
Reported-by: Ilya Maximets 
Signed-off-by: Mike Pattrick 
---
 ipsec/ovs-monitor-ipsec.in | 46 +-
 tests/system-ipsec.at  |  8 +++
 2 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/ipsec/ovs-monitor-ipsec.in b/ipsec/ovs-monitor-ipsec.in
index 2b602c75f..37c509ac6 100755
--- a/ipsec/ovs-monitor-ipsec.in
+++ b/ipsec/ovs-monitor-ipsec.in
@@ -459,6 +459,7 @@ conn prevent_unencrypted_vxlan
 def __init__(self, libreswan_root_prefix, args):
 # Collect version infromation
 self.IPSEC = libreswan_root_prefix + "/usr/sbin/ipsec"
+self.IPSEC_AUTO = [self.IPSEC]
 proc = subprocess.Popen([self.IPSEC, "--version"],
 stdout=subprocess.PIPE,
 encoding="latin1")
@@ -470,6 +471,11 @@ conn prevent_unencrypted_vxlan
 except:
 version = 0
 
+if version < 5:
+# With v5, LibreSWAN removed the auto command, however, it is
+# still required for older versions
+self.IPSEC_AUTO.append("auto")
+
 if version >= 4:
 ipsec_d = args.ipsec_d if args.ipsec_d else "/var/lib/ipsec/nss"
 else:
@@ -593,7 +599,7 @@ conn prevent_unencrypted_vxlan
 
 def refresh(self, monitor):
 vlog.info("Refreshing LibreSwan configuration")
-subprocess.call([self.IPSEC, "auto", "--ctlsocket", self.IPSEC_CTL,
+subprocess.call(self.IPSEC_AUTO + ["--ctlsocket", self.IPSEC_CTL,
 "--config", self.IPSEC_CONF, "--rereadsecrets"])
 tunnels = set(monitor.tunnels.keys())
 
@@ -621,7 +627,7 @@ conn prevent_unencrypted_vxlan
 
 if not tunnel or tunnel.version != ver:
 vlog.info("%s is outdated %u" % (conn, ver))
-subprocess.call([self.IPSEC, "auto", "--ctlsocket",
+subprocess.call(self.IPSEC_AUTO + ["--ctlsocket",
 self.IPSEC_CTL, "--config",
 self.IPSEC_CONF, "--delete", conn])
 elif ifname in tunnels:
@@ -643,44 +649,44 @@ conn prevent_unencrypted_vxlan
 # Update shunt policy if changed
 if monitor.conf_in_use["skb_mark"] != monitor.conf["skb_mark"]:
 if monitor.conf["skb_mark"]:
-subprocess.call([self.IPSEC, "auto",
-"--config", self.IPSEC_CONF,
+subprocess.call(self.IPSEC_AUTO +
+["--config", self.IPSEC_CONF,
 "--ctlsocket", self.IPSEC_CTL,
 "--add",
 "--asynchronous", "prevent_unencrypted_gre"])
-subprocess.call([self.IPSEC, "auto",
-"--config", self.IPSEC_CONF,
+subprocess.call(self.IPSEC_AUTO +
+["--config", self.IPSEC_CONF,
 "--ctlsocket", self.IPSEC_CTL,
 "--add",
 "--asynchronous", "prevent_unencrypted_geneve"])
-subprocess.call([self.IPSEC, "auto",
-"--config", self.IPSEC_CONF,
+subprocess.call(self.IPSEC_AUTO +
+["--config", self.IPSEC_CONF,
 "--ctlsocket", self.IPSEC_CTL,
 "--add",
 "--asynchronous", "prevent_unencrypted_stt"])
-subprocess.call([self.IPSEC, "auto",
-"--config", self.IPSEC_CONF,
+subprocess.call(self.IPSEC_AUTO +
+["--config", self.IPSEC_CONF,
 "--ctlsocket", self.IPSEC_CTL,
 "--add",
 "--asynchronous", "prevent_unencrypted_vxlan"])
 else:
-subprocess.call([self.IPSEC, "auto",
-"--config", self.IPSEC_CONF,
+subprocess.call(self.IPSEC_AUTO +
+["--config", self.IPSEC_CONF,
 "--ctlsocket", self.IPSEC_CTL,
 "--delete",
 "--asynchronous", "prevent_unencrypted_gre"])
-subprocess.call([self.IPSEC, "auto",
-"--config", self.IPSEC_CONF,
+subpr

[ovs-dev] [PATCH v1 1/2] ovs-monitor-ipsec: LibreSwan autodetect verion.

[Mike Pattrick]

Previously a change was made to LibreSwan necessitating the detection
of version numbers. However, this change didn't properly account for all
possible output from "ipsec version". When installed from the git
repository, LibreSwan will report versions differently then when
installed from a package.

Fixes: 3ddb31f60487 ("ovs-monitor-ipsec: LibreSwan autodetect paths.")
Signed-off-by: Mike Pattrick 
---
 ipsec/ovs-monitor-ipsec.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipsec/ovs-monitor-ipsec.in b/ipsec/ovs-monitor-ipsec.in
index bc7ac5523..2b602c75f 100755
--- a/ipsec/ovs-monitor-ipsec.in
+++ b/ipsec/ovs-monitor-ipsec.in
@@ -464,7 +464,7 @@ conn prevent_unencrypted_vxlan
 encoding="latin1")
 pout, perr = proc.communicate()
 
-v = re.match("^Libreswan (.*)$", pout)
+v = re.match("^Libreswan v?(.*)$", pout)
 try:
 version = int(v.group(1).split(".")[0])
 except:
-- 
2.39.3

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

Re: [ovs-dev] [PATCH v3 ovn] northd: Add bfd, static_routes and route_policies I-P nodes.

[Numan Siddique]

On Wed, Jun 5, 2024 at 10:55 AM Lorenzo Bianconi
 wrote:
>
> Introduce bfd, static_routes and route_policies nodes to northd I-P
> engine to track bfd connections and northd static_route/policy_route
> changes.
>
> Reported-at: https://issues.redhat.com/browse/FDP-600
> Signed-off-by: Lorenzo Bianconi 
> ---
> Changes since v2:
> - add incremental processing routines
> - split bfd_consumer node in static_routes and route_policies nodes
> Changes since v1:
> - add incremental processing logic for bfd_consumer node to avoid a full
>   recompute

Hi Lorenzo,

Thanks for the v3.  Please see below for some comments.

This patch is missing the test cases for the I-P handling.  Please see
the existing test cases [1]
in ovn-northd.at and please add test cases accordingly, making sure
that ovn-northd generates
the logical flows  as expected when a change occurs and then after recompute.
Please also check the engine stats.

[1] - https://github.com/ovn-org/ovn/blob/main/tests/ovn-northd.at#L11076

Thanks
Numan

> ---
>  northd/en-lflow.c|  25 +-
>  northd/en-northd.c   | 192 
>  northd/en-northd.h   |  15 +
>  northd/inc-proc-northd.c |  31 +-
>  northd/northd.c  | 648 +++
>  northd/northd.h  |  62 +++-
>  6 files changed, 757 insertions(+), 216 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index c4b927fb8..3dba5034b 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -41,6 +41,11 @@ lflow_get_input_data(struct engine_node *node,
>   struct lflow_input *lflow_input)
>  {
>  struct northd_data *northd_data = engine_get_input_data("northd", node);
> +struct bfd_data *bfd_data = engine_get_input_data("bfd", node);
> +struct static_routes_data *static_routes_data =
> +engine_get_input_data("static_routes", node);
> +struct route_policies_data *route_policies_data =
> +engine_get_input_data("route_policies", node);
>  struct port_group_data *pg_data =
>  engine_get_input_data("port_group", node);
>  struct sync_meters_data *sync_meters_data =
> @@ -50,10 +55,6 @@ lflow_get_input_data(struct engine_node *node,
>  struct ed_type_ls_stateful *ls_stateful_data =
>  engine_get_input_data("ls_stateful", node);
>
> -lflow_input->nbrec_bfd_table =
> -EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> -lflow_input->sbrec_bfd_table =
> -EN_OVSDB_GET(engine_get_input("SB_bfd", node));
>  lflow_input->sbrec_logical_flow_table =
>  EN_OVSDB_GET(engine_get_input("SB_logical_flow", node));
>  lflow_input->sbrec_multicast_group_table =
> @@ -78,7 +79,10 @@ lflow_get_input_data(struct engine_node *node,
>  lflow_input->meter_groups = &sync_meters_data->meter_groups;
>  lflow_input->lb_datapaths_map = &northd_data->lb_datapaths_map;
>  lflow_input->svc_monitor_map = &northd_data->svc_monitor_map;
> -lflow_input->bfd_connections = NULL;
> +lflow_input->bfd_connections = &bfd_data->bfd_connections;
> +lflow_input->parsed_routes = &static_routes_data->parsed_routes;
> +lflow_input->route_tables = &static_routes_data->route_tables;
> +lflow_input->route_policies = &route_policies_data->route_policies;
>
>  struct ed_type_global_config *global_config =
>  engine_get_input_data("global_config", node);
> @@ -95,25 +99,14 @@ void en_lflow_run(struct engine_node *node, void *data)
>  struct lflow_input lflow_input;
>  lflow_get_input_data(node, &lflow_input);
>
> -struct hmap bfd_connections = HMAP_INITIALIZER(&bfd_connections);
> -lflow_input.bfd_connections = &bfd_connections;
> -
>  stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
>
>  struct lflow_data *lflow_data = data;
>  lflow_table_clear(lflow_data->lflow_table);
>  lflow_reset_northd_refs(&lflow_input);
>
> -build_bfd_table(eng_ctx->ovnsb_idl_txn,
> -lflow_input.nbrec_bfd_table,
> -lflow_input.sbrec_bfd_table,
> -lflow_input.lr_ports,
> -&bfd_connections);
>  build_lflows(eng_ctx->ovnsb_idl_txn, &lflow_input,
>   lflow_data->lflow_table);
> -bfd_cleanup_connections(lflow_input.nbrec_bfd_table,
> -&bfd_connections);
> -hmap_destroy(&bfd_connections);
>  stopwatch_stop(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
>
>  engine_set_node_state(node, EN_UPDATED);
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 4479b4aff..a4de71ba1 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -236,6 +236,150 @@ northd_global_config_handler(struct engine_node *node, 
> void *data OVS_UNUSED)
>  return true;
>  }
>
> +bool
> +route_policies_change_handler(struct engine_node *node, void *data)
> +{
> +struct northd_data *northd_data = engine_get_input_data("northd", node);
> +struct route_policies_data *route_

Re: [ovs-dev] [PATCH net-next v3 0/7] selftests: net: Switch pmtu.sh to use the internal ovs script.

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski :

On Tue, 25 Jun 2024 13:22:38 -0400 you wrote:
> Currently, if a user wants to run pmtu.sh and cover all the provided test
> cases, they need to install the Open vSwitch userspace utilities.  This
> dependency is difficult for users as well as CI environments, because the
> userspace build and setup may require lots of support and devel packages
> to be installed, system setup to be correct, and things like permissions
> and selinux policies to be properly configured.
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/7] selftests: openvswitch: Support explicit tunnel port 
creation.
https://git.kernel.org/netdev/net-next/c/f94ecbc92092
  - [net-next,v3,2/7] selftests: openvswitch: Refactor actions parsing.
https://git.kernel.org/netdev/net-next/c/37de65a764ed
  - [net-next,v3,3/7] selftests: openvswitch: Add set() and set_masked() 
support.
https://git.kernel.org/netdev/net-next/c/a4126f90a35f
  - [net-next,v3,4/7] selftests: openvswitch: Add support for tunnel() key.
https://git.kernel.org/netdev/net-next/c/fefe3b7d6bec
  - [net-next,v3,5/7] selftests: openvswitch: Support implicit ipv6 arguments.
https://git.kernel.org/netdev/net-next/c/51458e1084d0
  - [net-next,v3,6/7] selftests: net: Use the provided dpctl rather than the 
vswitchd for tests.
https://git.kernel.org/netdev/net-next/c/b7ce46fc614d
  - [net-next,v3,7/7] selftests: net: add config for openvswitch
(no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html


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

Re: [ovs-dev] [PATCH v2] python: ovsdb-idl: Add custom transaction operations.

[Ilya Maximets]

On 4/15/24 18:20, Terry Wilson wrote:
> It can be useful to be able to send raw transaction operations
> through the Idl's connection. For example, to clean up MAC_Binding
> entries for floating IPs without having to monitor the MAC_Binding
> table which can be quite large.
> 
> Signed-off-by: Terry Wilson 

Thanks, Terry.  I wish there were a better way to deal with this issue,
but this solution seems fine as well.  See some comments below.

> ---
>  NEWS |  2 ++
>  python/ovs/db/idl.py | 18 ++-
>  tests/ovsdb-idl.at   | 27 ++
>  tests/test-ovsdb.py  | 55 
>  4 files changed, 101 insertions(+), 1 deletion(-)
> 
> diff --git a/NEWS b/NEWS
> index b92cec532..f30b859fd 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -7,6 +7,8 @@ Post-v3.3.0
> - The primary development branch has been renamed from 'master' to 'main'.
>   The OVS tree remains hosted on GitHub.
>   https://github.com/openvswitch/ovs.git
> +   - Python:
> + * Add custom transaction support to the Idl via add_op().
>  
>  
>  v3.3.0 - 16 Feb 2024
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index a80da84e7..1220e4cc6 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -1703,6 +1703,8 @@ class Transaction(object):
>  
>  self._inserted_rows = {}  # Map from UUID to _InsertedRow
>  
> +self._operations = []
> +
>  def add_comment(self, comment):
>  """Appends 'comment' to the comments that will be passed to the OVSDB
>  server when this transaction is committed.  (The comment will be
> @@ -1838,7 +1840,7 @@ class Transaction(object):
> "rows": [rows]})
>  
>  # Add updates.
> -any_updates = False
> +any_updates = bool(self._operations)
>  for row in self._txn_rows.values():
>  if row._changes is None:
>  if row._table.is_root:
> @@ -1977,6 +1979,7 @@ class Transaction(object):
>  if self.dry_run:
>  operations.append({"op": "abort"})
>  
> +operations += self._operations

I think, we should add them before the abort, so the execution gets
to these even with dry run.

>  if not any_updates:
>  self._status = Transaction.UNCHANGED
>  else:
> @@ -1991,6 +1994,19 @@ class Transaction(object):
>  self.__disassemble()
>  return self._status
>  
> +def add_op(self, op):
> +"""Add a raw OVSDB operation to the transaction
> +
> +This can be useful for re-using the existing Idl connection to take
> +actions that are difficult or expensive to do with the Idl itself. 
> e.g.
> +deleting a bunch of rows on the server that you don't want to store
> +in memory.

The docs generally shouldn't talk to the reader, but describe the
behavior instead.  How about 'E.g. bulk deleting rows from the server
without downloading them into a local cache.' ?

Also, we need to document that these operations are always added to
the end of transaction, i.e. after all other operations.

> +
> +:param op: An "op" for an OVSDB "transact" request (rfc 7047 Sec 5.2)
> +:type op:   dict
> +"""
> +self._operations.append(op)
> +
>  def commit_block(self):
>  """Attempts to commit this transaction, blocking until the commit
>  either succeeds or fails.  Returns the final commit status, which may
> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> index fb568dd82..487545a13 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -2758,6 +2758,33 @@ OVSDB_CHECK_IDL_PERS_UUID_INSERT([simple idl, 
> persistent uuid insert],
>[['This UUID would duplicate a UUID already present within the table or 
> deleted within the same transaction']])
>  
>  
> +OVSDB_CHECK_IDL_PY([simple idl, python, add_op],
> +  [],
> +  [['insert 1, insert 2, insert 3, insert 1' \
> +'add_op {"op": "delete", "table": "simple", "where": [["i", "==", 1]]}' \
> +'add_op {"op": "insert", "table": "simple", "row": {"i": 2}}, delete 3' \
> +'insert 2, add_op {"op": "update", "table": "simple", "row": {"i": 1}, 
> "where": [["i", "==", 2]]}'
> +  ]],
> +  [[000: empty
> +001: commit, status=success
> +002: table simple: i=1 r=0 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] 
> uuid=<1>
> +002: table simple: i=1 r=0 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] 
> uuid=<2>
> +002: table simple: i=2 r=0 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] 
> uuid=<3>
> +002: table simple: i=3 r=0 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] 
> uuid=<4>
> +003: commit, status=success
> +004: table simple: i=2 r=0 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] 
> uuid=<3>
> +004: table simple: i=3 r=0 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] 
> uuid=<4>
> +005: commit, status=success
> +006: table simple: i=2 r=0 b=false s= u=<0> ia=[] ra=[] ba=[] sa=[] ua=[] 
> uuid=<3>
> 

Re: [ovs-dev] [PATCH v4] ofproto: Add upcall/dump-ufid-rules command to map UFIDs to OpenFlow.

[Ilya Maximets]

On 6/18/24 21:41, Dumitru Ceara wrote:
> It improves the debugging experience if we can easily get a list of
> OpenFlow rules and groups that contribute to the creation of a datapath
> flow.
> 
> The suggested workflow is:
> a. dump datapath flows (along with UUIDs), this also prints the core IDs
> (PMD IDs) when applicable.
> 
>   $ ovs-appctl dpctl/dump-flows -m
>   flow-dump from pmd on cpu core: 7
>   ufid:7460db8f..., recirc_id(0), 
> 
> b. dump related OpenFlow rules and groups:
>   $ ovs-appctl upcall/dump-ufid-rules ufid:7460db8f... pmd=7
>   cookie=0x12345678, table=0 
> priority=100,ip,in_port=2,nw_dst=10.0.0.2,actions=resubmit(,1)
>   cookie=0x0, table=1 priority=200,actions=group:1
>   group_id=1,bucket=bucket_id:0,actions=ct(commit,table=2,nat(dst=20.0.0.2))
>   cookie=0x0, table=2 actions=output:1

Hi, Dumitru.  Thanks for the patch!

This is definitely an interesting debug interface.  See some comments inline.


As an idea, should we call it ofproto/detrace ?

> 
> The new command only shows rules and groups attached to ukeys that are
> in states UKEY_VISIBLE or UKEY_OPERATIONAL.  That should be fine as all
> other ukeys should not be relevant for the use case presented above.
> 
> For ukeys that don't have an xcache populated yet, the command goes
> ahead and populates one.  In theory this is creates a slight overhead as
> those ukeys might not need an xcache until they see traffic (and get
> revalidated) but in practice the overhead should be minimal.

The flow dumped for the first time should always be revalidated,
even if it didn't see any more traffic.  See the should_revalidate()
function.  This means that there is only very narrow window between
the flow installation and the first revalidation, so the cache should
be availabel in all practical cases.

Kepeing that in mind, I don't think that populating xcache from the
debug appctl is a good idea.  We're holding the mutex for a long time
while effectively revalidating the ukey from the appctl and more
importantly we're revalidating it with side effects allowed.  This
means we can learn some rules or update some other caches or even
emit some packets.  We should not do that from the debug appctl.

I suggest to avoid this and just return early if the cache is not
yet available.  It should be available in most practical situations.

> 
> This commit tries to mimic the output format of the ovs-ofctl
> dump-flows/dump-groups commands.  For groups it actually uses
> ofputil_group/_bucket functions for formatting.  For rules it uses
> flow_stats_ds() (the original function was exported and renamed to
> ofproto_rule_stats_ds()).
> 
> Signed-off-by: Dumitru Ceara 
> ---
> V4:
> - Addressed Mike's comment:
>   - use predictable port IDs.
> V3:
> - Addressed Mike's comments:
>   - use ds_put_char()
>   - make the test less flaky
> V2:
> - Addressed Adrian's comments:
>   - check return value of populate_xcache()
>   - use flow_stats_ds() (renamed to ofproto_rule_stats_ds()) instead of
> custom printing
>   - move ukey->state check to caller
>   - handle case when group bucket is not available
>   - update test case to cover the point above
> ---
>  NEWS|   3 +
>  include/openvswitch/ofp-group.h |   7 ++
>  lib/ofp-group.c | 110 +++-
>  ofproto/ofproto-dpif-upcall.c   |  85 
>  ofproto/ofproto-dpif.c  |  24 +++
>  ofproto/ofproto-dpif.h  |   2 +
>  ofproto/ofproto-provider.h  |   1 +
>  ofproto/ofproto.c   |  85 
>  tests/ofproto-dpif.at   |  47 ++
>  tests/ofproto-macros.at |   9 ++-
>  10 files changed, 285 insertions(+), 88 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 5ae0108d552b..1bc085f97045 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -9,6 +9,9 @@ Post-v3.3.0
>   https://github.com/openvswitch/ovs.git
> - DPDK:
>   * OVS validated with DPDK 23.11.1.
> +   - ovs-appctl:
> + * Added 'upcall/dump-ufid-rules' to output the set of OpenFlow rules and
> +   groups that contributed to the creation of a specific datapath flow.

I'd move this to the top, alpabetically.  I know that some entries
here are not in order, but it's better to keep them at least in
approximate order.

>  
>  
>  v3.3.0 - 16 Feb 2024
> diff --git a/include/openvswitch/ofp-group.h b/include/openvswitch/ofp-group.h
> index cd7af0ebff9c..79fcb3a4c0d1 100644
> --- a/include/openvswitch/ofp-group.h
> +++ b/include/openvswitch/ofp-group.h
> @@ -70,6 +70,11 @@ struct ofputil_bucket *ofputil_bucket_find(const struct 
> ovs_list *,
>  bool ofputil_bucket_check_duplicate_id(const struct ovs_list *);
>  struct ofputil_bucket *ofputil_bucket_list_front(const struct ovs_list *);
>  struct ofputil_bucket *ofputil_bucket_list_back(const struct ovs_list *);
> +void ofputil_bucket_format(const struct ofputil_bucket *,
> +   enum ofp11_group_type, enum ofp_versio

Re: [ovs-dev] [PATCH v2 ovn] Do not reply on unicast arps for IPv4 targets.

[Numan Siddique]

On Fri, May 24, 2024 at 4:00 AM Vasyl Saienko  wrote:
>
> Reply only if target ethernet address is broadcast, if
> address is specified explicitly do noting to let target
> reply by itself. This technique allows to monitor target
> aliveness with arping.
>
> Closes  #239
>
> Signed-off-by: Vasyl Saienko 

Sorry for the late reviews.

Acked-by: Numan Siddique 

Numan

> ---
>  northd/northd.c | 11 +--
>  northd/ovn-northd.8.xml |  7 ---
>  tests/ovn-northd.at |  4 ++--
>  3 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 37f443e70..e80e1885d 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -8832,8 +8832,15 @@ build_lswitch_arp_nd_responder_known_ips(struct 
> ovn_port *op,
>  for (size_t i = 0; i < op->n_lsp_addrs; i++) {
>  for (size_t j = 0; j < op->lsp_addrs[i].n_ipv4_addrs; j++) {
>  ds_clear(match);
> -ds_put_format(match, "arp.tpa == %s && arp.op == 1",
> -op->lsp_addrs[i].ipv4_addrs[j].addr_s);
> +/* NOTE(vsaienko): Do not reply on unicast ARPs, forward
> + * them to the target to have ability to monitor target
> + * aliveness via ARPs.
> +*/
> +ds_put_format(match,
> +"arp.tpa == %s && "
> +"arp.op == 1 && "
> +"eth.dst == ff:ff:ff:ff:ff:ff",
> +op->lsp_addrs[i].ipv4_addrs[j].addr_s);
>  ds_clear(actions);
>  ds_put_format(actions,
>  "eth.dst = eth.src; "
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index b14a30285..ffdd67895 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -1435,9 +1435,10 @@
>
>
>  
> -  Priority-50 flows that match ARP requests to each known IP address
> -  A of every logical switch port, and respond with ARP
> -  replies directly with corresponding Ethernet address E:
> +  Priority-50 flows that match only broadcast ARP requests to each
> +  known IPv4 address A of every logical switch port, and
> +  respond with ARP replies directly with corresponding Ethernet
> +  address E:
>  
>
>  
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index be006fb32..4162196f4 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -9283,9 +9283,9 @@ AT_CAPTURE_FILE([S1flows])
>
>  AT_CHECK([grep -e "ls_in_arp_rsp" S1flows | ovn_strip_lflows], [0], [dnl
>table=??(ls_in_arp_rsp  ), priority=0, match=(1), action=(next;)
> -  table=??(ls_in_arp_rsp  ), priority=100  , match=(arp.tpa == 
> 192.168.0.10 && arp.op == 1 && inport == "S1-vm1"), action=(next;)
> +  table=??(ls_in_arp_rsp  ), priority=100  , match=(arp.tpa == 
> 192.168.0.10 && arp.op == 1 && eth.dst == ff:ff:ff:ff:ff:ff && inport == 
> "S1-vm1"), action=(next;)
>table=??(ls_in_arp_rsp  ), priority=100  , match=(nd_ns && ip6.dst == 
> {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10 && inport == "S1-vm1"), 
> action=(next;)
> -  table=??(ls_in_arp_rsp  ), priority=50   , match=(arp.tpa == 
> 192.168.0.10 && arp.op == 1), action=(eth.dst = eth.src; eth.src = 
> 50:54:00:00:00:10; arp.op = 2; /* ARP reply */ arp.tha = arp.sha; arp.sha = 
> 50:54:00:00:00:10; arp.tpa = arp.spa; arp.spa = 192.168.0.10; outport = 
> inport; flags.loopback = 1; output;)
> +  table=??(ls_in_arp_rsp  ), priority=50   , match=(arp.tpa == 
> 192.168.0.10 && arp.op == 1 && eth.dst == ff:ff:ff:ff:ff:ff), action=(eth.dst 
> = eth.src; eth.src = 50:54:00:00:00:10; arp.op = 2; /* ARP reply */ arp.tha = 
> arp.sha; arp.sha = 50:54:00:00:00:10; arp.tpa = arp.spa; arp.spa = 
> 192.168.0.10; outport = inport; flags.loopback = 1; output;)
>table=??(ls_in_arp_rsp  ), priority=50   , match=(nd_ns && ip6.dst == 
> {fd00::10, ff02::1:ff00:10} && nd.target == fd00::10), action=(nd_na { 
> eth.src = 50:54:00:00:00:10; ip6.src = fd00::10; nd.target = fd00::10; nd.tll 
> = 50:54:00:00:00:10; outport = inport; flags.loopback = 1; output; };)
>  ])
>
> --
> 2.43.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] Userspace: Software fallback for UDP encapsulated TCP segmentation.

[David Marchand]

On Fri, Jun 21, 2024 at 5:07 PM David Marchand
 wrote:
> net/mlx5 is claiming (geneve|vxlan)_tso_offload while not supporting
> outer udp checksum.

This may be a set of capabilities we can use if, for example, no udp
checksum is requested.

How about adding:
# git diff
diff --git a/lib/netdev.c b/lib/netdev.c
index 1d59bbe5d..429c86a22 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -926,6 +926,17 @@ netdev_send(struct netdev *netdev, int qid,
struct dp_packet_batch *batch,
 return netdev_send_tso(netdev, qid, batch, concurrent_txq);
 }
 }
+} else if (!(netdev_flags & NETDEV_TX_OFFLOAD_OUTER_UDP_CKSUM)) {
+DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
+if (!dp_packet_hwol_is_tso(packet) &&
+!dp_packet_hwol_is_tunnel_vxlan(packet) &&
+!dp_packet_hwol_is_tunnel_geneve(packet)) {
+continue;
+}
+if (dp_packet_hwol_is_outer_udp_cksum(packet)) {
+return netdev_send_tso(netdev, qid, batch, concurrent_txq);
+}
+}
 }
 }

Here are some numbers on mlx5, with the udp length fix I mentionned
previously + this hunk above:

# Sw GSO when csum enabled
- ovs-vsctl del-port tunnel0 -- add-port br-int tunnel0 -- set
interface tunnel0 type=vxlan options:remote_ip=172.31.3.1
options:key=0 options:csum=true

Connecting to host 172.31.2.2, port 5201
Reverse mode, remote host 172.31.2.2 is sending
[  5] local 172.31.2.1 port 58748 connected to 172.31.2.2 port 5201
[ ID] Interval   Transfer Bitrate
[  5]   0.00-1.00   sec   580 MBytes  4.87 Gbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval   Transfer Bitrate Retr
[  5]   0.00-1.04   sec   582 MBytes  4.70 Gbits/sec0 sender
[  5]   0.00-1.00   sec   580 MBytes  4.87 Gbits/sec  receiver

iperf Done.

# Hw GSO when csum disabled
- ovs-vsctl del-port tunnel0 -- add-port br-int tunnel0 -- set
interface tunnel0 type=vxlan options:remote_ip=172.31.3.1
options:key=0 options:csum=false

Connecting to host 172.31.2.2, port 5201
Reverse mode, remote host 172.31.2.2 is sending
[  5] local 172.31.2.1 port 39080 connected to 172.31.2.2 port 5201
[ ID] Interval   Transfer Bitrate
[  5]   0.00-1.00   sec  1.50 GBytes  12.9 Gbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval   Transfer Bitrate Retr
[  5]   0.00-1.04   sec  1.50 GBytes  12.4 Gbits/sec  213 sender
[  5]   0.00-1.00   sec  1.50 GBytes  12.9 Gbits/sec  receiver

iperf Done.



-- 
David Marchand

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

Re: [ovs-dev] [RFC v2 4/9] vswitchd: Add local sampling to vswitchd schema.

[Eelco Chaudron]

On 27 Jun 2024, at 13:37, Adrián Moreno wrote:

> On Wed, Jun 26, 2024 at 02:11:00PM GMT, Eelco Chaudron wrote:
>> On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
>>
>>> Add as new column in the Flow_Sample_Collector_Set table named
>>> "local_sample_group" which enables this feature.
>>>
>>> Signed-off-by: Adrian Moreno 
>>> ---
>>>  NEWS   |  4 ++
>>>  vswitchd/bridge.c  | 78 +++---
>>>  vswitchd/vswitch.ovsschema |  9 -
>>>  vswitchd/vswitch.xml   | 39 +--
>>>  4 files changed, 119 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/NEWS b/NEWS
>>> index b92cec532..1c05a7120 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -7,6 +7,10 @@ Post-v3.3.0
>>> - The primary development branch has been renamed from 'master' to 
>>> 'main'.
>>>   The OVS tree remains hosted on GitHub.
>>>https://github.com/openvswitch/ovs.git
>>> +   - Local sampling is introduced. It reuses the OpenFlow sample action and
>>> + allows samples to be emitted locally (instead of via IPFIX) in a
>>> + datapath-specific manner via the new datapath action called 
>>> "emit_sample".
>>> + Linux kernel datapath is the first to support this feature.
>>>
>>>
>>>  v3.3.0 - 16 Feb 2024
>>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
>>> index 95a65fcdc..cd7dc6646 100644
>>> --- a/vswitchd/bridge.c
>>> +++ b/vswitchd/bridge.c
>>> @@ -288,6 +288,7 @@ static void bridge_configure_mac_table(struct bridge *);
>>>  static void bridge_configure_mcast_snooping(struct bridge *);
>>>  static void bridge_configure_sflow(struct bridge *, int 
>>> *sflow_bridge_number);
>>>  static void bridge_configure_ipfix(struct bridge *);
>>> +static void bridge_configure_lsample(struct bridge *);
>>>  static void bridge_configure_spanning_tree(struct bridge *);
>>>  static void bridge_configure_tables(struct bridge *);
>>>  static void bridge_configure_dp_desc(struct bridge *);
>>> @@ -989,6 +990,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch 
>>> *ovs_cfg)
>>>  bridge_configure_netflow(br);
>>>  bridge_configure_sflow(br, &sflow_bridge_number);
>>>  bridge_configure_ipfix(br);
>>> +bridge_configure_lsample(br);
>>>  bridge_configure_spanning_tree(br);
>>>  bridge_configure_tables(br);
>>>  bridge_configure_dp_desc(br);
>>> @@ -1537,10 +1539,11 @@ ovsrec_ipfix_is_valid(const struct ovsrec_ipfix 
>>> *ipfix)
>>>  return ipfix && ipfix->n_targets > 0;
>>>  }
>>>
>>> -/* Returns whether a Flow_Sample_Collector_Set row is valid. */
>>> +/* Returns whether a Flow_Sample_Collector_Set row constains valid IPFIX
>>
>> constains -> 'contains a'
>>
>
> Ack.
>
>>> + * configuration. */
>>>  static bool
>>> -ovsrec_fscs_is_valid(const struct ovsrec_flow_sample_collector_set *fscs,
>>> - const struct bridge *br)
>>> +ovsrec_fscs_is_valid_ipfix(const struct ovsrec_flow_sample_collector_set 
>>> *fscs,
>>> +   const struct bridge *br)
>>>  {
>>>  return ovsrec_ipfix_is_valid(fscs->ipfix) && fscs->bridge == br->cfg;
>>>  }
>>> @@ -1558,7 +1561,7 @@ bridge_configure_ipfix(struct bridge *br)
>>>  const char *virtual_obs_id;
>>>
>>>  OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
>>> -if (ovsrec_fscs_is_valid(fe_cfg, br)) {
>>> +if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
>>>  n_fe_opts++;
>>>  }
>>>  }
>>> @@ -1621,7 +1624,7 @@ bridge_configure_ipfix(struct bridge *br)
>>>  fe_opts = xcalloc(n_fe_opts, sizeof *fe_opts);
>>>  opts = fe_opts;
>>>  OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
>>> -if (ovsrec_fscs_is_valid(fe_cfg, br)) {
>>> +if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
>>>  opts->collector_set_id = fe_cfg->id;
>>>  sset_init(&opts->targets);
>>>  sset_add_array(&opts->targets, fe_cfg->ipfix->targets,
>>> @@ -1667,6 +1670,71 @@ bridge_configure_ipfix(struct bridge *br)
>>>  }
>>>  }
>>>
>>> +/* Returns whether a Flow_Sample_Collector_Set row contains valid local
>>> + * sampling configuraiton. */
>>
>> ...row contains a valid local...
>>++
>>
>> configuraiton -> configuration
>
> Ack.
>
>>
>>> +static bool
>>> +ovsrec_fscs_is_valid_local(const struct ovsrec_flow_sample_collector_set 
>>> *fscs,
>>> +   const struct bridge *br)
>>> +{
>>> +return fscs->local_sample_group && fscs->n_local_sample_group == 1 &&
>>> +   fscs->bridge == br->cfg;
>>> +}
>>> +
>>> +/* Set local sample configuration on 'br'. */
>>> +static void
>>> +bridge_configure_lsample(struct bridge *br)
>>> +{
>>> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>>> +const struct ovsrec_flow_sample_collector_set *fscs;
>>> +struct ofproto_lsample_options *opts_array, *opts;
>>> +size_t n_opts = 0;
>>> +int ret;
>>> +
>>> +/

Re: [ovs-dev] [RFC v2 3/9] ofproto: Add ofproto-dpif-lsample.

[Eelco Chaudron]

On 27 Jun 2024, at 13:16, Adrián Moreno wrote:

> On Wed, Jun 26, 2024 at 02:10:27PM GMT, Eelco Chaudron wrote:
>> On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
>>
>>> Add a new resource in ofproto-dpif and the corresponding API in
>>> ofproto_provider.h to represent and local sampling configuration.
>>>
>>> Signed-off-by: Adrian Moreno 
>>
>> See some comments below.
>>
>> Cheers,
>>
>> Eelco
>>
>>> ---
>>>  ofproto/automake.mk|   2 +
>>>  ofproto/ofproto-dpif-lsample.c | 185 +
>>>  ofproto/ofproto-dpif-lsample.h |  34 ++
>>>  ofproto/ofproto-dpif.c |  37 +++
>>>  ofproto/ofproto-dpif.h |   1 +
>>>  ofproto/ofproto-provider.h |   9 ++
>>>  ofproto/ofproto.c  |  12 +++
>>>  ofproto/ofproto.h  |   8 ++
>>>  8 files changed, 288 insertions(+)
>>>  create mode 100644 ofproto/ofproto-dpif-lsample.c
>>>  create mode 100644 ofproto/ofproto-dpif-lsample.h
>>>
>>> diff --git a/ofproto/automake.mk b/ofproto/automake.mk
>>> index 7c08b563b..fd39bf561 100644
>>> --- a/ofproto/automake.mk
>>> +++ b/ofproto/automake.mk
>>> @@ -34,6 +34,8 @@ ofproto_libofproto_la_SOURCES = \
>>> ofproto/ofproto-dpif-mirror.h \
>>> ofproto/ofproto-dpif-monitor.c \
>>> ofproto/ofproto-dpif-monitor.h \
>>> +   ofproto/ofproto-dpif-lsample.c \
>>> +   ofproto/ofproto-dpif-lsample.h \
>>
>> Guess these need to move before the dpif-m* files.
>>
>
> Ack
>
>>> ofproto/ofproto-dpif-rid.c \
>>> ofproto/ofproto-dpif-rid.h \
>>> ofproto/ofproto-dpif-sflow.c \
>>> diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
>>> new file mode 100644
>>> index 0..7bdafac19
>>> --- /dev/null
>>> +++ b/ofproto/ofproto-dpif-lsample.c
>>> @@ -0,0 +1,185 @@
>>> +/*
>>> + * Copyright (c) 2024 Red Hat, Inc.
>>> + *
>>> + * Licensed under the Apache License, Version 2.0 (the "License");
>>> + * you may not use this file except in compliance with the License.
>>> + * You may obtain a copy of the License at:
>>> + *
>>> + * http://www.apache.org/licenses/LICENSE-2.0
>>> + *
>>> + * Unless required by applicable law or agreed to in writing, software
>>> + * distributed under the License is distributed on an "AS IS" BASIS,
>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>>> + * See the License for the specific language governing permissions and
>>> + * limitations under the License.
>>> + */
>>> +
>>> +#include 
>>> +#include "ofproto-dpif-lsample.h"
>>> +
>>> +#include "cmap.h"
>>> +#include "hash.h"
>>> +#include "ofproto.h"
>>> +#include "openvswitch/thread.h"
>>> +
>>> +/* Dpif local sampling.
>>> + *
>>> + * Thread safety: dpif_lsample allows lockless concurrent reads of local
>>> + * sampling exporters as long as the following restrictions are met:
>>> + *   1) While the last reference is being dropped, i.e: a thread is calling
>>> + *  "dpif_lsample_unref" on the last reference, other threads cannot 
>>> call
>>> + *  "dpif_lsample_ref".
>>> + *   2) Threads do not quiese while holding references to internal
>>> + *  lsample_exporter objects.
>>> + */
>>> +
>>> +struct dpif_lsample {
>>> +struct cmap exporters;  /* Contains lsample_exporter_node 
>>> instances
>>> +   indexed by collector_set_id. */
>>> +struct ovs_mutex mutex; /* Protects concurrent 
>>> insertion/deletion
>>> + * of exporters. */
>>> +
>>> +struct ovs_refcount ref_cnt;/* Controls references to this 
>>> instance. */
>>> +};
>>> +
>>> +struct lsample_exporter {
>>> +struct ofproto_lsample_options options;
>>> +};
>>> +
>>> +struct lsample_exporter_node {
>>> +struct cmap_node node;  /* In dpif_lsample->exporters. */
>>> +struct lsample_exporter exporter;
>>> +};
>>> +
>>> +static void
>>> +dpif_lsample_delete_exporter(struct dpif_lsample *lsample,
>>> + struct lsample_exporter_node *node)
>>> +{
>>> +ovs_mutex_lock(&lsample->mutex);
>>> +cmap_remove(&lsample->exporters, &node->node,
>>> +hash_int(node->exporter.options.collector_set_id, 0));
>>> +ovs_mutex_unlock(&lsample->mutex);
>>> +
>>> +ovsrcu_postpone(free, node);
>>> +}
>>> +
>>> +/* Adds an exporter with the provided options which are copied. */
>>> +static struct lsample_exporter_node *
>>> +dpif_lsample_add_exporter(struct dpif_lsample *lsample,
>>> +  const struct ofproto_lsample_options *options)
>>> +{
>>> +struct lsample_exporter_node *node;
>>
>> New line between definitions and code.
>>
>
> Ack.
>
>>> +node = xzalloc(sizeof *node);
>>> +node->exporter.options = *options;
>>> +
>>> +ovs_mutex_lock(&lsample->mutex);
>>> +cmap_insert(&lsample->exporters, &node->node,
>>> +hash_int(options->collector_set_id, 0));
>>> +ovs_mutex_unlock(&lsample->mutex);
>>> +
>>> +return node;
>>> +}
>

Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action

[Adrián Moreno]

On Thu, Jun 27, 2024 at 09:30:08AM GMT, Aaron Conole wrote:
> Ilya Maximets  writes:
>
> > On 6/27/24 12:15, Adrián Moreno wrote:
> >> On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote:
> >>>
> >>>
> >>> On 27 Jun 2024, at 11:23, Ilya Maximets wrote:
> >>>
>  On 6/27/24 11:14, Eelco Chaudron wrote:
> >
> >
> > On 27 Jun 2024, at 10:36, Ilya Maximets wrote:
> >
> >> On 6/27/24 09:52, Adrián Moreno wrote:
> >>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote:
> 
> 
>  On 26 Jun 2024, at 22:34, Adrián Moreno wrote:
> 
> > On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
> >>
> >>
> >> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
> >>
> >>> Add support for a new action: emit_sample.
> >>>
> >>> This action accepts a u32 group id and a variable-length cookie 
> >>> and uses
> >>> the psample multicast group to make the packet available for
> >>> observability.
> >>>
> >>> The maximum length of the user-defined cookie is set to 16, same 
> >>> as
> >>> tc_cookie, to discourage using cookies that will not be 
> >>> offloadable.
> >>
> >> I’ll add the same comment as I had in the user space part, and that
> >> is that I feel from an OVS perspective this action should be called
> >> emit_local() instead of emit_sample() to make it Datapath 
> >> independent.
> >> Or quoting the earlier comment:
> >>
> >>
> >> “I’ll start the discussion again on the naming. The name 
> >> "emit_sample()"
> >> does not seem appropriate. This function's primary role is to copy 
> >> the
> >> packet and send it to a local collector, which varies depending on 
> >> the
> >> datapath. For the kernel datapath, this collector is psample, 
> >> while for
> >> userspace, it will likely be some kind of probe. This action is 
> >> distinct
> >> from the sample() action by design; it is a standalone action that 
> >> can
> >> be combined with others.
> >>
> >> Furthermore, the action itself does not involve taking a sample; it
> >> consistently pushes the packet to the local collector. Therefore, I
> >> suggest renaming "emit_sample()" to "emit_local()". This same goes 
> >> for
> >> all the derivative ATTR naming.”
> >>
> >
> > This is a blurry semantic area.
> > IMO, "sample" is the act of extracting (potentially a piece of)
> > someting, in this case, a packet. It is common to only take some 
> > packets
> > as samples, so this action usually comes with some kind of "rate", 
> > but
> > even if the rate is 1, it's still sampling in this context.
> >
> > OTOH, OVS kernel design tries to be super-modular and define small
> > combinable actions, so the rate or probability generation is done 
> > with
> > another action which is (IMHO unfortunately) named "sample".
> >
> > With that interpretation of the term it would actually make more 
> > sense
> > to rename "sample" to something like "random" (of course I'm not
> > suggestion we do it). "sample" without any nested action that 
> > actually
> > sends the packet somewhere is not sampling, it's just doing 
> > something or
> > not based on a probability. Where as "emit_sample" is sampling even 
> > if
> > it's not nested inside a "sample".
> 
>  You're assuming we are extracting a packet for sampling, but this 
>  function
>  can be used for various other purposes. For instance, it could 
>  handle the
>  packet outside of the OVS pipeline through an eBPF program (so we 
>  are not
>  taking a sample, but continue packet processing outside of the OVS
>  pipeline). Calling it emit_sampling() in such cases could be very
>  confusing.
> >>
> >> We can't change the implementation of the action once it is part of 
> >> uAPI.
> >> We have to document where users can find these packets and we can't 
> >> just
> >> change the destination later.
> >
> > I'm not suggesting we change the uAPI implementation, but we could use 
> > the
> > emit_xxx() action with an eBPF probe on the action to perform other 
> > tasks.
> > This is just an example.
> 
>  Yeah, but as Adrian said below, you could do that with any action and
>  this doesn't change the semantics of the action itself.
> >>>
> >>> Well this was just an example, what if we have some other need for getting
> >>> a packet to userspace through emit_local() other than sampling? The
> 

Re: [ovs-dev] [PATCH net-next v3 6/7] selftests: net: Use the provided dpctl rather than the vswitchd for tests.

[Aaron Conole]

Simon Horman  writes:

> On Tue, Jun 25, 2024 at 01:22:44PM -0400, Aaron Conole wrote:
>> The current pmtu test infrastucture requires an installed copy of the
>> ovs-vswitchd userspace.  This means that any automated or constrained
>> environments may not have the requisite tools to run the tests.  However,
>> the pmtu tests don't require any special classifier processing.  Indeed
>> they are only using the vswitchd in the most basic mode - as a NORMAL
>> switch.
>> 
>> However, the ovs-dpctl kernel utility can now program all the needed basic
>> flows to allow traffic to traverse the tunnels and provide support for at
>> least testing some basic pmtu scenarios.  More complicated flow pipelines
>> can be added to the internal ovs test infrastructure, but that is work for
>> the future.  For now, enable the most common cases - wide mega flows with
>> no other prerequisites.
>> 
>> Enhance the pmtu testing to try testing using the internal utility, first.
>> As a fallback, if the internal utility isn't running, then try with the
>> ovs-vswitchd userspace tools.
>> 
>> Additionally, make sure that when the pyroute2 package is not available
>> the ovs-dpctl utility will error out to properly signal an error has
>> occurred and skip using the internal utility.
>
> Hi Aaron,
>
> I don't feel strongly about this, but it does feel like the
> change to ovs-dpctl.py could live in a separate patch.

I can do it if others feel like it should be a separate change.  I
debated it as a separate patch, but I felt that it wasn't really a bug
fix, more like a behavior change that would be associated with this pmtu
script.  I didn't (at the time, and still don't) see a reason to
separately backport them, but it could also be considered as a separate
orthogonal change, and I'm okay with it being a different patch.  Like
you, I don't feel strongly either way.

If I do separate it, I will also add your Reviewed and Tested tags to
that patch.

>> Reviewed-by: Stefano Brivio 
>> Signed-off-by: Aaron Conole 
>
> The above not withstanding,
>
>
> Reviewed-by: Simon Horman 
>
> I have tested pmtu.sh with this change on Fedora 40 both
> with python3-pyroute2 installed, which uses ovs-dpctl,
> and without, which uses ovs-vswitchd userspace tools.
>
> Tested-by: Simon Horman 

Thanks!

> ...

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

Re: [ovs-dev] [PATCH 1/4] .gitignore: add clangd configuration file

[Aaron Conole]

Grigorii Nazarov  writes:

> On Thursday, June 20, 2024 9:52:07 PM GMT+3 Aaron Conole wrote:
>> Why is this file existing?  Maybe it would be better to generate the
>> compile_commands.json in automake?  Or generate it via the makefile?
> I'm not checking in the file. It's local configuration file, used by clangd, 
> which is clang-based language server used by IDEs like Visual Studio Code. It 
> shouldn't be generated by project files and strictly speaking is not part of 
> a 
> project in the first place. I found some other local files being mentioned in 
> .gitignore, and thus considered adding this one. In general it's a common 
> practice to gitignore files like that. I can work it out differently
> if it's not
> to be here.

My understanding is that the compile_flags.txt is editor / clangd
generated when there is no compile_commands.json file.  My suggestion is
to add the relevant code to the makefile to generate the
compile_commands.json file, and then add that to the gitignore as well.

The reason is that after reading quickly on it, I see that the
compile_commands.json file supposedly can accommodate per-file flags,
where as compile_flags.txt cannot.  It seems that having this additional
support would be quite useful, and I don't think it would be too
difficult to add?  At least from documentation here:

https://github.com/Sarcasm/notes/blob/master/dev/compilation-database.rst#clang

Do you think this would be okay for you to do?

>> >  .gitignore | 1 +
>> >  1 file changed, 1 insertion(+)
>> > 
>> > diff --git a/.gitignore b/.gitignore
>> > index 26ed8d3d0..3c7250159 100644
>> > --- a/.gitignore
>> > +++ b/.gitignore
>> > @@ -79,3 +79,4 @@ testsuite.tmp.orig
>> > 
>> >  /Documentation/_build
>> >  /.venv
>> >  /cxx-check
>> > 
>> > +/compile_flags.txt

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

Re: [ovs-dev] [RFC v2 1/9] odp-util: Add support OVS_ACTION_ATTR_EMIT_SAMPLE.

[Eelco Chaudron]

On 27 Jun 2024, at 12:45, Adrián Moreno wrote:

> On Wed, Jun 26, 2024 at 02:08:27PM GMT, Eelco Chaudron wrote:
>> On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
>>
>>> Add support for parsing and formatting the new action.
>>>
>>> Also, flag OVS_ACTION_ATTR_SAMPLE as requiring datapath assistance if it
>>> contains a nested OVS_ACTION_ATTR_EMIT_SAMPLE. The reason is that the
>>> sampling rate form the parent "sample" is made available to the nested
>>> "emit_sample" by the kernel.
>>
>> Hi Adrian,
>>
>> Thanks for this series! This email kicks off my review of the series,
>> see comments below and in the other patches.
>>
>> Cheers,
>>
>> Eelco
>>
>>> Signed-off-by: Adrian Moreno 
>>> ---
>>>  include/linux/openvswitch.h  |  25 +
>>>  lib/dpif-netdev.c|   1 +
>>>  lib/dpif.c   |   1 +
>>>  lib/odp-execute.c|  25 -
>>>  lib/odp-util.c   | 103 +++
>>>  lib/odp-util.h   |   3 +
>>>  ofproto/ofproto-dpif-ipfix.c |   1 +
>>>  ofproto/ofproto-dpif-sflow.c |   1 +
>>>  python/ovs/flow/odp.py   |   8 +++
>>>  tests/odp.at |  16 ++
>>>  10 files changed, 183 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>>> index d9fb991ef..b4e0647bd 100644
>>> --- a/include/linux/openvswitch.h
>>> +++ b/include/linux/openvswitch.h
>>> @@ -992,6 +992,30 @@ struct check_pkt_len_arg {
>>>  };
>>>  #endif
>>>
>>> +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
>>> +/**
>>> + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
>>> + * action.
>>> + *
>>> + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
>>> + * sample.
>>> + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that 
>>> contains
>>> + * user-defined metadata. The maximum length is 16 bytes.
>>> + *
>>> + * Sends the packet to the psample multicast group with the specified 
>>> group and
>>> + * cookie. It is possible to combine this action with the
>>> + * %OVS_ACTION_ATTR_TRUNC to limit the size of the packet being emitted.
>>
>> I'll start the discussion again on the naming. The name "emit_sample()"
>> does not seem appropriate. This function's primary role is to copy the
>> packet and send it to a local collector, which varies depending on the
>> datapath. For the kernel datapath, this collector is psample, while for
>> userspace, it will likely be some kind of probe. This action is distinct
>> from the sample() action by design; it is a standalone action that can
>> be combined with others.
>>
>> Furthermore, the action itself does not involve taking a sample; it
>> consistently pushes the packet to the local collector. Therefore, I
>> suggest renaming "emit_sample()" to "emit_local()". This same goes for
>> all the derivative ATTR naming.
>>
>
> Let's discuss this in the kernel ml.
>
>>> + */
>>> +enum ovs_emit_sample_attr {
>>> +   OVS_EMIT_SAMPLE_ATTR_UNPSEC,
>>> +   OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */
>>> +   OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
>>
>> Fix comment alignment? Maybe also change the order to be alphabetical?
>>
>
> What do you mean by comment alignment?

Well you are using tabs to index which might result in it looking like this:

+   OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */
+   OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */

(Most) of the other comments use spaces after the definition.

>>> +   __OVS_EMIT_SAMPLE_ATTR_MAX
>>> +};
>>> +
>>> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
>>> +
>>> +
>>>  /**
>>>   * enum ovs_action_attr - Action types.
>>>   *
>>> @@ -1087,6 +,7 @@ enum ovs_action_attr {
>>> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
>>> OVS_ACTION_ATTR_DEC_TTL,  /* Nested OVS_DEC_TTL_ATTR_*. */
>>> OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>>> +   OVS_ACTION_ATTR_EMIT_SAMPLE,  /* Nested OVS_EMIT_SAMPLE_ATTR_*. */
>>>
>>>  #ifndef __KERNEL__
>>> OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>>> index c7f9e1490..c1171890c 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -9519,6 +9519,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch 
>>> *packets_,
>>>  case OVS_ACTION_ATTR_DROP:
>>>  case OVS_ACTION_ATTR_ADD_MPLS:
>>>  case OVS_ACTION_ATTR_DEC_TTL:
>>> +case OVS_ACTION_ATTR_EMIT_SAMPLE:
>>>  case __OVS_ACTION_ATTR_MAX:
>>>  OVS_NOT_REACHED();
>>>  }
>>> diff --git a/lib/dpif.c b/lib/dpif.c
>>> index 23eb18495..489d6a095 100644
>>> --- a/lib/dpif.c
>>> +++ b/lib/dpif.c
>>> @@ -1192,6 +1192,7 @@ dpif_execute_helper_cb(void *aux_, struct 
>>> dp_packet_batch *packets_,
>>>  case OVS_ACTION_ATTR_TUNNEL_PUSH:
>>>  case OVS_ACTION_ATTR_TUNNEL_POP:
>>>  case OVS_ACTION_ATTR_USERSPACE:
>>> +   

Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action

[Eelco Chaudron]

On 27 Jun 2024, at 12:52, Ilya Maximets wrote:

> On 6/27/24 12:15, Adrián Moreno wrote:
>> On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote:
>>>
>>>
>>> On 27 Jun 2024, at 11:23, Ilya Maximets wrote:
>>>
 On 6/27/24 11:14, Eelco Chaudron wrote:
>
>
> On 27 Jun 2024, at 10:36, Ilya Maximets wrote:
>
>> On 6/27/24 09:52, Adrián Moreno wrote:
>>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote:


 On 26 Jun 2024, at 22:34, Adrián Moreno wrote:

> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
>>
>>
>> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
>>
>>> Add support for a new action: emit_sample.
>>>
>>> This action accepts a u32 group id and a variable-length cookie and 
>>> uses
>>> the psample multicast group to make the packet available for
>>> observability.
>>>
>>> The maximum length of the user-defined cookie is set to 16, same as
>>> tc_cookie, to discourage using cookies that will not be offloadable.
>>
>> I’ll add the same comment as I had in the user space part, and that
>> is that I feel from an OVS perspective this action should be called
>> emit_local() instead of emit_sample() to make it Datapath 
>> independent.
>> Or quoting the earlier comment:
>>
>>
>> “I’ll start the discussion again on the naming. The name 
>> "emit_sample()"
>> does not seem appropriate. This function's primary role is to copy 
>> the
>> packet and send it to a local collector, which varies depending on 
>> the
>> datapath. For the kernel datapath, this collector is psample, while 
>> for
>> userspace, it will likely be some kind of probe. This action is 
>> distinct
>> from the sample() action by design; it is a standalone action that 
>> can
>> be combined with others.
>>
>> Furthermore, the action itself does not involve taking a sample; it
>> consistently pushes the packet to the local collector. Therefore, I
>> suggest renaming "emit_sample()" to "emit_local()". This same goes 
>> for
>> all the derivative ATTR naming.”
>>
>
> This is a blurry semantic area.
> IMO, "sample" is the act of extracting (potentially a piece of)
> someting, in this case, a packet. It is common to only take some 
> packets
> as samples, so this action usually comes with some kind of "rate", but
> even if the rate is 1, it's still sampling in this context.
>
> OTOH, OVS kernel design tries to be super-modular and define small
> combinable actions, so the rate or probability generation is done with
> another action which is (IMHO unfortunately) named "sample".
>
> With that interpretation of the term it would actually make more sense
> to rename "sample" to something like "random" (of course I'm not
> suggestion we do it). "sample" without any nested action that actually
> sends the packet somewhere is not sampling, it's just doing something 
> or
> not based on a probability. Where as "emit_sample" is sampling even if
> it's not nested inside a "sample".

 You're assuming we are extracting a packet for sampling, but this 
 function
 can be used for various other purposes. For instance, it could handle 
 the
 packet outside of the OVS pipeline through an eBPF program (so we are 
 not
 taking a sample, but continue packet processing outside of the OVS
 pipeline). Calling it emit_sampling() in such cases could be very
 confusing.
>>
>> We can't change the implementation of the action once it is part of uAPI.
>> We have to document where users can find these packets and we can't just
>> change the destination later.
>
> I'm not suggesting we change the uAPI implementation, but we could use the
> emit_xxx() action with an eBPF probe on the action to perform other tasks.
> This is just an example.

 Yeah, but as Adrian said below, you could do that with any action and
 this doesn't change the semantics of the action itself.
>>>
>>> Well this was just an example, what if we have some other need for getting
>>> a packet to userspace through emit_local() other than sampling? The
>>> emit_sample() action naming in this case makes no sense.
>>>
>>> Well, I guess that would be clearly abusing the action. You could say
>>> that of anything really. You could hook into skb_consume and continue
>>> processing the skb but that doesn't change the intended behavior of the
>>> drop action.
>>>
>>> The intended behavior of the action is

Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action

[Aaron Conole]

Ilya Maximets  writes:

> On 6/27/24 12:15, Adrián Moreno wrote:
>> On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote:
>>>
>>>
>>> On 27 Jun 2024, at 11:23, Ilya Maximets wrote:
>>>
 On 6/27/24 11:14, Eelco Chaudron wrote:
>
>
> On 27 Jun 2024, at 10:36, Ilya Maximets wrote:
>
>> On 6/27/24 09:52, Adrián Moreno wrote:
>>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote:


 On 26 Jun 2024, at 22:34, Adrián Moreno wrote:

> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
>>
>>
>> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
>>
>>> Add support for a new action: emit_sample.
>>>
>>> This action accepts a u32 group id and a variable-length cookie and 
>>> uses
>>> the psample multicast group to make the packet available for
>>> observability.
>>>
>>> The maximum length of the user-defined cookie is set to 16, same as
>>> tc_cookie, to discourage using cookies that will not be offloadable.
>>
>> I’ll add the same comment as I had in the user space part, and that
>> is that I feel from an OVS perspective this action should be called
>> emit_local() instead of emit_sample() to make it Datapath 
>> independent.
>> Or quoting the earlier comment:
>>
>>
>> “I’ll start the discussion again on the naming. The name 
>> "emit_sample()"
>> does not seem appropriate. This function's primary role is to copy 
>> the
>> packet and send it to a local collector, which varies depending on 
>> the
>> datapath. For the kernel datapath, this collector is psample, while 
>> for
>> userspace, it will likely be some kind of probe. This action is 
>> distinct
>> from the sample() action by design; it is a standalone action that 
>> can
>> be combined with others.
>>
>> Furthermore, the action itself does not involve taking a sample; it
>> consistently pushes the packet to the local collector. Therefore, I
>> suggest renaming "emit_sample()" to "emit_local()". This same goes 
>> for
>> all the derivative ATTR naming.”
>>
>
> This is a blurry semantic area.
> IMO, "sample" is the act of extracting (potentially a piece of)
> someting, in this case, a packet. It is common to only take some 
> packets
> as samples, so this action usually comes with some kind of "rate", but
> even if the rate is 1, it's still sampling in this context.
>
> OTOH, OVS kernel design tries to be super-modular and define small
> combinable actions, so the rate or probability generation is done with
> another action which is (IMHO unfortunately) named "sample".
>
> With that interpretation of the term it would actually make more sense
> to rename "sample" to something like "random" (of course I'm not
> suggestion we do it). "sample" without any nested action that actually
> sends the packet somewhere is not sampling, it's just doing something 
> or
> not based on a probability. Where as "emit_sample" is sampling even if
> it's not nested inside a "sample".

 You're assuming we are extracting a packet for sampling, but this 
 function
 can be used for various other purposes. For instance, it could handle 
 the
 packet outside of the OVS pipeline through an eBPF program (so we are 
 not
 taking a sample, but continue packet processing outside of the OVS
 pipeline). Calling it emit_sampling() in such cases could be very
 confusing.
>>
>> We can't change the implementation of the action once it is part of uAPI.
>> We have to document where users can find these packets and we can't just
>> change the destination later.
>
> I'm not suggesting we change the uAPI implementation, but we could use the
> emit_xxx() action with an eBPF probe on the action to perform other tasks.
> This is just an example.

 Yeah, but as Adrian said below, you could do that with any action and
 this doesn't change the semantics of the action itself.
>>>
>>> Well this was just an example, what if we have some other need for getting
>>> a packet to userspace through emit_local() other than sampling? The
>>> emit_sample() action naming in this case makes no sense.
>>>
>>> Well, I guess that would be clearly abusing the action. You could say
>>> that of anything really. You could hook into skb_consume and continue
>>> processing the skb but that doesn't change the intended behavior of the
>>> drop action.
>>>
>>> The intended behavior of the action is sampling, as is the inten

Re: [ovs-dev] [PATCH v2] netdev-tc-offloads: Don't offload header modification on ip fragments.

[Eelco Chaudron]

On 4 Jun 2024, at 13:52, Ilya Maximets wrote:

> On 6/4/24 13:42, Eelco Chaudron wrote:
>>
>>
>> On 1 Jun 2024, at 0:08, Ilya Maximets wrote:
>>
>>> On 5/7/24 15:52, Eelco Chaudron wrote:
 While offloading header modifications to TC, OVS is using {TCA_PEDIT} +
 {TCA_CSUM} combination as that it the only way to represent header
 rewrite.  However, {TCA_CSUM} is unable to calculate L4 checksums for
 IP fragments.

 Since TC already applies fragmentation bit masking, this patch simply
 needs to prevent these packets from being processed through TC.

 Signed-off-by: Eelco Chaudron 
 ---
 v2: - Fixed and added some comments.
 - Use ovs-pcap to compare packets.

 NOTE: This patch needs an AVX512 fix before it can be applied.
   Intel is working on this.
 ---
  lib/netdev-offload-tc.c | 32 ++
  lib/tc.c|  5 ++-
  tests/system-traffic.at | 93 +
  3 files changed, 129 insertions(+), 1 deletion(-)

 diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
 index 921d52317..bdd307933 100644
 --- a/lib/netdev-offload-tc.c
 +++ b/lib/netdev-offload-tc.c
 @@ -1488,6 +1488,30 @@ parse_put_flow_ct_action(struct tc_flower *flower,
  return 0;
  }

 +static bool
 +will_tc_add_l4_checksum(struct tc_flower *flower, int type)
 +{
 +/* This function returns true if the tc layer will add a l4 checksum 
 action
 + * for this set action.  Refer to the csum_update_flag() function for
 + * detailed logic.  Note that even the kernel only supports updating 
 TCP,
 + * UDP and ICMPv6. */
>>>
>>> This comment should be outside of the function, I think.  It's strange
>>> to have it here.  I see csum_update_flag() has a comment inside, but
>>> that's strange as well.  That function has other style issues as well,
>>> there is no need to copy them.
>>
>> ACK, will clean this up in the next rev.
>>
 +switch (type) {
 +case OVS_KEY_ATTR_IPV4:
 +case OVS_KEY_ATTR_IPV6:
 +case OVS_KEY_ATTR_TCP:
 +case OVS_KEY_ATTR_UDP:
 +switch (flower->key.ip_proto) {
 +case IPPROTO_TCP:
 +case IPPROTO_UDP:
 +case IPPROTO_ICMPV6:
 +case IPPROTO_UDPLITE:
 +return true;
 +}
 +break;
 +}
 +return false;
 +}
 +
  static int
  parse_put_flow_set_masked_action(struct tc_flower *flower,
   struct tc_action *action,
 @@ -1520,6 +1544,14 @@ parse_put_flow_set_masked_action(struct tc_flower 
 *flower,
  return EOPNOTSUPP;
  }

 +if (flower->key.flags & TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT
>>>
>>> We're using this field to make an offloading decision.  We must
>>> also set in the mask.  If for some reason we're not matching on
>>> the fragment bits, we may first receive a non-fragmented packet
>>> and offload it, then fragmented traffic may match and fail the
>>> checksumming.  So, we need to enable the bit in the mask.
>>
>> The dp always matches on the fragment bit for IPv4 packets, I did some tests 
>> with this.
>> So if we sent a non-fragment packet first the dp rule will match on fragment 
>> = 0. Or
>> did I miss something?
>
> It is true today, but nothing ensures that on the netdev-offload-tc level.
> Moreover, the netdev-offload-tc is written in a way that it expects the
> frag bits to potentially not be in the mask:
>  
> https://github.com/openvswitch/ovs/blob/1d681ffe3b208a0db4945b6389142ab18404a4d1/lib/netdev-offload-tc.c#L2432-L2448
> So, it is going to be internally inconsistent if we do not set the bit
> explicitly.
>
> And if someday we'll stop adding the frag bit, we'll never know if we
> forget to set it in netdev-offload-tc.  At the very least we'll need an
> assertion that it is set.  But having an assertion will still be internally
> inconsistent with the code linked above.  So, it may be better to just fix
> it instead anyway.

I finally got some time to look at the code, and I can add the below abort()
in case OVS decides to no longer mask out the frag bits by default.

However, I have no clear idea how to make this work if we ever stop adding
this frag mask. I don't think we can report back to request a wider mask or
track what exists and force a revalidate.

@@ -2447,6 +2479,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match 
*match,
 }

 mask->nw_frag = 0;
+} else {
+ovs_abort(0, "TC offload assumes nw_frag is always masked");
 }

 +&& will_tc_add_l4_checksum(flower, type)) {
 +VLOG_DBG_RL(&rl, "set action type %d not supported on fragments "
 +"due to checksum limitation", type);
 +ofpbuf_uninit(&set_buf);
 +return 

Re: [ovs-dev] [RFC v2 4/9] vswitchd: Add local sampling to vswitchd schema.

[Adrián Moreno]

On Wed, Jun 26, 2024 at 02:11:00PM GMT, Eelco Chaudron wrote:
> On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
>
> > Add as new column in the Flow_Sample_Collector_Set table named
> > "local_sample_group" which enables this feature.
> >
> > Signed-off-by: Adrian Moreno 
> > ---
> >  NEWS   |  4 ++
> >  vswitchd/bridge.c  | 78 +++---
> >  vswitchd/vswitch.ovsschema |  9 -
> >  vswitchd/vswitch.xml   | 39 +--
> >  4 files changed, 119 insertions(+), 11 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index b92cec532..1c05a7120 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -7,6 +7,10 @@ Post-v3.3.0
> > - The primary development branch has been renamed from 'master' to 
> > 'main'.
> >   The OVS tree remains hosted on GitHub.
> > https://github.com/openvswitch/ovs.git
> > +   - Local sampling is introduced. It reuses the OpenFlow sample action and
> > + allows samples to be emitted locally (instead of via IPFIX) in a
> > + datapath-specific manner via the new datapath action called 
> > "emit_sample".
> > + Linux kernel datapath is the first to support this feature.
> >
> >
> >  v3.3.0 - 16 Feb 2024
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 95a65fcdc..cd7dc6646 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -288,6 +288,7 @@ static void bridge_configure_mac_table(struct bridge *);
> >  static void bridge_configure_mcast_snooping(struct bridge *);
> >  static void bridge_configure_sflow(struct bridge *, int 
> > *sflow_bridge_number);
> >  static void bridge_configure_ipfix(struct bridge *);
> > +static void bridge_configure_lsample(struct bridge *);
> >  static void bridge_configure_spanning_tree(struct bridge *);
> >  static void bridge_configure_tables(struct bridge *);
> >  static void bridge_configure_dp_desc(struct bridge *);
> > @@ -989,6 +990,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch 
> > *ovs_cfg)
> >  bridge_configure_netflow(br);
> >  bridge_configure_sflow(br, &sflow_bridge_number);
> >  bridge_configure_ipfix(br);
> > +bridge_configure_lsample(br);
> >  bridge_configure_spanning_tree(br);
> >  bridge_configure_tables(br);
> >  bridge_configure_dp_desc(br);
> > @@ -1537,10 +1539,11 @@ ovsrec_ipfix_is_valid(const struct ovsrec_ipfix 
> > *ipfix)
> >  return ipfix && ipfix->n_targets > 0;
> >  }
> >
> > -/* Returns whether a Flow_Sample_Collector_Set row is valid. */
> > +/* Returns whether a Flow_Sample_Collector_Set row constains valid IPFIX
>
> constains -> 'contains a'
>

Ack.

> > + * configuration. */
> >  static bool
> > -ovsrec_fscs_is_valid(const struct ovsrec_flow_sample_collector_set *fscs,
> > - const struct bridge *br)
> > +ovsrec_fscs_is_valid_ipfix(const struct ovsrec_flow_sample_collector_set 
> > *fscs,
> > +   const struct bridge *br)
> >  {
> >  return ovsrec_ipfix_is_valid(fscs->ipfix) && fscs->bridge == br->cfg;
> >  }
> > @@ -1558,7 +1561,7 @@ bridge_configure_ipfix(struct bridge *br)
> >  const char *virtual_obs_id;
> >
> >  OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
> > -if (ovsrec_fscs_is_valid(fe_cfg, br)) {
> > +if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
> >  n_fe_opts++;
> >  }
> >  }
> > @@ -1621,7 +1624,7 @@ bridge_configure_ipfix(struct bridge *br)
> >  fe_opts = xcalloc(n_fe_opts, sizeof *fe_opts);
> >  opts = fe_opts;
> >  OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fe_cfg, idl) {
> > -if (ovsrec_fscs_is_valid(fe_cfg, br)) {
> > +if (ovsrec_fscs_is_valid_ipfix(fe_cfg, br)) {
> >  opts->collector_set_id = fe_cfg->id;
> >  sset_init(&opts->targets);
> >  sset_add_array(&opts->targets, fe_cfg->ipfix->targets,
> > @@ -1667,6 +1670,71 @@ bridge_configure_ipfix(struct bridge *br)
> >  }
> >  }
> >
> > +/* Returns whether a Flow_Sample_Collector_Set row contains valid local
> > + * sampling configuraiton. */
>
> ...row contains a valid local...
>++
>
> configuraiton -> configuration

Ack.

>
> > +static bool
> > +ovsrec_fscs_is_valid_local(const struct ovsrec_flow_sample_collector_set 
> > *fscs,
> > +   const struct bridge *br)
> > +{
> > +return fscs->local_sample_group && fscs->n_local_sample_group == 1 &&
> > +   fscs->bridge == br->cfg;
> > +}
> > +
> > +/* Set local sample configuration on 'br'. */
> > +static void
> > +bridge_configure_lsample(struct bridge *br)
> > +{
> > +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > +const struct ovsrec_flow_sample_collector_set *fscs;
> > +struct ofproto_lsample_options *opts_array, *opts;
> > +size_t n_opts = 0;
> > +int ret;
> > +
> > +/* Iterate the Flow_Sample_Collector_Set table twice.
> > + * First

Re: [ovs-dev] [RFC v2 3/9] ofproto: Add ofproto-dpif-lsample.

[Adrián Moreno]

On Wed, Jun 26, 2024 at 02:10:27PM GMT, Eelco Chaudron wrote:
> On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
>
> > Add a new resource in ofproto-dpif and the corresponding API in
> > ofproto_provider.h to represent and local sampling configuration.
> >
> > Signed-off-by: Adrian Moreno 
>
> See some comments below.
>
> Cheers,
>
> Eelco
>
> > ---
> >  ofproto/automake.mk|   2 +
> >  ofproto/ofproto-dpif-lsample.c | 185 +
> >  ofproto/ofproto-dpif-lsample.h |  34 ++
> >  ofproto/ofproto-dpif.c |  37 +++
> >  ofproto/ofproto-dpif.h |   1 +
> >  ofproto/ofproto-provider.h |   9 ++
> >  ofproto/ofproto.c  |  12 +++
> >  ofproto/ofproto.h  |   8 ++
> >  8 files changed, 288 insertions(+)
> >  create mode 100644 ofproto/ofproto-dpif-lsample.c
> >  create mode 100644 ofproto/ofproto-dpif-lsample.h
> >
> > diff --git a/ofproto/automake.mk b/ofproto/automake.mk
> > index 7c08b563b..fd39bf561 100644
> > --- a/ofproto/automake.mk
> > +++ b/ofproto/automake.mk
> > @@ -34,6 +34,8 @@ ofproto_libofproto_la_SOURCES = \
> > ofproto/ofproto-dpif-mirror.h \
> > ofproto/ofproto-dpif-monitor.c \
> > ofproto/ofproto-dpif-monitor.h \
> > +   ofproto/ofproto-dpif-lsample.c \
> > +   ofproto/ofproto-dpif-lsample.h \
>
> Guess these need to move before the dpif-m* files.
>

Ack

> > ofproto/ofproto-dpif-rid.c \
> > ofproto/ofproto-dpif-rid.h \
> > ofproto/ofproto-dpif-sflow.c \
> > diff --git a/ofproto/ofproto-dpif-lsample.c b/ofproto/ofproto-dpif-lsample.c
> > new file mode 100644
> > index 0..7bdafac19
> > --- /dev/null
> > +++ b/ofproto/ofproto-dpif-lsample.c
> > @@ -0,0 +1,185 @@
> > +/*
> > + * Copyright (c) 2024 Red Hat, Inc.
> > + *
> > + * Licensed under the Apache License, Version 2.0 (the "License");
> > + * you may not use this file except in compliance with the License.
> > + * You may obtain a copy of the License at:
> > + *
> > + * http://www.apache.org/licenses/LICENSE-2.0
> > + *
> > + * Unless required by applicable law or agreed to in writing, software
> > + * distributed under the License is distributed on an "AS IS" BASIS,
> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> > + * See the License for the specific language governing permissions and
> > + * limitations under the License.
> > + */
> > +
> > +#include 
> > +#include "ofproto-dpif-lsample.h"
> > +
> > +#include "cmap.h"
> > +#include "hash.h"
> > +#include "ofproto.h"
> > +#include "openvswitch/thread.h"
> > +
> > +/* Dpif local sampling.
> > + *
> > + * Thread safety: dpif_lsample allows lockless concurrent reads of local
> > + * sampling exporters as long as the following restrictions are met:
> > + *   1) While the last reference is being dropped, i.e: a thread is calling
> > + *  "dpif_lsample_unref" on the last reference, other threads cannot 
> > call
> > + *  "dpif_lsample_ref".
> > + *   2) Threads do not quiese while holding references to internal
> > + *  lsample_exporter objects.
> > + */
> > +
> > +struct dpif_lsample {
> > +struct cmap exporters;  /* Contains lsample_exporter_node 
> > instances
> > +   indexed by collector_set_id. */
> > +struct ovs_mutex mutex; /* Protects concurrent 
> > insertion/deletion
> > + * of exporters. */
> > +
> > +struct ovs_refcount ref_cnt;/* Controls references to this 
> > instance. */
> > +};
> > +
> > +struct lsample_exporter {
> > +struct ofproto_lsample_options options;
> > +};
> > +
> > +struct lsample_exporter_node {
> > +struct cmap_node node;  /* In dpif_lsample->exporters. */
> > +struct lsample_exporter exporter;
> > +};
> > +
> > +static void
> > +dpif_lsample_delete_exporter(struct dpif_lsample *lsample,
> > + struct lsample_exporter_node *node)
> > +{
> > +ovs_mutex_lock(&lsample->mutex);
> > +cmap_remove(&lsample->exporters, &node->node,
> > +hash_int(node->exporter.options.collector_set_id, 0));
> > +ovs_mutex_unlock(&lsample->mutex);
> > +
> > +ovsrcu_postpone(free, node);
> > +}
> > +
> > +/* Adds an exporter with the provided options which are copied. */
> > +static struct lsample_exporter_node *
> > +dpif_lsample_add_exporter(struct dpif_lsample *lsample,
> > +  const struct ofproto_lsample_options *options)
> > +{
> > +struct lsample_exporter_node *node;
>
> New line between definitions and code.
>

Ack.

> > +node = xzalloc(sizeof *node);
> > +node->exporter.options = *options;
> > +
> > +ovs_mutex_lock(&lsample->mutex);
> > +cmap_insert(&lsample->exporters, &node->node,
> > +hash_int(options->collector_set_id, 0));
> > +ovs_mutex_unlock(&lsample->mutex);
> > +
> > +return node;
> > +}
> > +
> > +static struct lsample_exporter_node *
> > +dpif_lsample_find_expo

Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action

[Ilya Maximets]

On 6/27/24 12:15, Adrián Moreno wrote:
> On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote:
>>
>>
>> On 27 Jun 2024, at 11:23, Ilya Maximets wrote:
>>
>>> On 6/27/24 11:14, Eelco Chaudron wrote:


 On 27 Jun 2024, at 10:36, Ilya Maximets wrote:

> On 6/27/24 09:52, Adrián Moreno wrote:
>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote:
>>>
>>>
>>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote:
>>>
 On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
>
>
> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
>
>> Add support for a new action: emit_sample.
>>
>> This action accepts a u32 group id and a variable-length cookie and 
>> uses
>> the psample multicast group to make the packet available for
>> observability.
>>
>> The maximum length of the user-defined cookie is set to 16, same as
>> tc_cookie, to discourage using cookies that will not be offloadable.
>
> I’ll add the same comment as I had in the user space part, and that
> is that I feel from an OVS perspective this action should be called
> emit_local() instead of emit_sample() to make it Datapath independent.
> Or quoting the earlier comment:
>
>
> “I’ll start the discussion again on the naming. The name 
> "emit_sample()"
> does not seem appropriate. This function's primary role is to copy the
> packet and send it to a local collector, which varies depending on the
> datapath. For the kernel datapath, this collector is psample, while 
> for
> userspace, it will likely be some kind of probe. This action is 
> distinct
> from the sample() action by design; it is a standalone action that can
> be combined with others.
>
> Furthermore, the action itself does not involve taking a sample; it
> consistently pushes the packet to the local collector. Therefore, I
> suggest renaming "emit_sample()" to "emit_local()". This same goes for
> all the derivative ATTR naming.”
>

 This is a blurry semantic area.
 IMO, "sample" is the act of extracting (potentially a piece of)
 someting, in this case, a packet. It is common to only take some 
 packets
 as samples, so this action usually comes with some kind of "rate", but
 even if the rate is 1, it's still sampling in this context.

 OTOH, OVS kernel design tries to be super-modular and define small
 combinable actions, so the rate or probability generation is done with
 another action which is (IMHO unfortunately) named "sample".

 With that interpretation of the term it would actually make more sense
 to rename "sample" to something like "random" (of course I'm not
 suggestion we do it). "sample" without any nested action that actually
 sends the packet somewhere is not sampling, it's just doing something 
 or
 not based on a probability. Where as "emit_sample" is sampling even if
 it's not nested inside a "sample".
>>>
>>> You're assuming we are extracting a packet for sampling, but this 
>>> function
>>> can be used for various other purposes. For instance, it could handle 
>>> the
>>> packet outside of the OVS pipeline through an eBPF program (so we are 
>>> not
>>> taking a sample, but continue packet processing outside of the OVS
>>> pipeline). Calling it emit_sampling() in such cases could be very
>>> confusing.
>
> We can't change the implementation of the action once it is part of uAPI.
> We have to document where users can find these packets and we can't just
> change the destination later.

 I'm not suggesting we change the uAPI implementation, but we could use the
 emit_xxx() action with an eBPF probe on the action to perform other tasks.
 This is just an example.
>>>
>>> Yeah, but as Adrian said below, you could do that with any action and
>>> this doesn't change the semantics of the action itself.
>>
>> Well this was just an example, what if we have some other need for getting
>> a packet to userspace through emit_local() other than sampling? The
>> emit_sample() action naming in this case makes no sense.
>>
>> Well, I guess that would be clearly abusing the action. You could say
>> that of anything really. You could hook into skb_consume and continue
>> processing the skb but that doesn't change the intended behavior of the
>> drop action.
>>
>> The intended behavior of the action is sampling, as is the intended
>> behavior of "psample".
>
> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions",
> that is it takes some packets from the whole packet stream and ex

Re: [ovs-dev] [RFC v2 1/9] odp-util: Add support OVS_ACTION_ATTR_EMIT_SAMPLE.

[Adrián Moreno]

On Wed, Jun 26, 2024 at 02:08:27PM GMT, Eelco Chaudron wrote:
> On 5 Jun 2024, at 22:23, Adrian Moreno wrote:
>
> > Add support for parsing and formatting the new action.
> >
> > Also, flag OVS_ACTION_ATTR_SAMPLE as requiring datapath assistance if it
> > contains a nested OVS_ACTION_ATTR_EMIT_SAMPLE. The reason is that the
> > sampling rate form the parent "sample" is made available to the nested
> > "emit_sample" by the kernel.
>
> Hi Adrian,
>
> Thanks for this series! This email kicks off my review of the series,
> see comments below and in the other patches.
>
> Cheers,
>
> Eelco
>
> > Signed-off-by: Adrian Moreno 
> > ---
> >  include/linux/openvswitch.h  |  25 +
> >  lib/dpif-netdev.c|   1 +
> >  lib/dpif.c   |   1 +
> >  lib/odp-execute.c|  25 -
> >  lib/odp-util.c   | 103 +++
> >  lib/odp-util.h   |   3 +
> >  ofproto/ofproto-dpif-ipfix.c |   1 +
> >  ofproto/ofproto-dpif-sflow.c |   1 +
> >  python/ovs/flow/odp.py   |   8 +++
> >  tests/odp.at |  16 ++
> >  10 files changed, 183 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> > index d9fb991ef..b4e0647bd 100644
> > --- a/include/linux/openvswitch.h
> > +++ b/include/linux/openvswitch.h
> > @@ -992,6 +992,30 @@ struct check_pkt_len_arg {
> >  };
> >  #endif
> >
> > +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
> > +/**
> > + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
> > + * action.
> > + *
> > + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
> > + * sample.
> > + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that 
> > contains
> > + * user-defined metadata. The maximum length is 16 bytes.
> > + *
> > + * Sends the packet to the psample multicast group with the specified 
> > group and
> > + * cookie. It is possible to combine this action with the
> > + * %OVS_ACTION_ATTR_TRUNC to limit the size of the packet being emitted.
>
> I'll start the discussion again on the naming. The name "emit_sample()"
> does not seem appropriate. This function's primary role is to copy the
> packet and send it to a local collector, which varies depending on the
> datapath. For the kernel datapath, this collector is psample, while for
> userspace, it will likely be some kind of probe. This action is distinct
> from the sample() action by design; it is a standalone action that can
> be combined with others.
>
> Furthermore, the action itself does not involve taking a sample; it
> consistently pushes the packet to the local collector. Therefore, I
> suggest renaming "emit_sample()" to "emit_local()". This same goes for
> all the derivative ATTR naming.
>

Let's discuss this in the kernel ml.

> > + */
> > +enum ovs_emit_sample_attr {
> > +   OVS_EMIT_SAMPLE_ATTR_UNPSEC,
> > +   OVS_EMIT_SAMPLE_ATTR_GROUP, /* u32 number. */
> > +   OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
>
> Fix comment alignment? Maybe also change the order to be alphabetical?
>

What do you mean by comment alignment?


> > +   __OVS_EMIT_SAMPLE_ATTR_MAX
> > +};
> > +
> > +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
> > +
> > +
> >  /**
> >   * enum ovs_action_attr - Action types.
> >   *
> > @@ -1087,6 +,7 @@ enum ovs_action_attr {
> > OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> > OVS_ACTION_ATTR_DEC_TTL,  /* Nested OVS_DEC_TTL_ATTR_*. */
> > OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
> > +   OVS_ACTION_ATTR_EMIT_SAMPLE,  /* Nested OVS_EMIT_SAMPLE_ATTR_*. */
> >
> >  #ifndef __KERNEL__
> > OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index c7f9e1490..c1171890c 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -9519,6 +9519,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch 
> > *packets_,
> >  case OVS_ACTION_ATTR_DROP:
> >  case OVS_ACTION_ATTR_ADD_MPLS:
> >  case OVS_ACTION_ATTR_DEC_TTL:
> > +case OVS_ACTION_ATTR_EMIT_SAMPLE:
> >  case __OVS_ACTION_ATTR_MAX:
> >  OVS_NOT_REACHED();
> >  }
> > diff --git a/lib/dpif.c b/lib/dpif.c
> > index 23eb18495..489d6a095 100644
> > --- a/lib/dpif.c
> > +++ b/lib/dpif.c
> > @@ -1192,6 +1192,7 @@ dpif_execute_helper_cb(void *aux_, struct 
> > dp_packet_batch *packets_,
> >  case OVS_ACTION_ATTR_TUNNEL_PUSH:
> >  case OVS_ACTION_ATTR_TUNNEL_POP:
> >  case OVS_ACTION_ATTR_USERSPACE:
> > +case OVS_ACTION_ATTR_EMIT_SAMPLE:
> >  case OVS_ACTION_ATTR_RECIRC: {
> >  struct dpif_execute execute;
> >  struct ofpbuf execute_actions;
> > diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> > index 081e4d432..967abfd0a 100644
> > --- a/lib/odp-execute.c
> > +++ b/lib/odp-execute.c
> > @@ -818,13 +818,13 @@ requires_datapath_assistan

Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action

[Adrián Moreno]

On Thu, Jun 27, 2024 at 11:31:41AM GMT, Eelco Chaudron wrote:
>
>
> On 27 Jun 2024, at 11:23, Ilya Maximets wrote:
>
> > On 6/27/24 11:14, Eelco Chaudron wrote:
> >>
> >>
> >> On 27 Jun 2024, at 10:36, Ilya Maximets wrote:
> >>
> >>> On 6/27/24 09:52, Adrián Moreno wrote:
>  On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote:
> >
> >
> > On 26 Jun 2024, at 22:34, Adrián Moreno wrote:
> >
> >> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
> >>>
> >>>
> >>> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
> >>>
>  Add support for a new action: emit_sample.
> 
>  This action accepts a u32 group id and a variable-length cookie and 
>  uses
>  the psample multicast group to make the packet available for
>  observability.
> 
>  The maximum length of the user-defined cookie is set to 16, same as
>  tc_cookie, to discourage using cookies that will not be offloadable.
> >>>
> >>> I’ll add the same comment as I had in the user space part, and that
> >>> is that I feel from an OVS perspective this action should be called
> >>> emit_local() instead of emit_sample() to make it Datapath independent.
> >>> Or quoting the earlier comment:
> >>>
> >>>
> >>> “I’ll start the discussion again on the naming. The name 
> >>> "emit_sample()"
> >>> does not seem appropriate. This function's primary role is to copy the
> >>> packet and send it to a local collector, which varies depending on the
> >>> datapath. For the kernel datapath, this collector is psample, while 
> >>> for
> >>> userspace, it will likely be some kind of probe. This action is 
> >>> distinct
> >>> from the sample() action by design; it is a standalone action that can
> >>> be combined with others.
> >>>
> >>> Furthermore, the action itself does not involve taking a sample; it
> >>> consistently pushes the packet to the local collector. Therefore, I
> >>> suggest renaming "emit_sample()" to "emit_local()". This same goes for
> >>> all the derivative ATTR naming.”
> >>>
> >>
> >> This is a blurry semantic area.
> >> IMO, "sample" is the act of extracting (potentially a piece of)
> >> someting, in this case, a packet. It is common to only take some 
> >> packets
> >> as samples, so this action usually comes with some kind of "rate", but
> >> even if the rate is 1, it's still sampling in this context.
> >>
> >> OTOH, OVS kernel design tries to be super-modular and define small
> >> combinable actions, so the rate or probability generation is done with
> >> another action which is (IMHO unfortunately) named "sample".
> >>
> >> With that interpretation of the term it would actually make more sense
> >> to rename "sample" to something like "random" (of course I'm not
> >> suggestion we do it). "sample" without any nested action that actually
> >> sends the packet somewhere is not sampling, it's just doing something 
> >> or
> >> not based on a probability. Where as "emit_sample" is sampling even if
> >> it's not nested inside a "sample".
> >
> > You're assuming we are extracting a packet for sampling, but this 
> > function
> > can be used for various other purposes. For instance, it could handle 
> > the
> > packet outside of the OVS pipeline through an eBPF program (so we are 
> > not
> > taking a sample, but continue packet processing outside of the OVS
> > pipeline). Calling it emit_sampling() in such cases could be very
> > confusing.
> >>>
> >>> We can't change the implementation of the action once it is part of uAPI.
> >>> We have to document where users can find these packets and we can't just
> >>> change the destination later.
> >>
> >> I'm not suggesting we change the uAPI implementation, but we could use the
> >> emit_xxx() action with an eBPF probe on the action to perform other tasks.
> >> This is just an example.
> >
> > Yeah, but as Adrian said below, you could do that with any action and
> > this doesn't change the semantics of the action itself.
>
> Well this was just an example, what if we have some other need for getting
> a packet to userspace through emit_local() other than sampling? The
> emit_sample() action naming in this case makes no sense.
>
>  Well, I guess that would be clearly abusing the action. You could say
>  that of anything really. You could hook into skb_consume and continue
>  processing the skb but that doesn't change the intended behavior of the
>  drop action.
> 
>  The intended behavior of the action is sampling, as is the intended
>  behavior of "psample".
> >>>
> >>> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions",
> >>> that is it takes some packets from the whole packet stream and executes
> >>> actions of them.  Without tying this 

Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action

[Eelco Chaudron]

On 27 Jun 2024, at 11:23, Ilya Maximets wrote:

> On 6/27/24 11:14, Eelco Chaudron wrote:
>>
>>
>> On 27 Jun 2024, at 10:36, Ilya Maximets wrote:
>>
>>> On 6/27/24 09:52, Adrián Moreno wrote:
 On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote:
>
>
> On 26 Jun 2024, at 22:34, Adrián Moreno wrote:
>
>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
>>>
>>>
>>> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
>>>
 Add support for a new action: emit_sample.

 This action accepts a u32 group id and a variable-length cookie and 
 uses
 the psample multicast group to make the packet available for
 observability.

 The maximum length of the user-defined cookie is set to 16, same as
 tc_cookie, to discourage using cookies that will not be offloadable.
>>>
>>> I’ll add the same comment as I had in the user space part, and that
>>> is that I feel from an OVS perspective this action should be called
>>> emit_local() instead of emit_sample() to make it Datapath independent.
>>> Or quoting the earlier comment:
>>>
>>>
>>> “I’ll start the discussion again on the naming. The name "emit_sample()"
>>> does not seem appropriate. This function's primary role is to copy the
>>> packet and send it to a local collector, which varies depending on the
>>> datapath. For the kernel datapath, this collector is psample, while for
>>> userspace, it will likely be some kind of probe. This action is distinct
>>> from the sample() action by design; it is a standalone action that can
>>> be combined with others.
>>>
>>> Furthermore, the action itself does not involve taking a sample; it
>>> consistently pushes the packet to the local collector. Therefore, I
>>> suggest renaming "emit_sample()" to "emit_local()". This same goes for
>>> all the derivative ATTR naming.”
>>>
>>
>> This is a blurry semantic area.
>> IMO, "sample" is the act of extracting (potentially a piece of)
>> someting, in this case, a packet. It is common to only take some packets
>> as samples, so this action usually comes with some kind of "rate", but
>> even if the rate is 1, it's still sampling in this context.
>>
>> OTOH, OVS kernel design tries to be super-modular and define small
>> combinable actions, so the rate or probability generation is done with
>> another action which is (IMHO unfortunately) named "sample".
>>
>> With that interpretation of the term it would actually make more sense
>> to rename "sample" to something like "random" (of course I'm not
>> suggestion we do it). "sample" without any nested action that actually
>> sends the packet somewhere is not sampling, it's just doing something or
>> not based on a probability. Where as "emit_sample" is sampling even if
>> it's not nested inside a "sample".
>
> You're assuming we are extracting a packet for sampling, but this function
> can be used for various other purposes. For instance, it could handle the
> packet outside of the OVS pipeline through an eBPF program (so we are not
> taking a sample, but continue packet processing outside of the OVS
> pipeline). Calling it emit_sampling() in such cases could be very
> confusing.
>>>
>>> We can't change the implementation of the action once it is part of uAPI.
>>> We have to document where users can find these packets and we can't just
>>> change the destination later.
>>
>> I'm not suggesting we change the uAPI implementation, but we could use the
>> emit_xxx() action with an eBPF probe on the action to perform other tasks.
>> This is just an example.
>
> Yeah, but as Adrian said below, you could do that with any action and
> this doesn't change the semantics of the action itself.

Well this was just an example, what if we have some other need for getting
a packet to userspace through emit_local() other than sampling? The
emit_sample() action naming in this case makes no sense.

 Well, I guess that would be clearly abusing the action. You could say
 that of anything really. You could hook into skb_consume and continue
 processing the skb but that doesn't change the intended behavior of the
 drop action.

 The intended behavior of the action is sampling, as is the intended
 behavior of "psample".
>>>
>>> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions",
>>> that is it takes some packets from the whole packet stream and executes
>>> actions of them.  Without tying this to observability purposes the name
>>> makes sense as the first definition of the word is "to take a representative
>>> part or a single item from a larger whole or group".
>>>
>>> Now, our new action doesn't have this particular semantic in a way that
>>> it doesn't take a part of a whole packet stream but rather using the
>>> part al

Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action

[Ilya Maximets]

On 6/27/24 11:14, Eelco Chaudron wrote:
> 
> 
> On 27 Jun 2024, at 10:36, Ilya Maximets wrote:
> 
>> On 6/27/24 09:52, Adrián Moreno wrote:
>>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote:


 On 26 Jun 2024, at 22:34, Adrián Moreno wrote:

> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
>>
>>
>> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
>>
>>> Add support for a new action: emit_sample.
>>>
>>> This action accepts a u32 group id and a variable-length cookie and uses
>>> the psample multicast group to make the packet available for
>>> observability.
>>>
>>> The maximum length of the user-defined cookie is set to 16, same as
>>> tc_cookie, to discourage using cookies that will not be offloadable.
>>
>> I’ll add the same comment as I had in the user space part, and that
>> is that I feel from an OVS perspective this action should be called
>> emit_local() instead of emit_sample() to make it Datapath independent.
>> Or quoting the earlier comment:
>>
>>
>> “I’ll start the discussion again on the naming. The name "emit_sample()"
>> does not seem appropriate. This function's primary role is to copy the
>> packet and send it to a local collector, which varies depending on the
>> datapath. For the kernel datapath, this collector is psample, while for
>> userspace, it will likely be some kind of probe. This action is distinct
>> from the sample() action by design; it is a standalone action that can
>> be combined with others.
>>
>> Furthermore, the action itself does not involve taking a sample; it
>> consistently pushes the packet to the local collector. Therefore, I
>> suggest renaming "emit_sample()" to "emit_local()". This same goes for
>> all the derivative ATTR naming.”
>>
>
> This is a blurry semantic area.
> IMO, "sample" is the act of extracting (potentially a piece of)
> someting, in this case, a packet. It is common to only take some packets
> as samples, so this action usually comes with some kind of "rate", but
> even if the rate is 1, it's still sampling in this context.
>
> OTOH, OVS kernel design tries to be super-modular and define small
> combinable actions, so the rate or probability generation is done with
> another action which is (IMHO unfortunately) named "sample".
>
> With that interpretation of the term it would actually make more sense
> to rename "sample" to something like "random" (of course I'm not
> suggestion we do it). "sample" without any nested action that actually
> sends the packet somewhere is not sampling, it's just doing something or
> not based on a probability. Where as "emit_sample" is sampling even if
> it's not nested inside a "sample".

 You're assuming we are extracting a packet for sampling, but this function
 can be used for various other purposes. For instance, it could handle the
 packet outside of the OVS pipeline through an eBPF program (so we are not
 taking a sample, but continue packet processing outside of the OVS
 pipeline). Calling it emit_sampling() in such cases could be very
 confusing.
>>
>> We can't change the implementation of the action once it is part of uAPI.
>> We have to document where users can find these packets and we can't just
>> change the destination later.
> 
> I'm not suggesting we change the uAPI implementation, but we could use the
> emit_xxx() action with an eBPF probe on the action to perform other tasks.
> This is just an example.

Yeah, but as Adrian said below, you could do that with any action and
this doesn't change the semantics of the action itself.

> 
>>> Well, I guess that would be clearly abusing the action. You could say
>>> that of anything really. You could hook into skb_consume and continue
>>> processing the skb but that doesn't change the intended behavior of the
>>> drop action.
>>>
>>> The intended behavior of the action is sampling, as is the intended
>>> behavior of "psample".
>>
>> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions",
>> that is it takes some packets from the whole packet stream and executes
>> actions of them.  Without tying this to observability purposes the name
>> makes sense as the first definition of the word is "to take a representative
>> part or a single item from a larger whole or group".
>>
>> Now, our new action doesn't have this particular semantic in a way that
>> it doesn't take a part of a whole packet stream but rather using the
>> part already taken.  However, it is directly tied to the parent
>> OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent
>> action.  If there is no parent, then probability is assumed to be 100%,
>> but that's just a corner case.  The name of a psample module has the
>> same semantics in its name, it doesn't sample on it's own, but it is

Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action

[Eelco Chaudron]

On 27 Jun 2024, at 10:36, Ilya Maximets wrote:

> On 6/27/24 09:52, Adrián Moreno wrote:
>> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote:
>>>
>>>
>>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote:
>>>
 On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
>
>
> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
>
>> Add support for a new action: emit_sample.
>>
>> This action accepts a u32 group id and a variable-length cookie and uses
>> the psample multicast group to make the packet available for
>> observability.
>>
>> The maximum length of the user-defined cookie is set to 16, same as
>> tc_cookie, to discourage using cookies that will not be offloadable.
>
> I’ll add the same comment as I had in the user space part, and that
> is that I feel from an OVS perspective this action should be called
> emit_local() instead of emit_sample() to make it Datapath independent.
> Or quoting the earlier comment:
>
>
> “I’ll start the discussion again on the naming. The name "emit_sample()"
> does not seem appropriate. This function's primary role is to copy the
> packet and send it to a local collector, which varies depending on the
> datapath. For the kernel datapath, this collector is psample, while for
> userspace, it will likely be some kind of probe. This action is distinct
> from the sample() action by design; it is a standalone action that can
> be combined with others.
>
> Furthermore, the action itself does not involve taking a sample; it
> consistently pushes the packet to the local collector. Therefore, I
> suggest renaming "emit_sample()" to "emit_local()". This same goes for
> all the derivative ATTR naming.”
>

 This is a blurry semantic area.
 IMO, "sample" is the act of extracting (potentially a piece of)
 someting, in this case, a packet. It is common to only take some packets
 as samples, so this action usually comes with some kind of "rate", but
 even if the rate is 1, it's still sampling in this context.

 OTOH, OVS kernel design tries to be super-modular and define small
 combinable actions, so the rate or probability generation is done with
 another action which is (IMHO unfortunately) named "sample".

 With that interpretation of the term it would actually make more sense
 to rename "sample" to something like "random" (of course I'm not
 suggestion we do it). "sample" without any nested action that actually
 sends the packet somewhere is not sampling, it's just doing something or
 not based on a probability. Where as "emit_sample" is sampling even if
 it's not nested inside a "sample".
>>>
>>> You're assuming we are extracting a packet for sampling, but this function
>>> can be used for various other purposes. For instance, it could handle the
>>> packet outside of the OVS pipeline through an eBPF program (so we are not
>>> taking a sample, but continue packet processing outside of the OVS
>>> pipeline). Calling it emit_sampling() in such cases could be very
>>> confusing.
>
> We can't change the implementation of the action once it is part of uAPI.
> We have to document where users can find these packets and we can't just
> change the destination later.

I'm not suggesting we change the uAPI implementation, but we could use the
emit_xxx() action with an eBPF probe on the action to perform other tasks.
This is just an example.

>> Well, I guess that would be clearly abusing the action. You could say
>> that of anything really. You could hook into skb_consume and continue
>> processing the skb but that doesn't change the intended behavior of the
>> drop action.
>>
>> The intended behavior of the action is sampling, as is the intended
>> behavior of "psample".
>
> The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions",
> that is it takes some packets from the whole packet stream and executes
> actions of them.  Without tying this to observability purposes the name
> makes sense as the first definition of the word is "to take a representative
> part or a single item from a larger whole or group".
>
> Now, our new action doesn't have this particular semantic in a way that
> it doesn't take a part of a whole packet stream but rather using the
> part already taken.  However, it is directly tied to the parent
> OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent
> action.  If there is no parent, then probability is assumed to be 100%,
> but that's just a corner case.  The name of a psample module has the
> same semantics in its name, it doesn't sample on it's own, but it is
> assuming that sampling was performed as it relays the rate of it.
>
> And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and
> the psample module, the emit_sample() name makes sense to me.

This is the part I don't like. emit_sample() should be treated a

[ovs-dev] [PATCH ovn v2 1/2] ci: Save some DPDK compilation time.

[Ales Musil]

Add more options that will shave some compilation time off DPDK by
skipping unused parts of the code.

Suggested-by: David Marchand 
Signed-off-by: Ales Musil 
---
 utilities/containers/prepare.sh | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/utilities/containers/prepare.sh b/utilities/containers/prepare.sh
index 460d34a72..49bb2ac91 100755
--- a/utilities/containers/prepare.sh
+++ b/utilities/containers/prepare.sh
@@ -54,9 +54,9 @@ function build_dpdk()
   pushd dpdk-src
   fi
 
-  # Switching to 'default' machine to make the dpdk cache usable on
+  # Switching to 'generic' platform to make the dpdk cache usable on
   # different CPUs. We can't be sure that all CI machines are exactly same.
-  DPDK_OPTS="$DPDK_OPTS -Dmachine=default"
+  DPDK_OPTS="$DPDK_OPTS -Dplatform=generic"
 
   # Disable building DPDK unit tests. Not needed for OVS build or tests.
   DPDK_OPTS="$DPDK_OPTS -Dtests=false"
@@ -67,7 +67,9 @@ function build_dpdk()
 
   # OVS compilation and the "ovn-system-dpdk" unit tests (run in the CI)
   # only depend on virtio/tap drivers.
-  # We can disable all remaining drivers to save compilation time.
+  # We can disable all applications and remaining drivers to save
+  # compilation time.
+  DPDK_OPTS="$DPDK_OPTS -Ddisable_apps=*"
   DPDK_OPTS="$DPDK_OPTS -Denable_drivers=net/null,net/tap,net/virtio"
   # OVS depends on the vhost library (and its dependencies).
   # net/tap depends on the gso library.
@@ -76,7 +78,7 @@ function build_dpdk()
   # Install DPDK using prefix.
   DPDK_OPTS="$DPDK_OPTS --prefix=$DPDK_INSTALL_DIR"
 
-  meson $DPDK_OPTS build
+  meson setup $DPDK_OPTS build
   ninja -C build
   ninja -C build install
   popd
-- 
2.45.1

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

[ovs-dev] [PATCH ovn v2 2/2] tests: Skip memory error in DPDK tests.

[Ales Musil]

Add skip for memory error produced by DPDK on ARM with 2MB hugepages
which isn't harmful [0].

[0] http://mails.dpdk.org/archives/dev/2024-June/296764.html

Suggested-by: David Marchand 
Signed-off-by: Ales Musil 
---
 tests/system-dpdk-macros.at | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
index 440908af7..0d0a19130 100644
--- a/tests/system-dpdk-macros.at
+++ b/tests/system-dpdk-macros.at
@@ -68,6 +68,7 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START],
 m4_define([OVS_TRAFFIC_VSWITCHD_STOP],
   [OVS_VSWITCHD_STOP([dnl
 $1";/EAL: No \(available\|free\) .*hugepages reported/d
+/EAL: eal_memalloc_alloc_seg_bulk(): couldn't find suitable memseg_list/d
 /TELEMETRY: No legacy callbacks, legacy socket not created/d
 /dpif(revalidator.*)|WARN|netdev@ovs-netdev: failed to.*proto=2.*/d
 /dpif_netdev(revalidator.*)|ERR|internal error parsing flow key.*proto=2.*/d
-- 
2.45.1

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

Re: [ovs-dev] [PATCH ovn] ci: Save some compilation for DPDK and skip error on ARM.

[Ales Musil]

On Thu, Jun 27, 2024 at 10:23 AM David Marchand 
wrote:

> On Thu, Jun 27, 2024 at 10:16 AM Ales Musil  wrote:
> >
> > Add more options that will shave some compilation time of DPDK by
> > removing unused parts of the code. Also add skip for memory error
> > produced by DPDK on ARM with 2MB hugepages which isn't harmful [0].
> >
> > [0] http://mails.dpdk.org/archives/dev/2024-June/296764.html
> >
> > Suggested-by: David Marchand 
> > Signed-off-by: Ales Musil 
>
> I would split this in two patches (compilation changes in one patch,
> error log in another) but overall the changes lgtm.
> One nit below.
>

Yeah you are right.


>
> > ---
> >  tests/system-dpdk-macros.at |  3 ++-
> >  utilities/containers/prepare.sh | 10 ++
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
> > index 440908af7..39d19f366 100644
> > --- a/tests/system-dpdk-macros.at
> > +++ b/tests/system-dpdk-macros.at
> > @@ -77,7 +77,8 @@ $1";/EAL: No \(available\|free\) .*hugepages reported/d
> >  /netdev_linux.*obtaining netdev stats via vport failed/d
> >  /qdisc_create_multiq(): Could not add multiq qdisc (2): No such file or
> directory/d
> >  /tap_mp_req_on_rxtx(): Failed to send start req to secondary/d
> > -/tap_nl_dump_ext_ack(): Specified qdisc kind is unknown/d"])
> > +/tap_nl_dump_ext_ack(): Specified qdisc kind is unknown/d
> > +/EAL: eal_memalloc_alloc_seg_bulk(): couldn't find suitable
> memseg_list/d"])
>
> Nit: insert this "alphabetically", close to other EAL: ignored messages.
>


Ack, the others are not particularly in order, but I'll fix that in v2.


>
> > AT_CHECK([:; $2])
> >  ])
> >
>
>
> --
> David Marchand
>
>
Thanks,
Ales
-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA 

amu...@redhat.com

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

Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action

[Ilya Maximets]

On 6/27/24 09:52, Adrián Moreno wrote:
> On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote:
>>
>>
>> On 26 Jun 2024, at 22:34, Adrián Moreno wrote:
>>
>>> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:


 On 25 Jun 2024, at 22:51, Adrian Moreno wrote:

> Add support for a new action: emit_sample.
>
> This action accepts a u32 group id and a variable-length cookie and uses
> the psample multicast group to make the packet available for
> observability.
>
> The maximum length of the user-defined cookie is set to 16, same as
> tc_cookie, to discourage using cookies that will not be offloadable.

 I’ll add the same comment as I had in the user space part, and that
 is that I feel from an OVS perspective this action should be called
 emit_local() instead of emit_sample() to make it Datapath independent.
 Or quoting the earlier comment:


 “I’ll start the discussion again on the naming. The name "emit_sample()"
 does not seem appropriate. This function's primary role is to copy the
 packet and send it to a local collector, which varies depending on the
 datapath. For the kernel datapath, this collector is psample, while for
 userspace, it will likely be some kind of probe. This action is distinct
 from the sample() action by design; it is a standalone action that can
 be combined with others.

 Furthermore, the action itself does not involve taking a sample; it
 consistently pushes the packet to the local collector. Therefore, I
 suggest renaming "emit_sample()" to "emit_local()". This same goes for
 all the derivative ATTR naming.”

>>>
>>> This is a blurry semantic area.
>>> IMO, "sample" is the act of extracting (potentially a piece of)
>>> someting, in this case, a packet. It is common to only take some packets
>>> as samples, so this action usually comes with some kind of "rate", but
>>> even if the rate is 1, it's still sampling in this context.
>>>
>>> OTOH, OVS kernel design tries to be super-modular and define small
>>> combinable actions, so the rate or probability generation is done with
>>> another action which is (IMHO unfortunately) named "sample".
>>>
>>> With that interpretation of the term it would actually make more sense
>>> to rename "sample" to something like "random" (of course I'm not
>>> suggestion we do it). "sample" without any nested action that actually
>>> sends the packet somewhere is not sampling, it's just doing something or
>>> not based on a probability. Where as "emit_sample" is sampling even if
>>> it's not nested inside a "sample".
>>
>> You're assuming we are extracting a packet for sampling, but this function
>> can be used for various other purposes. For instance, it could handle the
>> packet outside of the OVS pipeline through an eBPF program (so we are not
>> taking a sample, but continue packet processing outside of the OVS
>> pipeline). Calling it emit_sampling() in such cases could be very
>> confusing.

We can't change the implementation of the action once it is part of uAPI.
We have to document where users can find these packets and we can't just
change the destination later.

>>
> 
> Well, I guess that would be clearly abusing the action. You could say
> that of anything really. You could hook into skb_consume and continue
> processing the skb but that doesn't change the intended behavior of the
> drop action.
> 
> The intended behavior of the action is sampling, as is the intended
> behavior of "psample".

The original OVS_ACTION_ATTR_SAMPLE "Probabilitically executes actions",
that is it takes some packets from the whole packet stream and executes
actions of them.  Without tying this to observability purposes the name
makes sense as the first definition of the word is "to take a representative
part or a single item from a larger whole or group".

Now, our new action doesn't have this particular semantic in a way that
it doesn't take a part of a whole packet stream but rather using the
part already taken.  However, it is directly tied to the parent
OVS_ACTION_ATTR_SAMPLE action, since it reports probability of that parent
action.  If there is no parent, then probability is assumed to be 100%,
but that's just a corner case.  The name of a psample module has the
same semantics in its name, it doesn't sample on it's own, but it is
assuming that sampling was performed as it relays the rate of it.

And since we're directly tied here with both OVS_ACTION_ATTR_SAMPLE and
the psample module, the emit_sample() name makes sense to me.

> 
>>> Having said that, I don't have a super strong favor for "emit_sample". I'm
>>> OK with "emit_local" or "emit_packet" or even just "emit".

The 'local' or 'packet' variants are not descriptive enough on what we're
trying to achieve and do not explain why the probability is attached to
the action, i.e. do not explain the link between this action and the
OVS_ACTION_ATTR_SAMPLE.

emit_Psa

Re: [ovs-dev] [PATCH ovn] ci: Save some compilation for DPDK and skip error on ARM.

[David Marchand]

On Thu, Jun 27, 2024 at 10:16 AM Ales Musil  wrote:
>
> Add more options that will shave some compilation time of DPDK by
> removing unused parts of the code. Also add skip for memory error
> produced by DPDK on ARM with 2MB hugepages which isn't harmful [0].
>
> [0] http://mails.dpdk.org/archives/dev/2024-June/296764.html
>
> Suggested-by: David Marchand 
> Signed-off-by: Ales Musil 

I would split this in two patches (compilation changes in one patch,
error log in another) but overall the changes lgtm.
One nit below.


> ---
>  tests/system-dpdk-macros.at |  3 ++-
>  utilities/containers/prepare.sh | 10 ++
>  2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
> index 440908af7..39d19f366 100644
> --- a/tests/system-dpdk-macros.at
> +++ b/tests/system-dpdk-macros.at
> @@ -77,7 +77,8 @@ $1";/EAL: No \(available\|free\) .*hugepages reported/d
>  /netdev_linux.*obtaining netdev stats via vport failed/d
>  /qdisc_create_multiq(): Could not add multiq qdisc (2): No such file or 
> directory/d
>  /tap_mp_req_on_rxtx(): Failed to send start req to secondary/d
> -/tap_nl_dump_ext_ack(): Specified qdisc kind is unknown/d"])
> +/tap_nl_dump_ext_ack(): Specified qdisc kind is unknown/d
> +/EAL: eal_memalloc_alloc_seg_bulk(): couldn't find suitable memseg_list/d"])

Nit: insert this "alphabetically", close to other EAL: ignored messages.

> AT_CHECK([:; $2])
>  ])
>


-- 
David Marchand

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

[ovs-dev] [PATCH ovn] ci: Save some compilation for DPDK and skip error on ARM.

[Ales Musil]

Add more options that will shave some compilation time of DPDK by
removing unused parts of the code. Also add skip for memory error
produced by DPDK on ARM with 2MB hugepages which isn't harmful [0].

[0] http://mails.dpdk.org/archives/dev/2024-June/296764.html

Suggested-by: David Marchand 
Signed-off-by: Ales Musil 
---
 tests/system-dpdk-macros.at |  3 ++-
 utilities/containers/prepare.sh | 10 ++
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/tests/system-dpdk-macros.at b/tests/system-dpdk-macros.at
index 440908af7..39d19f366 100644
--- a/tests/system-dpdk-macros.at
+++ b/tests/system-dpdk-macros.at
@@ -77,7 +77,8 @@ $1";/EAL: No \(available\|free\) .*hugepages reported/d
 /netdev_linux.*obtaining netdev stats via vport failed/d
 /qdisc_create_multiq(): Could not add multiq qdisc (2): No such file or 
directory/d
 /tap_mp_req_on_rxtx(): Failed to send start req to secondary/d
-/tap_nl_dump_ext_ack(): Specified qdisc kind is unknown/d"])
+/tap_nl_dump_ext_ack(): Specified qdisc kind is unknown/d
+/EAL: eal_memalloc_alloc_seg_bulk(): couldn't find suitable memseg_list/d"])
AT_CHECK([:; $2])
 ])
 
diff --git a/utilities/containers/prepare.sh b/utilities/containers/prepare.sh
index 460d34a72..49bb2ac91 100755
--- a/utilities/containers/prepare.sh
+++ b/utilities/containers/prepare.sh
@@ -54,9 +54,9 @@ function build_dpdk()
   pushd dpdk-src
   fi
 
-  # Switching to 'default' machine to make the dpdk cache usable on
+  # Switching to 'generic' platform to make the dpdk cache usable on
   # different CPUs. We can't be sure that all CI machines are exactly same.
-  DPDK_OPTS="$DPDK_OPTS -Dmachine=default"
+  DPDK_OPTS="$DPDK_OPTS -Dplatform=generic"
 
   # Disable building DPDK unit tests. Not needed for OVS build or tests.
   DPDK_OPTS="$DPDK_OPTS -Dtests=false"
@@ -67,7 +67,9 @@ function build_dpdk()
 
   # OVS compilation and the "ovn-system-dpdk" unit tests (run in the CI)
   # only depend on virtio/tap drivers.
-  # We can disable all remaining drivers to save compilation time.
+  # We can disable all applications and remaining drivers to save
+  # compilation time.
+  DPDK_OPTS="$DPDK_OPTS -Ddisable_apps=*"
   DPDK_OPTS="$DPDK_OPTS -Denable_drivers=net/null,net/tap,net/virtio"
   # OVS depends on the vhost library (and its dependencies).
   # net/tap depends on the gso library.
@@ -76,7 +78,7 @@ function build_dpdk()
   # Install DPDK using prefix.
   DPDK_OPTS="$DPDK_OPTS --prefix=$DPDK_INSTALL_DIR"
 
-  meson $DPDK_OPTS build
+  meson setup $DPDK_OPTS build
   ninja -C build
   ninja -C build install
   popd
-- 
2.45.1

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

Re: [ovs-dev] [PATCH net-next v5 10/10] selftests: openvswitch: add emit_sample test

[Adrián Moreno]

On Wed, Jun 26, 2024 at 01:15:21PM GMT, Ilya Maximets wrote:
> On 6/25/24 22:51, Adrian Moreno wrote:
> > Add a test to verify sampling packets via psample works.
> >
> > In order to do that, create a subcommand in ovs-dpctl.py to listen to
> > on the psample multicast group and print samples.
> >
> > Signed-off-by: Adrian Moreno 
> > ---
> >  .../selftests/net/openvswitch/openvswitch.sh  | 114 +-
> >  .../selftests/net/openvswitch/ovs-dpctl.py|  73 ++-
> >  2 files changed, 181 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh 
> > b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> > index 15bca0708717..aeb9bee772be 100755
> > --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh
> > +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh
> > @@ -20,7 +20,8 @@ tests="
> > nat_related_v4  ip4-nat-related: ICMP related 
> > matches work with SNAT
> > netlink_checks  ovsnl: validate netlink attrs 
> > and settings
> > upcall_interfaces   ovs: test the upcall interfaces
> > -   drop_reason drop: test drop reasons are 
> > emitted"
> > +   drop_reason drop: test drop reasons are 
> > emitted
> > +   emit_sample emit_sample: Sampling packets 
> > with psample"
>
> There is an extra space character right after emit_sample word.
> This makes './openvswitch.sh emit_sample' to not run the test,
> because 'emit_sample' != 'emit_sample '.
>

Wow, good catch! I'll get rid of that space.

Thanks.
Adrián

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

Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action

[Adrián Moreno]

On Thu, Jun 27, 2024 at 09:06:46AM GMT, Eelco Chaudron wrote:
>
>
> On 26 Jun 2024, at 22:34, Adrián Moreno wrote:
>
> > On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
> >>
> >>
> >> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
> >>
> >>> Add support for a new action: emit_sample.
> >>>
> >>> This action accepts a u32 group id and a variable-length cookie and uses
> >>> the psample multicast group to make the packet available for
> >>> observability.
> >>>
> >>> The maximum length of the user-defined cookie is set to 16, same as
> >>> tc_cookie, to discourage using cookies that will not be offloadable.
> >>
> >> I’ll add the same comment as I had in the user space part, and that
> >> is that I feel from an OVS perspective this action should be called
> >> emit_local() instead of emit_sample() to make it Datapath independent.
> >> Or quoting the earlier comment:
> >>
> >>
> >> “I’ll start the discussion again on the naming. The name "emit_sample()"
> >> does not seem appropriate. This function's primary role is to copy the
> >> packet and send it to a local collector, which varies depending on the
> >> datapath. For the kernel datapath, this collector is psample, while for
> >> userspace, it will likely be some kind of probe. This action is distinct
> >> from the sample() action by design; it is a standalone action that can
> >> be combined with others.
> >>
> >> Furthermore, the action itself does not involve taking a sample; it
> >> consistently pushes the packet to the local collector. Therefore, I
> >> suggest renaming "emit_sample()" to "emit_local()". This same goes for
> >> all the derivative ATTR naming.”
> >>
> >
> > This is a blurry semantic area.
> > IMO, "sample" is the act of extracting (potentially a piece of)
> > someting, in this case, a packet. It is common to only take some packets
> > as samples, so this action usually comes with some kind of "rate", but
> > even if the rate is 1, it's still sampling in this context.
> >
> > OTOH, OVS kernel design tries to be super-modular and define small
> > combinable actions, so the rate or probability generation is done with
> > another action which is (IMHO unfortunately) named "sample".
> >
> > With that interpretation of the term it would actually make more sense
> > to rename "sample" to something like "random" (of course I'm not
> > suggestion we do it). "sample" without any nested action that actually
> > sends the packet somewhere is not sampling, it's just doing something or
> > not based on a probability. Where as "emit_sample" is sampling even if
> > it's not nested inside a "sample".
>
> You're assuming we are extracting a packet for sampling, but this function
> can be used for various other purposes. For instance, it could handle the
> packet outside of the OVS pipeline through an eBPF program (so we are not
> taking a sample, but continue packet processing outside of the OVS
> pipeline). Calling it emit_sampling() in such cases could be very
> confusing.
>

Well, I guess that would be clearly abusing the action. You could say
that of anything really. You could hook into skb_consume and continue
processing the skb but that doesn't change the intended behavior of the
drop action.

The intended behavior of the action is sampling, as is the intended
behavior of "psample".

> > Having said that, I don't have a super strong favor for "emit_sample". I'm
> > OK with "emit_local" or "emit_packet" or even just "emit".
> > I don't think any term will fully satisfy everyone so I hope we can find
> > a reasonable compromise.
>
> My preference would be emit_local() as we hand it off to some local
> datapath entity.
>

I'm OK removing the controversial term. Let's see what others think.

> >>> Signed-off-by: Adrian Moreno 
> >>> ---
> >>>  Documentation/netlink/specs/ovs_flow.yaml | 17 +
> >>>  include/uapi/linux/openvswitch.h  | 28 ++
> >>>  net/openvswitch/Kconfig   |  1 +
> >>>  net/openvswitch/actions.c | 45 +++
> >>>  net/openvswitch/flow_netlink.c| 33 -
> >>>  5 files changed, 123 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/Documentation/netlink/specs/ovs_flow.yaml 
> >>> b/Documentation/netlink/specs/ovs_flow.yaml
> >>> index 4fdfc6b5cae9..a7ab5593a24f 100644
> >>> --- a/Documentation/netlink/specs/ovs_flow.yaml
> >>> +++ b/Documentation/netlink/specs/ovs_flow.yaml
> >>> @@ -727,6 +727,12 @@ attribute-sets:
> >>>  name: dec-ttl
> >>>  type: nest
> >>>  nested-attributes: dec-ttl-attrs
> >>> +  -
> >>> +name: emit-sample
> >>> +type: nest
> >>> +nested-attributes: emit-sample-attrs
> >>> +doc: |
> >>> +  Sends a packet sample to psample for external observation.
> >>>-
> >>>  name: tunnel-key-attrs
> >>>  enum-name: ovs-tunnel-key-attr
> >>> @@ -938,6 +944,17 @@ attribute-sets:
> >>>-
> >>>  name: gbp
> >>>  t

Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action

[Ilya Maximets]

On 6/26/24 22:07, Adrián Moreno wrote:
> On Wed, Jun 26, 2024 at 12:51:19PM GMT, Ilya Maximets wrote:
>> On 6/25/24 22:51, Adrian Moreno wrote:
>>> Add support for a new action: emit_sample.
>>>
>>> This action accepts a u32 group id and a variable-length cookie and uses
>>> the psample multicast group to make the packet available for
>>> observability.
>>>
>>> The maximum length of the user-defined cookie is set to 16, same as
>>> tc_cookie, to discourage using cookies that will not be offloadable.
>>>
>>> Signed-off-by: Adrian Moreno 
>>> ---
>>>  Documentation/netlink/specs/ovs_flow.yaml | 17 +
>>>  include/uapi/linux/openvswitch.h  | 28 ++
>>>  net/openvswitch/Kconfig   |  1 +
>>>  net/openvswitch/actions.c | 45 +++
>>>  net/openvswitch/flow_netlink.c| 33 -
>>>  5 files changed, 123 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/netlink/specs/ovs_flow.yaml 
>>> b/Documentation/netlink/specs/ovs_flow.yaml
>>> index 4fdfc6b5cae9..a7ab5593a24f 100644
>>> --- a/Documentation/netlink/specs/ovs_flow.yaml
>>> +++ b/Documentation/netlink/specs/ovs_flow.yaml
>>> @@ -727,6 +727,12 @@ attribute-sets:
>>>  name: dec-ttl
>>>  type: nest
>>>  nested-attributes: dec-ttl-attrs
>>> +  -
>>> +name: emit-sample
>>> +type: nest
>>> +nested-attributes: emit-sample-attrs
>>> +doc: |
>>> +  Sends a packet sample to psample for external observation.
>>>-
>>>  name: tunnel-key-attrs
>>>  enum-name: ovs-tunnel-key-attr
>>> @@ -938,6 +944,17 @@ attribute-sets:
>>>-
>>>  name: gbp
>>>  type: u32
>>> +  -
>>> +name: emit-sample-attrs
>>> +enum-name: ovs-emit-sample-attr
>>> +name-prefix: ovs-emit-sample-attr-
>>> +attributes:
>>> +  -
>>> +name: group
>>> +type: u32
>>> +  -
>>> +name: cookie
>>> +type: binary
>>>
>>>  operations:
>>>name-prefix: ovs-flow-cmd-
>>> diff --git a/include/uapi/linux/openvswitch.h 
>>> b/include/uapi/linux/openvswitch.h
>>> index efc82c318fa2..8cfa1b3f6b06 100644
>>> --- a/include/uapi/linux/openvswitch.h
>>> +++ b/include/uapi/linux/openvswitch.h
>>> @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
>>>  };
>>>  #endif
>>>
>>> +#define OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE 16
>>> +/**
>>> + * enum ovs_emit_sample_attr - Attributes for %OVS_ACTION_ATTR_EMIT_SAMPLE
>>> + * action.
>>> + *
>>> + * @OVS_EMIT_SAMPLE_ATTR_GROUP: 32-bit number to identify the source of the
>>> + * sample.
>>> + * @OVS_EMIT_SAMPLE_ATTR_COOKIE: A variable-length binary cookie that 
>>> contains
>>> + * user-defined metadata. The maximum length is 
>>> OVS_EMIT_SAMPLE_COOKIE_MAX_SIZE
>>> + * bytes.
>>> + *
>>> + * Sends the packet to the psample multicast group with the specified 
>>> group and
>>> + * cookie. It is possible to combine this action with the
>>> + * %OVS_ACTION_ATTR_TRUNC action to limit the size of the packet being 
>>> emitted.
>>> + */
>>> +enum ovs_emit_sample_attr {
>>> +   OVS_EMIT_SAMPLE_ATTR_GROUP = 1, /* u32 number. */
>>> +   OVS_EMIT_SAMPLE_ATTR_COOKIE,/* Optional, user specified cookie. */
>>> +
>>> +   /* private: */
>>> +   __OVS_EMIT_SAMPLE_ATTR_MAX
>>> +};
>>> +
>>> +#define OVS_EMIT_SAMPLE_ATTR_MAX (__OVS_EMIT_SAMPLE_ATTR_MAX - 1)
>>> +
>>>  /**
>>>   * enum ovs_action_attr - Action types.
>>>   *
>>> @@ -966,6 +991,8 @@ struct check_pkt_len_arg {
>>>   * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
>>>   * argument.
>>>   * @OVS_ACTION_ATTR_DROP: Explicit drop action.
>>> + * @OVS_ACTION_ATTR_EMIT_SAMPLE: Send a sample of the packet to external
>>> + * observers via psample.
>>>   *
>>>   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  
>>> Not all
>>>   * fields within a header are modifiable, e.g. the IPv4 protocol and 
>>> fragment
>>> @@ -1004,6 +1031,7 @@ enum ovs_action_attr {
>>> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
>>> OVS_ACTION_ATTR_DEC_TTL,  /* Nested OVS_DEC_TTL_ATTR_*. */
>>> OVS_ACTION_ATTR_DROP, /* u32 error code. */
>>> +   OVS_ACTION_ATTR_EMIT_SAMPLE,  /* Nested OVS_EMIT_SAMPLE_ATTR_*. */
>>>
>>> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
>>>* from userspace. */
>>> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
>>> index 29a7081858cd..2535f3f9f462 100644
>>> --- a/net/openvswitch/Kconfig
>>> +++ b/net/openvswitch/Kconfig
>>> @@ -10,6 +10,7 @@ config OPENVSWITCH
>>>(NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \
>>>  (!NF_NAT || NF_NAT) && \
>>>  (!NETFILTER_CONNCOUNT || 
>>> NETFILTER_CONNCOUNT)))
>>> +   depends on PSAMPLE || !PSAMPLE
>>> select LIBCRC32C
>>> select MPLS
>>> select NET_MPLS_GSO
>>> diff --git a/net/openv

Re: [ovs-dev] [PATCH net-next v5 02/10] net: sched: act_sample: add action cookie to sample

[Eelco Chaudron]

On 26 Jun 2024, at 22:06, Adrián Moreno wrote:

> On Wed, Jun 26, 2024 at 04:28:01PM GMT, Eelco Chaudron wrote:
>>
>>
>> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
>>
>>> If the action has a user_cookie, pass it along to the sample so it can
>>> be easily identified.
>>>
>>> Signed-off-by: Adrian Moreno 
>>> ---
>>>  net/sched/act_sample.c | 12 
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
>>> index a69b53d54039..2ceb4d141b71 100644
>>> --- a/net/sched/act_sample.c
>>> +++ b/net/sched/act_sample.c
>>> @@ -167,7 +167,9 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff 
>>> *skb,
>>>  {
>>> struct tcf_sample *s = to_sample(a);
>>> struct psample_group *psample_group;
>>> +   u8 cookie_data[TC_COOKIE_MAX_SIZE];
>>> struct psample_metadata md = {};
>>> +   struct tc_cookie *user_cookie;
>>> int retval;
>>>
>>> tcf_lastuse_update(&s->tcf_tm);
>>> @@ -189,6 +191,16 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff 
>>> *skb,
>>> if (skb_at_tc_ingress(skb) && tcf_sample_dev_ok_push(skb->dev))
>>> skb_push(skb, skb->mac_len);
>>>
>>> +   rcu_read_lock();
>>> +   user_cookie = rcu_dereference(a->user_cookie);
>>> +   if (user_cookie) {
>>> +   memcpy(cookie_data, user_cookie->data,
>>> +  user_cookie->len);
>>
>> Maybe I’m over paranoid, but can we assume user_cookie->len, will not be 
>> larger than TC_COOKIE_MAX_SIZE?
>> Or should we do something like min(user_cookie->len, sizeof(cookie_data))
>>
>
> I think it's good to be paranoid with this kind of things. I do,
> however, think it should be safe to use. The cookie is extracted from
> the netlink attribute directly and its length is verified with the
> nla_policy [1]. So nothing that comes into the kernel should be larger
> than TC_COOKIE_MAX_SIZE.

ACK, confirmed that [1] seems to be the only way to set the cookie. So this 
patch seems fine to me too.

Acked-by: Eelco Chaudron 

> I guess if there is some previous bug that allows for the size to get
> corrupted, then this might happen but doing those kind of checks in the
> fast path seems a bit excessive. For example, Ilya argued in v2 [2] that
> we should avoid zeroing "u8 cookie_data[TC_COOKIE_MAX_SIZE]" to safe the
> unneeded cycles.
>
> [1] 
> https://github.com/torvalds/linux/blob/55027e689933ba2e64f3d245fb1ff185b3e7fc81/net/sched/act_api.c#L1299
> [2] 
> https://patchwork.kernel.org/project/netdevbpf/patch/20240603185647.2310748-3-amore...@redhat.com/
>
> Thanks.
> Adrián
>
>>> +   md.user_cookie = cookie_data;
>>> +   md.user_cookie_len = user_cookie->len;
>>> +   }
>>> +   rcu_read_unlock();
>>> +
>>> md.trunc_size = s->truncate ? s->trunc_size : skb->len;
>>> psample_sample_packet(psample_group, skb, s->rate, &md);
>>>
>>> --
>>> 2.45.1
>>

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

Re: [ovs-dev] [PATCH net-next v5 05/10] net: openvswitch: add emit_sample action

[Eelco Chaudron]

On 26 Jun 2024, at 22:34, Adrián Moreno wrote:

> On Wed, Jun 26, 2024 at 04:28:17PM GMT, Eelco Chaudron wrote:
>>
>>
>> On 25 Jun 2024, at 22:51, Adrian Moreno wrote:
>>
>>> Add support for a new action: emit_sample.
>>>
>>> This action accepts a u32 group id and a variable-length cookie and uses
>>> the psample multicast group to make the packet available for
>>> observability.
>>>
>>> The maximum length of the user-defined cookie is set to 16, same as
>>> tc_cookie, to discourage using cookies that will not be offloadable.
>>
>> I’ll add the same comment as I had in the user space part, and that
>> is that I feel from an OVS perspective this action should be called
>> emit_local() instead of emit_sample() to make it Datapath independent.
>> Or quoting the earlier comment:
>>
>>
>> “I’ll start the discussion again on the naming. The name "emit_sample()"
>> does not seem appropriate. This function's primary role is to copy the
>> packet and send it to a local collector, which varies depending on the
>> datapath. For the kernel datapath, this collector is psample, while for
>> userspace, it will likely be some kind of probe. This action is distinct
>> from the sample() action by design; it is a standalone action that can
>> be combined with others.
>>
>> Furthermore, the action itself does not involve taking a sample; it
>> consistently pushes the packet to the local collector. Therefore, I
>> suggest renaming "emit_sample()" to "emit_local()". This same goes for
>> all the derivative ATTR naming.”
>>
>
> This is a blurry semantic area.
> IMO, "sample" is the act of extracting (potentially a piece of)
> someting, in this case, a packet. It is common to only take some packets
> as samples, so this action usually comes with some kind of "rate", but
> even if the rate is 1, it's still sampling in this context.
>
> OTOH, OVS kernel design tries to be super-modular and define small
> combinable actions, so the rate or probability generation is done with
> another action which is (IMHO unfortunately) named "sample".
>
> With that interpretation of the term it would actually make more sense
> to rename "sample" to something like "random" (of course I'm not
> suggestion we do it). "sample" without any nested action that actually
> sends the packet somewhere is not sampling, it's just doing something or
> not based on a probability. Where as "emit_sample" is sampling even if
> it's not nested inside a "sample".

You're assuming we are extracting a packet for sampling, but this function
can be used for various other purposes. For instance, it could handle the
packet outside of the OVS pipeline through an eBPF program (so we are not
taking a sample, but continue packet processing outside of the OVS
pipeline). Calling it emit_sampling() in such cases could be very
confusing.

> Having said that, I don't have a super strong favor for "emit_sample". I'm
> OK with "emit_local" or "emit_packet" or even just "emit".
> I don't think any term will fully satisfy everyone so I hope we can find
> a reasonable compromise.

My preference would be emit_local() as we hand it off to some local
datapath entity.

>>> Signed-off-by: Adrian Moreno 
>>> ---
>>>  Documentation/netlink/specs/ovs_flow.yaml | 17 +
>>>  include/uapi/linux/openvswitch.h  | 28 ++
>>>  net/openvswitch/Kconfig   |  1 +
>>>  net/openvswitch/actions.c | 45 +++
>>>  net/openvswitch/flow_netlink.c| 33 -
>>>  5 files changed, 123 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/netlink/specs/ovs_flow.yaml 
>>> b/Documentation/netlink/specs/ovs_flow.yaml
>>> index 4fdfc6b5cae9..a7ab5593a24f 100644
>>> --- a/Documentation/netlink/specs/ovs_flow.yaml
>>> +++ b/Documentation/netlink/specs/ovs_flow.yaml
>>> @@ -727,6 +727,12 @@ attribute-sets:
>>>  name: dec-ttl
>>>  type: nest
>>>  nested-attributes: dec-ttl-attrs
>>> +  -
>>> +name: emit-sample
>>> +type: nest
>>> +nested-attributes: emit-sample-attrs
>>> +doc: |
>>> +  Sends a packet sample to psample for external observation.
>>>-
>>>  name: tunnel-key-attrs
>>>  enum-name: ovs-tunnel-key-attr
>>> @@ -938,6 +944,17 @@ attribute-sets:
>>>-
>>>  name: gbp
>>>  type: u32
>>> +  -
>>> +name: emit-sample-attrs
>>> +enum-name: ovs-emit-sample-attr
>>> +name-prefix: ovs-emit-sample-attr-
>>> +attributes:
>>> +  -
>>> +name: group
>>> +type: u32
>>> +  -
>>> +name: cookie
>>> +type: binary
>>>
>>>  operations:
>>>name-prefix: ovs-flow-cmd-
>>> diff --git a/include/uapi/linux/openvswitch.h 
>>> b/include/uapi/linux/openvswitch.h
>>> index efc82c318fa2..8cfa1b3f6b06 100644
>>> --- a/include/uapi/linux/openvswitch.h
>>> +++ b/include/uapi/linux/openvswitch.h
>>> @@ -914,6 +914,31 @@ struct check_pkt_len_arg {
>>>  };
>>>  #endif
>>>
>>> +#define