On 10/17/23 20:20, Mike Pattrick wrote:
> On Fri, Oct 13, 2023 at 9:06 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
>>
>> On 10/8/23 06:21, 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.
>>>
>>> Signed-off-by: Mike Pattrick <m...@redhat.com>
>>>
>>
>> Hi, Mike. Thanks for the patch!
>>
>> Not a full review, but see some comments inline.
>>
>> Best regards, Ilya Maximets.
>>
>>> ---
>>> v3:
>>> - Added more tests
>>> - Refactored mirror wildcard modification out of mirror_get
>>> - Improved error handling
>>> v4:
>>> - Refactored mirror_set and mirror_get functions to reduce function
>>> parameters
>>> v5:
>>> - Fixed formating issues
>>> - Moved some code from patch 2 to patch 1 to fix compile
>>> v6:
>>> - Clarified filter format in documentation
>>> - Moved flow/mask operations to atomic set/get
>>> - Cleaned up and improved test
>>>
>>> ---
>>> Documentation/ref/ovs-tcpdump.8.rst | 8 +-
>>> NEWS | 4 +
>>> lib/flow.h | 9 +++
>>> ofproto/ofproto-dpif-mirror.c | 68 +++++++++++++++-
>>> ofproto/ofproto-dpif-mirror.h | 8 +-
>>> ofproto/ofproto-dpif-xlate.c | 16 +++-
>>> ofproto/ofproto-dpif.c | 9 +--
>>> ofproto/ofproto-dpif.h | 6 ++
>>> ofproto/ofproto.h | 3 +
>>> tests/ofproto-dpif.at | 116 ++++++++++++++++++++++++++++
>>> utilities/ovs-tcpdump.in | 13 +++-
>>> vswitchd/bridge.c | 13 +++-
>>> vswitchd/vswitch.ovsschema | 5 +-
>>> vswitchd/vswitch.xml | 13 ++++
>>> 14 files changed, 273 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/Documentation/ref/ovs-tcpdump.8.rst
>>> b/Documentation/ref/ovs-tcpdump.8.rst
>>> index b9f8cdf6f..e21e61211 100644
>>> --- a/Documentation/ref/ovs-tcpdump.8.rst
>>> +++ b/Documentation/ref/ovs-tcpdump.8.rst
>>> @@ -61,8 +61,14 @@ Options
>>>
>>> If specified, mirror all ports (optional).
>>>
>>> +* ``--filter <flow>``
>>> +
>>> + If specified, only mirror flows that match the provided OpenFlow filter.
>>> + The available fields are documented in ``ovs-fields(7)``.
>>> +
>>> See Also
>>> ========
>>>
>>> ``ovs-appctl(8)``, ``ovs-vswitchd(8)``, ``ovs-pcap(1)``,
>>> -``ovs-tcpundump(1)``, ``tcpdump(8)``, ``wireshark(8)``.
>>> +``ovs-fields(7)``, ``ovs-tcpundump(1)``, ``tcpdump(8)``,
>>> +``wireshark(8)``.
>>> diff --git a/NEWS b/NEWS
>>> index 6b45492f1..e95033b50 100644
>>> --- a/NEWS
>>> +++ b/NEWS
>>> @@ -6,6 +6,10 @@ Post-v3.2.0
>>> from older version is supported but it may trigger more leader
>>> elections
>>> during the process, and error logs complaining unrecognized fields
>>> may
>>> be observed on old nodes.
>>> + * Added a new filter column in the Mirror table which can be used to
>>> + apply filters to mirror ports.
>>> + - ovs-tcpdump:
>>> + * Added new --filter parameter to apply pre-selection on mirrored
>>> flows.
>>>
>>>
>>> v3.2.0 - 17 Aug 2023
>>> diff --git a/lib/flow.h b/lib/flow.h
>>> index a9d026e1c..f77d3553a 100644
>>> --- a/lib/flow.h
>>> +++ b/lib/flow.h
>>> @@ -939,6 +939,15 @@ 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 17ba6f799..792d3930c 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,11 @@ struct mirror {
>>> struct hmapx srcs; /* Contains "struct mbundle*"s. */
>>> struct hmapx dsts; /* Contains "struct mbundle*"s. */
>>>
>>> + /* Filter criteria. */
>>> + OVSRCU_TYPE(struct miniflow *) filter;
>>> + OVSRCU_TYPE(struct minimask *) mask;
>>> + char *filter_str;
>>> +
>>> /* This is accessed by handler threads assuming RCU protection (see
>>> * mirror_get()), but can be manipulated by mirror_set() without any
>>> * explicit synchronization. */
>>> @@ -207,11 +213,13 @@ mirror_bundle_dst(struct mbridge *mbridge, struct
>>> ofbundle *ofbundle)
>>> }
>>>
>>> int
>>> -mirror_set(struct mbridge *mbridge, void *aux,
>>> +mirror_set(const struct ofproto *ofproto_, void *aux,
>>> const struct ofproto_mirror_settings *ms, struct ofbundle
>>> **srcs,
>>> struct ofbundle **dsts, struct ofbundle *bundle)
>>>
>>> {
>>> + const struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
>>
>> This looks like a layering violation. Can we pass mbridge here instead?
>>
>>> + struct mbridge *mbridge = ofproto->mbridge;
>>> struct mbundle *mbundle, *out;
>>> mirror_mask_t mirror_bit;
>>> struct mirror *mirror;
>>> @@ -265,7 +273,8 @@ mirror_set(struct mbridge *mbridge, void *aux,
>>> && vlan_bitmap_equal(vlans, ms->src_vlans)
>>> && mirror->out == out
>>> && mirror->out_vlan == out_vlan
>>> - && mirror->snaplen == ms->snaplen)
>>> + && mirror->snaplen == ms->snaplen
>>> + && nullable_string_is_equal(mirror->filter_str, ms->filter))
>>> {
>>> hmapx_destroy(&srcs_map);
>>> hmapx_destroy(&dsts_map);
>>> @@ -289,6 +298,44 @@ mirror_set(struct mbridge *mbridge, void *aux,
>>> mirror->out_vlan = out_vlan;
>>> mirror->snaplen = ms->snaplen;
>>>
>>> + if (!nullable_string_is_equal(mirror->filter_str, ms->filter)) {
>>
>> THis check might not be enough. By allowing port names in the filter
>> there is a chance that ports will change their numbers without changing
>> their names. In this case the parsed filter flow should be invalidated.
>
> Thanks for the review Ilya, I was going through these comments and I
> noticed some discrepancy with how flows are handled when port number
> changes. With the sample flow rule "in_port=p1 actions=output:p2" and
> after changing the port number with "ovs-vsctl set int p1
> ofport_request=8", I see that the flow is not invalidated or updated,
> and instead retains the old now invalid port number: "in_port=1
> actions=output:p2". Is this the intended behaviour?
Port names is a syntactic sugar, they are not part of OpenFlow.
Names are resolved to port numbers by ovs-ofctl by first sending
a ports_desc OF request and getting the mapping, then it constructs
OpenFlow flow_mod request with just numbers and ovs-vswitchd
only receives OpenFlow port numbers. When you do a flow dump,
ovs-ofctl will send another port_desc OpenFlow request to get
the port names and then get the flow dump and print it out to
the user replacing the numbers with the names it can find.
So, ovs-vswtichd doesn't work wit names, only with numbers.
>
> I think it makes some amount of sense for this filter to act in a
> similar fashion to flows.
I'm not sure about that, both the ports and the filter are stored
in the database. So, the content of the database will be inconsistent
after the OpenFlow port number change. That is not the case for
ovs-ofctl, because the mapping in this case exists only momentarily
while translating user input into OpenFlow and back.
The situation with flows is reproducible, i.e. if you start
another ovs-vswitchd and the ovsdb-server with the same database
and the same OF rules (OF rules only have numbers!), the behavior
of the system will be the same.
With the filter, restart of ovs-vswitchd or just re-addition of the
same mirror will make it start sampling different packets, because
the name will be re-resolved into a different port number.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
