On Fri, Aug 25, 2023 at 10:28 AM Adrian Moreno <amore...@redhat.com> wrote:
>
>
>
> 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().
I think at the very least I should also add a check for the
ofproto_mirror_register() return value and cleanup/log as appropriate.
I'll investigate better locations for this code and the other
suggestions in an updated patch.
Thanks for the review,
M
>
> +
> > + 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
