On 5/10/24 12:06 PM, Eelco Chaudron wrote:
On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
Add a psample_group field to the Flow Sample Collector Set table and use
it to configure the psample ofproto layer.
See comments below,
Eelco
Signed-off-by: Adrian Moreno <[email protected]>
---
vswitchd/bridge.c | 54 ++++++++++++++++++++++++++++++++++----
vswitchd/vswitch.ovsschema | 7 ++++-
vswitchd/vswitch.xml | 32 +++++++++++++++++++---
3 files changed, 83 insertions(+), 10 deletions(-)
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 95a65fcdc..474eb1650 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_psample(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_psample(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
+ * 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,47 @@ bridge_configure_ipfix(struct bridge *br)
}
}
+/* Set psample configuration on 'br'. */
+static void
+bridge_configure_psample(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_psample_options *ps_options;
+ struct ovs_list options_list;
+ int ret;
+
+ ovs_list_init(&options_list);
+
+ OVSREC_FLOW_SAMPLE_COLLECTOR_SET_FOR_EACH(fscs, idl) {
+ if (!fscs->psample_group || fscs->n_psample_group != 1
+ || fscs->bridge != br->cfg)
+ continue;
+
+ ps_options = xzalloc(sizeof *ps_options);
As mentioned earlier, I think we should use an array rather than a list like
ofproto_ipfix_flow_exporter_options() this is more cache-friendly, and also
avoids small memory allocations.
Passing an dynamically sized array would still require a memory allocation
which, in most cases would just be a handful of these structs but I guess it's
still better. I just preferred the way a list simplifies the code (only 1
iteration through the table and one less argument in the function), but I don't
have a very strong opinion about it.
+ ps_options->collector_set_id = fscs->id;
+ ps_options->group_id = *fscs->psample_group;
+
+ ovs_list_insert(&options_list, &ps_options->list_node);
+ }
+
+ ret = ofproto_set_psample(br->ofproto, &options_list);
+
+ if (ret == ENOTSUP) {
+ if (!ovs_list_is_empty(&options_list)) {
Do we need this check? We already do this at ofproto_set_psample? My
preferences would be to remove it from ofproto_set_psample() as it looks odd
there.
We do, actually. The check in ofproto_set_psample() hides the error code if the
dpif class implementation does not support set_psample. This does the same error
"masking" when the dpif has the function implemented but the datapath does not
supported currently (e.g: old kernel).
But yeah, I agree we can remove the one in ofproto_set_psample and leave this
one as it'll cover both cases.
+ VLOG_WARN_RL(&rl, "bridge %s: ignoring psample configuration: "
+ "not supported by this datapath", br->name);
+ }
+ } else if (ret) {
+ VLOG_ERR_RL(&rl, "bridge %s: error configuring psample: %s",
+ br->name, ovs_strerror(ret));
+ }
+
+ LIST_FOR_EACH_POP(ps_options, list_node, &options_list) {
+ free(ps_options);
+ }
+}
+
static void
port_configure_stp(const struct ofproto *ofproto, struct port *port,
struct ofproto_port_stp_settings *port_s,
diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
index e2d5e2e85..a7ad9bcaa 100644
--- a/vswitchd/vswitch.ovsschema
+++ b/vswitchd/vswitch.ovsschema
@@ -1,6 +1,6 @@
{"name": "Open_vSwitch",
"version": "8.5.0",
- "cksum": "4040946650 27557",
+ "cksum": "1366215760 27764",
"tables": {
"Open_vSwitch": {
"columns": {
@@ -562,6 +562,11 @@
"type": {"key": {"type": "uuid",
"refTable": "IPFIX"},
"min": 0, "max": 1}},
+ "psample_group": {
+ "type": {"key": {"type": "integer",
+ "minInteger": 0,
+ "maxInteger": 4294967295},
+ "min": 0, "max": 1}},
"external_ids": {
"type": {"key": "string", "value": "string",
"min": 0, "max": "unlimited"}}},
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 8a1b607d7..ab6e6f479 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -7008,10 +7008,30 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
type=patch options:peer=p1 \
<table name="Flow_Sample_Collector_Set">
<p>
- A set of IPFIX collectors of packet samples generated by OpenFlow
- <code>sample</code> actions. This table is used only for IPFIX
- flow-based sampling, not for per-bridge sampling (see the <ref
- table="IPFIX"/> table for a description of the two forms).
+ A set of IPFIX or Psample collectors of packet samples generated by
We should avoid psample even here (system collector?), we can still mention
that for the kernel dp this gets translated to the psample_group.
+ OpenFlow <code>sample</code> actions.
+ </p>
+
+ <p>
+ If the column <code>ipfix</code> contains a reference to a
+ valid IPFIX entry, samples will be emitted via IPFIX. This mechanism
+ is known as flow-based IPFIX sampling, as opposed to bridge-based
+ sampling (see the <ref table="IPFIX"/> table for a description of the
+ two forms).
+ </p>
+
+ <p>
+ If the column <code>psample_group</code> contains an integer and the
+ running kernel supports it, samples will be sent to the psample group
+ specified by <code>psample_group</code>.
+
+ External processes can read the raw samples by joining the psample
netlink
+ multicast group.
+ </p>
+
+ <p>
+ Note both <code>psample_group</code> and <code>ipfix</code> can be set
+ simultaneously.
</p>
<column name="id">
@@ -7030,6 +7050,10 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch
options:peer=p1 \
record per sampled packet to.
</column>
+ <column name="psample_group">
+ Configuration of the psample group where the sample will be multicasted.
+ </column>
+
<group title="Common Columns">
The overall purpose of these columns is described under <code>Common
Columns</code> at the beginning of this document.
--
2.44.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
