On 7/18/23 21:38, Mike Pattrick wrote:
Currently a bridge mirror will collect all packets and tools like
ovs-tcpdump can apply additional filters after they have already been
duplicated by vswitchd. This can result in inefficient collection.

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

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

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

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


Looks really promising!

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

diff --git a/Documentation/ref/ovs-tcpdump.8.rst 
b/Documentation/ref/ovs-tcpdump.8.rst
index b9f8cdf6f..9163c8a5e 100644
--- a/Documentation/ref/ovs-tcpdump.8.rst
+++ b/Documentation/ref/ovs-tcpdump.8.rst
@@ -61,6 +61,10 @@ Options
If specified, mirror all ports (optional). +* ``--filter <flow>``
+
+  If specified, only mirror flows that match the provided filter.
+
  See Also
  ========
diff --git a/NEWS b/NEWS
index 7a852427e..26797ca56 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,7 @@
  Post-v3.2.0
  --------------------
+   - ovs-tcpdump:
+     * Added new --filter parameter to apply pre-selection on mirrored flows.

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

v3.2.0 - xx xxx xxxx
diff --git a/lib/flow.h b/lib/flow.h
index a9d026e1c..c2e67dfc5 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -939,6 +939,17 @@ flow_union_with_miniflow(struct flow *dst, const struct 
miniflow *src)
      flow_union_with_miniflow_subset(dst, src, src->map);
  }
+/* Perform a bitwise OR of minimask 'src' mask data with the equivalent
+ * fields in 'dst', storing the result in 'dst'. */
+static inline void
+flow_wildcards_union_with_minimask(struct flow_wildcards *dst,
+                                   const struct minimask *src)
+{
+    flow_union_with_miniflow_subset(&dst->masks,
+                                    &src->masks,
+                                    src->masks.map);
+}
+
  static inline bool is_ct_valid(const struct flow *flow,
                                 const struct flow_wildcards *mask,
                                 struct flow_wildcards *wc)
diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
index 343b75f0e..e4174a564 100644
--- a/ofproto/ofproto-dpif-mirror.c
+++ b/ofproto/ofproto-dpif-mirror.c
@@ -21,6 +21,7 @@
  #include "cmap.h"
  #include "hmapx.h"
  #include "ofproto.h"
+#include "ofproto-dpif-trace.h"
  #include "vlan-bitmap.h"
  #include "openvswitch/vlog.h"
@@ -57,6 +58,10 @@ struct mirror {
      struct hmapx srcs;          /* Contains "struct mbundle*"s. */
      struct hmapx dsts;          /* Contains "struct mbundle*"s. */
+ /* Filter criteria. */
+    struct miniflow *filter;
+    struct minimask *mask;
+
      /* This is accessed by handler threads assuming RCU protection (see
       * mirror_get()), but can be manipulated by mirror_set() without any
       * explicit synchronization. */
@@ -212,7 +217,9 @@ mirror_set(struct mbridge *mbridge, void *aux, const char 
*name,
             struct ofbundle **dsts, size_t n_dsts,
             unsigned long *src_vlans, struct ofbundle *out_bundle,
             uint16_t snaplen,
-           uint16_t out_vlan)
+           uint16_t out_vlan,
+           const char *filter,
+           const struct ofproto *ofproto)
  {
      struct mbundle *mbundle, *out;
      mirror_mask_t mirror_bit;
@@ -285,6 +292,33 @@ mirror_set(struct mbridge *mbridge, void *aux, const char 
*name,
      mirror->out_vlan = out_vlan;
      mirror->snaplen = snaplen;
+ if (mirror->filter) {
+        ovsrcu_postpone(free, mirror->filter);
+        ovsrcu_postpone(free, mirror->mask);
+        mirror->filter = NULL;
+        mirror->mask = NULL;
+    }
+    if (filter) {
+        struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map);
+        struct flow_wildcards wc;
+        struct flow flow;
+        char *err;
+
+        ofproto_append_ports_to_map(&map, ofproto->ports);
+        err = parse_ofp_exact_flow(&flow, &wc,
+                                   ofproto_get_tun_tab(ofproto),
+                                   filter, &map);
+        ofputil_port_map_destroy(&map);
+        if (err) {
+            VLOG_WARN("filter is invalid: %s", err);
+            free(err);
+            return EINVAL;
+        }


Not sure if this will clean the mirror correctly, it might leave it half-built (return value of ofproto_mirror_register() is not checked). Maybe it would be safer to parse the filter earlier during bridge.c's mirror_configure().

 +
+        mirror->mask = minimask_create(&wc);
+        mirror->filter = miniflow_create(&flow);
+    }
+
      /* Update mbundles. */
      mirror_bit = MIRROR_MASK_C(1) << mirror->idx;
      CMAP_FOR_EACH (mbundle, cmap_node, &mirror->mbridge->mbundles) {
@@ -340,6 +374,13 @@ mirror_destroy(struct mbridge *mbridge, void *aux)
          ovsrcu_postpone(free, vlans);
      }
+ if (mirror->filter) {
+        ovsrcu_postpone(free, mirror->filter);
+        ovsrcu_postpone(free, mirror->mask);
+        mirror->filter = NULL;
+        mirror->mask = NULL;
+    }
+
      mbridge->mirrors[mirror->idx] = NULL;
      /* mirror_get() might have just read the pointer, so we must postpone the
       * free. */
@@ -411,14 +452,17 @@ mirror_update_stats(struct mbridge *mbridge, 
mirror_mask_t mirrors,
   * in which a 1-bit indicates that the mirror includes a particular VLAN,
   * '*dup_mirrors' receives a bitmap of mirrors whose output duplicates mirror
   * 'index', '*out' receives the output ofbundle (if any), and '*out_vlan'
- * receives the output VLAN (if any).
+ * receives the output VLAN (if any). In cases where the mirror has a filter
+ * configured and the flow matches that filter, '*wc' is modified to include
+ * that filters mask.
   *
   * Everything returned here is assumed to be RCU protected.
   */
  bool
  mirror_get(struct mbridge *mbridge, int index, const unsigned long **vlans,
             mirror_mask_t *dup_mirrors, struct ofbundle **out,
-           int *snaplen, int *out_vlan)
+           int *snaplen, int *out_vlan, struct flow *flow,
+           struct flow_wildcards *wc)
  {
      struct mirror *mirror;
@@ -430,6 +474,16 @@ mirror_get(struct mbridge *mbridge, int index, const unsigned long **vlans,
      if (!mirror) {
          return false;
      }
+
+    if (wc && mirror->filter) {
+        if (miniflow_equal_flow_in_minimask(mirror->filter, flow,
+                                            mirror->mask)) {
+            flow_wildcards_union_with_minimask(wc, mirror->mask);
+        } else {
+            return false;
+        }
+    }
+

I don't think the flow filtering should be done here.
First, it changes the bevhavior of the function (which used to just lookup the mirror based on the index) and the meaning of it's return value. Actually, by doing the filter here we're probably not benefiting from the OVS_UNLIKELY macro that it's caller uses.

And secondly, it makes it more difficult to understand the relationship between flow filtering and vlan filtering (which, btw, should be documented. See below).

I think mirror_get() should just return the mirror information (filter and mask) and we should perform the filtering in mirror_packet() (after vlan filtering?).

WDYT?

      /* Assume 'mirror' is RCU protected, i.e., it will not be freed until this
       * thread quiesces. */
diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h
index eed63ec4a..6de742cbf 100644
--- a/ofproto/ofproto-dpif-mirror.h
+++ b/ofproto/ofproto-dpif-mirror.h
@@ -24,6 +24,9 @@ typedef uint32_t mirror_mask_t;
struct ofproto_dpif;
  struct ofbundle;
+struct ofproto;
+struct flow;
+struct flow_wildcards;
/* The following functions are used by handler threads without any locking,
   * assuming RCU protection. */
@@ -40,7 +43,8 @@ void mirror_update_stats(struct mbridge*, mirror_mask_t, 
uint64_t packets,
                           uint64_t bytes);
  bool mirror_get(struct mbridge *, int index, const unsigned long **vlans,
                  mirror_mask_t *dup_mirrors, struct ofbundle **out,
-                int *snaplen, int *out_vlan);
+                int *snaplen, int *out_vlan, struct flow *flow,
+                struct flow_wildcards *wc);
/* The remaining functions are assumed to be called by the main thread only. */ @@ -54,7 +58,8 @@ int mirror_set(struct mbridge *, void *aux, const char *name,
                 struct ofbundle **srcs, size_t n_srcs,
                 struct ofbundle **dsts, size_t n_dsts,
                 unsigned long *src_vlans, struct ofbundle *out_bundle,
-               uint16_t snaplen, uint16_t out_vlan);
+               uint16_t snaplen, uint16_t out_vlan, const char *filter,
+               const struct ofproto *ofproto);
  void mirror_destroy(struct mbridge *, void *aux);
  int mirror_get_stats(struct mbridge *, void *aux, uint64_t *packets,
                       uint64_t *bytes);
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 4928ea99c..4547caa31 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2246,8 +2246,10 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle 
*xbundle,
          /* Get the details of the mirror represented by the rightmost 1-bit. 
*/
          if (OVS_UNLIKELY(!mirror_get(xbridge->mbridge, raw_ctz(mirrors),
                                       &vlans, &dup_mirrors,
-                                     &out, &snaplen, &out_vlan))) {
-            /* The mirror got reconfigured before we got to read it's
+                                     &out, &snaplen, &out_vlan,
+                                     &ctx->xin->flow,
+                                     ctx->wc))) {
+            /* The mirror doesn't currently exist, we cannot retrieve it's
               * configuration. */
              mirrors = zero_rightmost_1bit(mirrors);
              continue;
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index fad7342b0..c03bcea60 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3662,7 +3662,7 @@ mirror_set__(struct ofproto *ofproto_, void *aux,
      error = mirror_set(ofproto->mbridge, aux, s->name, srcs, s->n_srcs, dsts,
                         s->n_dsts, s->src_vlans,
                         bundle_lookup(ofproto, s->out_bundle),
-                       s->snaplen, s->out_vlan);
+                       s->snaplen, s->out_vlan, s->filter, ofproto_);
      free(srcs);
      free(dsts);
      return error;
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 8efdb20a0..372e0e3fc 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -497,6 +497,9 @@ struct ofproto_mirror_settings {
      uint16_t out_vlan;          /* Output VLAN, only if out_bundle is NULL. */
      uint16_t snaplen;           /* Max packet size of a mirrored packet
                                     in byte, set to 0 equals 65535. */
+
+    /* Output filter */
+    char *filter;
  };
int ofproto_mirror_register(struct ofproto *, void *aux,
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 6824ce0bb..e55c480d1 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -5182,6 +5182,49 @@ OVS_VSWITCHD_STOP
  AT_CLEANUP
+AT_SETUP([ofproto-dpif - mirroring, filter])
+AT_KEYWORDS([mirror mirrors mirroring])
+OVS_VSWITCHD_START
+add_of_ports br0 1 2 3
+ovs-vsctl \
+        set Bridge br0 mirrors=@m --\
+        --id=@p3 get Port p3 --\
+        --id=@m create Mirror name=mymirror select_all=true output_port=@p3 
filter="icmp"
+
+AT_DATA([flows.txt], [dnl
+in_port=1 actions=output:2
+in_port=2 actions=output:1
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+
+flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
+AT_CHECK_UNQUOTED([tail -1 stdout], [0],
+  [Datapath actions: 3,2
+])
+
+flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=128,frag=no),tcp()"
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
+AT_CHECK_UNQUOTED([tail -1 stdout], [0],
+  [Datapath actions: 2
+])
+
+flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
+AT_CHECK_UNQUOTED([tail -1 stdout], [0],
+  [Datapath actions: 3,1
+])
+
+flow="in_port(2),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=6,tos=0,ttl=128,frag=no),tcp()"
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow"], [0], [stdout])
+AT_CHECK_UNQUOTED([tail -1 stdout], [0],
+  [Datapath actions: 1
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+
  AT_SETUP([ofproto-dpif - mirroring, select_all])
  AT_KEYWORDS([mirror mirrors mirroring])
  OVS_VSWITCHD_START
diff --git a/utilities/ovs-tcpdump.in b/utilities/ovs-tcpdump.in
index 420c11eb8..590f02c77 100755
--- a/utilities/ovs-tcpdump.in
+++ b/utilities/ovs-tcpdump.in
@@ -138,6 +138,7 @@ The following options are available:
     --mirror-to                The name for the mirror port to use (optional)
                                Default 'miINTERFACE'
     --span                     If specified, mirror all ports (optional)
+   --filter                   Set a preselection filter
  """ % {'prog': sys.argv[0]})
      sys.exit(0)
@@ -350,7 +351,7 @@ class OVSDB(object):
          return result
def bridge_mirror(self, intf_name, mirror_intf_name, br_name,
-                      mirror_select_all=False):
+                      mirror_select_all=False, mirror_filter=None):
txn = self._start_txn()
          mirror = txn.insert(self.get_table('Mirror'))
@@ -358,6 +359,9 @@ class OVSDB(object):
mirror.select_all = mirror_select_all + if mirror_filter is not None:
+            mirror.filter = mirror_filter
+
          mirrored_port = self._find_row_by_name('Port', intf_name)
mirror.verify('select_dst_port')
@@ -441,6 +445,7 @@ def main():
      mirror_interface = None
      mirror_select_all = False
      dump_cmd = 'tcpdump'
+    mirror_filter = None
for cur, nxt in argv_tuples(sys.argv[1:]):
          if skip_next:
@@ -470,6 +475,10 @@ def main():
          elif cur in ['--span']:
              mirror_select_all = True
              continue
+        elif cur in ['--filter']:
+            mirror_filter = nxt
+            skip_next = True
+            continue
          tcpdargs.append(cur)
if interface is None:
@@ -522,7 +531,7 @@ def main():
          ovsdb.make_port(mirror_interface, ovsdb.port_bridge(interface))
          ovsdb.bridge_mirror(interface, mirror_interface,
                              ovsdb.port_bridge(interface),
-                            mirror_select_all)
+                            mirror_select_all, mirror_filter=mirror_filter)
      except OVSDBException as oe:
          print("ERROR: Unable to properly setup the mirror: %s." % str(oe))
          sys.exit(1)
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index e9110c1d8..50213a1f9 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -5166,6 +5166,13 @@ mirror_configure(struct mirror *m)
      /* Get VLAN selection. */
      s.src_vlans = vlan_bitmap_from_array(cfg->select_vlan, 
cfg->n_select_vlan);
+ /* Set filter, this value is not retained after mirror_set() is called. */
+    if (cfg->filter && strlen(cfg->filter)) {
+        s.filter = xstrdup(cfg->filter);
+    } else {
+        s.filter = NULL;
+    }
+
      /* Configure. */
      ofproto_mirror_register(m->bridge->ofproto, m, &s);
@@ -5175,6 +5182,7 @@ mirror_configure(struct mirror *m)
      }
      free(s.srcs);
      free(s.src_vlans);
+    free(s.filter);
return true;
  }
diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
index 2d395ff95..5400a5bed 100644
--- a/vswitchd/vswitch.ovsschema
+++ b/vswitchd/vswitch.ovsschema
@@ -1,6 +1,6 @@
  {"name": "Open_vSwitch",
   "version": "8.4.0",
- "cksum": "2738838700 27127",
+ "cksum": "59426185 27174",
   "tables": {
     "Open_vSwitch": {
       "columns": {
@@ -461,6 +461,8 @@
           "type": {"key": "string", "value": "integer",
                    "min": 0, "max": "unlimited"},
           "ephemeral": true},
+       "filter": {
+         "type": "string"},
         "external_ids": {
           "type": {"key": "string", "value": "string",
                    "min": 0, "max": "unlimited"}}}},
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index cfcde34ff..3965054d5 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -5237,6 +5237,12 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch 
options:peer=p1 \
             are not truncated.
          </p>
        </column>
+      <column name="filter">
+        <p>Filter to apply to mirror.</p>
+        <p>Flows that match <ref column="filter"/> will flow through this
+           mirror, flows that do not match will be ignored.
+        </p>
+      </column> >       </group>

IIUC, this adds "filter" option to "Mirroring Destination Configuration" group. I think it's better suited in the "Selecting Packets for Mirroring" group, where a sentence could be added explaining the relationship between flow-filtering and vlan-filtering.

<group title="Statistics: Mirror counters">

Thanks.
--
Adrián Moreno

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

Reply via email to